REGRESSION(r257759, r258623): Web Inspector: Settings icon sometimes placed below the tab bar

• Component: Web Inspector
• Status: RESOLVED FIXED

[Nikita Vasilyev]

Created attachment 392481 [details]
[Video] Bug

When docked, the gear icon "jumps" to the second line at certain Web Inspector width.

[Radar WebKit Bug Importer]

<rdar://problem/60108967>

[Nikita Vasilyev]

Created attachment 393199 [details]
Patch

cq- for now because I still don't fully understand everything inside of WI.TabBar.prototype.layout.

[Nikita Vasilyev]

Comment on attachment 393199 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=393199&action=review

> Source/WebInspectorUI/UserInterface/Views/TabBar.js:-469
>          let totalItemWidth = recalculateItemWidths();
>          if (totalItemWidth > availableSpace) {
> -            totalItemWidth = recalculateItemWidths();
> -            if (totalItemWidth > availableSpace) {

These two lines were literally repeated twice.

[Nikita Vasilyev]

I asked questions in https://bugs.webkit.org/show_bug.cgi?id=204627#c55.

[Devin Rousso]

Comment on attachment 393199 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=393199&action=review

r-, for reason described in TabBar.js

> Source/WebInspectorUI/UserInterface/Views/TabBar.js:468
> +                availableSpace -= measureItemWidth(tabBarItem);

We don't want to do this here because `WI.PinnedTabBarItem` is considered in `recalculateItemWidths()`, meaning that we will double count it's width.

Please see my comment in <https://webkit.org/b/204627#c56>.

[Nikita Vasilyev]

Devin, do you want to take this bug? You already posted the code in https://webkit.org/b/204627#c56. If not, you'd find yourself reviewing your own code, and I don't think it's a good practice.

[Devin Rousso]

Created attachment 393731 [details]
Patch

[BJ Burg]

Comment on attachment 393731 [details]
Patch

r=me

[WebKit Commit Bot]

Comment on attachment 393731 [details]
Patch

Clearing flags on attachment: 393731

Committed r258550: <https://trac.webkit.org/changeset/258550>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Devin Rousso]

*** Bug 209161 has been marked as a duplicate of this bug. ***

[Nikita Vasilyev]

Created attachment 394015 [details]
[Video] Bug (again)

I can still reproduce the bug. Now it feels like a rounding error.

[Nikita Vasilyev]

This was fixed in r258550 (patch in this bug) and then broken again in r258623 (Bug 209200 - Web Inspector: the width of `WI.TabBarItem` can change if the detached window is resized).

[Devin Rousso]

Created attachment 394117 [details]
Patch

[Nikita Vasilyev]

Created attachment 394119 [details]
[Image] Bug (with logging)

I added logging and this is what I found. The bug happens when normalTabBarItemsWidth and availableSpace are the exact numbers.

[Nikita Vasilyev]

Created attachment 394121 [details]
Patch

[Nikita Vasilyev]

Comment on attachment 394121 [details]
Patch

Subtracting 1 makes it such that the detached tab bar always thinks there is not enough room.

[Nikita Vasilyev]

The bug only happens when Web Inspector is in the debug mode (Command-Option-Shift-D toggles debug mode). In the debug mode, Web Inspector has a button to open a 2nd level inspector. It's a text button and it has a float number width.

Devin's patch does fix the problem.

[Devin Rousso]

Created attachment 394129 [details]
Patch

[Nikita Vasilyev]

Comment on attachment 394129 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=394129&action=review

> Source/WebInspectorUI/ChangeLog:13
> +        Speculative fix, as I haven't been able to reproduce this myself. My guess as to what's
> +        happening is that the sum width of all `WI.GeneralTabBarItem` is a non-integer value, which
> +        needs to be properly rounded when compared to the width of the container `WI.TabBar`. This
> +        may be necessary because CSS often rounds to the nearest pixel, meaning that `99.5px` would
> +        actually render as `100px`, whereas `99.4px` would render as `99px`.

I thought you reproduced it.

[Devin Rousso]

Comment on attachment 394129 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=394129&action=review

>> Source/WebInspectorUI/ChangeLog:13
>> +        actually render as `100px`, whereas `99.4px` would render as `99px`.
> 
> I thought you reproduced it.

Oops.  Didn't update the ChangeLog 😅

[Devin Rousso]

Created attachment 394130 [details]
Patch

[Nikita Vasilyev]

Comment on attachment 394130 [details]
Patch

I hate to say it, but I can still reproduce the bug.

Undocked
Debug mode is on
Body width is 910px. You can set it with window.resizeTo(910, 500).

[Nikita Vasilyev]

(In reply to Nikita Vasilyev from comment #23)
> Comment on attachment 394130 [details]
> Patch
> 
> I hate to say it, but I can still reproduce the bug.
> 
> Undocked
> Debug mode is on
> Body width is 910px. You can set it with window.resizeTo(910, 500).

Hm, the body width should be 915. window.resizeTo(915, 500).

[Devin Rousso]

(In reply to Nikita Vasilyev from comment #24)
> (In reply to Nikita Vasilyev from comment #23)
> > Comment on attachment 394130 [details]
> > Patch
> > 
> > I hate to say it, but I can still reproduce the bug.
> > 
> > Undocked
> > Debug mode is on
> > Body width is 910px. You can set it with window.resizeTo(910, 500).
> 
> Hm, the body width should be 915. window.resizeTo(915, 500).

This works fine for me, both at 910 and 915.
 - Are you zoomed in?
 - Are there any warnings/errors in the console (both of that inspector and of inspector^2)?
 - What tabs do you have open?
 - Which tab is selected?

[Devin Rousso]

Created attachment 394135 [details]
Patch

Handle the case where the rounded width of all `WI.GeneralTabBarItem` is equal to the width of the container `WI.TabBar`.

[EWS]

Committed r258898: <https://trac.webkit.org/changeset/258898>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 394135 [details].