Bug 178489 - Web Inspector: Styles Redesign: Editing selector should not hide the rule
Summary: Web Inspector: Styles Redesign: Editing selector should not hide the rule
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Devin Rousso
URL:
Keywords: InRadar
Depends on:
Blocks: 177471
  Show dependency treegraph
 
Reported: 2017-10-18 15:37 PDT by Nikita Vasilyev
Modified: 2019-03-20 17:22 PDT (History)
8 users (show)

See Also:


Attachments
Patch (15.80 KB, patch)
2017-12-15 00:01 PST, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (12.23 KB, patch)
2018-03-13 22:24 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews205 for win-future (12.02 MB, application/zip)
2018-03-14 00:16 PDT, Build Bot
no flags Details
Patch (11.65 KB, patch)
2018-05-17 13:55 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews206 for win-future (12.75 MB, application/zip)
2018-05-17 22:00 PDT, Build Bot
no flags Details
Patch (43.23 KB, patch)
2018-12-06 13:43 PST, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (20.75 KB, patch)
2018-12-21 15:54 PST, Devin Rousso
no flags Details | Formatted Diff | Diff
[Video] Bug (attachment 357995) (4.66 MB, video/quicktime)
2018-12-21 16:30 PST, Nikita Vasilyev
no flags Details
Patch (20.95 KB, patch)
2018-12-21 17:01 PST, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (46.85 KB, patch)
2019-02-01 15:43 PST, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (53.90 KB, patch)
2019-03-20 15:38 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (54.81 KB, patch)
2019-03-20 16:40 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (54.81 KB, patch)
2019-03-20 16:47 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Nikita Vasilyev 2017-10-18 15:37:36 PDT
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.
Comment 1 Radar WebKit Bug Importer 2017-10-18 15:37:46 PDT
<rdar://problem/35062434>
Comment 2 Devin Rousso 2017-12-15 00:01:43 PST
Created attachment 329468 [details]
Patch
Comment 3 Joseph Pecoraro 2018-01-17 11:21:43 PST
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?
Comment 4 Devin Rousso 2018-01-17 22:30:39 PST
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
Comment 5 Nikita Vasilyev 2018-03-10 16:52:32 PST
Devin, could you rebaseline the patch? It no longer applies to WebKit ToT. I'd like to get this landed soon.
Comment 6 Devin Rousso 2018-03-13 22:24:22 PDT
Created attachment 335759 [details]
Patch
Comment 7 Build Bot 2018-03-14 00:16:25 PDT Comment hidden (obsolete)
Comment 8 Build Bot 2018-03-14 00:16:37 PDT Comment hidden (obsolete)
Comment 9 Matt Baker 2018-05-04 15:59:51 PDT
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.
Comment 10 Matt Baker 2018-05-04 16:05:11 PDT
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.
Comment 11 Devin Rousso 2018-05-17 13:55:33 PDT
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.
Comment 12 Build Bot 2018-05-17 22:00:09 PDT Comment hidden (obsolete)
Comment 13 Build Bot 2018-05-17 22:00:20 PDT Comment hidden (obsolete)
Comment 14 Devin Rousso 2018-12-06 13:43:37 PST
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.
Comment 15 Matt Baker 2018-12-21 15:36:25 PST
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?
Comment 16 Devin Rousso 2018-12-21 15:42:43 PST
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.
Comment 17 Devin Rousso 2018-12-21 15:54:33 PST
Created attachment 357995 [details]
Patch
Comment 18 Matt Baker 2018-12-21 15:55:28 PST
(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.
Comment 19 Matt Baker 2018-12-21 16:09:24 PST
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.
Comment 20 Devin Rousso 2018-12-21 16:28:11 PST
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`.
Comment 21 Devin Rousso 2018-12-21 16:28:12 PST
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`.
Comment 22 Nikita Vasilyev 2018-12-21 16:30:43 PST
Created attachment 358002 [details]
[Video] Bug (attachment 357995 [details])

r- because it creates section duplicates.
Comment 23 Devin Rousso 2018-12-21 17:01:35 PST
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.
Comment 24 Matt Baker 2018-12-21 17:11:58 PST
(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.
Comment 25 Devin Rousso 2018-12-21 17:53:23 PST
(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.
Comment 26 Matt Baker 2018-12-21 19:30:26 PST
(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.
Comment 27 Devin Rousso 2019-02-01 15:43:30 PST
Created attachment 360915 [details]
Patch
Comment 28 Timothy Hatcher 2019-03-20 09:31:26 PDT
Comment on attachment 360915 [details]
Patch

Does this latest patch fix the duplicate section issue?
Comment 29 Devin Rousso 2019-03-20 15:38:56 PDT
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`.
Comment 30 Timothy Hatcher 2019-03-20 16:12:02 PDT
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.
Comment 31 Devin Rousso 2019-03-20 16:40:51 PDT
Created attachment 365434 [details]
Patch
Comment 32 WebKit Commit Bot 2019-03-20 16:42:59 PDT Comment hidden (obsolete)
Comment 33 Devin Rousso 2019-03-20 16:47:25 PDT
Created attachment 365436 [details]
Patch
Comment 34 WebKit Commit Bot 2019-03-20 17:22:12 PDT
Comment on attachment 365436 [details]
Patch

Clearing flags on attachment: 365436

Committed r243264: <https://trac.webkit.org/changeset/243264>
Comment 35 WebKit Commit Bot 2019-03-20 17:22:14 PDT
All reviewed patches have been landed.  Closing bug.