Bug 204627 - Web Inspector: Items in the toolbar take up to much vertical space
Summary: Web Inspector: Items in the toolbar take up to much vertical space
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: 212398 207228 208555 209366 209525 210947
  Show dependency treegraph
 
Reported: 2019-11-26 11:50 PST by Devin Rousso
Modified: 2020-11-18 15:44 PST (History)
11 users (show)

See Also:


Attachments
[Image] Screenshot of issue (652.16 KB, image/png)
2019-11-26 11:50 PST, Devin Rousso
no flags Details
Patch (329.21 KB, patch)
2019-11-26 12:44 PST, Devin Rousso
no flags Details | Formatted Diff | Diff
[Image] After Patch is applied (undocked) (462.80 KB, image/png)
2019-11-26 12:44 PST, Devin Rousso
no flags Details
[Image] After Patch is applied (undocked with console messages) (497.76 KB, image/png)
2019-11-26 12:44 PST, Devin Rousso
no flags Details
[Image] After Patch is applied (docked side) (2.61 MB, image/png)
2019-11-26 12:45 PST, Devin Rousso
no flags Details
[Image] After Patch is applied (docked side with console messages) (2.64 MB, image/png)
2019-11-26 12:45 PST, Devin Rousso
no flags Details
[Image] After Patch is applied (docked bottom) (3.12 MB, image/png)
2019-11-26 12:46 PST, Devin Rousso
no flags Details
[Image] After Patch is applied (docked bottom with console messages) (3.12 MB, image/png)
2019-11-26 12:46 PST, Devin Rousso
no flags Details
[Image] After Patch is applied (Network) (716.91 KB, image/png)
2019-11-26 12:46 PST, Devin Rousso
no flags Details
[Image] After Patch is applied (docked bottom with console messages) (3.16 MB, image/png)
2019-11-26 12:47 PST, Devin Rousso
no flags Details
Patch (329.42 KB, patch)
2019-11-26 14:57 PST, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (329.84 KB, patch)
2019-11-26 20:50 PST, Devin Rousso
no flags Details | Formatted Diff | Diff
[Image] After Patch is applied (undocked) (465.66 KB, image/png)
2019-11-26 20:50 PST, Devin Rousso
no flags Details
[Image] After Patch is applied (undocked with console messages) (484.51 KB, image/png)
2019-11-26 20:50 PST, Devin Rousso
no flags Details
Patch (329.85 KB, patch)
2019-12-02 23:10 PST, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (329.80 KB, patch)
2019-12-05 12:08 PST, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (342.89 KB, patch)
2019-12-06 17:17 PST, Devin Rousso
no flags Details | Formatted Diff | Diff
[Image] After Patch is applied (docked side) (1.80 MB, image/png)
2019-12-06 17:18 PST, Devin Rousso
no flags Details
[Image] After Patch is applied (docked side with window unfocused) (1.43 MB, image/png)
2019-12-06 17:18 PST, Devin Rousso
no flags Details
[Image] After Patch is applied (docked bottom) (2.64 MB, image/png)
2019-12-06 17:18 PST, Devin Rousso
no flags Details
[Image] After Patch is applied (docked bottom with window unfocused) (2.25 MB, image/png)
2019-12-06 17:18 PST, Devin Rousso
no flags Details
[Image] After Patch is applied (undocked) (499.19 KB, image/png)
2019-12-06 17:19 PST, Devin Rousso
no flags Details
[Image] After Patch is applied (undocked with console messages) (519.83 KB, image/png)
2019-12-06 17:19 PST, Devin Rousso
no flags Details
[Image] After Patch is applied (undocked with window unfocused) (334.30 KB, image/png)
2019-12-06 17:20 PST, Devin Rousso
no flags Details
[Image] After Patch is applied (undocked with window unfocused with console messages) (354.97 KB, image/png)
2019-12-06 17:20 PST, Devin Rousso
no flags Details
[Image] suggested/example icon stacking (14.20 KB, image/png)
2019-12-16 11:41 PST, esisk
no flags Details
Patch (354.12 KB, patch)
2020-01-30 17:28 PST, Devin Rousso
no flags Details | Formatted Diff | Diff
[Image] After Patch is applied (undocked) (777.06 KB, image/png)
2020-01-30 17:29 PST, Devin Rousso
no flags Details
[Image] After Patch is applied (undocked with console messages) (833.71 KB, image/png)
2020-01-30 17:29 PST, Devin Rousso
no flags Details
Patch (374.11 KB, patch)
2020-03-02 16:53 PST, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (379.83 KB, patch)
2020-03-02 17:01 PST, 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 Devin Rousso 2019-11-26 11:50:31 PST
Created attachment 384369 [details]
[Image] Screenshot of issue

