Bug 197960 - Implement ElementInternals
Summary: Implement ElementInternals
Status: REOPENED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Enhancement
Assignee: Alexey Shvayka
URL:
Keywords: InRadar
Depends on: 233023
Blocks: 154907 197963
  Show dependency treegraph
 
Reported: 2019-05-16 13:22 PDT by Domenic Denicola
Modified: 2022-05-03 09:51 PDT (History)
43 users (show)

See Also:


Attachments
Patch (104.85 KB, patch)
2021-11-17 10:07 PST, Alexey Shvayka
rniwa: review-
ews-feeder: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Domenic Denicola 2019-05-16 13:22:10 PDT
Spec:

- Some steps of https://html.spec.whatwg.org/multipage/custom-elements.html#dom-customelementregistry-define for disabledFeatures
- Step 3 of https://dom.spec.whatwg.org/#dom-element-attachshadow for disabledFeatures = ['shadow'] specifically
- https://html.spec.whatwg.org/multipage/custom-elements.html#the-elementinternals-interface for attachInternals() and the (empty-for-now) ElementInternals class

Tests:

- https://github.com/web-platform-tests/wpt/blob/master/shadow-dom/Element-interface-attachShadow-custom-element.html
- https://github.com/web-platform-tests/wpt/blob/master/custom-elements/HTMLElement-attachInternals.html

