Web Inspector: Show Selector's Specificity

• Component: Web Inspector
• Status: RESOLVED FIXED

[Joseph Pecoraro]

* SUMMARY:
The inspector should show selector's specificity

[Joseph Pecoraro]

<rdar://problem/18717361>

[Radar WebKit Bug Importer]

<rdar://problem/18817190>

[Joseph Pecoraro]

Created attachment 240632 [details]
[PATCH] Proposed Fix

[Benjamin Poulain]

Comment on attachment 240632 [details]
[PATCH] Proposed Fix

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

The C++ side looks reasonable, no idea about the JS part. You may want to get a second review :)

> LayoutTests/inspector/css/selector-specificity.html:15
> +h1:before,
> +body h1:before,
> +body.a h1.b:before {

:before -> ::before.

":before" is the legacy syntax from CSS 2.

> Source/JavaScriptCore/inspector/protocol/CSS.json:66
> +                { "name": "specificity", "optional": true, "type": "array", "items": { "type": "integer" }, "description": "Specificity (a, b, c) tuple. If missing the specificity is not known statically and may be dynamic." }

What would be the value for dynamic specificity?

> Source/WebCore/css/CSSSelector.h:53
> +        static const unsigned maxValueMask = 0xffffff;
> +        static const unsigned idMask = 0xff0000;
> +        static const unsigned classMask = 0xff00;
> +        static const unsigned elementMask = 0xff;

I would prefer a typed enum to namespace this:
enum class SpecificityMask {
    Id,
    Class,
    Element,
    All
}

[Joseph Pecoraro]

(In reply to comment #4)
> Comment on attachment 240632 [details]
> [PATCH] Proposed Fix
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=240632&action=review
> 
> The C++ side looks reasonable, no idea about the JS part. You may want to
> get a second review :)
> 
> > LayoutTests/inspector/css/selector-specificity.html:15
> > +h1:before,
> > +body h1:before,
> > +body.a h1.b:before {
> 
> :before -> ::before.
> 
> ":before" is the legacy syntax from CSS 2.

Oops!


> > Source/JavaScriptCore/inspector/protocol/CSS.json:66
> > +                { "name": "specificity", "optional": true, "type": "array", "items": { "type": "integer" }, "description": "Specificity (a, b, c) tuple. If missing the specificity is not known statically and may be dynamic." }
> 
> What would be the value for dynamic specificity?

It could just be no specificity = dynamic.

> > Source/WebCore/css/CSSSelector.h:53
> > +        static const unsigned maxValueMask = 0xffffff;
> > +        static const unsigned idMask = 0xff0000;
> > +        static const unsigned classMask = 0xff00;
> > +        static const unsigned elementMask = 0xff;
> 
> I would prefer a typed enum to namespace this:
> enum class SpecificityMask {
>     Id,
>     Class,
>     Element,
>     All
> }

I don't understand this comment.

The masks are bitmasks for the different pieces of the specificity tuple stored in the "unsigned" returned from CSSSelector::specificity. I'm not sure a typed enum is useful here.

[Timothy Hatcher]

Comment on attachment 240632 [details]
[PATCH] Proposed Fix

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

> Source/WebInspectorUI/UserInterface/Views/CSSStyleDeclarationSection.js:183
> +                selectorElement.title = WebInspector.UIString("Specificity: (%d, %d, %d)").format(specificity[0], specificity[1], specificity[2]);

I expected a single number in the UI, not a tuple. It is hard to understand what the three numbers represent. The CSS spec totals the three into a final, easier to compare, number.

http://www.w3.org/TR/selectors/#specificity

[Joseph Pecoraro]

(In reply to comment #6)
> Comment on attachment 240632 [details]
> [PATCH] Proposed Fix
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=240632&action=review
> 
> > Source/WebInspectorUI/UserInterface/Views/CSSStyleDeclarationSection.js:183
> > +                selectorElement.title = WebInspector.UIString("Specificity: (%d, %d, %d)").format(specificity[0], specificity[1], specificity[2]);
> 
> I expected a single number in the UI, not a tuple. It is hard to understand
> what the three numbers represent. The CSS spec totals the three into a
> final, easier to compare, number.

Yes, I thought about this too.


The spec says:
<http://www.w3.org/TR/selectors/#specificity>
> Concatenating the three numbers a-b-c (in a number system with a large base) gives the specificity.

However, assuming a specific number base (x) can produce an incorrect number if individually either (a), (b), or (c) is larger then the base (x).

This explains it clearly:
<http://juicystudio.com/article/selector-specificity.php>
> One solution suggested is to count the numbers in each column, and multiply by its denary position.
> In simple terms, multiply a by 100, b by 10, and c by 1 (remains the same), and then add them
> together. The problem with this approach is that it falls down when one of the columns has a count
> of ten or higher. For example, 11 elements (c) would outweigh a single class (b) using this method,
> but that cannot be true. A single class would outweigh 100 or more elements. The simplest way to
> cater for the large base is to just accept that b outweighs any number of c, and that a outweighs
> any number of b (which is true of any number system).

However, it is not uncommon to do simple math for selectors with (1000, 100, 10, 1). As mentioned above and elsewhere:
<http://www.smashingmagazine.com/2007/07/27/css-specificity-things-you-should-know/>
> Start at 0, add 1000 for style attribute, add 100 for each ID, add 10 for each attribute,
> class or pseudo-class, add 1 for each element name or pseudo-element.

I think that is reasonable. How do you feel about displaying something like:

    Specificity (5, 4, 2) ≈ 542.

I think in practice we won't find many selectors that have more than 10 tags. We may see selectors with more than 10 class/pseudo components but it is probably rare. We can always see if this happens by just checking if any value is > 10 and decide to do something else. Having the "approx" sign instead of "equal" sign gives us a get out of jail card for these edge cases.

[Joseph Pecoraro]

Comment on attachment 240632 [details]
[PATCH] Proposed Fix

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

>>> Source/WebCore/css/CSSSelector.h:53
>>> +        static const unsigned elementMask = 0xff;
>> 
>> I would prefer a typed enum to namespace this:
>> enum class SpecificityMask {
>>     Id,
>>     Class,
>>     Element,
>>     All
>> }
> 
> I don't understand this comment.
> 
> The masks are bitmasks for the different pieces of the specificity tuple stored in the "unsigned" returned from CSSSelector::specificity. I'm not sure a typed enum is useful here.

Oh, I see now. I forgot to include the CSSSelector.cpp changes here. I'm just moving these masks from the .cpp to the .h so the inspector code can use it. New patch soon.

[Benjamin Poulain]

Personally I find the single number unreadable.

Cases with >= 10 in a column are fairly common too in practice.

In WebKit, the max value is type 255. You could have funny cases:
-(1, 0, 0) = 100
-(0, 0, 255) = 255

You could adds zeros I guess:
-(1, 0, 0) = 001000000
-(0, 0, 255) = 000000255

[Joseph Pecoraro]

Created attachment 240772 [details]
[PATCH] Proposed Fix

Take 2.

[Joseph Pecoraro]

Created attachment 240773 [details]
[IMAGE] Example

I think we hit the sweet spot with:

    Specificity: (1, 0, 1) ≈ 101

The tuple is the hard truth. The number we show after is a mere approximation I would say most web developers are thinking of in their head.

[Timothy Hatcher]

Comment on attachment 240772 [details]
[PATCH] Proposed Fix

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

> Source/WebInspectorUI/UserInterface/Models/DOMNodeStyles.js:763
> +            return selectors.map(function(selectorText) {
> +                return new WebInspector.CSSSelector(selectorText);
> +            });

Wild return ... return here. JS, stay crazy.

[WebKit Commit Bot]

Comment on attachment 240772 [details]
[PATCH] Proposed Fix

Clearing flags on attachment: 240772

Committed r175483: <http://trac.webkit.org/changeset/175483>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.