The toolbar area takes up 36px height, which a non-insignificant amount of space, especially when undocked or docked bottom, especially if the toolbar is mostly empty space (see attached '[Image] Screenshot of issue').

On smaller screens, this is even more of a problem, as it's not possible to increase the vertical height of Web Inspector past a certain point, so those 36px become even more valuable.

Additionally, most tabs don't actually need that much space, and the icons will also automatically be hidden if the width of Web Inspector becomes narrow enough.  As such, we could move most of the items in the toolbar to be part of the tab bar, pinned to one edge.
Comment 1 Devin Rousso 2019-11-26 12:44:05 PST
Created attachment 384371 [details]
Patch
Comment 2 Devin Rousso 2019-11-26 12:44:28 PST
Created attachment 384372 [details]
[Image] After Patch is applied (undocked)
Comment 3 Devin Rousso 2019-11-26 12:44:50 PST
Created attachment 384373 [details]
[Image] After Patch is applied (undocked with console messages)
Comment 4 Devin Rousso 2019-11-26 12:45:20 PST
Created attachment 384374 [details]
[Image] After Patch is applied (docked side)
Comment 5 Devin Rousso 2019-11-26 12:45:43 PST
Created attachment 384375 [details]
[Image] After Patch is applied (docked side with console messages)
Comment 6 Devin Rousso 2019-11-26 12:46:11 PST
Created attachment 384376 [details]
[Image] After Patch is applied (docked bottom)
Comment 7 Devin Rousso 2019-11-26 12:46:27 PST Comment hidden (obsolete)
Comment 8 Devin Rousso 2019-11-26 12:46:46 PST
Created attachment 384378 [details]
[Image] After Patch is applied (Network)
Comment 9 Devin Rousso 2019-11-26 12:47:39 PST
Created attachment 384379 [details]
[Image] After Patch is applied (docked bottom with console messages)
Comment 10 Devin Rousso 2019-11-26 14:57:40 PST
Created attachment 384382 [details]
Patch

