Implement :defined pseudo-class for custom elements

RESOLVED FIXED in Firefox 63

Status

()

Core
DOM
P2
normal
RESOLVED FIXED
2 years ago
23 days ago

People

(Reporter: edgar, Assigned: smaug)

Tracking

(Blocks: 1 bug, {dev-doc-needed})

unspecified
mozilla63
dev-doc-needed
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox63 fixed)

Details

(Whiteboard: dom-ce-m4, URL)

Attachments

(1 attachment, 2 obsolete attachments)

Updated

2 years ago
Whiteboard: dom-ce-m3

Updated

2 years ago
Priority: -- → P3
Keywords: dev-doc-needed

Updated

a year ago
Assignee: nobody → jdai
Status: NEW → ASSIGNED

Updated

a year ago
Depends on: 1299363, 1301024
Whiteboard: dom-ce-m3 → dom-ce-m4
(Reporter)

Comment 1

6 months ago
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

6 months ago
Depends on: 1417829

Comment 2

6 months ago
Created attachment 8942579 [details] [diff] [review]
patch, v1

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

Comment 3

6 months ago
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

6 months ago
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

6 months ago
(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

6 months ago
(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

6 months ago
(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

6 months ago
(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

6 months ago
Created attachment 8943105 [details] [diff] [review]
patch, v2
Attachment #8942579 - Attachment is obsolete: true

Comment 10

6 months ago
(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

6 months ago
(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

6 months ago
(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)

Updated

6 months ago
Assignee: jdai → nobody
Status: ASSIGNED → NEW
Need to change the fix for bug 1441029. Basically remove the assertion, since random 'is' values are allowed.
Created attachment 8988275 [details] [diff] [review]
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
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+
(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

24 days ago
Pushed by opettay@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b32be80ba883
Implement :defined pseudo-class for custom elements, r=emilio
Blocks: 1471871

Comment 18

23 days ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/b32be80ba883
Status: NEW → RESOLVED
Last Resolved: 23 days ago
status-firefox63: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
You need to log in before you can comment on or make changes to this bug.