Bug 222990 – Web Inspector: `Styles` sidebar pseudo-class checkboxes appear cramped after resizing window at narrow widths

• Component: Web Inspector
• Status: RESOLVED FIXED

[Patrick Angle]

Created attachment 422755 [details]
Video of Issue

Description:
The `Styles` sidebar can become narrower than intended (down to the minimum width of a sidebar, 250px) when resizing the window.

Steps to reproduce:
1. Inspect `webkit.org` with an inspector wide enough to show the `Styles` sidebar as an independent sidebar.
2. Size the window down until the sidebars become a single sidebar due to lack of space.
3. Size the window back up until the sidebars split again.

Result:
The `Styles` sidebar will be narrower than the pseudo-element checkboxes + their padding.

[Radar WebKit Bug Importer]

<rdar://problem/75270368>

[Patrick Angle]

Created attachment 423009 [details]
Video of Patch v1.0-1.1 (With Exaggerated Checkbox Labels)

[Patrick Angle]

Created attachment 423010 [details]
Patch v1.0

[Devin Rousso]

Comment on attachment 423010 [details]
Patch v1.0

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

> Source/WebInspectorUI/UserInterface/Views/MultiSidebar.js:177
> +        if (!flag && this.isAttached)

Please use `this.collapsed` instead as that has the fallback `|| false`.

Also, I think `flag` should be removed altogether because the callsite doesn't provide any arguments 😅

> Source/WebInspectorUI/UserInterface/Views/MultiSidebar.js:208
> +        if (!this._requiredMinimumWidthForMultipleSidebars && this.isAttached)

Rather than have this here, I think it may be worth inverting this and moving it to `_updateMinimumWidthForMultipleSidebars` (and probably add a `force` parameter that will overrule `!this._requiredMinimumWidthForMultipleSidebars`) so that we don't try to set `_requiredMinimumWidthForMultipleSidebars` until we know we're ready.

> Source/WebInspectorUI/UserInterface/Views/SingleSidebar.js:91
> +        if (!this.selectedSidebarPanel)
> +            this.selectedSidebarPanel = sidebarPanel;

this.selectedSidebarPanel ||= sidebarPanel;

> Source/WebInspectorUI/UserInterface/Views/SingleSidebar.js:105
> +            if (!this.collapsed)

Why?  I would consider it a bug to try to `set selectedSidebarPanel` while `collapsed`.

> Source/WebInspectorUI/UserInterface/Views/SingleSidebar.js:115
> +            if (!this.collapsed)

ditto (:105)

[Patrick Angle]

Comment on attachment 423010 [details]
Patch v1.0

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

>> Source/WebInspectorUI/UserInterface/Views/MultiSidebar.js:177
>> +        if (!flag && this.isAttached)
> 
> Please use `this.collapsed` instead as that has the fallback `|| false`.
> 
> Also, I think `flag` should be removed altogether because the callsite doesn't provide any arguments 😅

Based on you comment below, this check should probably just be a part of `_updateMinimumWidthForMultipleSidebars` as well.

>> Source/WebInspectorUI/UserInterface/Views/MultiSidebar.js:208
>> +        if (!this._requiredMinimumWidthForMultipleSidebars && this.isAttached)
> 
> Rather than have this here, I think it may be worth inverting this and moving it to `_updateMinimumWidthForMultipleSidebars` (and probably add a `force` parameter that will overrule `!this._requiredMinimumWidthForMultipleSidebars`) so that we don't try to set `_requiredMinimumWidthForMultipleSidebars` until we know we're ready.

Sounds reasonable to me.

>> Source/WebInspectorUI/UserInterface/Views/SingleSidebar.js:91
>> +            this.selectedSidebarPanel = sidebarPanel;
> 
> this.selectedSidebarPanel ||= sidebarPanel;

How quickly I forget after spending so long working in the backend 😅

