Title: [136331] trunk
Revision
136331
Author
[email protected]
Date
2012-12-02 03:38:01 -0800 (Sun, 02 Dec 2012)

Log Message

documentElement should not always get a renderer
https://bugs.webkit.org/show_bug.cgi?id=103475

Patch by Elliott Sprehn <[email protected]> on 2012-12-02
Reviewed by Ojan Vafai.

Source/WebCore:

Only SVG documents need a renderer on the documentElement when it's
display: none. This behavior is wrong, but there's assumptions in
FrameView and SVGImage that depend on a RenderSVGRoot existing for all
loaded SVG document and images.

The current code has logic that makes it appear as if we always create
a renderer for the documentElement even when it's display: none, but
because HTMLElement::createRenderer doesn't call super we end up with
HTMLElement::rendererIsNeeded returning true for display: none, but
HTMLElement::createRenderer returning null.

Instead of this generic documentElement logic only special case SVG
until someone can fix Bug 103493 so we don't need it.

Test: fast/css/display-none-documentElement.html

* dom/Element.cpp:
(WebCore::Element::rendererIsNeeded):
(WebCore::Element::createRenderer):
* svg/SVGSVGElement.cpp:
(WebCore::SVGSVGElement::rendererIsNeeded):
* svg/SVGSVGElement.h:
(SVGSVGElement):

LayoutTests:

Test that using display: none on the documentElement hides the document.

* fast/css/display-none-documentElement-expected.html: Added.
* fast/css/display-none-documentElement.html: Added.

Modified Paths

Added Paths

Diff

Modified: trunk/LayoutTests/ChangeLog (136330 => 136331)


--- trunk/LayoutTests/ChangeLog	2012-12-02 11:30:50 UTC (rev 136330)
+++ trunk/LayoutTests/ChangeLog	2012-12-02 11:38:01 UTC (rev 136331)
@@ -1,3 +1,15 @@
+2012-12-02  Elliott Sprehn  <[email protected]>
+
+        documentElement should not always get a renderer
+        https://bugs.webkit.org/show_bug.cgi?id=103475
+
+        Reviewed by Ojan Vafai.
+
+        Test that using display: none on the documentElement hides the document.
+
+        * fast/css/display-none-documentElement-expected.html: Added.
+        * fast/css/display-none-documentElement.html: Added.
+
 2012-12-02  Pan Deng  <[email protected]>
 
         [Resource Timing]Test cases of cross origin resource timing.

Added: trunk/LayoutTests/fast/css/display-none-documentElement-expected.html (0 => 136331)


--- trunk/LayoutTests/fast/css/display-none-documentElement-expected.html	                        (rev 0)
+++ trunk/LayoutTests/fast/css/display-none-documentElement-expected.html	2012-12-02 11:38:01 UTC (rev 136331)
@@ -0,0 +1,5 @@
+<!DOCTYPE html>
+
+<iframe></iframe>
+
+<iframe></iframe>

Added: trunk/LayoutTests/fast/css/display-none-documentElement.html (0 => 136331)


--- trunk/LayoutTests/fast/css/display-none-documentElement.html	                        (rev 0)
+++ trunk/LayoutTests/fast/css/display-none-documentElement.html	2012-12-02 11:38:01 UTC (rev 136331)
@@ -0,0 +1,14 @@
+<!DOCTYPE html>
+
+<iframe srcdoc="<html style='display: none'>Foo"></iframe>
+
+<iframe id="doc"></iframe>
+
+<script>
+    // Test display:none on a non-HTMLElement that is the documentElement.
+    var doc = document.getElementById('doc').contentDocument;
+    var root = doc.implementation.createDocument().createElement('not-a-real-element');
+    doc.replaceChild(root, doc.documentElement);
+    root.appendChild(document.createElement('style')).textContent = 'not-a-real-element { display: none }';
+    root.appendChild(document.createElement('p')).textContent = 'Should not be visible';
+</script>

Modified: trunk/Source/WebCore/ChangeLog (136330 => 136331)


