Bug 208603

Summary:

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

Product:

WebKit

Reporter:

Nikita Vasilyev <nvasilyev>

Component:

Web Inspector

Assignee:

Devin Rousso <hi>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

bburg, commit-queue, hi, inspector-bugzilla-changes, timothy, webkit-bug-importer

Priority:

Keywords:

InRadar

Version:

WebKit Nightly Build

Hardware:

All

OS:

All

See Also:

https://bugs.webkit.org/show_bug.cgi?id=209161

Bug Depends on:

Bug Blocks:

209200

Attachments:

Description	Flags
[Video] Bug	none
Patch	none
Patch	none
[Video] Bug (again)	none
Patch	none
[Image] Bug (with logging)	none
Patch	nvasilyev: review-, nvasilyev: commit-queue-
Patch	none
Patch	none
Patch	none

Nikita Vasilyev

Reported 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.

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
Show Obsolete (6) View All Add attachment proposed patch, testcase, etc.

Radar WebKit Bug Importer

Comment 1 2020-03-05 17:22:08 PST

<rdar://problem/60108967>

Nikita Vasilyev

Comment 2 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.

Nikita Vasilyev

Comment 3 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.

Nikita Vasilyev

Comment 4 2020-03-10 19:11:46 PDT

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

Devin Rousso

Comment 5 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>.

Nikita Vasilyev

Comment 6 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.

Devin Rousso

Comment 7 2020-03-16 22:37:51 PDT

Created attachment 393731 [details] Patch

Blaze Burg

Comment 8 2020-03-17 07:55:02 PDT

Comment on attachment 393731 [details] Patch r=me

WebKit Commit Bot

Comment 9 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>

WebKit Commit Bot

Comment 10 2020-03-17 08:40:05 PDT

All reviewed patches have been landed. Closing bug.

Devin Rousso

Comment 11 2020-03-17 14:49:06 PDT

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

Nikita Vasilyev

Comment 12 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.

Nikita Vasilyev

Comment 13 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).

Devin Rousso

Comment 14 2020-03-20 13:31:36 PDT

Created attachment 394117 [details] Patch

Nikita Vasilyev

Comment 15 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.

Nikita Vasilyev

Comment 16 2020-03-20 14:00:00 PDT Comment hidden (obsolete)

Created attachment 394121 [details] Patch

Nikita Vasilyev

Comment 17 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.

Nikita Vasilyev

Comment 18 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.

Devin Rousso

Comment 19 2020-03-20 14:32:03 PDT

Created attachment 394129 [details] Patch

Nikita Vasilyev

Comment 20 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.

Devin Rousso

Comment 21 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 😅

Devin Rousso

Comment 22 2020-03-20 14:38:47 PDT

Created attachment 394130 [details] Patch

Nikita Vasilyev

Comment 23 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).

Nikita Vasilyev

Comment 24 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).

Devin Rousso

Comment 25 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?

Devin Rousso

Comment 26 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`.

EWS

Comment 27 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].

Note You need to log in before you can comment on or make changes to this bug.