Bug 149242 - HTMLSlotElement should render its assigned nodes
Summary: HTMLSlotElement should render its assigned nodes
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Antti Koivisto
URL:
Keywords: InRadar
Depends on: 149241
Blocks: 148695
  Show dependency treegraph
 
Reported: 2015-09-16 18:07 PDT by Ryosuke Niwa
Modified: 2015-09-21 15:28 PDT (History)
9 users (show)

See Also:


Attachments
patch (10.29 KB, patch)
2015-09-21 14:51 PDT, Antti Koivisto
rniwa: review+
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-16 18:07:18 PDT
Support rendering the assigned nodes under HTMLSlotElement added in the bug 149241.
Comment 1 Radar WebKit Bug Importer 2015-09-17 18:11:36 PDT
<rdar://problem/22750917>
Comment 2 Antti Koivisto 2015-09-21 14:51:54 PDT
Created attachment 261686 [details]
patch
Comment 3 Ryosuke Niwa 2015-09-21 15:06:11 PDT
Comment on attachment 261686 [details]
patch

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

> Source/WebCore/dom/SlotAssignment.cpp:187
> +        auto defaultSlotEntry = m_slots.find(emptyAtom);

!? This would result in a hash lookup in each iteration.

> Source/WebCore/style/StyleResolveTree.cpp:473
> +static void attachSlot(HTMLSlotElement& slot, RenderStyle& inheritedStyle, RenderTreePosition& renderTreePosition)

Attach sounds like we're attaching a renderer for a slot.
Maybe attachSlotContents would sound better?
attachSlotAssignee?

> Source/WebCore/style/StyleResolveTree.cpp:481
> +                continue;
> +            }
> +            if (is<Element>(*child))

Why don't we just do elseif?

> Source/WebCore/style/StyleResolveTree.cpp:586
> +                continue;
> +            }
> +            if (is<Element>(*child))

Why don't we just do "else if" instead?

> Source/WebCore/style/StyleResolveTree.cpp:616
> +#if ENABLE(SHADOW_DOM)
> +    else if (is<HTMLSlotElement>(current))
> +        detachSlot(downcast<HTMLSlotElement>(current), detachType);
> +#endif
> +    else if (ShadowRoot* shadowRoot = current.shadowRoot())
>          detachShadowRoot(*shadowRoot, detachType);

What happens when a slot element has its own shadow root?

> Source/WebCore/style/StyleResolveTree.cpp:857
> +                continue;
> +            }
> +            if (is<Element>(*child))

Ditto.
Comment 4 Antti Koivisto 2015-09-21 15:16:24 PDT
(In reply to comment #3)
> Comment on attachment 261686 [details]
> patch
> 
> View in context:
> !? This would result in a hash lookup in each iteration.

This fixes a bug, the cached iterator becomes invalid after mutation.
 
> > Source/WebCore/style/StyleResolveTree.cpp:616
> > +#if ENABLE(SHADOW_DOM)
> > +    else if (is<HTMLSlotElement>(current))
> > +        detachSlot(downcast<HTMLSlotElement>(current), detachType);
> > +#endif
> > +    else if (ShadowRoot* shadowRoot = current.shadowRoot())
> >          detachShadowRoot(*shadowRoot, detachType);
> 
> What happens when a slot element has its own shadow root?

We ignore shadow root for slot element when creating the render tree so no need to detach it either. I assume the spec forbids it.
Comment 5 Antti Koivisto 2015-09-21 15:28:16 PDT
https://trac.webkit.org/r190084