>> Source/WebInspectorUI/UserInterface/Views/SingleSidebar.js:105
>> +            if (!this.collapsed)
> 
> Why?  I would consider it a bug to try to `set selectedSidebarPanel` while `collapsed`.

I need to dive deeper on this. I’m not sure I understand the ramifications of r270134 on this logic, but without this check it was possible to attempt to add the sub view twice. It seems as if we should be able to set a selected panel before uncollapsing a sidebar, though. Otherwise we need to wait until the sidebar is showing to set the selected panel. I’ll look tomorrow.

[Patrick Angle]

Comment on attachment 423010 [details]
Patch v1.0

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

>>> Source/WebInspectorUI/UserInterface/Views/SingleSidebar.js:105
>>> +            if (!this.collapsed)
>> 
>> Why?  I would consider it a bug to try to `set selectedSidebarPanel` while `collapsed`.
> 
> I need to dive deeper on this. I’m not sure I understand the ramifications of r270134 on this logic, but without this check it was possible to attempt to add the sub view twice. It seems as if we should be able to set a selected panel before uncollapsing a sidebar, though. Otherwise we need to wait until the sidebar is showing to set the selected panel. I’ll look tomorrow.

There are a few places where a selectedSidebarPanel will be set regardless of the collapsed state of said sidebar:

• `ContentBrowserTabContentView.__prototype__.showNavigationSidebarPanel::140-149` - Last user setting  is honored for collapsed state, and then the only panel is set as the selected panel.
• `ContentBrowserTabContentView.__prototype__.showDetailsSidebarPanels::195-203` - Last user setting  is honored for collapsed state, and then a panel is selected based on the last panel the user selected.
• `MultiSidebar.__prototype__._makeSidebarPanelExclusive` - Will need to set a selected panel as well, either transitively through addSidebarPanel or explicitly. Since this is the case I'm needing to fix, I'm going to just reorder here and make sure the panel is set as selected after un-collapsing it.

[Patrick Angle]

Created attachment 423058 [details]
Patch v1.1 - Revised setting selected panel for exclusive SingleSidebars

[Devin Rousso]

Comment on attachment 423058 [details]
Patch v1.1 - Revised setting selected panel for exclusive SingleSidebars

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

r=me

> Source/WebInspectorUI/UserInterface/Views/MultiSidebar.js:189
> +    _updateMinimumWidthForMultipleSidebars({ignoreExistingComputedValue} = {})

NIT: While I do appreciate you using an object for options like this, personally for things that are inside the same class (and only have one argument) I'd just make this a `force` parameter that's not in an object.  Nothing wrong with how you have it here tho so your call :)

> Source/WebInspectorUI/UserInterface/Views/MultiSidebar.js:256
>          sidebar.selectedSidebarPanel = sidebarPanel;

Remove this?

[Patrick Angle]

Comment on attachment 423058 [details]
Patch v1.1 - Revised setting selected panel for exclusive SingleSidebars

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

>> Source/WebInspectorUI/UserInterface/Views/MultiSidebar.js:189
>> +    _updateMinimumWidthForMultipleSidebars({ignoreExistingComputedValue} = {})
> 
> NIT: While I do appreciate you using an object for options like this, personally for things that are inside the same class (and only have one argument) I'd just make this a `force` parameter that's not in an object.  Nothing wrong with how you have it here tho so your call :)

I went back and forth on this myself, but erred on the side of being slightly more verbose so it was clear at the callsites what was being configured, although I can appreciate that the answer in this case would be less than 50 lines below each call site.

>> Source/WebInspectorUI/UserInterface/Views/MultiSidebar.js:256
>>          sidebar.selectedSidebarPanel = sidebarPanel;
> 
> Remove this?

Oh, yep...

[Patrick Angle]

Created attachment 423295 [details]
Patch v1.2 - Review notes

[EWS]

Committed r274465: <https://commits.webkit.org/r274465>

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