Created attachment 384371 [details] Patch

Created attachment 384372 [details] [Image] After Patch is applied (undocked)

Created attachment 384373 [details] [Image] After Patch is applied (undocked with console messages)

Created attachment 384374 [details] [Image] After Patch is applied (docked side)

Created attachment 384375 [details] [Image] After Patch is applied (docked side with console messages)

Created attachment 384376 [details] [Image] After Patch is applied (docked bottom)

Created attachment 384378 [details] [Image] After Patch is applied (Network)

Created attachment 384379 [details] [Image] After Patch is applied (docked bottom with console messages)

Created attachment 384382 [details] Patch Minor ChangeLog modifications

Overall, I like this! (In reply to Devin Rousso from comment #5) > Created attachment 384375 [details] > [Image] After Patch is applied (docked side with console messages) Can we move console icons to the right? I don't like when the icon appears, it shifts all tabs by ~20px. I like error and warning console icons, but I don't see a lot of value in the log messages icon. Can we remove it?

(In reply to Nikita Vasilyev from comment #11) > Can we move console icons to the right? I don't like when the icon appears, it shifts all tabs by ~20px. I actually kinda like how jarring it is, as it makes it _really_ obvious that I have a new log/warning/error. Putting it on the right is also a bit weird to me because there's no good place for it. Placing it after Settings (or in between Search and Settings), is weird because it's so far away from the actual "content" tabs, and Settings is kinda supposed to be the most "out of the way" thing in the tab bar, so to suddenly have useful things after it could be confusing. It also doesn't make sense for it to follow the Console Tab, as we wouldn't want it to appear in the middle of all the tabs. Another alternative would be to have them always be visible and use the colorless version by default, only switching to the color version when there is a message. That way, there is no shifting, and we still see something visual happen. > I like error and warning console icons, but I don't see a lot of value in the log messages icon. Can we remove it? I think it's useful to know when something is logged to the console (e.g. if you're waiting to hit some log statement while debugging), but I also think that logs are so common that having the icon might not really tell you anything you wouldn't already expect. I think I agree that we should remove it. I'll give it a shot and see how it feels.

Created attachment 384393 [details] Patch

Created attachment 384394 [details] [Image] After Patch is applied (undocked)

Created attachment 384395 [details] [Image] After Patch is applied (undocked with console messages)

Created attachment 384693 [details] Patch

Eh, I'm not a fan of the compact look. I'm not convinced that saving 36px is worth it.

If we do end up going forward with this design, can we hide download and reload buttons (unless we remote debugging)? I still think it's better to keep the warning and error icons next to the console, the last tab by default. (There are two search icons next to each other on the last couple of screenshots 😅)

(In reply to Nikita Vasilyev from comment #18) > If we do end up going forward with this design, can we hide download and reload buttons (unless we remote debugging)? My goal with this patch is not to make any more changes than necessary (e.g. moving the network/resource data to the Network Tab) to get rid of the toolbar/dashboard. Deciding to show/hide the download/reload buttons is a decision that should be made in a separate patch. > I still think it's better to keep the warning and error icons next to the console, the last tab by default. If the buttons stay in a fixed place, I don't have a particular preference one way or the other. I don't want the warning/error buttons to follow the Console Tab around. I do think that having them towards the left (start) will be more prominently seen, as that better matches the reading direction of Web Inspector as a whole (things start from the left (start) and go to the right (end) in LTR, and the opposite is true for RTL). Furthermore, things towards the right (end) tend to be more related to Web Inspector itself, rather than content (e.g. settings, docking/undocking, close), while these icons are really very important and should be highlighted. I don't think I'd notice it if it was that far away. > (There are two search icons next to each other on the last couple of screenshots 😅) I fixed this in the most recent patch. Also, for what it's worth, it would only show up once on the first time Web Inspector was opened after this patch, and only if you had a Search Tab opened previously.

Created attachment 384931 [details] Patch Switch the position of the download and reload buttons to match the existing order

Comment on attachment 384374 [details] [Image] After Patch is applied (docked side) Simon pointed out that we have a double border at the top of the Web Inspector when docked to the side. That should be fixed 😬

Comment on attachment 384394 [details] [Image] After Patch is applied (undocked) Simon had an idea of putting a pause "||" (and resume "|>") next to the warning and error icons that would change depending on whether the debugger was paused or not. Clicking it would have the same effect as clicking that icon in the Sources Tab, similar to what used to be in the dashboard area. We could also potentially pulse the icon when paused so it's more obvious/visible, but that could be annoying, so perhaps just sticking with a blue icon would be enough.

Created attachment 385061 [details] Patch Fixed issue where network load time statistic logic would throw an error when reloading the page if the Network Tab hadn't yet been shown. Reworked how the top border of the tab bar is created in order to make styling simpler for the various states. Added `.window-docked-inactive` styles to places that already had `.window-inactive`.

Created attachment 385062 [details] [Image] After Patch is applied (docked side)

Created attachment 385063 [details] [Image] After Patch is applied (docked side with window unfocused)

Created attachment 385064 [details] [Image] After Patch is applied (docked bottom)

Created attachment 385065 [details] [Image] After Patch is applied (docked bottom with window unfocused)

Created attachment 385066 [details] [Image] After Patch is applied (undocked)

Created attachment 385067 [details] [Image] After Patch is applied (undocked with console messages)

Created attachment 385068 [details] [Image] After Patch is applied (undocked with window unfocused)

Created attachment 385069 [details] [Image] After Patch is applied (undocked with window unfocused with console messages)

Created attachment 385792 [details] [Image] suggested/example icon stacking The contrast for the active error/warning icon states drops significantly on the darker background. To remedy this, try adding a light gray, single pixel wide stroke. Secondly, since we're no longer providing counts for each type, have you considered "stacking" the icons? Attached an example of what I mean by stacking.

(In reply to esisk from comment #32) > Secondly, since we're no longer providing counts for each type, have you > considered "stacking" the icons? Attached an example of what I mean by > stacking. I like the idea of stacking!

(In reply to Nikita Vasilyev from comment #33) > (In reply to esisk from comment #32) > > Secondly, since we're no longer providing counts for each type, have you considered "stacking" the icons? Attached an example of what I mean by stacking. > I like the idea of stacking! I am a fan of this idea, but I am concerned with the clickability of the icons. If they're overlapping, it becomes that much harder to click on the one that is further behind. Also, I would consider this to be outside the scope of this change. I'm happy to investigate/experiment as a followup :)

(In reply to Devin Rousso from comment #34) > Also, I would consider this to be outside the scope of this change. I'm > happy to investigate/experiment as a followup :) Sounds good. I can write a followup bug if you want.

Stacking the icons is good.

Created attachment 389311 [details] Patch

Created attachment 389313 [details] [Image] After Patch is applied (undocked)

Created attachment 389314 [details] [Image] After Patch is applied (undocked with console messages)

The commit-queue encountered the following flaky tests while processing attachment 389311 [details]: editing/spelling/spellcheck-attribute.html bug 206178 (authors: g.czajkowski@samsung.com, mark.lam@apple.com, and rniwa@webkit.org) The commit-queue is continuing to process your patch.

Comment on attachment 389311 [details] Patch Clearing flags on attachment: 389311 Committed r255547: <https://trac.webkit.org/changeset/255547>

All reviewed patches have been landed. Closing bug.

<rdar://problem/59091905>

It seems this broke several GTK API tests as the EWS detected. I'll investigate.

Views/SizesToFitNavigationBar.js was removed but still listed in Main.html. The script that combines sources failed during the build (but for some reason isn't fatal in CMake at least) leaving the original Main.html (still referring to Base/Main.js that doesn't exist).

Committed r255557: <https://trac.webkit.org/changeset/255557>

Comment on attachment 389311 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=389311&action=review > Source/WebInspectorUI/ChangeLog:25 > + When undocked, the tab bar and all the content below it are pushed down by 22px to make room > + for the system close/minimize/maximize buttons and the window title. This is mac specific behavior, would it be possible to do that only for mac? Now we have 22px of nothing in the inspector when undocked in the GTK port.

Comment on attachment 389311 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=389311&action=review >> Source/WebInspectorUI/ChangeLog:25 >> + for the system close/minimize/maximize buttons and the window title. > > This is mac specific behavior, would it be possible to do that only for mac? Now we have 22px of nothing in the inspector when undocked in the GTK port. That’s fine by me. Please do whatever you need to make Web Inspector feel at home on GTK :)

Comment on attachment 389311 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=389311&action=review >>> Source/WebInspectorUI/ChangeLog:25 >>> + for the system close/minimize/maximize buttons and the window title. >> >> This is mac specific behavior, would it be possible to do that only for mac? Now we have 22px of nothing in the inspector when undocked in the GTK port. > > That’s fine by me. Please do whatever you need to make Web Inspector feel at home on GTK :) I took the liberty of moving this logic to a `body.mac-platform:not(.docked)` CSS rule so it should only apply for macOS :) <https://webkit.org/b/207228> Web Inspector: the height of the undocked title area shouldn't change when zoomed

Reopening. This was reverted by https://trac.webkit.org/changeset/256086/webkit in b207422.

Created attachment 392214 [details] Patch adjusted design based on offline feedback

Created attachment 392217 [details] Patch rebase

Comment on attachment 392217 [details] Patch Clearing flags on attachment: 392217 Committed r257759: <https://trac.webkit.org/changeset/257759>

All reviewed patches have been landed. Closing bug.

Comment on attachment 392217 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=392217&action=review > Source/WebInspectorUI/UserInterface/Views/TabBar.js:439 > + let flexibleSpaceSizeWithHiddenItems = this._flexibleSpaceBeforeElement.realOffsetWidth - this._flexibleSpaceAfterElement.realOffsetWidth; This doesn't look right. This value is generally 0 because the left and the right flexible spaces are the same width. > Source/WebInspectorUI/UserInterface/Views/TabBar.js:446 > + let flexibleSpaceSizeWithoutHiddenItems = this._flexibleSpaceBeforeElement.realOffsetWidth - this._flexibleSpaceAfterElement.realOffsetWidth; This doesn't look right either. This value is sometimes negative (because adding `.calculate-width` may causes tabBar items to span more than one line). Shouldn't we be adding instead of deducting?

Comment on attachment 392217 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=392217&action=review >> Source/WebInspectorUI/UserInterface/Views/TabBar.js:439 >> + let flexibleSpaceSizeWithHiddenItems = this._flexibleSpaceBeforeElement.realOffsetWidth - this._flexibleSpaceAfterElement.realOffsetWidth; > > This doesn't look right. This value is generally 0 because the left and the right flexible spaces are the same width. No, this was intentional, albeit probably not the best way of going about it. The idea here is that when we add `.calculate-width`, the tab bar can wrap, which means that the `_flexibleSpaceAfterElement` can become _super_ wide, which would affect these calculations. In reality, it should probably be a `Math.min` between the two values, as when we're done with `layout()` we should probably be in a state where `this._flexibleSpaceBeforeElement.realOffsetWidth === this._flexibleSpaceAfterElement.realOffsetWidth` (or is at the very least super close to it) and could therefore just multiply the smaller flexible space size by 2, especially since we've set `justify-content: space-around;`. In hindsight, I don't think we should be considering the size of the flexible space at all, and really should be using every `px` available. Instead, we should probably do something like the following: ```js let availableSpace = this._tabContainer.realOffsetWidth - tabBarHorizontalPadding; this._tabContainer.classList.add("calculate-width"); this._hiddenTabBarItems = []; let normalTabBarItems = []; let normalTabBarItemsWidth = 0; for (let tabBarItem of this._tabBarItemsFromLeftToRight()) { if (tabBarItem === this._tabPickerTabBarItem) { tabBarItem.hidden = true; continue; } tabBarItem.hidden = false; if (tabBarItem instanceof WI.PinnedTabBarItem) barWidth -= measureItemWidth(tabBarItem); else { normalTabBarItems.push(tabBarItem); normalTabBarItemsWidth += measureItemWIdth(tabBarItem); } } if (normalTabBarItemWidth > availableSpace) { this._tabPickerTabBarItem.hidden = false; availableSpace -= measureItemWidth(this._tabPickerTabBarItem); let index = normalTabBarItems.length - 1; do { let tabBarItem = normalTabBarItems[index]; if (tabBarItem === this._selectedTabBarItem) continue; normalTabBarItemWidth -= measureItemWidth(tabBarItem); tabBarItem.hidden = true; this._hiddenTabBarItems.push(tabBarItem); } while (normalTabBarItemWidth > availableSpace && --index >= 0); } this._tabContainer.classList.remove("calculate-width"); ``` We shouldn't need to worry about the value stored in `[WI.TabBar.CachedWidthSymbol]` changing anymore (meaning we don't need to reset it), as we no longer hide icons, meaning that the width of every `WI.TabBarItem` should be constant.