Web Inspector: CSS rule with 2 pseudo-selectors appears twice

[Nikita Vasilyev]

Created attachment 256079 [details]
[HTML] Reduction

Originally reported by https://twitter.com/toolmantim.
Animated GIF: https://cloudup.com/idEbO3X5_OS

[Radar WebKit Bug Importer]

<rdar://problem/21666285>

[Nikita Vasilyev]

Created attachment 256080 [details]
Animated GIF of the problem

[Devin Rousso]

I have a fix for this, but it relies upon https://bugs.webkit.org/show_bug.cgi?id=145979, so once that lands I can fix this.

[Devin Rousso]

Created attachment 256161 [details]
Patch

[Timothy Hatcher]

Comment on attachment 256161 [details]
Patch

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

> Source/WebInspectorUI/UserInterface/Views/RulesStyleDetailsPanel.js:183
> +                if (pseudoElementSelectors.length && pseudoElementSelectors.lastValue.selectorText === selectorText)
> +                    continue;

What if the rules have different source locations?

[Build Bot]

Comment on attachment 256161 [details]
Patch

Attachment 256161 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/5429194611752960

New failing tests:
platform/mac-wk2/tiled-drawing/scrolling/scroll-snap/scroll-snap-mandatory-mainframe-slow-vertical.html

[Build Bot]

Created attachment 256163 [details]
Archive of layout-test-results from ews104 for mac-mavericks-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-mavericks-wk2  Platform: Mac OS X 10.9.5

[Devin Rousso]

Created attachment 256164 [details]
Patch

[Timothy Hatcher]

Comment on attachment 256164 [details]
Patch

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

> Source/WebInspectorUI/UserInterface/Views/RulesStyleDetailsPanel.js:182
> +                if (pseudoElementSelectors.length && pseudoElementSelectors.lastValue.style.ownerRule.isEqualTo(style.ownerRule))

Wouldn't just preventing duplicates of the same style/ownerRule be enough? Why does the selector need a replace done and compared to others?

[Timothy Hatcher]

Comment on attachment 256164 [details]
Patch

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

>> Source/WebInspectorUI/UserInterface/Views/RulesStyleDetailsPanel.js:182
>> +                if (pseudoElementSelectors.length && pseudoElementSelectors.lastValue.style.ownerRule.isEqualTo(style.ownerRule))
> 
> Wouldn't just preventing duplicates of the same style/ownerRule be enough? Why does the selector need a replace done and compared to others?

Ah-ha, I think I understand now.

Per pseudo element, uniqueOrderedStyles is called and it strips duplicates. But that duplicate matching does not happen across pseudo-elements, in the case of a selector like "*::before, *::after" which applies to two pseudo elements.

Perhaps the same logic as uniqueOrderedStyles, should be used here? Not just adjacent duplicates, but all duplicates are removed.

[Timothy Hatcher]

Comment on attachment 256164 [details]
Patch

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

>>> Source/WebInspectorUI/UserInterface/Views/RulesStyleDetailsPanel.js:182
>>> +                if (pseudoElementSelectors.length && pseudoElementSelectors.lastValue.style.ownerRule.isEqualTo(style.ownerRule))
>> 
>> Wouldn't just preventing duplicates of the same style/ownerRule be enough? Why does the selector need a replace done and compared to others?
> 
> Ah-ha, I think I understand now.
> 
> Per pseudo element, uniqueOrderedStyles is called and it strips duplicates. But that duplicate matching does not happen across pseudo-elements, in the case of a selector like "*::before, *::after" which applies to two pseudo elements.
> 
> Perhaps the same logic as uniqueOrderedStyles, should be used here? Not just adjacent duplicates, but all duplicates are removed.

One way to do this would be to concat all pseudoElement.orderedStyles together, then use uniqueOrderedStyles. Then loop over that uniqueOrderedStyles result and populate pseudoElementSelectors.

[Devin Rousso]

Created attachment 256172 [details]
Patch

[Timothy Hatcher]

Comment on attachment 256172 [details]
Patch

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

> Source/WebInspectorUI/UserInterface/Views/RulesStyleDetailsPanel.js:173
> +        var pseudoElementsStyle = [];

Nit: pseudoElementStyles would be better, since this is an array.

[WebKit Commit Bot]

Comment on attachment 256172 [details]
Patch

Clearing flags on attachment: 256172

Committed r186288: <http://trac.webkit.org/changeset/186288>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.