Bug 176187 – Web Inspector: Styles Redesign: display related pseudo-elements

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

[Nikita Vasilyev]

Bug 175343 does not implement showing matching pseudo-styles.

Steps:
1. Open http://nv.github.io/webkit-inspector-bugs/css-pseudo-element/
2. Inspect <a>

Expected:
a:before rule is shown at the end of the styles panel, after a:any-link.

Actual:
a:before isn't shown.

[Radar WebKit Bug Importer]

<rdar://problem/34194917>

[Nikita Vasilyev]

To clarify, pseudo styles can be edited just fine when selecting a pseudo-styles node in the DOM tree, e.g. ::before.

This bug is about showing pseudo-styles when selecting an actual element in the DOM tree, e.g. <div>.

[Devin Rousso]

Created attachment 328332 [details]
Patch

[Devin Rousso]

Created attachment 328333 [details]
[Image] After Patch is applied

[Blaze Burg]

Comment on attachment 328332 [details]
Patch

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

I'd like Nikita or Matt to look at this.

> Source/WebInspectorUI/UserInterface/Models/DOMNodeStyles.js:-68
> -        this.refresh();

Ouch, thanks for fixing this.

[Timothy Hatcher]

Comment on attachment 328332 [details]
Patch

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

> Source/WebInspectorUI/UserInterface/Models/DOMNode.js:640
> +        if (this.isPseudoElement())
> +            return "::" + this._pseudoType;

Does this have any broader impact on other places in the Inspector?

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetRulesStyleDetailsPanel.js:128
> +        Promise.all(pseudoElements.map((pseudoElement) => WI.cssStyleManager.stylesForNode(pseudoElement).refreshIfNeeded()))

Are the multiple the refreshIfNeeded() calls needed here? Wouldn't a refresh have happened to get here (WI.SpreadsheetRulesStyleDetailsPanel.prototype.refresh) in the first place?

[Devin Rousso]

Comment on attachment 328332 [details]
Patch

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

>> Source/WebInspectorUI/UserInterface/Models/DOMNode.js:640
>> +            return "::" + this._pseudoType;
> 
> Does this have any broader impact on other places in the Inspector?

Not that I could tell, and even then it probably makes more sense than displaying "<pseudo>".

>> Source/WebInspectorUI/UserInterface/Views/SpreadsheetRulesStyleDetailsPanel.js:128
>> +        Promise.all(pseudoElements.map((pseudoElement) => WI.cssStyleManager.stylesForNode(pseudoElement).refreshIfNeeded()))
> 
> Are the multiple the refreshIfNeeded() calls needed here? Wouldn't a refresh have happened to get here (WI.SpreadsheetRulesStyleDetailsPanel.prototype.refresh) in the first place?

So `this.nodeStyles.node` would be a different value than `pseudoElement`, meaning that it's possible for the user to select a node that has styles for it, but not for its pseudo-elements.  This is to ensure that we fetch styles not only for the selected node (already done by StyleDetailsPanel), but also for any related pseudo-elements.  Since a given node can have more than one pseudo-element (e.g. ::before and ::after), we need a refresh for the styles of each of those pseudo-element nodes.

[Timothy Hatcher]

Comment on attachment 328332 [details]
Patch

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

>>> Source/WebInspectorUI/UserInterface/Models/DOMNode.js:640
>>> +            return "::" + this._pseudoType;
>> 
>> Does this have any broader impact on other places in the Inspector?
> 
> Not that I could tell, and even then it probably makes more sense than displaying "<pseudo>".

Yeah.

>>> Source/WebInspectorUI/UserInterface/Views/SpreadsheetRulesStyleDetailsPanel.js:128
>>> +        Promise.all(pseudoElements.map((pseudoElement) => WI.cssStyleManager.stylesForNode(pseudoElement).refreshIfNeeded()))
>> 
>> Are the multiple the refreshIfNeeded() calls needed here? Wouldn't a refresh have happened to get here (WI.SpreadsheetRulesStyleDetailsPanel.prototype.refresh) in the first place?
> 
> So `this.nodeStyles.node` would be a different value than `pseudoElement`, meaning that it's possible for the user to select a node that has styles for it, but not for its pseudo-elements.  This is to ensure that we fetch styles not only for the selected node (already done by StyleDetailsPanel), but also for any related pseudo-elements.  Since a given node can have more than one pseudo-element (e.g. ::before and ::after), we need a refresh for the styles of each of those pseudo-element nodes.

Alright. I had assumed they would be refreshed in sync in DOMNodeStyles somewhere. I don't remember, it has been a while.

[WebKit Commit Bot]

Comment on attachment 328332 [details]
Patch

Rejecting attachment 328332 [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-01', 'land-attachment', '--force-clean', '--non-interactive', '--parent-command=commit-queue', 328332, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit

Last 500 characters of output:
    -> origin/master
Partial-rebuilding .git/svn/refs/remotes/origin/master/.rev_map.268f45cc-cd09-0410-ab3c-d52691b4dbfc ...
Currently at 225568 = 5cf19e403b6b766c52fb8630564e59a3f608c9a4
r225569 = fd36e1404f49d6831c75faae3f9355dc7f913ac9
r225570 = ccea8e8adb33ad907447e8f3bf7e50ff78b435a4
Done rebuilding .git/svn/refs/remotes/origin/master/.rev_map.268f45cc-cd09-0410-ab3c-d52691b4dbfc
First, rewinding head to replay your work on top of it...
Fast-forwarded master to refs/remotes/origin/master.

Full output: http://webkit-queues.webkit.org/results/5511619

[Devin Rousso]

Created attachment 328557 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 328557 [details]
Patch

Clearing flags on attachment: 328557

Committed r225572: <https://trac.webkit.org/changeset/225572>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.