Bug 198508

Summary:

REGRESSION(r244268): Web Inspector: Styles: navigating from Computed to Styles doesn't work

Product:

WebKit

Reporter:

Nikita Vasilyev <nvasilyev>

Component:

Web Inspector

Assignee:

Nikita Vasilyev <nvasilyev>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

commit-queue, inspector-bugzilla-changes, mattbaker, webkit-bug-importer

Priority:

Keywords:

InRadar

Version:

WebKit Nightly Build

Hardware:

All

OS:

All

Attachments:

Description	Flags
Patch	none
Patch	none

Description Nikita Vasilyev 2019-06-03 17:22:20 PDT

Steps:
1. Open https://webkit.org
2. Inspect <body>
3. Open Computed panel in the right sidebar
4. Hover "background-color: rgb(0, 37, 61)"
5. Click on the arrow icon on the right.

Expected:
Styles panel open with "background-color" property selected.

Actual:
Styles panel open but no properties are selected.

Notes:
This must have regressed in the last 2 months.

Comment 1 Radar WebKit Bug Importer 2019-06-03 17:22:38 PDT

<rdar://problem/51375503>

Comment 2 Nikita Vasilyev 2019-06-03 17:34:57 PDT

Broken in r244268 Web Inspector: sidebar panels shouldn't be added as subviews unless visible

Comment 3 Nikita Vasilyev 2019-06-03 17:41:54 PDT

After clicking the arrow icon:

[Error] TypeError: null is not an object (evaluating 'this.parentSidebar.sidebarPanels')
	computedStyleDetailsPanelShowProperty (GeneralStyleDetailsSidebarPanel.js:75)
	spreadsheetCSSStyleDeclarationEditorShowProperty (ComputedStyleDetailsPanel.js:78)
	spreadsheetStylePropertyShowProperty (ComputedStyleSection.js:207)
	(anonymous function) (SpreadsheetStyleProperty.js:227)

Comment 4 Nikita Vasilyev 2019-06-08 17:31:58 PDT

Created attachment 371687 [details]
Patch

Comment 5 Matt Baker 2019-06-10 12:37:26 PDT

Comment on attachment 371687 [details]
Patch

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

> Source/WebInspectorUI/UserInterface/Views/GeneralStyleDetailsSidebarPanel.js:72
> +        let parentSidebar = this.parentSidebar;

Instead of saving a reference, why not reorder the operations in the function body?

computedStyleDetailsPanelShowProperty(property)
{
    let styleRulesPanel = this.parentSidebar.sidebarPanels.find((panel) => panel instanceof WI.RulesStyleDetailsSidebarPanel);
    console.assert(styleRulesPanel, "Styles panel is missing.");
    if (!styleRulesPanel)
        return;

    this.parentSidebar.selectedSidebarPanel = styleRulesPanel;
    styleRulesPanel.panel.scrollToSectionAndHighlightProperty(property);
}

Also, this delegate method seems like it belongs in ComputedStyleDetailsPanel, not in a generic base class.

Comment 6 Nikita Vasilyev 2019-06-10 13:00:55 PDT

(In reply to Matt Baker from comment #5)
> Comment on attachment 371687 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=371687&action=review
> 
> > Source/WebInspectorUI/UserInterface/Views/GeneralStyleDetailsSidebarPanel.js:72
> > +        let parentSidebar = this.parentSidebar;
> 
> Instead of saving a reference, why not reorder the operations in the
> function body?

Makes sense. Also, I like how you didn't use a string identifier.

Comment 7 Nikita Vasilyev 2019-06-10 13:07:31 PDT

Created attachment 371772 [details]
Patch

Comment 8 Matt Baker 2019-06-10 13:28:12 PDT

Comment on attachment 371772 [details]
Patch

r=me

Comment 9 WebKit Commit Bot 2019-06-10 14:14:01 PDT

Comment on attachment 371772 [details]
Patch

Clearing flags on attachment: 371772

Committed r246279: <https://trac.webkit.org/changeset/246279>

Comment 10 WebKit Commit Bot 2019-06-10 14:14:03 PDT

All reviewed patches have been landed.  Closing bug.