Bug 178489 – Web Inspector: Styles Redesign: Editing selector should not hide the rule

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector

[Nikita Vasilyev]

When selector is modified and no longer matches the selected element, it should stay on screen until the new element is selected.

This is a new enhancement; we don't do it in the old styles sidebar.

[Radar WebKit Bug Importer]

<rdar://problem/35062434>

[Devin Rousso]

Created attachment 329468 [details]
Patch

[Joseph Pecoraro]

Comment on attachment 329468 [details]
Patch

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

> Source/WebInspectorUI/ChangeLog:14
> +        Drive-by fix: fix tabbing so that it will wrap to the first/last rule.

I recommend splitting this into its own change.

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationSection.js:217
> -    _renderSelector()
> +    _renderSelector(selectors)

Should this be renamed "_renderSelectors" plural?

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetRulesStyleDetailsPanel.js:237
> -                break;
> +                index = this._sections.length - 1;

What is up here? Seems like we could get into an infinite loop if all sections are not editable.

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetRulesStyleDetailsPanel.js:284
> +    _focusedSectionDoesNotMatch()

This reads poorly. Does not match what?

[Devin Rousso]

Comment on attachment 329468 [details]
Patch

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

>> Source/WebInspectorUI/UserInterface/Views/SpreadsheetRulesStyleDetailsPanel.js:237
>> +                index = this._sections.length - 1;
> 
> What is up here? Seems like we could get into an infinite loop if all sections are not editable.

This could get into an infinite loop, but I don't think it's possible since if every section is not editable then none of the section switching code would ever be reachable.  We only call these functions when the section's selector or property editor is selected, meaning that if it's not editable then we don't have any keydown listeners.

>> Source/WebInspectorUI/UserInterface/Views/SpreadsheetRulesStyleDetailsPanel.js:284
>> +    _focusedSectionDoesNotMatch()
> 
> This reads poorly. Does not match what?

The idea is to return false if the section would not re-appear if we were to regenerate the list of sections based on the DOMNodeStyles.  I had a hard time naming this :P

[Nikita Vasilyev]

Devin, could you rebaseline the patch? It no longer applies to WebKit ToT. I'd like to get this landed soon.

[Devin Rousso]

Created attachment 335759 [details]
Patch

[EWS Watchlist]

Comment on attachment 335759 [details]
Patch

Attachment 335759 [details] did not pass win-ews (win):
Output: http://webkit-queues.webkit.org/results/6947382

New failing tests:
http/tests/preload/download_resources.html

[EWS Watchlist]

Created attachment 335764 [details]
Archive of layout-test-results from ews205 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews205  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit

[Matt Baker]

Comment on attachment 335759 [details]
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.

[Matt Baker]

Comment on attachment 329468 [details]
Patch

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

>> Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationSection.js:217
>> +    _renderSelector(selectors)
> 
> Should this be renamed "_renderSelectors" plural?

We're just rendering a single selector, but it is a little confusing because of the argument's name.

[Devin Rousso]

Created attachment 340634 [details]
Patch

This doesn\'t quite work yet, but it's close.  There seems to be an issue with how `significantChange` is calculated, since `WI.DOMNodeStyles` seems to think that the changing of the selector warrants that, even though it probably shouldn\'t.

[EWS Watchlist]

Comment on attachment 340634 [details]
Patch

Attachment 340634 [details] did not pass win-ews (win):
Output: http://webkit-queues.webkit.org/results/7718582

New failing tests:
http/tests/security/canvas-remote-read-remote-video-redirect.html

[EWS Watchlist]

Created attachment 340679 [details]
Archive of layout-test-results from ews206 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews206  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit

[Devin Rousso]

Created attachment 356749 [details]
Patch

Trying a different approach.  The patch could probably be cleaned up a bit, but I wanted to get everyone's opinion on this approach before really committing to it.

[Matt Baker]

Comment on attachment 356749 [details]
Patch

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

Seems to work, but It's hard to tell which changes are essential to fixing the problem. Could you drop the unnecessary refactoring and add a change log comment or two? Doesn't have to be long, just a sentence or two explaining the approach you took.

> Source/WebInspectorUI/UserInterface/Models/CSSRule.js:59
> +    update(sourceCodeLocation, selectorText, selectors, matchedSelectorIndices, style, mediaList)

Why was the flag dropped?

[Devin Rousso]

Comment on attachment 356749 [details]
Patch

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

>> Source/WebInspectorUI/UserInterface/Models/CSSRule.js:59
>> +    update(sourceCodeLocation, selectorText, selectors, matchedSelectorIndices, style, mediaList)
> 
> Why was the flag dropped?

It's no longer needed, as the only use of it was to prevent `WI.CSSRule.Event.Changed` from being fired.  There were no listeners for that event, so it felt fine to remove.

[Devin Rousso]

Created attachment 357995 [details]
Patch

[Matt Baker]

(In reply to Devin Rousso from comment #16)
> Comment on attachment 356749 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=356749&action=review
> 
> >> Source/WebInspectorUI/UserInterface/Models/CSSRule.js:59
> >> +    update(sourceCodeLocation, selectorText, selectors, matchedSelectorIndices, style, mediaList)
> > 
> > Why was the flag dropped?
> 
> It's no longer needed, as the only use of it was to prevent
> `WI.CSSRule.Event.Changed` from being fired.  There were no listeners for
> that event, so it felt fine to remove.

Okay! That kind of thing is pretty hard to determine in a code review.

[Matt Baker]

Comment on attachment 357995 [details]
Patch

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

Looks good! I'd feel better if Nikita looked over this too since he works on this code a lot.

> Source/WebInspectorUI/UserInterface/Models/DOMNodeStyles.js:54
> +    static parseSelectorListPayload(selectorList)

Why was this made static? It is only used within DOMNodeStyles. Is it going to be reused, or is it purely because it wasn't referencing `this`?

> Source/WebInspectorUI/UserInterface/Models/DOMNodeStyles.js:73
> +    static createSourceCodeLocation(sourceURL, {line, column, documentNode} = {})

Why was this made static? Is it going to be used outside this class? Unlike the other method, making this static required changing the signature. Both these changes feel like unnecessary churn for no benefit.

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetRulesStyleDetailsPanel.js:213
> +

Style: I'd drop this empty line, and add one...

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetRulesStyleDetailsPanel.js:222
> +        });