A separate bug will be filed for form-associated custom elements, which will make ElementInternals not empty. But this seems like a separable chunk of work.
Comment 1 Jeroen Zwartepoorte 2021-10-12 07:52:37 PDT
Firefox 93 just released with support for `attachInternals` (https://hacks.mozilla.org/2021/10/lots-to-see-in-firefox-93/). Chromium already supports it. So webkit is the only one not yet supporting it.
Comment 2 Alexey Shvayka 2021-11-17 10:07:30 PST
Created attachment 444531 [details]
Patch
Comment 3 Ryosuke Niwa 2021-11-17 17:10:18 PST
It seems like you need to rebaseline the results for imported/w3c/web-platform-tests/html/dom/idlharness.https.html
Comment 4 Ryosuke Niwa 2021-11-17 17:43:41 PST
Comment on attachment 444531 [details]
Patch

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

> Source/WebCore/dom/ElementInternals.h:52
> +    HTMLElement& m_element;

Please don't use a raw reference to a node like this in new code.
Use WeakPtr<HTMLElement> instead. r- because of this.

> Source/WebCore/dom/ElementInternals.idl:31
> +    // Shadow root access

This comment is useless. Please remove.

> Source/WebCore/dom/Node.h:591
> +        Precustomized = 4,

Let's not waste an entire bit for having this value.
The following condition should be used for an element to be precustomsized:
!hasNodeFlag(NodeFlag::IsUnknownElement) && customElementState() == CustomElementState::Failed
r- because of this.

> Source/WebCore/dom/Node.h:598
> +        uint16_t areInternalsAttached : 1;

Please don't waste a bitfield for this. Also, elementInternals is a singular noun object.
r- because of this.

> Source/WebCore/dom/Node.h:616
> +    bool areInternalsAttached() const { return rareDataBitfields().areInternalsAttached; }
> +    void setInternalsAttached();
> +

This shouldn't be here. ElementInternals should be stored in ElementRareData.
There is no need to avoid accessing when we need to check this condition since it never happens in a hot code path.

> Source/WebCore/dom/ShadowRoot.h:78
> +    bool availableToElementInternals() const { return m_availableToElementInternals; }
> +

Nit: should be named this instead: isAvailableToElementInternals
https://webkit.org/code-style-guidelines/#names-verb

> Source/WebCore/html/HTMLElement.cpp:1273
> +    auto* elementInterface = registry->findInterface(*this);

This is a very inefficient way of getting the element interface.
Use Element::reactionQueue() which has a back reference to JSCustomElementInterface.

> Source/WebCore/html/HTMLElement.cpp:1287
> +    setInternalsAttached();
> +    return ElementInternals::create(*this);

We should store this in ElementRareData or CustomElementReactionQueue)
(the latter is preferable to avoid increasing the size of ElementRareData by sizeof(void*)).
Comment 5 Alexey Shvayka 2021-11-18 13:33:59 PST
(In reply to Ryosuke Niwa from comment #4)

Wow, thanks for such thorough review, Ryosuke! Much appreciated.

>
> > Source/WebCore/dom/ElementInternals.h:52
> > +    HTMLElement& m_element;
> 
> Please don't use a raw reference to a node like this in new code.
> Use WeakPtr<HTMLElement> instead. r- because of this.

Let's suppose a case of a detached & unreferenced HTML element, with only ElementInternals instance in userland JS code retaining a reference to it. Per spec, this reference should be strong and ElementInternals methods should work as expected.

Can we make this work with WeakPtr?

>
> > Source/WebCore/dom/ElementInternals.idl:31
> > +    // Shadow root access
> 
> This comment is useless. Please remove.

👍🏻

>
> > Source/WebCore/dom/Node.h:591
> > +        Precustomized = 4,
> 
> Let's not waste an entire bit for having this value.
> The following condition should be used for an element to be precustomsized:
> !hasNodeFlag(NodeFlag::IsUnknownElement) && customElementState() ==
> CustomElementState::Failed
> r- because of this.

I don't like adding a bit to CustomElementState as well, yet proposed solution for "precustomized" state fails a lot of tests where custom element is synchronously created.

Also, please note that "failed" => "precustomized" state transition happens only after _disable shadow_ is checked (step 8.2 of https://html.spec.whatwg.org/#upgrades); I'm not sure how we can get around with "failed" state only.

>
> > Source/WebCore/dom/Node.h:598
> > +        uint16_t areInternalsAttached : 1;
> 
> Please don't waste a bitfield for this. Also, elementInternals is a singular
> noun object.
> r- because of this.
> 
> > Source/WebCore/dom/Node.h:616
> > +    bool areInternalsAttached() const { return rareDataBitfields().areInternalsAttached; }
> > +    void setInternalsAttached();
> > +
> 
> This shouldn't be here. ElementInternals should be stored in ElementRareData.
> There is no need to avoid accessing when we need to check this condition
> since it never happens in a hot code path.

It feels to me that CustomElementState should also be stored in ElementRareData, yet it's here in Node. I suppose the reason it's there is free bits in RareDataBitFields. This patch is neutral on sizeof(RareDataBitFields).

How can we move it to ElementRareData without increasing its size?

>
> > Source/WebCore/dom/ShadowRoot.h:78
> > +    bool availableToElementInternals() const { return m_availableToElementInternals; }
> > +
> 
> Nit: should be named this instead: isAvailableToElementInternals
> https://webkit.org/code-style-guidelines/#names-verb

👍🏻, my bad.

> 
> > Source/WebCore/html/HTMLElement.cpp:1273
> > +    auto* elementInterface = registry->findInterface(*this);
> 
> This is a very inefficient way of getting the element interface.
> Use Element::reactionQueue() which has a back reference to
> JSCustomElementInterface.

Nice 👍🏻, this is a great simplification as well.

>
> > Source/WebCore/html/HTMLElement.cpp:1287
> > +    setInternalsAttached();
> > +    return ElementInternals::create(*this);
> 
> We should store this in ElementRareData or CustomElementReactionQueue)
> (the latter is preferable to avoid increasing the size of ElementRareData by
> sizeof(void*)).

I've checked the spec and seems like there is no necessity to store ElementInternals instance like we do for `classList` or `dataset`. It's a one-way API, from userland JS code to DOM, and it's not a [SameObject].

What am I missing?
Comment 6 Ryosuke Niwa 2021-11-18 14:03:36 PST
(In reply to Alexey Shvayka from comment #5)
> (In reply to Ryosuke Niwa from comment #4)
> 
> Wow, thanks for such thorough review, Ryosuke! Much appreciated.
> 
> >
> > > Source/WebCore/dom/ElementInternals.h:52
> > > +    HTMLElement& m_element;
> > 
> > Please don't use a raw reference to a node like this in new code.
> > Use WeakPtr<HTMLElement> instead. r- because of this.
> 
> Let's suppose a case of a detached & unreferenced HTML element, with only
> ElementInternals instance in userland JS code retaining a reference to it.
> Per spec, this reference should be strong and ElementInternals methods
> should work as expected.
> 
> Can we make this work with WeakPtr?

Yes. The way to keep HTML element alive in that case is to treat it as the opaque root of the ElementInternals. So, when GC visits ElementInternals, we need to be adding the associated element's opaque root to the slot visitors. Talk to Geoff / Chris if you have further questions.

> > > Source/WebCore/dom/Node.h:591
> > > +        Precustomized = 4,
> > 
> > Let's not waste an entire bit for having this value.
> > The following condition should be used for an element to be precustomsized:
> > !hasNodeFlag(NodeFlag::IsUnknownElement) && customElementState() ==
> > CustomElementState::Failed
> > r- because of this.
> 
> I don't like adding a bit to CustomElementState as well, yet proposed
> solution for "precustomized" state fails a lot of tests where custom element
> is synchronously created.
> 
> Also, please note that "failed" => "precustomized" state transition happens
> only after _disable shadow_ is checked (step 8.2 of
> https://html.spec.whatwg.org/#upgrades); I'm not sure how we can get around
> with "failed" state only.

Hm... I see. Can we replace CustomElementState::Uncustomized with CustomElementState::Precustomized? Because we should be able to differentiate a custom element from the rest by asking if it's unknown or not (Node::isUnknownElement() which checks NodeFlag::IsUnknownElement).

> > > Source/WebCore/dom/Node.h:616
> > > +    bool areInternalsAttached() const { return rareDataBitfields().areInternalsAttached; }
> > > +    void setInternalsAttached();
> > > +
> > 
> > This shouldn't be here. ElementInternals should be stored in ElementRareData.
> > There is no need to avoid accessing when we need to check this condition
> > since it never happens in a hot code path.
> 
> It feels to me that CustomElementState should also be stored in
> ElementRareData, yet it's here in Node.

That's for performance because we have to check it in various very hot code paths.

> I suppose the reason it's there is free bits in RareDataBitFields. This patch is neutral on
> sizeof(RareDataBitFields).

We don't want to waste bits in RareDataBitFields because we may use it for other purposes.

> How can we move it to ElementRareData without increasing its size?

Use CompactUniquePtrTuple for CustomElementReactionQueue and store it there?

> > > Source/WebCore/html/HTMLElement.cpp:1287
> > > +    setInternalsAttached();
> > > +    return ElementInternals::create(*this);
> > 
> > We should store this in ElementRareData or CustomElementReactionQueue)
> > (the latter is preferable to avoid increasing the size of ElementRareData by
> > sizeof(void*)).
> 
> I've checked the spec and seems like there is no necessity to store
> ElementInternals instance like we do for `classList` or `dataset`. It's a
> one-way API, from userland JS code to DOM, and it's not a [SameObject].

Ok, I see. Didn't realize that. That's pretty neat.
Comment 7 Radar WebKit Bug Importer 2022-03-08 14:12:49 PST
<rdar://problem/89989084>
Comment 8 Matteo Flores 2022-03-09 14:32:42 PST
(In reply to Ryosuke Niwa from comment #3)
> It seems like you need to rebaseline the results for
> imported/w3c/web-platform-tests/html/dom/idlharness.https.html

Looks like a rebasline is still needed for this test for iPad queue's constant failure.
Comment 9 Matteo Flores 2022-04-21 13:30:56 PDT
Pull request: https://github.com/WebKit/WebKit/pull/348
Comment 10 EWS 2022-04-21 13:34:36 PDT
Committed r293183 (249861@main): <https://commits.webkit.org/249861@main>

Reviewed commits have been landed. Closing PR #348 and removing active labels.
Comment 11 Tim Nguyen (:ntim) 2022-04-21 14:15:36 PDT
The commit that closed this a test gardening commit, not the actual implementation commit.