[webkit-reviews] review denied: [Bug 178489] Web Inspector: Styles Redesign: Editing selector should not hide the rule : [Attachment 335759] Patch

[bugzilla-daemon at webkit.org]

Matt Baker <mattbaker at apple.com> has denied Devin Rousso
<webkit at devinrousso.com>'s request for review:
Bug 178489: Web Inspector: Styles Redesign: Editing selector should not hide
the rule
https://bugs.webkit.org/show_bug.cgi?id=178489

Attachment 335759: Patch

https://bugs.webkit.org/attachment.cgi?id=335759&action=review




--- Comment #9 from Matt Baker <mattbaker at apple.com> ---
Comment on attachment 335759
  --> https://bugs.webkit.org/attachment.cgi?id=335759
Patch

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

Overall this looks good. r- only because of the following issue encountered
when testing:

Changes to the selector are lost when committing with Tab or Enter.

Steps to Reproduce:
1. Inspect:
<style>
    p { color: blue; }
</style>
<p>Some text.</p>

2. Elements tab > select the <p> DOM node
3. Styles sidebar > click on the "p" selector, change to "p.foo".
4. Hit enter or tab.

Expected:
  => Selector shows "p.foo", first property name becomes selected.

Actual:
  => Selector reverts back to "p".

Note:
Repeating the steps a second time works. The selector shows "p.foo" and is
grayed out.

> Source/WebInspectorUI/ChangeLog:10
> +	   (SpreadsheetRulesStyleDetailsPanel) who keeps track of the focused
section. When a refresh

who -> which

>
Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationSection
.js:105
> +	       this.element.addEventListener("click",
this._handleClick.bind(this));

This line doesn't need to move, it just causes unnecessary code churn.

>
Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationSection
.js:269
> +	       var usingCustomSelectors = selectors !==
this._style.ownerRule.selectors;

This should move into the `if (selectors.length) { ... }` block below, this
that is the only place it's used.

>
Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationSection
.js:427
> +

Remove extra newline.

>
Source/WebInspectorUI/UserInterface/Views/SpreadsheetRulesStyleDetailsPanel.js:
71
> +	   let uniqueOrderedStyles = (styles) => {

Is there a good reason for renaming this? Seems like unnecessary churn for no
benefit.