Bug 148695 - Implement v1 shadow DOM API
Summary: Implement v1 shadow DOM API
Status: NEW
Alias: None
Product: WebKit
Classification: Unclassified
Component: HTML DOM (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Ryosuke Niwa
URL:
Keywords: InRadar, WebExposed
Depends on: 149443 151380 157375 157437 157443 157506 162645 166484 167652 169948 170211 172446 173027 174288 118803 148694 149148 149230 149242 149243 149330 149363 149434 149437 149439 149440 149441 149445 149447 149589 149591 149592 149593 149698 149701 149702 149739 149782 149997 150162 150237 151379 153533 153534 153537 153538 153577 153681 154770 155226 155388 155424 157063 157225 157245 157369 157374 157444 157475 157705 157706 157789 158241 158378 158415 158421 158471 158503 158550 158686 158900 159474 159482 160038 160331 160427 160538 160683 160740 160754 160864 160866 160919 162091 162457 162629 162656 162744 162882 164002 164577 164608 164609 164675 164693 164770 165146 165551 165647 165757 170762
Blocks:
  Show dependency treegraph
 
Reported: 2015-09-01 16:44 PDT by Ryosuke Niwa
Modified: 2017-07-08 02:07 PDT (History)
18 users (show)

See Also:


Attachments
WIP1 (90.92 KB, patch)
2015-09-10 07:38 PDT, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
WIP2 (125.14 KB, patch)
2015-09-14 15:01 PDT, Ryosuke Niwa
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ryosuke Niwa 2015-09-01 16:44:10 PDT
Implement the newly spec'ed slot shadow DOM API:
https://w3c.github.io/webcomponents/spec/shadow/
Comment 1 Ryosuke Niwa 2015-09-10 07:38:22 PDT
Created attachment 260929 [details]
WIP1
Comment 2 Ryosuke Niwa 2015-09-14 15:01:44 PDT
Created attachment 261133 [details]
WIP2
Comment 3 Chris Dumez 2015-09-14 15:29:09 PDT
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?
Comment 4 Chris Dumez 2015-09-14 15:42:11 PDT
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*
Comment 5 Antti Koivisto 2015-09-14 22:14:53 PDT
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?
Comment 6 Ryosuke Niwa 2015-09-14 22:34:20 PDT
(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.
Comment 7 Ryosuke Niwa 2015-09-14 23:36:48 PDT
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.
Comment 8 Antti Koivisto 2015-09-15 11:29:41 PDT
Sounds good.
Comment 9 Radar WebKit Bug Importer 2015-09-16 14:07:53 PDT
<rdar://problem/22726874>
Comment 10 Carlos Alberto Lopez Perez 2016-06-10 11:42:16 PDT
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.
Comment 11 Ryosuke Niwa 2016-06-10 13:34:56 PDT
(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!
Comment 12 Michael Catanzaro 2016-06-19 16:41:15 PDT
(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.