Implement :defined pseudo-class for custom elements

RESOLVED FIXED in Firefox 63

Status

()

defect
P2
normal
RESOLVED FIXED
2 years ago
3 months ago

People

(Reporter: edgar, Assigned: smaug)

Tracking

({dev-doc-complete})

unspecified
mozilla63
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox63 fixed)

Details

(Whiteboard: dom-ce-m4, )

Attachments

(1 attachment, 2 obsolete attachments)

Whiteboard: dom-ce-m3
Priority: -- → P3
Assignee: nobody → jdai
Status: NEW → ASSIGNED
Whiteboard: dom-ce-m3 → dom-ce-m4
Reporter

Comment 1

Last year
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.
Priority: P3 → P2
Reporter

Updated

Last year
Depends on: 1417829
Posted patch patch, v1 (obsolete) — Splinter Review
Patch for implementing :defined pseudo-class for custom elements, still working on web-platform tests.
Reporter

Comment 3

Last year
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.
Reporter

Comment 4

Last year
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?
(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
Reporter

Comment 6

Last year
(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.
Reporter

Comment 7

Last year
(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.
(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.
Posted patch patch, v2 (obsolete) — Splinter Review
Attachment #8942579 - Attachment is obsolete: true
(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.
Reporter

Comment 11

Last year
(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?
(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)
Assignee: jdai → nobody
Status: ASSIGNED → NEW
Assignee

Comment 13

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

Comment 14

Last year
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
Assignee: nobody → bugs
Attachment #8943105 - Attachment is obsolete: true
Attachment #8988275 - Flags: review?(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.
Attachment #8988275 - Flags: review?(emilio) → review+
Assignee

Comment 16

Last year
(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

Last year
Pushed by opettay@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b32be80ba883
Implement :defined pseudo-class for custom elements, r=emilio
Assignee

Updated

Last year
Blocks: 1471871
https://hg.mozilla.org/mozilla-central/rev/b32be80ba883
Status: NEW → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla63

Updated

11 months ago
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.