Bug 178286 – Web Inspector: [PARITY] Styles Redesign: clicking on the go-to arrow in Computed tab should work

• Component: Web Inspector
• Status: RESOLVED FIXED

[Nikita Vasilyev]

Clicking on the go-to arrow in Computed tab should switch to "Styles — Rules" panel and highlight the corresponding property,
similarly to the old styles sidebar.

[Radar WebKit Bug Importer]

<rdar://problem/34986379>

[Nikita Vasilyev]

Created attachment 323818 [details]
Patch

[Nikita Vasilyev]

Created attachment 323819 [details]
[Animated GIF] With patch applied

[Joseph Pecoraro]

Comment on attachment 323818 [details]
Patch

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

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationEditor.css:101
> +@keyframes style-property-highlight {
> +    from { background-color: yellow; }
> +}

Shouldn't this have some kind of `to`?

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationSection.js:110
> +        if (!this.didInitialLayout)
> +            this.updateLayout();

This seems peculiar. You should have a comment describing why we force a layout here. It seems you could do the same "defer until layout the property to highlight", but given this is a user action this probably isn't too bad.

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:45
> +        this._property.__view = this;

__view is already used elsewhere. Every `Views/*View.js` sets __view. So I suggest using a different name to avoid potential conflicts. Maybe __spreadsheetStyleProperty or __propertyView.

[Nikita Vasilyev]

Comment on attachment 323818 [details]
Patch

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

>> Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationEditor.css:101
>> +}
> 
> Shouldn't this have some kind of `to`?

I could add

    to { background-color: transparent; }

but it works just fine without "to"! In the old styles sidebar we omitted "to" as well, see `@keyframes text-editor-highlight-fadeout`.

[Nikita Vasilyev]

Created attachment 323854 [details]
Patch

Joe: you left cq-. Does it mean that I shouldn't cq+ the updated patch myself and wait for you to take another look?

[Joseph Pecoraro]

(In reply to Nikita Vasilyev from comment #6)
> Created attachment 323854 [details]
> Patch
> 
> Joe: you left cq-. Does it mean that I shouldn't cq+ the updated patch
> myself and wait for you to take another look?

cq- just means don't land the previous patch as is. If you have addressed the comments in the new patch you can cq+ or land the new patch given it has addressed the comments.

[WebKit Commit Bot]

Comment on attachment 323854 [details]
Patch

Clearing flags on attachment: 323854

Committed r223333: <https://trac.webkit.org/changeset/223333>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.