Attachment 149947 Details for Bug 90079 – Patch

[patch] Patch

bug-90079-20120628171000.patch (text/plain), 15.94 KB, created by Andrei Bucur on 2012-06-28 07:10:02 PDT

(hide)

Description:

Filename:

MIME Type:

Creator: Andrei Bucur

Created: 2012-06-28 07:10:02 PDT

Size: 15.94 KB

patch

obsolete

>Subversion Revision: 121341
>diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
>index b14d5d3a2a1a36e4a225d61a8b17b21070766e79..e253416d3f08fac000c276d4e2440228cbfb3df6 100755
>--- a/Source/WebCore/ChangeLog
>+++ b/Source/WebCore/ChangeLog
>@@ -1,3 +1,32 @@
>+2012-06-28  Andrei Bucur  <abucur@adobe.com>
>+
>+        [CSS Regions] It should be possible to specify ::BEFORE/AFTER as regions
>+        https://bugs.webkit.org/show_bug.cgi?id=90079
>+
>+        The patch replaces the calls for RenderRegion::node() with RenderRegion::generatingNode() to allow pseudo-elements to have
>+        RenderRegion renderers. Also, the compare function that establishes the region order was adapted to take into account
>+        pseudo-regions.
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        Test: fast/regions/content-flowed-into-pseudo-regions.html
>+
>+        * dom/WebKitNamedFlow.cpp:
>+        (WebCore::WebKitNamedFlow::getRegionsByContentNode):
>+        * rendering/RenderFlowThread.cpp:
>+        (WebCore::RenderFlowThread::regionLayoutUpdateEventTimerFired):
>+        * rendering/RenderNamedFlowThread.cpp:
>+        (WebCore::compareRenderRegions):
>+        * rendering/RenderRegion.cpp:
>+        (WebCore::RenderRegion::nodeAtPoint):
>+        (WebCore::RenderRegion::styleDidChange):
>+        (WebCore::RenderRegion::computeStyleInRegion):
>+        * rendering/RenderRegion.h:
>+        (RenderRegion):
>+        (WebCore::RenderRegion::isChildAllowed):
>+        * rendering/RenderTreeAsText.cpp:
>+        (WebCore::writeRenderNamedFlowThreads):
>+
> 2012-06-27  Oswald Buddenhagen  <oswald.buddenhagen@nokia.com>
> 
>         [Qt] Remove redundant NDEBUG definition
>diff --git a/Source/WebCore/dom/WebKitNamedFlow.cpp b/Source/WebCore/dom/WebKitNamedFlow.cpp
>index 19e122922c3019ab2df6eff6285d036937a8ebe0..1f4e326a5e1899e6cd65d61e23a4816fa18a54ba 100644
>--- a/Source/WebCore/dom/WebKitNamedFlow.cpp
>+++ b/Source/WebCore/dom/WebKitNamedFlow.cpp
>@@ -72,7 +72,9 @@ PassRefPtr<NodeList> WebKitNamedFlow::getRegionsByContentNode(Node* contentNode)
>             const RenderRegion* renderRegion = *iter;
>             if (!renderRegion->isValid())
>                 continue;
>-            if (m_parentFlowThread->objectInFlowRegion(contentNode->renderer(), renderRegion))
>+
>+            // FIXME: Pseudo-regions are not included; the Region interface is required in this case.
>+            if (m_parentFlowThread->objectInFlowRegion(contentNode->renderer(), renderRegion) && renderRegion->node())
>                 regionNodes.append(renderRegion->node());
>         }
>     }
>diff --git a/Source/WebCore/rendering/RenderFlowThread.cpp b/Source/WebCore/rendering/RenderFlowThread.cpp
>index 669d46d4dd808af5c2251acec42d3e65792920c6..6d4cba880b46855e262c0652c22c71c7e625a2f6 100644
>--- a/Source/WebCore/rendering/RenderFlowThread.cpp
>+++ b/Source/WebCore/rendering/RenderFlowThread.cpp
>@@ -718,7 +718,7 @@ void RenderFlowThread::computeOverflowStateForRegions(LayoutUnit oldClientAfterE
>     }
> 
>     // With the regions overflow state computed we can also set the overset flag for the named flow.
>-    // If there are no valid regions in the chain, overset is true
>+    // If there are no valid regions in the chain, overset is true.
>     RenderRegion* lastReg = lastRegion();
>     m_overset = lastReg ? lastReg->regionState() == RenderRegion::RegionOverflow : true;
> }
>@@ -730,12 +730,15 @@ void RenderFlowThread::regionLayoutUpdateEventTimerFired(Timer<RenderFlowThread>
>     regionNodes.reserveCapacity(m_regionList.size());
>     for (RenderRegionList::iterator iter = m_regionList.begin(); iter != m_regionList.end(); ++iter) {
>         RenderRegion* region = *iter;
>-        ASSERT(region->node() && region->node()->isElementNode());
>-        // dispatch the event only for marked regions and only for those who have a listener
>-        if (region->shouldDispatchRegionLayoutUpdateEvent()) {
>-            regionNodes.append(region->node());
>-            // clear the dispatch flag here, as it is possible to be set again due to event listeners
>-            region->setDispatchRegionLayoutUpdateEvent(false);
>+
>+        // FIXME: For pseudo-regions do not dispatch the event; this code will be removed once the event is dispatched on the NamedFlow object.
>+        if (region->node() && region->node()->isElementNode()) {
>+            // Dispatch the event only for marked regions and only for those who have a listener.
>+            if (region->shouldDispatchRegionLayoutUpdateEvent()) {
>+                regionNodes.append(region->node());
>+                // Clear the dispatch flag here, as it is possible to be set again due to event listeners.
>+                region->setDispatchRegionLayoutUpdateEvent(false);
>+            }
>         }
>     }
>     for (Vector<RefPtr<Node> >::const_iterator it = regionNodes.begin(); it != regionNodes.end(); ++it) {
>@@ -746,7 +749,7 @@ void RenderFlowThread::regionLayoutUpdateEventTimerFired(Timer<RenderFlowThread>
>         RenderObject* renderer = node->renderer();
>         if (renderer && renderer->isRenderRegion()) {
>             node->dispatchRegionLayoutUpdateEvent();
>-            // Layout needs to be uptodate after each event listener
>+            // Layout needs to be uptodate after each event listener.
>             document->updateLayoutIgnorePendingStylesheets();
>         }
>     }
>diff --git a/Source/WebCore/rendering/RenderNamedFlowThread.cpp b/Source/WebCore/rendering/RenderNamedFlowThread.cpp
>index 088b87248da355259a20b6990355d88746713df4..7fc57384b4aa64cd6f05570f31622e13fe91510a 100644
>--- a/Source/WebCore/rendering/RenderNamedFlowThread.cpp
>+++ b/Source/WebCore/rendering/RenderNamedFlowThread.cpp
>@@ -129,12 +129,45 @@ static bool compareRenderRegions(const RenderRegion* firstRegion, const RenderRe
>     ASSERT(firstRegion);
>     ASSERT(secondRegion);
> 
>-    // If the regions have the same region-index, compare their position in dom.
>-    ASSERT(firstRegion->node());
>-    ASSERT(secondRegion->node());
>+    ASSERT(firstRegion->generatingNode());
>+    ASSERT(secondRegion->generatingNode());
> 
>-    unsigned short position = firstRegion->node()->compareDocumentPosition(secondRegion->node());
>-    return (position & Node::DOCUMENT_POSITION_FOLLOWING);
>+    // If the regions belong to different nodes, compare their position in the DOM.
>+    if (firstRegion->generatingNode() != secondRegion->generatingNode()) {
>+        unsigned short position = firstRegion->generatingNode()->compareDocumentPosition(secondRegion->generatingNode());
>+
>+        // If the second region is contained in the first one, the first region is "less" if it's :before.
>+        if (position & Node::DOCUMENT_POSITION_CONTAINED_BY) {
>+            ASSERT(secondRegion->style()->styleType() == NOPSEUDO);
>+            return firstRegion->style()->styleType() == BEFORE;
>+        }
>+
>+        // If the second region contains the first region, the first region is "less" if the second is :after.
>+        if (position & Node::DOCUMENT_POSITION_CONTAINS) {
>+            ASSERT(firstRegion->style()->styleType() == NOPSEUDO);
>+            return secondRegion->style()->styleType() == AFTER;
>+        }
>+
>+        return (position & Node::DOCUMENT_POSITION_FOLLOWING);
>+    }
>+
>+    // FIXME: Currently it's not possible for an element to be both a region and have pseudo-children. The case is covered anyway.
>+    switch (firstRegion->style()->styleType()) {
>+    case BEFORE:
>+        // The second region can be the node or the after pseudo-element (before is smaller than any of those).
>+        return true;
>+    case AFTER:
>+        // The second region can be the node or the before pseudo-element (after is greater than any of those).
>+        return false;
>+    case NOPSEUDO:
>+        // The second region can either be the before or the after pseudo-element (the node is only smaller than the after pseudo-element).
>+        return firstRegion->style()->styleType() == AFTER;
>+    default:
>+        break;
>+    }
>+
>+    ASSERT_NOT_REACHED();
>+    return true;
> }
> 
> void RenderNamedFlowThread::addRegionToThread(RenderRegion* renderRegion)
>diff --git a/Source/WebCore/rendering/RenderRegion.cpp b/Source/WebCore/rendering/RenderRegion.cpp
>index 9938dffb06bbf9b8e69e035a9bffd47b723e0b71..d9af43c0e9d91ea41047572eef0764b825dfc2cd 100644
>--- a/Source/WebCore/rendering/RenderRegion.cpp
>+++ b/Source/WebCore/rendering/RenderRegion.cpp
>@@ -134,7 +134,7 @@ bool RenderRegion::nodeAtPoint(const HitTestRequest& request, HitTestResult& res
>         if (m_flowThread && m_flowThread->hitTestRegion(this, request, result, pointInContainer, LayoutPoint(adjustedLocation.x() + borderLeft() + paddingLeft(), adjustedLocation.y() + borderTop() + paddingTop())))
>             return true;
>         updateHitTestResult(result, pointInContainer.point() - toLayoutSize(adjustedLocation));
>-        if (!result.addNodeToRectBasedTestResult(node(), pointInContainer, boundsRect))
>+        if (!result.addNodeToRectBasedTestResult(generatingNode(), pointInContainer, boundsRect))
>             return true;
>     }
> 
>@@ -145,10 +145,13 @@ void RenderRegion::styleDidChange(StyleDifference diff, const RenderStyle* oldSt
> {
>     RenderReplaced::styleDidChange(diff, oldStyle);
>     bool customRegionStyle = false;
>+
>+    // FIXME: Region styling doesn't work for pseudo elements.
>     if (node()) {
>         Element* regionElement = static_cast<Element*>(node());
>         customRegionStyle = view()->document()->styleResolver()->checkRegionStyle(regionElement);
>     }
>+
>     setHasCustomRegionStyle(customRegionStyle);
>     flowThread()->checkRegionsWithStyling();
> }
>@@ -333,6 +336,7 @@ PassRefPtr<RenderStyle> RenderRegion::computeStyleInRegion(const RenderObject* o
>     ASSERT(!object->isAnonymous());
>     ASSERT(object->node() && object->node()->isElementNode());
> 
>+    // FIXME: Region styling fails for pseudo-elements because the renderers don't have a node.
>     Element* element = toElement(object->node());
>     RefPtr<RenderStyle> renderObjectRegionStyle = object->view()->document()->styleResolver()->styleForElement(element, 0, DisallowStyleSharing, MatchAllRules, this);
> 
>diff --git a/Source/WebCore/rendering/RenderRegion.h b/Source/WebCore/rendering/RenderRegion.h
>index 826e9807226b1b34e9b405842a4e95ccb86073e3..2c1d9e52a0230d17fa5e7efcee91c5da26ba5b98 100644
>--- a/Source/WebCore/rendering/RenderRegion.h
>+++ b/Source/WebCore/rendering/RenderRegion.h
>@@ -45,6 +45,9 @@ public:
> 
>     virtual bool isRenderRegion() const { return true; }
> 
>+    // Regions can't have any children.
>+    virtual bool isChildAllowed(RenderObject*, RenderStyle*) const { return false; }
>+
>     virtual void paintReplaced(PaintInfo&, const LayoutPoint&);
>     virtual bool nodeAtPoint(const HitTestRequest&, HitTestResult&, const HitTestPoint& pointInContainer, const LayoutPoint& accumulatedOffset, HitTestAction) OVERRIDE;
> 
>diff --git a/Source/WebCore/rendering/RenderTreeAsText.cpp b/Source/WebCore/rendering/RenderTreeAsText.cpp
>index 59ceebd685c2659f700b4b5d74836b4acf12baef..9201d544e8c2c1390442ced190f4fb70a18b5c42 100644
>--- a/Source/WebCore/rendering/RenderTreeAsText.cpp
>+++ b/Source/WebCore/rendering/RenderTreeAsText.cpp
>@@ -702,12 +702,12 @@ static void writeRenderNamedFlowThreads(TextStream& ts, RenderView* renderView,
>                 RenderRegion* renderRegion = *itRR;
>                 writeIndent(ts, indent + 2);
>                 ts << "RenderRegion";
>-                if (renderRegion->node()) {
>-                    String tagName = getTagName(renderRegion->node());
>+                if (renderRegion->generatingNode()) {
>+                    String tagName = getTagName(renderRegion->generatingNode());
>                     if (!tagName.isEmpty())
>                         ts << " {" << tagName << "}";
>-                    if (renderRegion->node()->isElementNode() && renderRegion->node()->hasID()) {
>-                        Element* element = static_cast<Element*>(renderRegion->node());
>+                    if (renderRegion->generatingNode()->isElementNode() && renderRegion->generatingNode()->hasID()) {
>+                        Element* element = static_cast<Element*>(renderRegion->generatingNode());
>                         ts << " #" << element->idForStyleResolution();
>                     }
>                     if (renderRegion->hasCustomRegionStyle())
>diff --git a/LayoutTests/ChangeLog b/LayoutTests/ChangeLog
>index d1f5ea3c3af26792a70fb432f31d8919630ce81a..877e5dfee1ee86dbd0623039a7064959d9b8ae51 100644
>--- a/LayoutTests/ChangeLog
>+++ b/LayoutTests/ChangeLog
>@@ -1,3 +1,22 @@
>+2012-06-28  Andrei Bucur  <abucur@adobe.com>
>+
>+        [CSS Regions] It should be possible to specify ::BEFORE/AFTER as regions
>+        https://bugs.webkit.org/show_bug.cgi?id=90079
>+
>+        The new ref-test has a block with both a :before and :after pseudo-elements marked as regions.
>+        In the block there's another region.
>+
>+        The test verifies the content is correctly flowed (the region order is respected) in the region chain:
>+        :before region -> child region -> :after region
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        * fast/regions/content-flowed-into-pseudo-regions-expected.html: Added.
>+        * fast/regions/content-flowed-into-pseudo-regions.html: Added.
>+        * fast/regions/resources/region-style.css:
>+        (.regionArticleAfter::after, .regionArticleBefore::before):
>+        (.borderBoxNoFloat):
>+
> 2012-06-27  Alexander Pavlov  <apavlov@chromium.org>
> 
>         Unexpected end of style sheet in @font-face rule discards it rather than closes all open constructs
>diff --git a/LayoutTests/fast/regions/content-flowed-into-pseudo-regions-expected.html b/LayoutTests/fast/regions/content-flowed-into-pseudo-regions-expected.html
>new file mode 100644
>index 0000000000000000000000000000000000000000..18c87143e9d9eeab5f838febaa4025b9e658c3ae
>--- /dev/null
>+++ b/LayoutTests/fast/regions/content-flowed-into-pseudo-regions-expected.html
>@@ -0,0 +1,11 @@
>+<!doctype html>
>+<html>
>+    <head>
>+        <link rel="stylesheet" href="resources/region-style.css">
>+    </head>
>+    <body>
>+        <div class="greenBox borderBoxNoFloat"></div>
>+        <div class="blueBox borderBoxNoFloat"></div>
>+        <div class="orangeBox borderBoxNoFloat"></div>
>+    </body>
>+</html>
>diff --git a/LayoutTests/fast/regions/content-flowed-into-pseudo-regions.html b/LayoutTests/fast/regions/content-flowed-into-pseudo-regions.html
>new file mode 100644
>index 0000000000000000000000000000000000000000..b69e2a2bacd2ff162c2ebe5b4720307f28bc3319
>--- /dev/null
>+++ b/LayoutTests/fast/regions/content-flowed-into-pseudo-regions.html
>@@ -0,0 +1,23 @@
>+<!doctype html>
>+<html>
>+    
>+    <head>
>+        <link rel="stylesheet" href="resources/region-style.css">
>+    </head>
>+    <body>
>+        <div class="flowArticle">
>+            <div class="greenBox"></div>
>+            <div class="blueBox"></div>
>+            <div class="orangeBox"></div>
>+        </div>
>+        <div class="regionArticleBefore regionArticleAfter">
>+            <div class="regionArticle borderBoxNoFloat"></div>
>+        </div>
>+    </body>
>+</html>
>diff --git a/LayoutTests/fast/regions/resources/region-style.css b/LayoutTests/fast/regions/resources/region-style.css
>index 134a748faed51f439d15b21f140561f4ee53b416..26d9f3a01eda990f928daac6ac26369bf8ebc05c 100644
>--- a/LayoutTests/fast/regions/resources/region-style.css
>+++ b/LayoutTests/fast/regions/resources/region-style.css
>@@ -30,6 +30,15 @@
>     -webkit-flow-from: article;
> }
> 
>+.regionArticleAfter::after, .regionArticleBefore::before {
>+    display: block;
>+    width: 50px;
>+    height: 50px;
>+    content: '';
>+    border: 1px solid black;
>+    -webkit-flow-from: article;
>+}
>+
> .borderBox {
>     width: 50px;
>     height: 50px;
>@@ -37,6 +46,12 @@
>     border: 1px solid black;
> }
> 
>+.borderBoxNoFloat {
>+    width: 50px;
>+    height: 50px;
>+    border: 1px solid black;
>+}
>+
> .pass {
>     font-weight: bold;
>     color: green;

Actions: View | Formatted Diff | Diff

Attachments on bug 90079: 149947 | 161429 | 161683