Minor ChangeLog modifications
Comment 11 Nikita Vasilyev 2019-11-26 16:50:03 PST
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?
Comment 12 Devin Rousso 2019-11-26 20:39:22 PST
(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.
Comment 13 Devin Rousso 2019-11-26 20:50:14 PST
Created attachment 384393 [details]
Patch
Comment 14 Devin Rousso 2019-11-26 20:50:44 PST
Created attachment 384394 [details]
[Image] After Patch is applied (undocked)
Comment 15 Devin Rousso 2019-11-26 20:50:58 PST
Created attachment 384395 [details]
[Image] After Patch is applied (undocked with console messages)
Comment 16 Devin Rousso 2019-12-02 23:10:17 PST
Created attachment 384693 [details]
Patch
Comment 17 Timothy Hatcher 2019-12-03 18:09:17 PST
Eh, I'm not a fan of the compact look. I'm not convinced that saving 36px is worth it.
Comment 18 Nikita Vasilyev 2019-12-04 17:34:53 PST
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 ๐Ÿ˜…)
Comment 19 Devin Rousso 2019-12-04 23:03:47 PST
(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.
Comment 20 Devin Rousso 2019-12-05 12:08:28 PST
Created attachment 384931 [details]
Patch

Switch the position of the download and reload buttons to match the existing order
Comment 21 Devin Rousso 2019-12-06 15:15:19 PST
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 22 Devin Rousso 2019-12-06 15:16:59 PST
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.
Comment 23 Devin Rousso 2019-12-06 17:17:33 PST
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`.
Comment 24 Devin Rousso 2019-12-06 17:18:06 PST
Created attachment 385062 [details]
[Image] After Patch is applied (docked side)
Comment 25 Devin Rousso 2019-12-06 17:18:24 PST
Created attachment 385063 [details]
[Image] After Patch is applied (docked side with window unfocused)
Comment 26 Devin Rousso 2019-12-06 17:18:42 PST
Created attachment 385064 [details]
[Image] After Patch is applied (docked bottom)
Comment 27 Devin Rousso 2019-12-06 17:18:58 PST
Created attachment 385065 [details]
[Image] After Patch is applied (docked bottom with window unfocused)
Comment 28 Devin Rousso 2019-12-06 17:19:32 PST
Created attachment 385066 [details]
[Image] After Patch is applied (undocked)
Comment 29 Devin Rousso 2019-12-06 17:19:48 PST
Created attachment 385067 [details]
[Image] After Patch is applied (undocked with console messages)
Comment 30 Devin Rousso 2019-12-06 17:20:08 PST
Created attachment 385068 [details]
[Image] After Patch is applied (undocked with window unfocused)
Comment 31 Devin Rousso 2019-12-06 17:20:25 PST
Created attachment 385069 [details]
[Image] After Patch is applied (undocked with window unfocused with console messages)
Comment 32 esisk 2019-12-16 11:41:57 PST
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.
Comment 33 Nikita Vasilyev 2019-12-16 16:01:45 PST
(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!
Comment 34 Devin Rousso 2019-12-17 15:24:15 PST
(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 :)
Comment 35 esisk 2019-12-17 15:27:36 PST
(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.
Comment 36 Timothy Hatcher 2019-12-18 10:36:06 PST
Stacking the icons is good.
Comment 37 Devin Rousso 2020-01-30 17:28:10 PST
Created attachment 389311 [details]
Patch
Comment 38 Devin Rousso 2020-01-30 17:29:00 PST
Created attachment 389313 [details]
[Image] After Patch is applied (undocked)
Comment 39 Devin Rousso 2020-01-30 17:29:20 PST
Created attachment 389314 [details]
[Image] After Patch is applied (undocked with console messages)
Comment 40 WebKit Commit Bot 2020-02-01 19:39:37 PST
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 41 WebKit Commit Bot 2020-02-01 19:40:38 PST
Comment on attachment 389311 [details]
Patch

Clearing flags on attachment: 389311

Committed r255547: <https://trac.webkit.org/changeset/255547>
Comment 42 WebKit Commit Bot 2020-02-01 19:40:41 PST
All reviewed patches have been landed.  Closing bug.
Comment 43 Radar WebKit Bug Importer 2020-02-01 19:41:24 PST
<rdar://problem/59091905>
Comment 44 Carlos Garcia Campos 2020-02-03 01:21:54 PST
It seems this broke several GTK API tests as the EWS detected. I'll investigate.
Comment 45 Carlos Garcia Campos 2020-02-03 02:29:10 PST
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).
Comment 46 Carlos Garcia Campos 2020-02-03 02:37:54 PST
Committed r255557: <https://trac.webkit.org/changeset/255557>
Comment 47 Carlos Garcia Campos 2020-02-03 03:05:21 PST
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 48 Devin Rousso 2020-02-03 09:37:40 PST
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 49 Devin Rousso 2020-02-04 14:31:20 PST
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
Comment 50 Jon Lee 2020-02-07 20:23:02 PST
Reopening. This was reverted by https://trac.webkit.org/changeset/256086/webkit in b207422.
Comment 51 Devin Rousso 2020-03-02 16:53:21 PST
Created attachment 392214 [details]
Patch

adjusted design based on offline feedback
Comment 52 Devin Rousso 2020-03-02 17:01:40 PST
Created attachment 392217 [details]
Patch

rebase
Comment 53 WebKit Commit Bot 2020-03-02 19:22:08 PST
Comment on attachment 392217 [details]
Patch

Clearing flags on attachment: 392217

Committed r257759: <https://trac.webkit.org/changeset/257759>
Comment 54 WebKit Commit Bot 2020-03-02 19:22:11 PST
All reviewed patches have been landed.  Closing bug.
Comment 55 Nikita Vasilyev 2020-03-10 19:11:06 PDT
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 56 Devin Rousso 2020-03-16 17:04:03 PDT
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.