1331334 - Implement :defined pseudo-class for custom elements

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect, P2)

Description

[Edgar Chen [:edgar] (Parental leave until Oct. 09.)]

as specified: https://html.spec.whatwg.org/multipage/scripting.html#selector-defined

Comment 1

[Edgar Chen [:edgar] (Parental leave until Oct. 09.)]

Look like Google Chrome and Safari both support :defined pseudo-class, http://software.hixie.ch/utilities/js/live-dom-viewer/?saved=5680, we probably need this before shipping Custom Elemenets, so set priority to P2. Feel free to change back if you think it is not necessary to be a P2 bug.

Comment 2

[John Dai[:jdai]]

Attachment: patch, v1

Patch for implementing :defined pseudo-class for custom elements, still working on web-platform tests.

Comment 3

[Edgar Chen [:edgar] (Parental leave until Oct. 09.)]

Comment on attachment 8942579 [details] [diff] [review]
patch, v1

Review of attachment 8942579 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/base/CustomElementRegistry.cpp
@@ +956,5 @@
>    // Step 6 and step 7.
>    DoUpgrade(aElement, aDefinition->mConstructor, aRv);
>    if (aRv.Failed()) {
>      data->mState = CustomElementData::State::eFailed;
> +    aElement->RemoveStatesSilently(NS_EVENT_STATE_DEFINED);

Why we need to remove NS_EVENT_STATE_DEFINED here? Is it possible that we add NS_EVENT_STATE_DEFINED on a "undefined" element?

::: dom/base/Element.cpp
@@ +1730,5 @@
>        }
> +    } else {
> +      // An element whose custom element state is "uncustomized" is said
> +      // to be defined. https://dom.spec.whatwg.org/#concept-element-defined
> +      AddStatesSilently(NS_EVENT_STATE_DEFINED);

It seems not necessary to set the state for every BindToTree call for "uncustomized" case, we probably could set the state at the element creation time, I mean in nsContentUtils::NewXULOrHTMLElement().

@@ +4408,5 @@
> +  // https://dom.spec.whatwg.org/#concept-element-defined
> +  if (aData->mState == CustomElementData::State::eCustom) {
> +    AddStatesSilently(NS_EVENT_STATE_DEFINED);
> +  } else {
> +    RemoveStatesSilently(NS_EVENT_STATE_DEFINED);

Similar question here, is it possible that we add NS_EVENT_STATE_DEFINED on a element which state is not "custom"?

::: dom/events/EventStates.h
@@ +243,5 @@
>  #define NS_EVENT_STATE_REQUIRED      NS_DEFINE_EVENT_STATE_MACRO(21)
>  // Content is optional (and can be required).
>  #define NS_EVENT_STATE_OPTIONAL      NS_DEFINE_EVENT_STATE_MACRO(22)
> +// Element is a defined custom element
> +#define NS_EVENT_STATE_DEFINED NS_DEFINE_EVENT_STATE_MACRO(23)

Nit: Align NS_DEFINE_EVENT_STATE_MACRO with previous one.

@@ +360,5 @@
>    NS_EVENT_STATE_FOCUSRING |                  \
>    NS_EVENT_STATE_FOCUS_WITHIN |               \
>    NS_EVENT_STATE_FULL_SCREEN |                \
>    NS_EVENT_STATE_HOVER |                      \
> +  NS_EVENT_STATE_DEFINED |                    \

Nit: List it in alphabetical order?

::: layout/style/nsCSSPseudoClassList.h
@@ +89,5 @@
>  // Match nodes that are HTML but not XHTML
>  CSS_PSEUDO_CLASS(mozIsHTML, ":-moz-is-html", 0, "")
>  
> +// Match all custom elements that is defined.
> + CSS_STATE_PSEUDO_CLASS(defined, ":defined", 0, "", NS_EVENT_STATE_DEFINED)

We should put :defined behind the "dom.webcomponent.customelements.enable" pref.

Comment 4

[Edgar Chen [:edgar] (Parental leave until Oct. 09.)]

Comment on attachment 8942579 [details] [diff] [review]
patch, v1

Review of attachment 8942579 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/base/CustomElementRegistry.cpp
@@ +964,5 @@
>    }
>  
>    // Step 8.
>    data->mState = CustomElementData::State::eCustom;
> +  aElement->AddStatesSilently(NS_EVENT_STATE_DEFINED);

Do we need to use AddStates() to notify that the state is changed so that the :define rule can be applied once the element is upgraded?

Comment 5

[John Dai[:jdai]]

(In reply to Edgar Chen [:edgar] from comment #3)
> Comment on attachment 8942579 [details] [diff] [review]
> patch, v1
> 
> Review of attachment 8942579 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/base/CustomElementRegistry.cpp
> @@ +956,5 @@
> >    // Step 6 and step 7.
> >    DoUpgrade(aElement, aDefinition->mConstructor, aRv);
> >    if (aRv.Failed()) {
> >      data->mState = CustomElementData::State::eFailed;
> > +    aElement->RemoveStatesSilently(NS_EVENT_STATE_DEFINED);
> 
> Why we need to remove NS_EVENT_STATE_DEFINED here? Is it possible that we
> add NS_EVENT_STATE_DEFINED on a "undefined" element?
> 
I was considering clone a built-in custom elements, and we change its is attribute to upgrade fail, then :defined state will be removed. But it looks like state will not be cloned to a new node. I'll remove this change. Thank you.

> ::: dom/base/Element.cpp
> @@ +1730,5 @@
> >        }
> > +    } else {
> > +      // An element whose custom element state is "uncustomized" is said
> > +      // to be defined. https://dom.spec.whatwg.org/#concept-element-defined
> > +      AddStatesSilently(NS_EVENT_STATE_DEFINED);
> 
> It seems not necessary to set the state for every BindToTree call for
> "uncustomized" case, we probably could set the state at the element creation
> time, I mean in nsContentUtils::NewXULOrHTMLElement().
> 
I agree set the state at the element creation time, we should also consider set the state for parser created normal element at nsHtml5TreeOperation::CreateHTMLElement[1]. The nsContentUtils::NewXULOrHTMLElement is a case for createElement which custom element state is "uncustomized".

[1] https://searchfox.org/mozilla-central/rev/41925c0b6c6d58578690121de439d2a8d3d690f3/parser/html/nsHtml5TreeOperation.cpp#464

> @@ +4408,5 @@
> > +  // https://dom.spec.whatwg.org/#concept-element-defined
> > +  if (aData->mState == CustomElementData::State::eCustom) {
> > +    AddStatesSilently(NS_EVENT_STATE_DEFINED);
> > +  } else {
> > +    RemoveStatesSilently(NS_EVENT_STATE_DEFINED);
> 
> Similar question here, is it possible that we add NS_EVENT_STATE_DEFINED on
> a element which state is not "custom"?
> 
I'll remove this change. Thank you.

> ::: dom/events/EventStates.h
> @@ +243,5 @@
> >  #define NS_EVENT_STATE_REQUIRED      NS_DEFINE_EVENT_STATE_MACRO(21)
> >  // Content is optional (and can be required).
> >  #define NS_EVENT_STATE_OPTIONAL      NS_DEFINE_EVENT_STATE_MACRO(22)
> > +// Element is a defined custom element
> > +#define NS_EVENT_STATE_DEFINED NS_DEFINE_EVENT_STATE_MACRO(23)
> 
> Nit: Align NS_DEFINE_EVENT_STATE_MACRO with previous one.
> 
Will do.

> @@ +360,5 @@
> >    NS_EVENT_STATE_FOCUSRING |                  \
> >    NS_EVENT_STATE_FOCUS_WITHIN |               \
> >    NS_EVENT_STATE_FULL_SCREEN |                \
> >    NS_EVENT_STATE_HOVER |                      \
> > +  NS_EVENT_STATE_DEFINED |                    \
> 
> Nit: List it in alphabetical order?
> 
Will do.

> ::: layout/style/nsCSSPseudoClassList.h
> @@ +89,5 @@
> >  // Match nodes that are HTML but not XHTML
> >  CSS_PSEUDO_CLASS(mozIsHTML, ":-moz-is-html", 0, "")
> >  
> > +// Match all custom elements that is defined.
> > + CSS_STATE_PSEUDO_CLASS(defined, ":defined", 0, "", NS_EVENT_STATE_DEFINED)
> 
> We should put :defined behind the "dom.webcomponent.customelements.enable"
> pref.
Will do.

(In reply to Edgar Chen [:edgar] from comment #4)
> Comment on attachment 8942579 [details] [diff] [review]
> patch, v1
> 
> Review of attachment 8942579 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/base/CustomElementRegistry.cpp
> @@ +964,5 @@
> >    }
> >  
> >    // Step 8.
> >    data->mState = CustomElementData::State::eCustom;
> > +  aElement->AddStatesSilently(NS_EVENT_STATE_DEFINED);
> 
> Do we need to use that the state is changed so that
> the :define rule can be applied once the element is upgraded?
It seems no need to use AddStates() to notify, because states passed for AddStates must only be EXTERNALLY_MANAGED_STATES[1][2]. However, NS_EVENT_STATE_DEFINED isn't a EXTERNALLY_MANAGED_STATES.

[1] https://searchfox.org/mozilla-central/rev/41925c0b6c6d58578690121de439d2a8d3d690f3/dom/base/Element.h#664-674
[2] https://searchfox.org/mozilla-central/rev/41925c0b6c6d58578690121de439d2a8d3d690f3/dom/events/EventStates.h#347-364

Comment 6

[Edgar Chen [:edgar] (Parental leave until Oct. 09.)]

(In reply to John Dai[:jdai] from comment #5)
> The nsContentUtils::NewXULOrHTMLElement is a case for createElement which custom
> element state is "uncustomized".
I guess you meant to say "custom element state is *not* "uncustomized""?


> It seems no need to use AddStates() to notify, because states passed for
> AddStates must only be EXTERNALLY_MANAGED_STATES.
But NS_EVENT_STATE_DEFINED is marked as EXTERNALLY_MANAGED_STATES in this patch.

Comment 7

[Edgar Chen [:edgar] (Parental leave until Oct. 09.)]

(In reply to Edgar Chen [:edgar] from comment #6)
> > It seems no need to use AddStates() to notify, because states passed for
> > AddStates must only be EXTERNALLY_MANAGED_STATES.
> But NS_EVENT_STATE_DEFINED is marked as EXTERNALLY_MANAGED_STATES in this
> patch.
And using AddStatesSilently() without notifying probably won't handle certain circumstances well, something like http://software.hixie.ch/utilities/js/live-dom-viewer/?saved=5741.

Comment 8

[John Dai[:jdai]]

(In reply to Edgar Chen [:edgar] from comment #6)
> (In reply to John Dai[:jdai] from comment #5)
> > The nsContentUtils::NewXULOrHTMLElement is a case for createElement which custom
> > element state is "uncustomized".
> I guess you meant to say "custom element state is *not* "uncustomized""?
> 
Yes, I am trying to said custom element state is *not* "uncustomized".

> 
> > It seems no need to use AddStates() to notify, because states passed for
> > AddStates must only be EXTERNALLY_MANAGED_STATES.
> But NS_EVENT_STATE_DEFINED is marked as EXTERNALLY_MANAGED_STATES in this
> patch.
Yes, it's necessary to use AddStates() to notify.

(In reply to Edgar Chen [:edgar] from comment #7)
> (In reply to Edgar Chen [:edgar] from comment #6)
> > > It seems no need to use AddStates() to notify, because states passed for
> > > AddStates must only be EXTERNALLY_MANAGED_STATES.
> > But NS_EVENT_STATE_DEFINED is marked as EXTERNALLY_MANAGED_STATES in this
> > patch.
> And using AddStatesSilently() without notifying probably won't handle
> certain circumstances well, something like
> http://software.hixie.ch/utilities/js/live-dom-viewer/?saved=5741.
Thanks for providing a test.

Comment 9

[John Dai[:jdai]]

Attachment: patch, v2

Comment 10

[John Dai[:jdai]]

(In reply to John Dai[:jdai] from comment #8)
> (In reply to Edgar Chen [:edgar] from comment #6)
> > (In reply to John Dai[:jdai] from comment #5)
> > > The nsContentUtils::NewXULOrHTMLElement is a case for createElement which custom
> > > element state is "uncustomized".
> > I guess you meant to say "custom element state is *not* "uncustomized""?
> > 
> Yes, I am trying to said custom element state is *not* "uncustomized".
>
Err...I revise my comment, we should also handle created a normal element from document.createElement which state should be "uncustomized", the code patch will go into nsContentUtils::NewXULOrHTMLElement. Sorry for not comment clearly.

Comment 11

[Edgar Chen [:edgar] (Parental leave until Oct. 09.)]

(In reply to John Dai[:jdai] from comment #5)
> we should also consider set the state for parser created normal element at
> nsHtml5TreeOperation::CreateHTMLElement[1].

And also the cloneNode case around https://searchfox.org/mozilla-central/rev/1f9b4f0f0653b129b4db1b9fc5e9068054afaac0/dom/base/nsNodeUtils.cpp#492-525?

Comment 12

[John Dai[:jdai]]

(In reply to Edgar Chen [:edgar] from comment #11)
> (In reply to John Dai[:jdai] from comment #5)
> > we should also consider set the state for parser created normal element at
> > nsHtml5TreeOperation::CreateHTMLElement[1].
> 
> And also the cloneNode case around
> https://searchfox.org/mozilla-central/rev/
> 1f9b4f0f0653b129b4db1b9fc5e9068054afaac0/dom/base/nsNodeUtils.cpp#492-525?

Yes, Not only we need to consider cloneNode case, but also createElementNS for not HTML namespace[1], because an element's namespace is not HTML, it should be "uncustomized"; i.e., "defined"[2].

[1] https://infra.spec.whatwg.org/#namespaces
[2] https://dom.spec.whatwg.org/#concept-create-element (Step 7.2)

Comment 13

[Olli Pettay [:smaug][[e-mail]]]

Need to change the fix for bug 1441029. Basically remove the assertion, since random 'is' values are allowed.

Comment 14

[Olli Pettay [:smaug][[e-mail]]]

Attachment: defined_pseudoclass.diff

this needs a stylo review, and the DOM part is style-related anyhow....

remote: View your changes here:
remote:   https://hg.mozilla.org/try/rev/9a6b65e5c4a1f76ed65894f5e0c46d33bed3d1f9
remote:   https://hg.mozilla.org/try/rev/e49a83d2ead94511a86cfa3e9a870704e10eb39c
remote: 
remote: Follow the progress of your build on Treeherder:
remote:   https://treeherder.mozilla.org/#/jobs?repo=try&revision=e49a83d2ead94511a86cfa3e9a870704e10eb39c
remote: recorded changegroup in replication log in 0.064s


In theory the state could be intrinsic, but since calculating that is a tad slow (because of several indirect accesses), went with EXTERNALLY_MANAGED_STATES

Comment 15

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8988275 [details] [diff] [review]
defined_pseudoclass.diff

Review of attachment 8988275 [details] [diff] [review]:
-----------------------------------------------------------------

r=me, thanks!

::: dom/base/CustomElementRegistry.cpp
@@ +1149,5 @@
>    if (aRv.Failed()) {
>      data->mState = CustomElementData::State::eFailed;
>      // Empty element's custom element reaction queue.
>      data->mReactionQueue.Clear();
> +    aElement->SetDefined(false);

If data was in eCustom state, we'd have bailed out early. I guess this is needed for is=""? If so, a comment would be nice.

::: dom/base/Element.cpp
@@ +4274,5 @@
>  {
>    SetHasCustomElementData();
> +
> +  if (aData->mState != CustomElementData::State::eCustom) {
> +    SetDefined(false);

This doesn't really have to notify either it seems.

::: dom/base/nsContentUtils.cpp
@@ +10024,5 @@
>            NS_IF_ADDREF(*aResult = NS_NewHTMLUnknownElement(nodeInfo.forget(), aFromParser));
>          } else {
>            NS_IF_ADDREF(*aResult = nsXULElement::Construct(nodeInfo.forget()));
>          }
> +        (*aResult)->SetDefined(false);

This doesn't really need to notify, right? Not that we'd do anything, but saves a bunch of pointer-chases and virtual calls.

Comment 16

[Olli Pettay [:smaug][[e-mail]]]

(In reply to Emilio Cobos Álvarez [:emilio] from comment #15)

> ::: dom/base/CustomElementRegistry.cpp
> @@ +1149,5 @@
> >    if (aRv.Failed()) {
> >      data->mState = CustomElementData::State::eFailed;
> >      // Empty element's custom element reaction queue.
> >      data->mReactionQueue.Clear();
> > +    aElement->SetDefined(false);
> 
> If data was in eCustom state, we'd have bailed out early. I guess this is
> needed for is=""? If so, a comment would be nice.
er, actually  this shouldn't be needed

> > +  if (aData->mState != CustomElementData::State::eCustom) {
> > +    SetDefined(false);
> 
> This doesn't really have to notify either it seems.
It doesn't notify, because we aren't in document.

> This doesn't really need to notify, right? Not that we'd do anything, but
> saves a bunch of pointer-chases and virtual calls.
Doesn't notify, since we aren't in document.
I'm just trying to have sane API. The current state handling is rather error prone.

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b32be80ba883
Implement :defined pseudo-class for custom elements, r=emilio

Comment 18

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/b32be80ba883