--- trunk/Source/WebCore/ChangeLog	2012-12-02 11:30:50 UTC (rev 136330)
+++ trunk/Source/WebCore/ChangeLog	2012-12-02 11:38:01 UTC (rev 136331)
@@ -1,3 +1,34 @@
+2012-12-02  Elliott Sprehn  <[email protected]>
+
+        documentElement should not always get a renderer
+        https://bugs.webkit.org/show_bug.cgi?id=103475
+
+        Reviewed by Ojan Vafai.
+
+        Only SVG documents need a renderer on the documentElement when it's
+        display: none. This behavior is wrong, but there's assumptions in
+        FrameView and SVGImage that depend on a RenderSVGRoot existing for all
+        loaded SVG document and images.
+
+        The current code has logic that makes it appear as if we always create
+        a renderer for the documentElement even when it's display: none, but
+        because HTMLElement::createRenderer doesn't call super we end up with
+        HTMLElement::rendererIsNeeded returning true for display: none, but
+        HTMLElement::createRenderer returning null.
+
+        Instead of this generic documentElement logic only special case SVG
+        until someone can fix Bug 103493 so we don't need it.
+
+        Test: fast/css/display-none-documentElement.html
+
+        * dom/Element.cpp:
+        (WebCore::Element::rendererIsNeeded):
+        (WebCore::Element::createRenderer):
+        * svg/SVGSVGElement.cpp:
+        (WebCore::SVGSVGElement::rendererIsNeeded):
+        * svg/SVGSVGElement.h:
+        (SVGSVGElement):
+
 2012-12-02  Pan Deng  <[email protected]>
 
         [Resource Timing] implementation of cross origin resouce timing restrictions.

Modified: trunk/Source/WebCore/dom/Element.cpp (136330 => 136331)


--- trunk/Source/WebCore/dom/Element.cpp	2012-12-02 11:30:50 UTC (rev 136330)
+++ trunk/Source/WebCore/dom/Element.cpp	2012-12-02 11:38:01 UTC (rev 136331)
@@ -1054,18 +1054,11 @@
 
 bool Element::rendererIsNeeded(const NodeRenderingContext& context)
 {
-    return (document()->documentElement() == this) || (context.style()->display() != NONE);
+    return context.style()->display() != NONE;
 }
 
-RenderObject* Element::createRenderer(RenderArena* arena, RenderStyle* style)
+RenderObject* Element::createRenderer(RenderArena*, RenderStyle* style)
 {
-    if (document()->documentElement() == this && style->display() == NONE) {
-        // Ignore display: none on root elements.  Force a display of block in that case.
-        RenderBlock* result = new (arena) RenderBlock(this);
-        if (result)
-            result->setAnimatableStyle(style);
-        return result;
-    }
     return RenderObject::createObject(this, style);
 }
 

Modified: trunk/Source/WebCore/svg/SVGSVGElement.cpp (136330 => 136331)


--- trunk/Source/WebCore/svg/SVGSVGElement.cpp	2012-12-02 11:30:50 UTC (rev 136330)
+++ trunk/Source/WebCore/svg/SVGSVGElement.cpp	2012-12-02 11:38:01 UTC (rev 136331)
@@ -470,6 +470,17 @@
     return transform.multiply(viewBoxTransform);
 }
 
+bool SVGSVGElement::rendererIsNeeded(const NodeRenderingContext& context)
+{
+    // FIXME: We should respect display: none on the documentElement svg element
+    // but many things in FrameView and SVGImage depend on the RenderSVGRoot when
+    // they should instead depend on the RenderView.
+    // https://bugs.webkit.org/show_bug.cgi?id=103493
+    if (document()->documentElement() == this)
+        return true;
+    return StyledElement::rendererIsNeeded(context);
+}
+
 RenderObject* SVGSVGElement::createRenderer(RenderArena* arena, RenderStyle*)
 {
     if (isOutermostSVGSVGElement())

Modified: trunk/Source/WebCore/svg/SVGSVGElement.h (136330 => 136331)


--- trunk/Source/WebCore/svg/SVGSVGElement.h	2012-12-02 11:30:50 UTC (rev 136330)
+++ trunk/Source/WebCore/svg/SVGSVGElement.h	2012-12-02 11:38:01 UTC (rev 136331)
@@ -146,7 +146,7 @@
     
     virtual void parseAttribute(const QualifiedName&, const AtomicString&) OVERRIDE;
 
-    virtual bool rendererIsNeeded(const NodeRenderingContext& context) { return StyledElement::rendererIsNeeded(context); }
+    virtual bool rendererIsNeeded(const NodeRenderingContext&) OVERRIDE;
     virtual RenderObject* createRenderer(RenderArena*, RenderStyle*);
 
     virtual InsertionNotificationRequest insertedInto(ContainerNode*) OVERRIDE;
_______________________________________________
webkit-changes mailing list
[email protected]
http://lists.webkit.org/mailman/listinfo/webkit-changes

Reply via email to