Closed Bug 1111633 Opened 9 years ago Closed 9 years ago

Implement Unresolved Element Pseudoclass for Custom Elements

Categories

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

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla38

People

(Reporter: gkrizsanits, Assigned: gkrizsanits)

References

(Blocks 1 open bug)

Details

(Keywords: dev-doc-needed)

Attachments

(1 file, 1 obsolete file)

Assignee: nobody → gkrizsanits
Status: NEW → ASSIGNED
Attached patch unresolved (obsolete) — Splinter Review
I have a first draft and I think it's time to get some feedback. I hope I'm doing the right thing, also wondering what else am I forgetting about.

I use a hashtable for the unresolved elements, since I'm afraid checking the tag name and |is| attribute for all the elements might be too slow. Maybe this is a premature optimization, I cannot tell since I have only a very vague understanding of how style processing work in gecko...

Question is when to update this hashtable. It's not clear to me from the spec what should happen for unresolved elements detached from the tree, maybe it's not even that important. 

I interpret the spec (http://w3c.github.io/webcomponents/spec/custom/#dfn-type-extension) as changing the |is| attribute should not update the hashtable.
Attachment #8543974 - Flags: feedback?(bzbarsky)
Comment on attachment 8543974 [details] [diff] [review]
unresolved

The right way to do this is to use an event state (see EventStates.h, which we should maybe find a better name for).

This will lead to simpler code (e.g. you won't need any changes to layout/style at all, won't need a restyle manager dependency in nsDocument.

It will also lead to more correct code.  For example, the patch attached here fails to handle this testcase, because it doesn't correctly restyle the right set of elements when the unresolved state changes:

  <!DOCTYPE html>
  <head>
    <style>
      span { color: green; }
      x-foo:unresolved + span { color: red; }
    </style>
  <body>
    <x-foo></x-foo>
    <span>This text should be green</span>
    <script>
    // Force a style flush on the span:
    var c = getComputedStyle(document.querySelector("span")).color;
    if (c != "rgb(255, 0, 0)") {
      alert("bogus background color before we register stuff");
    }

    var Foo = document.registerElement('x-foo', { prototype: Object.create(HTMLElement.prototype) });
    </script>
  </body>

You basically have two implementation options for using event states.  Either you keep the hashtable (or some other equivalently-fast data structure, e.g. another boolean flag on the element) and then update Element::IntrinsicState to look at that and call Element::UpdateState in the places where the state changes.  Or you can add your new state to ESM_MANAGED_STATES (which we should _also_ rename, sigh, since these are no longer all managed by the ESM) and manually do AddStates/RemoveStates when the state changes.

> It's not clear to me from the spec what should happen for unresolved elements
> detached from the tree

Raise a spec issue?  This should obviously be defined.
Attachment #8543974 - Flags: feedback?(bzbarsky) → feedback-
(In reply to Boris Zbarsky [:bz] from comment #2)
> It will also lead to more correct code.  For example, the patch attached
> here fails to handle this testcase, because it doesn't correctly restyle the
> right set of elements when the unresolved state changes:

Oh... thanks for the test! I thought this will just work :(

> You basically have two implementation options for using event states. 
> Either you keep the hashtable (or some other equivalently-fast data
> structure, e.g. another boolean flag on the element)

Do you mean flags like mxr.mozilla.org/mozilla-central/source/dom/base/Element.h?force=1#77 ?
Because it seemed to me that those are already quite crowded that's why I used the table (and
also because I kind of felt like that at some point for something we want to enumerate all these
unresolved elements, for tooling or whatever...)

> and then update
> Element::IntrinsicState to look at that and call Element::UpdateState in the
> places where the state changes.  Or you can add your new state to
> ESM_MANAGED_STATES (which we should _also_ rename, sigh, since these are no
> longer all managed by the ESM) and manually do AddStates/RemoveStates when
> the state changes.

I would prefer approach one if that is not worse option for some reason than the
ESM one.

> 
> > It's not clear to me from the spec what should happen for unresolved elements
> > detached from the tree
> 
> Raise a spec issue?  This should obviously be defined.

Alright, and thanks a lot for the quick feedback!
> I thought this will just work

It can't, because you're explicitly restyling just the subtree rooted at the element, but that testcase required restyling a sibling.  The style system has a mechanism to do that, but it relies on the style system being told that the pseudo-class state changed; that's what the state thing gives you, since you have to explicitly update the state and that notifies the style system.

> Do you mean flags like 
> mxr.mozilla.org/mozilla-central/source/dom/base/Element.h?force=1#77 ?

Yes.  And yes, those are pretty crowded.  On the other hand, I'm not super-excited about the idea of hashtable lookups on this hashtable every time an element's state might have changed (e.g. after every single DOM attribute set).  This is why I think approach two is somewhat better than approach one here...
(In reply to Boris Zbarsky [:bz] from comment #5)
> > I thought this will just work
> 
> It can't, because you're explicitly restyling just the subtree rooted at the
> element, but that testcase required restyling a sibling.  The style system
> has a mechanism to do that, but it relies on the style system being told
> that the pseudo-class state changed; that's what the state thing gives you,
> since you have to explicitly update the state and that notifies the style
> system.

Thanks it all makes sense now (at least conceptually...)

> > Do you mean flags like 
> > mxr.mozilla.org/mozilla-central/source/dom/base/Element.h?force=1#77 ?
> 
> Yes.  And yes, those are pretty crowded.  On the other hand, I'm not
> super-excited about the idea of hashtable lookups on this hashtable every
> time an element's state might have changed (e.g. after every single DOM
> attribute set).  This is why I think approach two is somewhat better than
> approach one here...

Thanks for the thoughts, I changed my mind too in the meanwhile, the spec will
also use flag so there is no reason to diverge from that in the implementation.
Especially given the performance concern you mentioned.
(In reply to Boris Zbarsky [:bz] from comment #5)

> Yes.  And yes, those are pretty crowded.

They seem to be so crowded that we are out of them :( at least for nsXULElement...
I guess we will have to stick to ESM_MANAGED_STATES then.
Right, that's what I meant by "approach two".
(In reply to Boris Zbarsky [:bz] from comment #2)
> 
> This will lead to simpler code (e.g. you won't need any changes to
> layout/style at all, won't need a restyle manager dependency in nsDocument.
> 

I think I don't get this part. I have added the ESM state and the Add/RemoveStates calls.
But I don't get how could I add the pseudo class without nsCSSPesudoClassList/nsCSSRuleProcessor...

Also if I add it there something seems to be still missing... Where can I learn more about the interaction between ESM/EventStates and styles? (a problem I'm hitting right now is that
if I have a rule for x-foo and one for :unresolved, after the unresolved flag is removed, the style
for x-foo elements are not refreshed)
> But I don't get how could I add the pseudo class without
> nsCSSPesudoClassList/nsCSSRuleProcessor...

Er, you do need the change to nsCSSPseudoClassList, you're right.  You don't need changes to nsCSSRuleProcessor.

In nsCSSPseudoClassList you'd use CSS_STATE_PSEUDO_CLASS to define this pseudo-class. That tells the style system which state change affects the matching of this pseudo-class.

> Also if I add it there something seems to be still missing..

How did you add it?

> Where can I learn more about the interaction between ESM/EventStates and styles?

The interaction goes like this.  Elements have a state bitfield.  When this changes, they send ContentStateChanged notifications.  The RestyleManager is one of the things that watches those; when they happen it looks for style rules that have selectors depending on that state, and if it finds them it restyles the correct set of elements based on the actual selectors involved.

For the most part, the things that depend on the state are pseudo-classes; using CSS_STATE_PSEUDO_CLASS (which has an argument for the bit the pseudo-class matches on) tells the style system both how to match it and whether selectors using the pseudo-class depends on the state.  There's also CSS_STATE_DEPENDENT_PSEUDO_CLASS for cases when you want the "depends on the state" done automatically but want to do something other than just checking the bit for matching.  This part is all documented in nsCSSPseudoClassList.h.

Does that help?
(In reply to Boris Zbarsky [:bz] from comment #11)
> In nsCSSPseudoClassList you'd use CSS_STATE_PSEUDO_CLASS to define this
> pseudo-class. That tells the style system which state change affects the
> matching of this pseudo-class.

Bah... I just forgot to update the definition to be a CSS_STATE_PSEUDO_CLASS.
I forgot about this part completely even though I read this header once :(

Anyway, thanks for the help, it seems to work fine now.
Attachment #8543974 - Attachment is obsolete: true
Attachment #8547490 - Flags: review?(bzbarsky)
Comment on attachment 8547490 [details] [diff] [review]
unresloved pseudo class v2

r=me
Attachment #8547490 - Flags: review?(bzbarsky) → review+
https://hg.mozilla.org/mozilla-central/rev/95a7e60a6970
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.