...here! IMO it makes this section easier to read.

[Devin Rousso]

Comment on attachment 357995 [details]
Patch

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

>> Source/WebInspectorUI/UserInterface/Models/DOMNodeStyles.js:54
>> +    static parseSelectorListPayload(selectorList)
> 
> Why was this made static? It is only used within DOMNodeStyles. Is it going to be reused, or is it purely because it wasn't referencing `this`?

This is also used by `WI.CSSRule.prototype._selectorResolved`.

>> Source/WebInspectorUI/UserInterface/Models/DOMNodeStyles.js:73
>> +    static createSourceCodeLocation(sourceURL, {line, column, documentNode} = {})
> 
> Why was this made static? Is it going to be used outside this class? Unlike the other method, making this static required changing the signature. Both these changes feel like unnecessary churn for no benefit.

It's used by `WI.CSSRule.prototype._selectorResolved`.

[Devin Rousso]

Comment on attachment 357995 [details]
Patch

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

>> Source/WebInspectorUI/UserInterface/Models/DOMNodeStyles.js:54
>> +    static parseSelectorListPayload(selectorList)
> 
> Why was this made static? It is only used within DOMNodeStyles. Is it going to be reused, or is it purely because it wasn't referencing `this`?

This is also used by `WI.CSSRule.prototype._selectorResolved`.

>> Source/WebInspectorUI/UserInterface/Models/DOMNodeStyles.js:73
>> +    static createSourceCodeLocation(sourceURL, {line, column, documentNode} = {})
> 
> Why was this made static? Is it going to be used outside this class? Unlike the other method, making this static required changing the signature. Both these changes feel like unnecessary churn for no benefit.

It's used by `WI.CSSRule.prototype._selectorResolved`.

[Nikita Vasilyev]

Created attachment 358002 [details]
[Video] Bug (attachment 357995 [details])

r- because it creates section duplicates.

[Devin Rousso]

Created attachment 358003 [details]
Patch

When a selector is changed back to something that applies to the current node, `WI.DOMNodeStyles` will create a new `WI.CSSStyleDeclaration` for that style, but the id will remain the same as the old style.  Don\'t add any sections if there already exists a section that has the same id.

[Matt Baker]

(In reply to Devin Rousso from comment #23)
> Created attachment 358003 [details]
> Patch
> 
> When a selector is changed back to something that applies to the current
> node, `WI.DOMNodeStyles` will create a new `WI.CSSStyleDeclaration` for that
> style, but the id will remain the same as the old style.  Don\'t add any
> sections if there already exists a section that has the same id.

It makes sense that the selector applies to the current node, and so a declaration is created. I don't follow why there is another section though.

[Devin Rousso]

(In reply to Matt Baker from comment #24)
> It makes sense that the selector applies to the current node, and so a declaration is created. I don't follow why there is another section though.
Two sections appear because we’ve saved them inside `preservedSections`.  One of them comes from the newly created section for the newly created store after the refresh, and the other comes from the section we’ve kept around in the preserved list.

[Matt Baker]

(In reply to Devin Rousso from comment #25)
> (In reply to Matt Baker from comment #24)
> > It makes sense that the selector applies to the current node, and so a declaration is created. I don't follow why there is another section though.
> Two sections appear because we’ve saved them inside `preservedSections`. 
> One of them comes from the newly created section for the newly created store
> after the refresh, and the other comes from the section we’ve kept around in
> the preserved list.

I understand there is a technical rationale for the behavior, but it is unexpected and feels like a bug.

[Devin Rousso]

Created attachment 360915 [details]
Patch

[Timothy Hatcher]

Comment on attachment 360915 [details]
Patch

Does this latest patch fix the duplicate section issue?

[Devin Rousso]

Created attachment 365415 [details]
Patch

The previous patch fixed the "double section" issue.  This update fixes an issue with pseudo-element styles using the same key in the maps on each `WI.DOMNodeStyles`.

[Timothy Hatcher]

Comment on attachment 365415 [details]
Patch

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

> Source/WebInspectorUI/UserInterface/Models/DOMNodeStyles.js:80
> +         // Try to use the node to find the frame which has the correct resource first.

Bad indentation.

> Source/WebInspectorUI/UserInterface/Models/DOMNodeStyles.js:89
> +         // If that didn't find the resource, then search all frames.

Ditto.

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetRulesStyleDetailsPanel.js:366
> +        section.__showingForNode = this.nodeStyles.node;
> +        section.__index = this._sections.indexOf(section);

Should change these to symbols so they are only visible in this file.

[Devin Rousso]

Created attachment 365434 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 365434 [details]
Patch

Rejecting attachment 365434 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-02', 'validate-changelog', '--check-oops', '--non-interactive', 365434, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

ChangeLog entry in LayoutTests/ChangeLog contains OOPS!.

Full output: https://webkit-queues.webkit.org/results/11587525

[Devin Rousso]

Created attachment 365436 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 365436 [details]
Patch

Clearing flags on attachment: 365436

Committed r243264: <https://trac.webkit.org/changeset/243264>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.