Bug 148695 – Implement v1 shadow DOM API

• Component: DOM
• Status: NEW

[Ryosuke Niwa]

Implement the newly spec'ed slot shadow DOM API:
https://w3c.github.io/webcomponents/spec/shadow/

[Ryosuke Niwa]

Created attachment 260929 [details]
WIP1

[Ryosuke Niwa]

Created attachment 261133 [details]
WIP2

[Chris Dumez]

Comment on attachment 261133 [details]
WIP2

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

> Source/WebCore/html/HTMLKeygenElement.h:55
> +    virtual bool canHaveUserAgentShadowRoot() const override final { return true; }

final is redundant, the class is final

> Source/WebCore/html/HTMLMarqueeElement.h:65
> +    virtual bool canHaveUserAgentShadowRoot() const override final { return true; }

final is redundant, the class is final

> Source/WebCore/html/HTMLMeterElement.h:79
> +    virtual bool canHaveUserAgentShadowRoot() const override final { return true; }

final is redundant, the class is final

> Source/WebCore/html/HTMLProgressElement.h:65
> +    virtual bool canHaveUserAgentShadowRoot() const override final { return true; }

final is redundant, the class is final

> Source/WebCore/html/HTMLSlotElement.cpp:51
> +        if (ShadowRoot* shadowRoot = containingShadowRoot()) {

auto*

> Source/WebCore/html/HTMLSlotElement.cpp:63
> +    Vector<RefPtr<Node>> distributedNodes;

May not matter much but we could reserveInitialCapacity() / uncheckedAppend() here since we know how many nodes there are.

> Source/WebCore/html/HTMLSlotElement.cpp:64
> +    for (auto node : distribution())

I would use auto* for clarity. Since you're not using auto&, it would cause refcounting churn for e.g. if distribution() would start returning a Vector of RefPtr.

> Source/WebCore/html/HTMLSlotElement.h:29
> +#include "HTMLElement.h"

Why this include? I think we need Element.h for AttributeModificationReason though.

> Source/WebCore/html/HTMLSlotElement.h:31
> +#include "Range.h"

Why this include?

> Source/WebCore/html/HTMLSlotElement.h:36
> +class HTMLSlotElement : public InsertionPoint {

Could be final?

> Source/WebCore/html/HTMLSlotElement.h:42
> +protected:

Could be private? or are you planning to subclass this at some point?

[Chris Dumez]

Comment on attachment 261133 [details]
WIP2

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

> Source/WebCore/html/HTMLSummaryElement.h:47
> +    virtual bool canHaveUserAgentShadowRoot() const override final { return true; }

class is already final

> Source/WebCore/html/HTMLTextAreaElement.h:71
> +    virtual bool canHaveUserAgentShadowRoot() const override final { return true; }

class is already final

> Source/WebCore/html/shadow/ContentDistributor.cpp:52
> +InsertionPoint* ContentDistributor::findSlotElementByNode(const Node* node, ShadowRoot& root)

looks like node cannot be null? if so, we should consider making it a reference.

> Source/WebCore/html/shadow/ContentDistributor.cpp:55
> +    const AtomicString& name = is<Element>(node) ? downcast<Element>(*node).fastGetAttribute(HTMLNames::slotAttr) : nullAtom;

is<Element>(*node) to avoid the null check, assuming node cannot be null.

> Source/WebCore/html/shadow/ContentDistributor.cpp:77
> +    for (auto slot : m_slots)

auto*

> Source/WebCore/html/shadow/InsertionPoint.cpp:79
> +    if (ShadowRoot* shadow = containingShadowRoot())

auto*

> Source/WebCore/html/shadow/InsertionPoint.cpp:109
> +    if (ShadowRoot* shadowRoot = shadowRootOfParentForDistribution(projectedNode))

auto*

> Source/WebCore/style/RenderTreePosition.cpp:41
> +        if (nextSiblingRenderer(node, m_parent) != m_nextSibling) {

curly brackets not needed

> Source/WebCore/style/RenderTreePosition.cpp:68
> +    if (auto parent = textNode.parentElement()) {

I personally prefer auto*

> Source/WebCore/style/RenderTreePosition.cpp:69
> +        if (PseudoElement* before = parent->beforePseudoElement())

auto*

> Source/WebCore/style/StyleResolveTree.cpp:363
> +    for (Node* current : distribution) {

auto*

> Source/WebCore/style/StyleResolveTree.cpp:520
> +    for (Node* current : insertionPoint.distribution()) {

auto*

[Antti Koivisto]

Comment on attachment 261133 [details]
WIP2

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

New DOM and API stuff looks good.

The existing insertion point and distribution code is basically remnants of the old shadow DOM implementation that is left there to support <details> element. I don't think I left it in very good state and some aspects of this patch might be overly influenced by it.
.

> Source/WebCore/dom/NodeRenderingTraversal.cpp:133
> +static Node* findLastFromDistributedNode(const Node* node, const InsertionPoint& insertionPoint)
>  {
> -    for (const Node* next = node; next; next = insertionPoint->previousDistributedTo(next)) {
> -        if (Node* found = findLastEnteringInsertionPoints(next))
> -            return found;
> +    const auto& distribution = insertionPoint.distribution();
> +    size_t index = distribution.find(node);
> +    if (index != notFound) {
> +        while (index) {
> +            if (Node* found = findLastEnteringInsertionPoints(distribution[index]))
> +                return found;
> +            if (!index)
> +                break;
> +            index--;
> +        }
>      }
>      return nullptr;

It would be nice to get rid of these confusing functions rather than expand them.

> Source/WebCore/dom/ShadowRoot.cpp:75
> +    ASSERT(m_distributor);

Could distributor just be null for trees that don't use distribution (old style ua shadow trees)? That would save some memory.

> Source/WebCore/html/HTMLDetailsElement.cpp:82
> +class DetailsContentDistributor final : public ContentDistributor {

Instead of having a separate distributor could the normal distributor handle this? Details code would add a named slot for summary and a default slot for the rest. The only special case left would be make <summary> behave as if it had slot="-webkit-summary", right?

> Source/WebCore/html/HTMLSlotElement.cpp:55
> +            distributor.removeSlotElementByName(oldValue, *this, *shadowRoot);
> +            distributor.addSlotElementByName(newValue, *this, *shadowRoot);
> +            distributor.invalidateDistribution(shadowRoot->host());

Distributor should take care of invalidation itself without explicit invalidateDistribution call. (I think it already does actually)

>> Source/WebCore/html/HTMLSlotElement.h:36
>> +class HTMLSlotElement : public InsertionPoint {
> 
> Could be final?

I suspect we'd take a cleaner path to the desired end state if we started with 

class HTMLSlotElement : HTMLElement {

instead and figured out the migration path for <details> as a separate problem.

> Source/WebCore/html/shadow/ContentDistributor.cpp:83
> +void ContentDistributor::distributeHostChildren(ShadowRoot& root)

Naming: what else would we distribute than host children?

> Source/WebCore/html/shadow/ContentDistributor.cpp:105
> +void ContentDistributor::distributeNode(Node& hostChild, InsertionPoint& slot)
>  {
> -    ASSERT(shadowRoot);
> -
> -    Vector<ShadowRoot*, 8> shadowRoots;
> -    for (Element* current = shadowRoot->host(); current; current = current->shadowHost()) {
> -        ShadowRoot* currentRoot = current->shadowRoot();
> -        if (!currentRoot->distributor().needsDistribution())
> -            break;
> -        shadowRoots.append(currentRoot);
> -    }
> -
> -    for (size_t i = shadowRoots.size(); i > 0; --i)
> -        shadowRoots[i - 1]->distributor().distribute(shadowRoots[i - 1]->host());
> +    ASSERT(hostChild.parentElement()->shadowRoot() == slot.containingShadowRoot());
> +    slot.m_distribution.append(&hostChild);

This is a weird function. It doesn't "distribute" anything, it just adds one argument to an array in another argument. It could be static. Should it be a function of slot instead?

Mixing InsertionPoint and slot terminology is confusing. Even if it is temporary I'd prefer if intermediate patches avoided it.

> Source/WebCore/html/shadow/ContentDistributor.h:50
>  class ContentDistributor {

"content" is rather generic. Something like ShadowTreeDistributor might describe the purpose better.

> Source/WebCore/html/shadow/ContentDistributor.h:54
> +    virtual ~ContentDistributor();

Code would be more understandable if this wasn't a virtual class.

> Source/WebCore/html/shadow/ContentDistributor.h:59
> +    virtual InsertionPoint* findSlotElementByNode(const Node*, ShadowRoot&);
>  
> -    void distributeSelectionsTo(InsertionPoint*, Element* host);
> +    void addSlotElementByName(const AtomicString&, InsertionPoint&, ShadowRoot&);
> +    void removeSlotElementByName(const AtomicString&, InsertionPoint&, ShadowRoot&);

Do these ever take ShadowRoot other than the owner? Could we just have a backpointer to the owner? Not having one doesn't seem like much of a memory optimization.

> Source/WebCore/html/shadow/ContentDistributor.h:62
>      void invalidateDistribution(Element* host);

Why does this take host instead of ShadowRoot? Seems inconsistent. (you can get host from shadow root).

> Source/WebCore/html/shadow/ContentDistributor.h:64
> +    void redistribute(ShadowRoot&);

Is there difference between distributing and redistributing?

> Source/WebCore/html/shadow/ContentDistributor.h:74
> +    unsigned m_isDistributionValid : 1;

Please just use bool. Bitfield doesn't help.

> Source/WebCore/html/shadow/InsertionPoint.cpp:54
>  bool InsertionPoint::shouldUseFallbackElements() const
>  {
> -    return isActive() && !hasDistribution();
> +    return isActive() && distribution().isEmpty();
>  }

What are "fallback elements"?

> Source/WebCore/html/shadow/InsertionPoint.cpp:85
>  void InsertionPoint::removedFrom(ContainerNode& insertionPoint)

InsertionPoint removed from insertionPoint? This is confusing.

> Source/WebCore/html/shadow/InsertionPoint.cpp:102
> +const Vector<Node*>& InsertionPoint::distribution() const
>  {
> -    if (!m_hasDistribution)
> -        return 0;
> -    for (Node* current = shadowHost()->lastChild(); current; current = current->previousSibling()) {
> -        if (matchTypeFor(current) == InsertionPoint::AlwaysMatches)
> -            return current;
> +    if (auto* root = containingShadowRoot()) {
> +        if (root->distributor().isDistributionValid())
> +            return m_distribution;
> +        root->distributor().redistribute(*root);

It is bit annoying how this breaks constness (by having const containingShadowRoot return non-const ShadowRoot*).

>> Source/WebCore/html/shadow/InsertionPoint.cpp:109
>>  InsertionPoint* findInsertionPointOf(const Node* projectedNode)
>>  {
>> -    if (ShadowRoot* shadowRoot = shadowRootOfParentForDistribution(projectedNode)) {
>> -        if (ShadowRoot* root = projectedNode->containingShadowRoot())
>> -            ContentDistributor::ensureDistribution(root);
>> -        return shadowRoot->distributor().findInsertionPointFor(projectedNode);
>> -    }
>> -    return 0;
>> +    if (ShadowRoot* shadowRoot = shadowRootOfParentForDistribution(projectedNode))
> 
> auto*

Old terminology like "projectedNode" here is confusing. 

shadowRootOfParentForDistribution is very confusing too.

> Source/WebCore/html/shadow/InsertionPoint.cpp:110
> +        return shadowRoot->distributor().findSlotElementByNode(projectedNode, *shadowRoot);

Especially when mixed with new slot terminology.

> Source/WebCore/html/shadow/InsertionPoint.h:61
> +    mutable Vector<Node*> m_distribution;

How does Node get removed from here when it is removed from the tree?

> Source/WebCore/html/shadow/InsertionPoint.h:63
> +    friend class ContentDistributor;

Is it really necessary to use friend?

> Source/WebCore/style/RenderTreePosition.cpp:44
>      if (m_hasValidNextSibling) {
>          // Stop validating at some point so the assert doesn't make us O(N^2) on debug builds.
> +        if (nextSiblingRenderer(node, m_parent) != m_nextSibling) {
> +            nextSiblingRenderer(node, m_parent);
> +        }
>          ASSERT(m_parent.isRenderView() || ++m_assertionLimitCounter > 20 || nextSiblingRenderer(node, m_parent) == m_nextSibling);

This looks wrong. The m_hasValidNextSibling case is for when there is nothing to do. And why are you calling nextSiblingRenderer twice and not doing anything with the result?

[Ryosuke Niwa]

(In reply to comment #5)
> Comment on attachment 261133 [details]
>
> > Source/WebCore/html/HTMLDetailsElement.cpp:82
> > +class DetailsContentDistributor final : public ContentDistributor {
> 
> Instead of having a separate distributor could the normal distributor handle
> this? Details code would add a named slot for summary and a default slot for
> the rest. The only special case left would be make <summary> behave as if it
> had slot="-webkit-summary", right?

I definitely don't want to have a Web-exposed slot name like -webkit-summary in details elements. We shouldn't be introducing that kind of observable vendor prefixed stuff moving forward just for implementation purposes unless there is a compelling use case.

We could add some sort of a virtual function on HTMLSlotElement and override it in HTMLSummaryElement.  However, since the details element picks the very first summary element, the value of this attribute needs to be context sensitive.

I don't think making slot attribute behave polymorphicly like that accomplishes anything. It's exactly the kind of generalization the old shadow DOM code had which ended up bloating the code complexity.  It's better to re-write the whole distribution algorithm.

[Ryosuke Niwa]

Okay, I think we can still reuse named slots with HTMLSlotElement for details elements if we just override the algorithms to select nodes for each slot. Namely, the first summary element will be assigned to a slot named "summary" and the rest will be assigned to the default slot for details elements. Once we've done that slot assignments, then we can rely on the rest of the slotting mechanism to take care of the rest.

So if we had a HashMap<AtomicString, SlotInfo> on ShadowRoot as we talked about earlier, we'll be able to implement this fairly easily by overriding a member function that updates this hash map in a subclass since the distribution doesn't need to modify HTMLSlotElements as SlotInfo will contain the list of distributed nodes for each slot.

[Antti Koivisto]

Sounds good.

[Radar WebKit Bug Importer]

<rdar://problem/22726874>

[Carlos Alberto Lopez Perez]

The shadow-dom tests are marked as expected to fail for all ports on the general TestExpectations file.

I updated this expectation on r201931 <http://trac.webkit.org/changeset/201931/trunk/LayoutTests/TestExpectations> to include also Timeout as expected because some shadow-dom tests were timing out constantly on the GTK+ port.

[Ryosuke Niwa]

(In reply to comment #10)
> The shadow-dom tests are marked as expected to fail for all ports on the
> general TestExpectations file.
> 
> I updated this expectation on r201931
> <http://trac.webkit.org/changeset/201931/trunk/LayoutTests/TestExpectations>
> to include also Timeout as expected because some shadow-dom tests were
> timing out constantly on the GTK+ port.

I think it's time to enable the feature on all ports now!

[Michael Catanzaro]

(In reply to comment #10)
> The shadow-dom tests are marked as expected to fail for all ports on the
> general TestExpectations file.
> 
> I updated this expectation on r201931
> <http://trac.webkit.org/changeset/201931/trunk/LayoutTests/TestExpectations>
> to include also Timeout as expected because some shadow-dom tests were
> timing out constantly on the GTK+ port.

I've removed these expectations since we've enabled shadow DOM; we'll have to find which tests are timing out and mark them individually.