Bug 224774 – REGRESSION (r268691 && r270134): Web Inspector: Clicking on go-to arrow in Computed panel no longer works

• Component: Web Inspector
• Status: RESOLVED FIXED

[Nikita Vasilyev]

It supposed to navigate to the corresponding style in the Styles. Now it doesn't do anything.

[Error] Assertion Failed: Styles panel is missing.
	computedStyleDetailsPanelShowProperty (ComputedStyleDetailsSidebarPanel.js:38)
	spreadsheetCSSStyleDeclarationEditorShowProperty (ComputedStyleDetailsPanel.js:85)
	spreadsheetStylePropertyShowProperty (ComputedStyleSection.js:249)
	(anonymous function) (SpreadsheetStyleProperty.js:228)

[Radar WebKit Bug Importer]

<rdar://problem/76854775>

[Patrick Angle]

This issue occurs when the Style panel is shown independently. When the Style panel is collapsed into the Details sidebar due to lack of space, this still works.

Most likely the logic here doesn't know how to find the Style panel when it is part of a different Sidebar.

[Nikita Vasilyev]

Created attachment 426452 [details]
[Video] Doesn't select CSS property

Note that when "Show independent styles sidebar" is unchecked, clicking on the go-to arrow open the Styles sidebar but doesn't focus on the CSS property. I haven't checked if this regressed in r268691 or not (if it did, it could be a separate bug/patch).

[Patrick Angle]

(In reply to Nikita Vasilyev from comment #3)
> Note that when "Show independent styles sidebar" is unchecked, clicking on
> the go-to arrow open the Styles sidebar but doesn't focus on the CSS
> property. I haven't checked if this regressed in r268691 or not (if it did,
> it could be a separate bug/patch).

Just checked, the highlighting regressed somewhere else.

[Patrick Angle]

The second half of this behavior was regressed in r270134 due to a missed occurrence of `this._visible` (which should not have been accessed here at all as it was a private member of the superclass) in `WI.SpreadsheetRulesStyleDetailsPanel`.

[Patrick Angle]

Created attachment 426478 [details]
Patch v1.0

[Devin Rousso]

Comment on attachment 426478 [details]
Patch v1.0

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

> Source/WebInspectorUI/UserInterface/Views/ComputedStyleDetailsSidebarPanel.js:42
> +                styleRulesSidebar = styleRulesSidebar.parentView;

This kinda sorta feels like a layering violation :/

Is the issue here that the `WI.RulesStyleDetailsSidebarPanel` isn't kept inside the `sidebarPanels` of a `WI.MultiSidebar`?  I feel like the `WI.MultiSidebar.prototype.get sidebarPanels` should return the list of _all_ sidebar panels across _all_ of its child/managed `WI.SingleSidebar`.

[Patrick Angle]

Comment on attachment 426478 [details]
Patch v1.0

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

>> Source/WebInspectorUI/UserInterface/Views/ComputedStyleDetailsSidebarPanel.js:42
>> +                styleRulesSidebar = styleRulesSidebar.parentView;
> 
> This kinda sorta feels like a layering violation :/
> 
> Is the issue here that the `WI.RulesStyleDetailsSidebarPanel` isn't kept inside the `sidebarPanels` of a `WI.MultiSidebar`?  I feel like the `WI.MultiSidebar.prototype.get sidebarPanels` should return the list of _all_ sidebar panels across _all_ of its child/managed `WI.SingleSidebar`.

The issue is the `parentSidebar` is a SingleSidebar, which is a child of the MultiSidebar.

[Devin Rousso]

Comment on attachment 426478 [details]
Patch v1.0

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

> Source/WebInspectorUI/UserInterface/Views/ComputedStyleDetailsSidebarPanel.js:43
> +        }

Can we just use `WI.detailsSidebar` then?  Reaching into the `WI.View` hierarchy of a different class seems like a place where issues (likely undiscovered) could arise.  It's also a bit presumptuous IMO to expect the `WI.RulesStyleDetailsSidebarPanel` to be in the same `WI.Sidebar` as this panel (as evidenced by this bug).

[Patrick Angle]

Comment on attachment 426478 [details]
Patch v1.0

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

>> Source/WebInspectorUI/UserInterface/Views/ComputedStyleDetailsSidebarPanel.js:43
>> +        }
> 
> Can we just use `WI.detailsSidebar` then?  Reaching into the `WI.View` hierarchy of a different class seems like a place where issues (likely undiscovered) could arise.  It's also a bit presumptuous IMO to expect the `WI.RulesStyleDetailsSidebarPanel` to be in the same `WI.Sidebar` as this panel (as evidenced by this bug).

That would certainly work. Sounds good to me.

[Patrick Angle]

Created attachment 426488 [details]
Patch v1.1 - Revised approach

[Devin Rousso]

Comment on attachment 426488 [details]
Patch v1.1 - Revised approach

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

r=me, nice fix :)

> Source/WebInspectorUI/ChangeLog:9
> +        The Style panel will not always be part of the same sidebar as the Computed panel as of r268691, so we should
> +        look for the Style panel in WI.detailsSidebar, which will report all of the panels it manages across multiple

NIT: "Styles panel"

> Source/WebInspectorUI/UserInterface/Views/ComputedStyleDetailsSidebarPanel.js:37
> -        let styleRulesPanel = this.parentSidebar.sidebarPanels.find((panel) => panel instanceof WI.RulesStyleDetailsSidebarPanel);
> +        let styleRulesPanel = WI.detailsSidebar.sidebarPanels.find((panel) => panel instanceof WI.RulesStyleDetailsSidebarPanel);

As a note for future readers, the reason why I don't see this as a layering violation is because this is the `WI.ComputedStyle*DETAILS*SidebarPanel`, so it's reasonable for a panel in the details sidebar to do things to the `WI.detailsSidebar`.

Ideally we'd have a way to get to the related `WI.TabBrowser.prototype.get detailsSidebar` instead of assuming the global singleton (or doing as @Patrick Angle suggested offline and having a `get topLevelParentSidebar`), but in the interest of getting this fixed I think it's fine for now.

[Patrick Angle]

Created attachment 426492 [details]
Patch v1.2 - Fix typos in changelog

[EWS]

Committed r276278 (236760@main): <https://commits.webkit.org/236760@main>

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