Bug 208603 - REGRESSION(r257759, r258623): Web Inspector: Settings icon sometimes placed below the tab bar
Summary: REGRESSION(r257759, r258623): Web Inspector: Settings icon sometimes placed b...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Devin Rousso
URL:
Keywords: InRadar
Depends on:
Blocks: 209200
  Show dependency treegraph
 
Reported: 2020-03-04 14:12 PST by Nikita Vasilyev
Modified: 2020-03-23 19:22 PDT (History)
6 users (show)

See Also:


Attachments
[Video] Bug (1.26 MB, video/quicktime)
2020-03-04 14:12 PST, Nikita Vasilyev
no flags Details
Patch (2.17 KB, patch)
2020-03-10 18:50 PDT, Nikita Vasilyev
no flags Details | Formatted Diff | Diff
Patch (5.68 KB, patch)
2020-03-16 22:37 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff
[Video] Bug (again) (1.69 MB, video/quicktime)
2020-03-19 13:43 PDT, Nikita Vasilyev
no flags Details
Patch (2.59 KB, patch)
2020-03-20 13:31 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff
[Image] Bug (with logging) (571.42 KB, image/png)
2020-03-20 13:45 PDT, Nikita Vasilyev
no flags Details
Patch (1.48 KB, patch)
2020-03-20 14:00 PDT, Nikita Vasilyev
nvasilyev: review-
nvasilyev: commit-queue-
Details | Formatted Diff | Diff
Patch (5.14 KB, patch)
2020-03-20 14:32 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (5.02 KB, text/plain)
2020-03-20 14:38 PDT, Devin Rousso
no flags Details
Patch (5.20 KB, patch)
2020-03-20 15:10 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Nikita Vasilyev 2020-03-04 14:12:39 PST
Created attachment 392481 [details]
[Video] Bug

When docked, the gear icon "jumps" to the second line at certain Web Inspector width.
Comment 1 Radar WebKit Bug Importer 2020-03-05 17:22:08 PST
<rdar://problem/60108967>
Comment 2 Nikita Vasilyev 2020-03-10 18:50:22 PDT
Created attachment 393199 [details]
Patch

cq- for now because I still don't fully understand everything inside of WI.TabBar.prototype.layout.
Comment 3 Nikita Vasilyev 2020-03-10 18:52:10 PDT
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.
Comment 4 Nikita Vasilyev 2020-03-10 19:11:46 PDT
I asked questions in https://bugs.webkit.org/show_bug.cgi?id=204627#c55.
Comment 5 Devin Rousso 2020-03-16 17:04:55 PDT
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>.
Comment 6 Nikita Vasilyev 2020-03-16 17:08:45 PDT
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.
Comment 7 Devin Rousso 2020-03-16 22:37:51 PDT
Created attachment 393731 [details]
Patch
Comment 8 Brian Burg 2020-03-17 07:55:02 PDT
Comment on attachment 393731 [details]
Patch

r=me
Comment 9 WebKit Commit Bot 2020-03-17 08:40:03 PDT
Comment on attachment 393731 [details]
Patch

Clearing flags on attachment: 393731

Committed r258550: <https://trac.webkit.org/changeset/258550>
Comment 10 WebKit Commit Bot 2020-03-17 08:40:05 PDT
All reviewed patches have been landed.  Closing bug.
Comment 11 Devin Rousso 2020-03-17 14:49:06 PDT
*** Bug 209161 has been marked as a duplicate of this bug. ***
Comment 12 Nikita Vasilyev 2020-03-19 13:43:17 PDT
Created attachment 394015 [details]
[Video] Bug (again)

I can still reproduce the bug. Now it feels like a rounding error.
Comment 13 Nikita Vasilyev 2020-03-19 22:21:00 PDT
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).
Comment 14 Devin Rousso 2020-03-20 13:31:36 PDT
Created attachment 394117 [details]
Patch
Comment 15 Nikita Vasilyev 2020-03-20 13:45:02 PDT
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.
Comment 16 Nikita Vasilyev 2020-03-20 14:00:00 PDT Comment hidden (obsolete)
Comment 17 Nikita Vasilyev 2020-03-20 14:16:58 PDT
Comment on attachment 394121 [details]
Patch

Subtracting 1 makes it such that the detached tab bar always thinks there is not enough room.
Comment 18 Nikita Vasilyev 2020-03-20 14:31:05 PDT
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.
Comment 19 Devin Rousso 2020-03-20 14:32:03 PDT
Created attachment 394129 [details]
Patch
Comment 20 Nikita Vasilyev 2020-03-20 14:34:34 PDT
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.
Comment 21 Devin Rousso 2020-03-20 14:37:19 PDT
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 😅
Comment 22 Devin Rousso 2020-03-20 14:38:47 PDT
Created attachment 394130 [details]
Patch
Comment 23 Nikita Vasilyev 2020-03-20 14:55:22 PDT
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).
Comment 24 Nikita Vasilyev 2020-03-20 14:59:25 PDT
(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).
Comment 25 Devin Rousso 2020-03-20 15:01:28 PDT
(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?
Comment 26 Devin Rousso 2020-03-20 15:10:37 PDT
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`.
Comment 27 EWS 2020-03-23 19:22:56 PDT
Committed r258898: <https://trac.webkit.org/changeset/258898>

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