Attachment 275500 Details for Bug 156144 – patch

[patch] patch

slot-style-2.patch (text/plain), 12.26 KB, created by Antti Koivisto on 2016-04-03 08:03:04 PDT

(hide)

Description:

Filename:

MIME Type:

Creator: Antti Koivisto

Created: 2016-04-03 08:03:04 PDT

Size: 12.26 KB

patch

obsolete

>Index: Source/WebCore/ChangeLog
>===================================================================
>--- Source/WebCore/ChangeLog	(revision 198984)
>+++ Source/WebCore/ChangeLog	(working copy)
>@@ -1,3 +1,55 @@
>+2016-04-03  Antti Koivisto  <antti@apple.com>
>+
>+        Shadow DOM: Slot style is not computed
>+        https://bugs.webkit.org/show_bug.cgi?id=156144
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        We donât currently compute style for active slots. While slots have have implicit display:contents and donât create
>+        boxes themselves the style should still inherit to slotted children.
>+
>+        Basically
>+
>+        <slot style=âcolor:redâ></slot>
>+
>+        should work as expected.
>+
>+        The implementation basically falls out from the new style resolve architecture and this patch mostly just removes
>+        special case code that prevented this from working.
>+
>+        Test: fast/shadow-dom/css-scoping-shadow-slot-style.html
>+
>+        * html/HTMLSlotElement.h:
>+        (WebCore::hasImplicitDisplayContents):
>+
>+            Move to shared location.
>+
>+        * style/RenderTreeUpdater.cpp:
>+        (WebCore::RenderTreeUpdater::RenderTreeUpdater):
>+        (WebCore::findRenderingRoot):
>+        (WebCore::RenderTreeUpdater::updateRenderTree):
>+
>+            Remove the special case code. What remains is a display:contents test for rendererless elements.
>+
>+        (WebCore::RenderTreeUpdater::updateElementRenderer):
>+
>+            Don't create renderers for (implicit) display:contents.
>+
>+        (WebCore::hasDisplayContents): Deleted.
>+        * style/StyleTreeResolver.cpp:
>+        (WebCore::Style::detachRenderTree):
>+        (WebCore::Style::affectsRenderedSubtree):
>+
>+            Factor into a function.
>+
>+        (WebCore::Style::TreeResolver::resolveElement):
>+
>+            Remove the special case code.
>+
>+        (WebCore::Style::TreeResolver::resolveComposedTree):
>+
>+            Always resolve slots as we don't currently save their style.
>+
> 2016-04-03  Carlos Garcia Campos  <cgarcia@igalia.com>
> 
>         Replace all RenderTheme::popupInternalPadding methods with a single one returning a LengthBox
>Index: Source/WebCore/html/HTMLSlotElement.h
>===================================================================
>--- Source/WebCore/html/HTMLSlotElement.h	(revision 198944)
>+++ Source/WebCore/html/HTMLSlotElement.h	(working copy)
>@@ -53,6 +53,17 @@ private:
>     bool m_hasEnqueuedSlotChangeEvent { false };
> };
> 
>+// Slots have implicit display:contents until it is supported for reals.
>+inline bool hasImplicitDisplayContents(const Element& element) { return is<HTMLSlotElement>(element); }
>+
>+}
>+
>+#else
>+
>+namespace WebCore {
>+
>+inline bool hasImplicitDisplayContents(const Element&) { return false; }
>+
> }
> 
> #endif
>Index: Source/WebCore/style/RenderTreePosition.cpp
>===================================================================
>--- Source/WebCore/style/RenderTreePosition.cpp	(revision 198944)
>+++ Source/WebCore/style/RenderTreePosition.cpp	(working copy)
>@@ -89,7 +89,7 @@ RenderObject* RenderTreePosition::nextSi
> 
>     while (it != end) {
>         auto& node = *it;
>-        bool hasDisplayContents = is<HTMLSlotElement>(node);
>+        bool hasDisplayContents = is<Element>(node) && hasImplicitDisplayContents(downcast<Element>(node));
>         if (hasDisplayContents) {
>             it.traverseNext();
>             continue;
>Index: Source/WebCore/style/RenderTreeUpdater.cpp
>===================================================================
>--- Source/WebCore/style/RenderTreeUpdater.cpp	(revision 198944)
>+++ Source/WebCore/style/RenderTreeUpdater.cpp	(working copy)
>@@ -61,19 +61,13 @@ RenderTreeUpdater::RenderTreeUpdater(Doc
> {
> }
> 
>-// Slots have implicit display:contents until it is supported for reals.
>-static bool hasDisplayContents(const Node& node)
>-{
>-    return is<HTMLSlotElement>(node);
>-}
>-
> static ContainerNode& findRenderingRoot(ContainerNode& node)
> {
>     auto& document = node.document();
>     for (ComposedTreeAncestorIterator it(document, node), end(document); it != end; ++it) {
>         if (it->renderer())
>             return *it;
>-        ASSERT(hasDisplayContents(*it));
>+        ASSERT(hasImplicitDisplayContents(downcast<Element>(*it)));
>     }
>     ASSERT_NOT_REACHED();
>     return document;
>@@ -137,29 +131,20 @@ void RenderTreeUpdater::updateRenderTree
>         auto& element = downcast<Element>(node);
> 
>         auto* elementUpdate = m_styleUpdate->elementUpdate(element);
>-
>-        auto changeType = Style::NoChange;
>-        if (elementUpdate) {
>-            if (hasDisplayContents(element)) {
>-                if (!shouldCreateRenderer(element, renderTreePosition().parent())) {
>-                    it.traverseNextSkippingChildren();
>-                    continue;
>-                }
>-                pushParent(element, parent().styleChange);
>-                it.traverseNext();
>-                continue;
>-            }
>-
>-            updateElementRenderer(element, *elementUpdate);
>-            changeType = elementUpdate->change;
>+        if (!elementUpdate) {
>+            it.traverseNextSkippingChildren();
>+            continue;
>         }
> 
>-        if (!element.renderer() || !elementUpdate) {
>+        updateElementRenderer(element, *elementUpdate);
>+
>+        bool mayHaveRenderedDescendants = element.renderer() || (hasImplicitDisplayContents(element) && shouldCreateRenderer(element, renderTreePosition().parent()));
>+        if (!mayHaveRenderedDescendants) {
>             it.traverseNextSkippingChildren();
>             continue;
>         }
> 
>-        pushParent(element, changeType);
>+        pushParent(element, elementUpdate ? elementUpdate->change : Style::NoChange);
> 
>         it.traverseNext();
>     }
>@@ -246,7 +231,7 @@ void RenderTreeUpdater::updateElementRen
>     if (shouldTearDownRenderers)
>         detachRenderTree(element, Style::ReattachDetach);
> 
>-    bool shouldCreateNewRenderer = !element.renderer() && update.style;
>+    bool shouldCreateNewRenderer = !element.renderer() && update.style && !hasImplicitDisplayContents(element);
>     if (shouldCreateNewRenderer) {
>         if (element.hasCustomStyleResolveCallbacks())
>             element.willAttachRenderers();
>Index: Source/WebCore/style/StyleTreeResolver.cpp
>===================================================================
>--- Source/WebCore/style/StyleTreeResolver.cpp	(revision 198944)
>+++ Source/WebCore/style/StyleTreeResolver.cpp	(working copy)
>@@ -385,14 +385,29 @@ void detachRenderTree(Element& current, 
>         current.didDetachRenderers();
> }
> 
>+static bool affectsRenderedSubtree(Element& element, const RenderStyle& newStyle)
>+{
>+    if (element.renderer())
>+        return true;
>+    if (newStyle.display() != NONE)
>+        return true;
>+    // FIXME: Make 'contents' an actual display property value.
>+    if (hasImplicitDisplayContents(element))
>+        return true;
>+    if (element.rendererIsNeeded(newStyle))
>+        return true;
>+    if (element.shouldMoveToFlowThread(newStyle))
>+        return true;
>+    return false;
>+}
>+
> ElementUpdate TreeResolver::resolveElement(Element& element)
> {
>     auto newStyle = styleForElement(element, parent().style);
> 
>     auto* renderer = element.renderer();
> 
>-    bool affectsRenderedSubtree = renderer || newStyle->display() != NONE || element.rendererIsNeeded(newStyle) || element.shouldMoveToFlowThread(newStyle);
>-    if (!affectsRenderedSubtree)
>+    if (!affectsRenderedSubtree(element, newStyle.get()))
>         return { };
> 
>     ElementUpdate update;
>@@ -599,7 +614,7 @@ void TreeResolver::resolveComposedTree()
>         ElementUpdate update;
>         update.style = element.renderStyle();
> 
>-        bool shouldResolve = parent.change >= Inherit || element.needsStyleRecalc() || shouldResolveForPseudoElement || affectedByPreviousSibling;
>+        bool shouldResolve = parent.change >= Inherit || element.needsStyleRecalc() || shouldResolveForPseudoElement || affectedByPreviousSibling || hasImplicitDisplayContents(element);
>         if (shouldResolve) {
> #if PLATFORM(IOS)
>             CheckForVisibilityChangeOnRecalcStyle checkForVisibilityChange(&element, element.renderStyle());
>@@ -627,21 +642,6 @@ void TreeResolver::resolveComposedTree()
>             element.clearNeedsStyleRecalc();
>         }
> 
>-
>-#if ENABLE(SHADOW_DOM) || ENABLE(DETAILS_ELEMENT)
>-        if (is<HTMLSlotElement>(element)) {
>-            // FIXME: We should compute style for the slot and use it as parent style.
>-            // Duplicate the style from the parent context.
>-            ElementUpdate slotUpdate;
>-            slotUpdate.style = parent.style.ptr();
>-            slotUpdate.change = update.change;
>-            if (!shouldResolve)
>-                m_update->addElement(element, parent.element, update);
>-            pushParent(element, slotUpdate);
>-            it.traverseNext();
>-            continue;
>-        }
>-#endif
>         if (!update.style) {
>             resetStyleForNonRenderedDescendants(element);
>             element.clearChildNeedsStyleRecalc();
>Index: LayoutTests/ChangeLog
>===================================================================
>--- LayoutTests/ChangeLog	(revision 198944)
>+++ LayoutTests/ChangeLog	(working copy)
>@@ -1,3 +1,13 @@
>+2016-04-03  Antti Koivisto  <antti@apple.com>
>+
>+        Shadow DOM: Slot style is not computed
>+        https://bugs.webkit.org/show_bug.cgi?id=156144
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        * fast/shadow-dom/css-scoping-shadow-slot-style-expected.html: Added.
>+        * fast/shadow-dom/css-scoping-shadow-slot-style.html: Added.
>+
> 2016-04-01  Joonghun Park  <jh718.park@samsung.com>
> 
>         Unreviewed EFL gardening about new RTL scrollbar testcase
>Index: LayoutTests/editing/execCommand/justify-right-then-indent-with-problematic-body-expected.txt
>===================================================================
>--- LayoutTests/editing/execCommand/justify-right-then-indent-with-problematic-body-expected.txt	(revision 198944)
>+++ LayoutTests/editing/execCommand/justify-right-then-indent-with-problematic-body-expected.txt	(working copy)
>@@ -1,3 +1,4 @@
> Pass.
>+
> WebKit didn't Crash.
> 
>Index: LayoutTests/fast/shadow-dom/css-scoping-shadow-slot-style-expected.html
>===================================================================
>--- LayoutTests/fast/shadow-dom/css-scoping-shadow-slot-style-expected.html	(revision 0)
>+++ LayoutTests/fast/shadow-dom/css-scoping-shadow-slot-style-expected.html	(working copy)
>@@ -0,0 +1,7 @@
>+<!DOCTYPE html>
>+<html>
>+<body>
>+    <p>Test passes if you see a single 100px by 100px green box below.</p>
>+    <div style="width: 100px; height: 100px; background: green;"></div>
>+</body>
>+</html>
>Index: LayoutTests/fast/shadow-dom/css-scoping-shadow-slot-style.html
>===================================================================
>--- LayoutTests/fast/shadow-dom/css-scoping-shadow-slot-style.html	(revision 0)
>+++ LayoutTests/fast/shadow-dom/css-scoping-shadow-slot-style.html	(working copy)
>@@ -0,0 +1,47 @@
>+<!DOCTYPE html>
>+<html>
>+<head>
>+    <title>CSS Scoping - ensure that slot style is inherited by slotted children</title>
>+    <link rel="author" title="Ryosuke Niwa" href="mailto:rniwa@webkit.org"/>
>+    <link rel="help" href="http://www.w3.org/TR/css-scoping-1/#selectors-data-model">
>+    <link rel="match" href="reference/green-box.html"/>
>+</head>
>+<body>
>+    <style>
>+    my-host, my-non-host {
>+        display: block;
>+        width: 100px;
>+        height: 50px;
>+        overflow: hidden;
>+        background: red;
>+        color: red;
>+    }
>+    div {
>+        width: 100%;
>+        height: 50%;
>+    }
>+    </style>
>+    <p>Test passes if you see a single 100px by 100px green box below.</p> 
>+    <my-host>
>+        <div slot="green" style="background: green;">FAIL</div>
>+        <div slot="green" style="background: inherit;">FAIL</div>
>+    </my-host>
>+    <my-non-host>
>+        <slot name="green" style="color: green; background: green">
>+            <div slot="green" style="background: green;">FAIL</div>
>+            <div slot="green" style="background: inherit;">FAIL</div>
>+        </slot>
>+    </my-non-host>
>+    <script>
>+
>+    try {
>+        var shadowHost = document.querySelector('my-host');
>+        shadowRoot = shadowHost.attachShadow({mode: 'open'});
>+        shadowRoot.innerHTML = '<slot name="green" style="color: green; background: green"></slot>';
>+    } catch (exception) {
>+        document.body.appendChild(document.createTextNode(exception));
>+    }
>+
>+    </script>
>+</body>
>+</html>

Flags:

darin: review+

Actions: View | Formatted Diff | Diff

Attachments on bug 156144: 275495 | 275496 | 275497 | 275498 | 275499 | 275500