Regression (198943): <marquee> shouldn't wrap text
authorantti@apple.com <antti@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 17 May 2017 15:12:48 +0000 (15:12 +0000)
committerantti@apple.com <antti@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 17 May 2017 15:12:48 +0000 (15:12 +0000)
https://bugs.webkit.org/show_bug.cgi?id=172217

Reviewed by Andreas Kling.

Source/WebCore:

RenderMarquee::updateMarqueeStyle mutated the style and then expected it to inherit to children.
This doesn't work anymore because render tree construction is now separated from style resolution
where inheritance happens.

Test: fast/html/marquee-child-wrap.html

* css/StyleResolver.cpp:
(WebCore::StyleResolver::adjustRenderStyle):

    Implement marquee hacks in adjustRenderStyle instead. This can't do the childrenInline check
    the previous code had but it wasn't working anyway (there are no children when updateMarqueeStyle
    gets called).

* rendering/RenderMarquee.cpp:
(WebCore::RenderMarquee::updateMarqueeStyle):

    This no longer needs mutable style.

LayoutTests:

* fast/html/marquee-child-wrap-expected.html: Added.
* fast/html/marquee-child-wrap.html: Added.

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@216973 268f45cc-cd09-0410-ab3c-d52691b4dbfc

LayoutTests/ChangeLog
LayoutTests/fast/html/marquee-child-wrap-expected.html [new file with mode: 0644]
LayoutTests/fast/html/marquee-child-wrap.html [new file with mode: 0644]
Source/WebCore/ChangeLog
Source/WebCore/css/StyleResolver.cpp
Source/WebCore/rendering/RenderMarquee.cpp

index c56c260..8c3117e 100644 (file)
@@ -1,3 +1,13 @@
+2017-05-17  Antti Koivisto  <antti@apple.com>
+
+        Regression (198943): <marquee> shouldn't wrap text
+        https://bugs.webkit.org/show_bug.cgi?id=172217
+
+        Reviewed by Andreas Kling.
+
+        * fast/html/marquee-child-wrap-expected.html: Added.
+        * fast/html/marquee-child-wrap.html: Added.
+
 2017-05-17  David Kilzer  <ddkilzer@apple.com>
 
         REGRESSION (r216966): fast/shadow-dom/slot-crash.html started to fail
diff --git a/LayoutTests/fast/html/marquee-child-wrap-expected.html b/LayoutTests/fast/html/marquee-child-wrap-expected.html
new file mode 100644 (file)
index 0000000..10f2471
--- /dev/null
@@ -0,0 +1,4 @@
+<div style="width:200px">
+    <div style="border:2px solid green"><span style="visibility:hidden">Text</span></div>
+</div>
+
diff --git a/LayoutTests/fast/html/marquee-child-wrap.html b/LayoutTests/fast/html/marquee-child-wrap.html
new file mode 100644 (file)
index 0000000..df751e8
--- /dev/null
@@ -0,0 +1,3 @@
+<div style="width:200px">
+<marquee style="border:2px solid green"><span>Text text text text text text text text text text text text text text text text text text text text text text text text text text text text text text text text text text text text text</span></marquee>
+</div>
index c623041..7ade803 100644 (file)
@@ -1,3 +1,28 @@
+2017-05-17  Antti Koivisto  <antti@apple.com>
+
+        Regression (198943): <marquee> shouldn't wrap text
+        https://bugs.webkit.org/show_bug.cgi?id=172217
+
+        Reviewed by Andreas Kling.
+
+        RenderMarquee::updateMarqueeStyle mutated the style and then expected it to inherit to children.
+        This doesn't work anymore because render tree construction is now separated from style resolution
+        where inheritance happens.
+
+        Test: fast/html/marquee-child-wrap.html
+
+        * css/StyleResolver.cpp:
+        (WebCore::StyleResolver::adjustRenderStyle):
+
+            Implement marquee hacks in adjustRenderStyle instead. This can't do the childrenInline check
+            the previous code had but it wasn't working anyway (there are no children when updateMarqueeStyle
+            gets called).
+
+        * rendering/RenderMarquee.cpp:
+        (WebCore::RenderMarquee::updateMarqueeStyle):
+
+            This no longer needs mutable style.
+
 2017-05-16  David Kilzer  <ddkilzer@apple.com>
 
         Remove C-style casts by using xmlDocPtr instead of void*
index ad027b4..7074d23 100644 (file)
@@ -930,10 +930,20 @@ void StyleResolver::adjustRenderStyle(RenderStyle& style, const RenderStyle& par
         if (!element->shadowPseudoId().isNull())
             style.setUserModify(READ_ONLY);
 
-        // For now, <marquee> requires an overflow clip to work properly.
         if (is<HTMLMarqueeElement>(*element)) {
+            // For now, <marquee> requires an overflow clip to work properly.
             style.setOverflowX(OHIDDEN);
             style.setOverflowY(OHIDDEN);
+
+            bool isVertical = style.marqueeDirection() == MUP || style.marqueeDirection() == MDOWN;
+            // Make horizontal marquees not wrap.
+            if (!isVertical) {
+                style.setWhiteSpace(NOWRAP);
+                style.setTextAlign(TASTART);
+            }
+            // Apparently this is the expected legacy behavior.
+            if (isVertical && style.height().isAuto())
+                style.setHeight(Length(200, Fixed));
         }
     }
 
index c8704c2..4350280 100644 (file)
@@ -197,7 +197,7 @@ void RenderMarquee::updateMarqueePosition()
 
 void RenderMarquee::updateMarqueeStyle()
 {
-    auto& style = m_layer->renderer().mutableStyle();
+    auto& style = m_layer->renderer().style();
     
     if (m_direction != style.marqueeDirection() || (m_totalLoops != style.marqueeLoopCount() && m_currentLoop >= m_totalLoops))
         m_currentLoop = 0; // When direction changes or our loopCount is a smaller number than our current loop, reset our loop.
@@ -210,23 +210,8 @@ void RenderMarquee::updateMarqueeStyle()
         // one loop.
         if (m_totalLoops <= 0 && style.marqueeBehavior() == MSLIDE)
             m_totalLoops = 1;
-        
-        // Hack alert: Set the white-space value to nowrap for horizontal marquees with inline children, thus ensuring
-        // all the text ends up on one line by default.  Limit this hack to the <marquee> element to emulate
-        // WinIE's behavior.  Someone using CSS3 can use white-space: nowrap on their own to get this effect.
-        // Second hack alert: Set the text-align back to auto.  WinIE completely ignores text-align on the
-        // marquee element.
-        // FIXME: Bring these up with the CSS WG.
-        if (isHorizontal() && m_layer->renderer().childrenInline()) {
-            style.setWhiteSpace(NOWRAP);
-            style.setTextAlign(TASTART);
-        }
     }
     
-    // Legacy hack - multiple browsers default vertical marquees to 200px tall.
-    if (!isHorizontal() && style.height().isAuto())
-        style.setHeight(Length(200, Fixed)); 
-   
     if (speed() != marqueeSpeed()) {
         m_speed = marqueeSpeed();
         if (m_timer.isActive())