[Shadow] Using isUnknownPseudoElement() for shadow pseudo id seems confusing

• Component: CSS
• Status: RESOLVED FIXED

[Shinya Kawanaka]

We would like to have more descriptive name for it.

[Dimitri Glazkov (Google)]

Not sure I agree. The name is pretty clear in that the pseudo element is unknown at the time of parsing the selector. It could be either a custom pseudo element (starts with "x-", a WebKit-specific custom pseudo element (starts with "-webkit-"), or some totally unknown thing.

Ohhh... I think I understand what you're saying. Since we have these three choices and they are knowable at the time of parsing, we might distinguish between them. Right?

[Shinya Kawanaka]

Yeah.. Actually it is actually used for shadowPseudoId, etc.
We can distinguish some specific types, so we should use some good name instead of UNKNOWN.
It's sometimes known.

[Takashi Sakamoto]

(In reply to comment #2)
> Yeah.. Actually it is actually used for shadowPseudoId, etc.
> We can distinguish some specific types, so we should use some good name instead of UNKNOWN.
> It's sometimes known.

I'm not sure what to do. However, I would like to ask one question. If we use some pseudo type for shadow pseudo ids or custom pseudo, i.e. -webkit or x-, we need unknown ids, e.g. -UNKNOWN-UNKNOWN?

I have to check CSS spec. However I'm just interested in your idea about such completely unknown ids.

[Shinya Kawanaka]

For completely UNKNOWN ids, we can skip using that CSS rule, right? It's because it won't match anything.
When parsing succeeded, we should already know the type of id.

[Dimitri Glazkov (Google)]

(In reply to comment #4)
> For completely UNKNOWN ids, we can skip using that CSS rule, right? It's because it won't match anything.
> When parsing succeeded, we should already know the type of id.

Yes, that's the win I see.

In terms of naming, I see three types of "unknown" pseudo-elements:

1) CustomPseudoElement --> CUSTOM
2) WebKitPseudoElement --> WEBKIT_CUSTOM
3) UnknownPseudoElement --> UNKNOWN

The interesting bit is that WebKitCustomElement should only match attributes that were set by the UA. We should _not_ allow any random Bob set "-webkit-slider-thumb" on their div and have it matched. That would be bad.

[Shinya Kawanaka]

Created attachment 172267 [details]
Patch

[WebKit Review Bot]

Comment on attachment 172267 [details]
Patch

Attachment 172267 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/14719717

New failing tests:
css3/selectors3/html/css3-modsel-161.html
fast/box-sizing/percentage-height.html
css3/selectors3/xhtml/css3-modsel-177b.xml
css3/selectors3/html/css3-modsel-177b.html
fast/dom/shadow/shadow-pseudo-id.html
css3/selectors3/xml/css3-modsel-161.xml
css3/selectors3/xml/css3-modsel-177b.xml
fast/selectors/177b.html
css3/selectors3/xhtml/css3-modsel-161.xml

[Build Bot]

Comment on attachment 172267 [details]
Patch

Attachment 172267 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/14745151

New failing tests:
css3/selectors3/html/css3-modsel-161.html
fast/box-sizing/percentage-height.html
css3/selectors3/xhtml/css3-modsel-177b.xml
css3/selectors3/html/css3-modsel-177b.html
css3/selectors3/xml/css3-modsel-161.xml
css3/selectors3/xml/css3-modsel-177b.xml
fast/selectors/177b.html
css3/selectors3/xhtml/css3-modsel-161.xml

[Shinya Kawanaka]

Created attachment 172454 [details]
Patch

[Hajime Morrita]

Comment on attachment 172267 [details]
Patch

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

> Source/WebCore/ChangeLog:8
> +        We used isUnknownPseudoElement() for these 3 meanings: 1) an element is custom pseudo element (starting with 'x-'),

s/a element is/the element is an/
By the way, what is "custom pseudo element"? Is it a standard-mentioned name?

> Source/WebCore/css/CSSParser.cpp:10111
> +    if (specifiers->isCustomPseudoElement() || specifiers->isWebKitCustomPseudoElement()) {

Looks like this combination is frequently used and better to have a shortcut method.
Otherwise this change makes the code more complex instead of less-confusing.

> Source/WebCore/css/CSSSelectorList.cpp:204
> +        return selector->isUnknownPseudoElement() || selector->isWebKitCustomPseudoElement() || selector->isCustomPseudoElement();

Let's define a CSSSelector method instead of scattering this kind of named logic here and there.

> LayoutTests/fast/dom/shadow/shadow-pseudo-id.html:30
> +</script>

Can we make this test code more descriptive? It's unclear what this test is trying to exercise,
which will make it hard to diagnose future regression.

[Dimitri Glazkov (Google)]

Comment on attachment 172454 [details]
Patch

unknown -> invalid?

[Shinya Kawanaka]

(In reply to comment #10)
> By the way, what is "custom pseudo element"? Is it a standard-mentioned name?

I've taken it from ShadowDOM spec.
https://dvcs.w3.org/hg/webcomponents/raw-file/tip/spec/shadow/index.html#custom-pseudo-elements

[Shinya Kawanaka]

I would like to use 'AuthorCustomPseudoElement' instead of 'CustomPseudoElement',
and use 'CustomPseudoElement' as (AuthorCustomPseudoElement or WebKitCustomPseudoElement)...

Someone can come up with a better name?

[Dimitri Glazkov (Google)]

(In reply to comment #13)
> I would like to use 'AuthorCustomPseudoElement' instead of 'CustomPseudoElement',
> and use 'CustomPseudoElement' as (AuthorCustomPseudoElement or WebKitCustomPseudoElement)...
> 
> Someone can come up with a better name?

I don't think prefixing the name with Author makes it less clear. The fact that there are WebKit-specific custom pseudo elements is where we need to dig, not the other way around.

UserAgentPseudoElement?

Also, I really like the "invalid" name. It makes a lot more sense than "unqueriable". It's un-queriable because it's invalid. Better statement of purpose.

[Shinya Kawanaka]

Hmm... But we have :invalid pseudo-class already :-(

[Dimitri Glazkov (Google)]

(In reply to comment #15)
> Hmm... But we have :invalid pseudo-class already :-(

That's ok. That's different. Look here: http://dev.w3.org/csswg/selectors4/#invalid

[Shinya Kawanaka]

Created attachment 172474 [details]
Patch

[Shinya Kawanaka]

Another try

[Hajime Morrita]

Comment on attachment 172474 [details]
Patch

Looks fine. wait test green being show up.

[WebKit Review Bot]

Comment on attachment 172474 [details]
Patch

Clearing flags on attachment: 172474

Committed r133577: <http://trac.webkit.org/changeset/133577>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.