Flex boxes (both old and new) don't handle max-height images correctly.
authorhyatt@apple.com <hyatt@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 26 Jun 2013 17:31:26 +0000 (17:31 +0000)
committerhyatt@apple.com <hyatt@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 26 Jun 2013 17:31:26 +0000 (17:31 +0000)
https://bugs.webkit.org/show_bug.cgi?id=118000

Reviewed by Beth Dakin.

Source/WebCore:

Tests: css3/flexbox/image-percent-max-height.html
       fast/flexbox/image-percent-max-height.html

* rendering/RenderBlock.cpp:
(WebCore::RenderBlock::dirtyForLayoutFromPercentageHeightDescendants):
(WebCore::RenderBlock::layoutBlockChildren):
Pull the percentage height descendant code that dirties those descendants
out of layoutBlockChildren and into a protected helper function,
dirtyForLayoutFromPercentageHeightDescendants, that can be called from the
flex box code.

Also patch dirtyForLayoutFromPercentageHeightDescendants so that it will dirty
preferred logical widths when a child has an aspect ratio, since we know that
percentage height changes will potentially affect the preferred widths of the image and
its ancestor blocks.

* rendering/RenderBlock.h:
Declaration of the new dirtyForLayoutFromPercentageHeightDescendants function.

* rendering/RenderDeprecatedFlexibleBox.cpp:
(WebCore::RenderDeprecatedFlexibleBox::layoutBlock):
Make the old flex box code call dirtyForLayoutFromPercentageHeightDescendants so
that everything is dirtied properly.

(WebCore::RenderDeprecatedFlexibleBox::layoutHorizontalBox):
(WebCore::RenderDeprecatedFlexibleBox::layoutVerticalBox):
Remove the isReplaced()/percentage height/width dirtying now that the old flexible
box is using the same dirtying mechanism as RenderBlock.

* rendering/RenderFlexibleBox.cpp:
(WebCore::RenderFlexibleBox::layoutBlock):
Patch the new flexible box code to use the dirtying mechanism that RenderBlock
uses for percentage heights/widths on replaced descendants.

* rendering/RenderObject.h:
(WebCore::RenderObject::hasAspectRatio):
Pulled the static helper function from RenderReplaced into a full-blown method
on RenderObject, so that dirtyForLayoutFromPercentageHeightDescendants can call
it to check if an object has an aspect ratio.

* rendering/RenderReplaced.cpp:
(WebCore::RenderReplaced::computeAspectRatioInformationForRenderBox):
(WebCore::RenderReplaced::computeIntrinsicRatioInformation):
Patch the call sites of the static helper function to use hasAspectRatio instead
and get rid of the static in the cpp file.

LayoutTests:

* css3/flexbox/image-percent-max-height-expected.html: Added.
* css3/flexbox/image-percent-max-height.html: Added.
* css3/flexbox/resources/hero.png: Added.
* fast/flexbox/image-percent-max-height-expected.html: Added.
* fast/flexbox/image-percent-max-height.html: Added.
* fast/flexbox/resources/hero.png: Added.

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

15 files changed:
LayoutTests/ChangeLog
LayoutTests/css3/flexbox/image-percent-max-height-expected.html [new file with mode: 0644]
LayoutTests/css3/flexbox/image-percent-max-height.html [new file with mode: 0644]
LayoutTests/css3/flexbox/resources/hero.png [new file with mode: 0644]
LayoutTests/fast/flexbox/image-percent-max-height-expected.html [new file with mode: 0644]
LayoutTests/fast/flexbox/image-percent-max-height.html [new file with mode: 0644]
LayoutTests/fast/flexbox/resources/hero.png [new file with mode: 0644]
LayoutTests/tables/mozilla_expected_failures/bugs/bug85016-expected.txt
Source/WebCore/ChangeLog
Source/WebCore/rendering/RenderBlock.cpp
Source/WebCore/rendering/RenderBlock.h
Source/WebCore/rendering/RenderDeprecatedFlexibleBox.cpp
Source/WebCore/rendering/RenderFlexibleBox.cpp
Source/WebCore/rendering/RenderObject.h
Source/WebCore/rendering/RenderReplaced.cpp

index d8fa2cd..49e8989 100644 (file)
@@ -1,3 +1,17 @@
+2013-06-25  David Hyatt  <hyatt@apple.com>
+
+        Flex boxes (both old and new) don't handle max-height images correctly.
+        https://bugs.webkit.org/show_bug.cgi?id=118000
+
+        Reviewed by Beth Dakin.
+
+        * css3/flexbox/image-percent-max-height-expected.html: Added.
+        * css3/flexbox/image-percent-max-height.html: Added.
+        * css3/flexbox/resources/hero.png: Added.
+        * fast/flexbox/image-percent-max-height-expected.html: Added.
+        * fast/flexbox/image-percent-max-height.html: Added.
+        * fast/flexbox/resources/hero.png: Added.
+
 2013-06-26  Ryosuke Niwa  <rniwa@webkit.org>
 
         editing/selection/doubleclick-crash.html can be flaky
diff --git a/LayoutTests/css3/flexbox/image-percent-max-height-expected.html b/LayoutTests/css3/flexbox/image-percent-max-height-expected.html
new file mode 100644 (file)
index 0000000..1596793
--- /dev/null
@@ -0,0 +1,25 @@
+<style>
+
+.wrapper {
+  top: 0;
+  left: 0;
+  position: absolute;
+  width: 100%;
+  height:100%;
+
+  background: #080;
+}
+
+img {
+  height:400px;
+  background: #00F;
+  margin-left:auto;
+  margin-right:auto;
+  display:block;
+}
+
+</style>
+
+<div id="test" style="width:400px;height:400px; background-color:#080">
+  <img id="between" src="resources/hero.png">
+</div>
diff --git a/LayoutTests/css3/flexbox/image-percent-max-height.html b/LayoutTests/css3/flexbox/image-percent-max-height.html
new file mode 100644 (file)
index 0000000..c696d8f
--- /dev/null
@@ -0,0 +1,39 @@
+<style>
+
+.wrapper {
+  top: 0;
+  left: 0;
+  position: absolute;
+  width: 100%;
+  height:100%;
+
+  display: -webkit-flex;
+  -webkit-flex-orientation: horizontal;
+  background: #080;
+}
+
+img {
+  max-width: 100%; 
+  max-height:100%;
+  background: #00F;
+  margin-left:auto;
+  margin-right:auto;
+  margin-top:auto;
+  margin-bottom:auto;
+}
+
+</style>
+
+<div id="test" style="width:500px;height:500px;position:relative">
+<div class="wrapper">
+  <img id="between" src="resources/hero.png">
+</div>
+</div>
+
+<script>
+document.body.offsetLeft
+document.getElementById('test').style.width = '400px'
+document.getElementById('test').style.height = '400px'
+document.body.offsetLeft
+</script>
+
diff --git a/LayoutTests/css3/flexbox/resources/hero.png b/LayoutTests/css3/flexbox/resources/hero.png
new file mode 100644 (file)
index 0000000..203e60e
Binary files /dev/null and b/LayoutTests/css3/flexbox/resources/hero.png differ
diff --git a/LayoutTests/fast/flexbox/image-percent-max-height-expected.html b/LayoutTests/fast/flexbox/image-percent-max-height-expected.html
new file mode 100644 (file)
index 0000000..543831a
--- /dev/null
@@ -0,0 +1,27 @@
+<style>
+
+.wrapper {
+  top: 0;
+  left: 0;
+  position: absolute;
+  width: 100%;
+  height:100%;
+  
+  display:-webkit-box;
+  -webkit-box-orient: horizontal;
+  -webkit-box-pack: center;
+  -webkit-box-align: center;
+
+  background: #080;
+}
+
+img { 
+  height:400px;
+  background: #00F;
+}
+
+</style>
+
+<div id="test" style="width:400px;height:400px; background-color:#080; position:relative">
+  <div class="wrapper"><img id="between" src="resources/hero.png"></div>
+</div>
diff --git a/LayoutTests/fast/flexbox/image-percent-max-height.html b/LayoutTests/fast/flexbox/image-percent-max-height.html
new file mode 100644 (file)
index 0000000..3c9c077
--- /dev/null
@@ -0,0 +1,36 @@
+<!doctype html>
+<style>
+
+.wrapper {
+  top: 0;
+  left: 0;
+  position: absolute;
+  width: 100%;
+  height:100%;
+
+  display: -webkit-box;
+  -webkit-box-orient:horizontal;
+  -webkit-box-pack: center;
+  -webkit-box-align: stretch;
+  background: #080;
+}
+
+img {
+  max-width: 100%; 
+  height:100%;
+  background: #00F;
+}
+
+</style>
+
+<div id="test" style="width:500px;height:500px;position:relative">
+<div class="wrapper"><img id="between" src="resources/hero.png" style="vertical-align:top"></div>
+</div>
+
+<script>
+document.body.offsetLeft
+document.getElementById('test').style.width = '400px'
+document.getElementById('test').style.height = '400px'
+document.body.offsetLeft
+</script>
+
diff --git a/LayoutTests/fast/flexbox/resources/hero.png b/LayoutTests/fast/flexbox/resources/hero.png
new file mode 100644 (file)
index 0000000..203e60e
Binary files /dev/null and b/LayoutTests/fast/flexbox/resources/hero.png differ
index ed86504..8920eb2 100644 (file)
@@ -1,4 +1,4 @@
-layer at (0,0) size 982x2059
+layer at (0,0) size 960x2059
   RenderView at (0,0) size 785x585
 layer at (0,0) size 785x2059
   RenderBlock {HTML} at (0,0) size 785x2059
@@ -12,7 +12,7 @@ layer at (0,0) size 785x2059
           text run at (0,0) width 541: "percentage height images in DIV with no height (red) in a DIV with no height (green)"
       RenderBlock {DIV} at (32,735) size 657x657 [border: (3px dotted #008000)]
         RenderBlock {DIV} at (35,35) size 587x587 [border: (1px solid #FF0000)]
-          RenderImage {IMG} at (1,1) size 882x585
+          RenderImage {IMG} at (1,1) size 860x585
           RenderText {#text} at (0,0) size 0x0
       RenderBlock {P} at (0,1424) size 721x18
         RenderText {#text} at (0,0) size 471x18
index adb7a7d..59ee305 100644 (file)
@@ -1,3 +1,56 @@
+2013-06-25  David Hyatt  <hyatt@apple.com>
+
+        Flex boxes (both old and new) don't handle max-height images correctly.
+        https://bugs.webkit.org/show_bug.cgi?id=118000
+
+        Reviewed by Beth Dakin.
+
+        Tests: css3/flexbox/image-percent-max-height.html
+               fast/flexbox/image-percent-max-height.html
+
+        * rendering/RenderBlock.cpp:
+        (WebCore::RenderBlock::dirtyForLayoutFromPercentageHeightDescendants):
+        (WebCore::RenderBlock::layoutBlockChildren):
+        Pull the percentage height descendant code that dirties those descendants
+        out of layoutBlockChildren and into a protected helper function,
+        dirtyForLayoutFromPercentageHeightDescendants, that can be called from the 
+        flex box code.
+        
+        Also patch dirtyForLayoutFromPercentageHeightDescendants so that it will dirty
+        preferred logical widths when a child has an aspect ratio, since we know that
+        percentage height changes will potentially affect the preferred widths of the image and
+        its ancestor blocks.
+
+        * rendering/RenderBlock.h:
+        Declaration of the new dirtyForLayoutFromPercentageHeightDescendants function.
+
+        * rendering/RenderDeprecatedFlexibleBox.cpp:
+        (WebCore::RenderDeprecatedFlexibleBox::layoutBlock):
+        Make the old flex box code call dirtyForLayoutFromPercentageHeightDescendants so
+        that everything is dirtied properly.
+
+        (WebCore::RenderDeprecatedFlexibleBox::layoutHorizontalBox):
+        (WebCore::RenderDeprecatedFlexibleBox::layoutVerticalBox):
+        Remove the isReplaced()/percentage height/width dirtying now that the old flexible
+        box is using the same dirtying mechanism as RenderBlock.
+
+        * rendering/RenderFlexibleBox.cpp:
+        (WebCore::RenderFlexibleBox::layoutBlock):
+        Patch the new flexible box code to use the dirtying mechanism that RenderBlock
+        uses for percentage heights/widths on replaced descendants.
+
+        * rendering/RenderObject.h:
+        (WebCore::RenderObject::hasAspectRatio):
+        Pulled the static helper function from RenderReplaced into a full-blown method
+        on RenderObject, so that dirtyForLayoutFromPercentageHeightDescendants can call
+        it to check if an object has an aspect ratio.
+
+        * rendering/RenderReplaced.cpp:
+        (WebCore::RenderReplaced::computeAspectRatioInformationForRenderBox):
+        (WebCore::RenderReplaced::computeIntrinsicRatioInformation):
+        Patch the call sites of the static helper function to use hasAspectRatio instead
+        and get rid of the static in the cpp file.
+
 2013-06-26  Kangil Han  <kangil.han@samsung.com>
 
         Adopt is/toHTMLAreaElement for code cleanup
index b3704ed..f74480c 100644 (file)
@@ -2519,25 +2519,40 @@ void RenderBlock::updateBlockChildDirtyBitsBeforeLayout(bool relayoutChildren, R
         child->setPreferredLogicalWidthsDirty(true, MarkOnlyThis);
 }
 
-void RenderBlock::layoutBlockChildren(bool relayoutChildren, LayoutUnit& maxFloatLogicalBottom)
+void RenderBlock::dirtyForLayoutFromPercentageHeightDescendants()
 {
-    if (gPercentHeightDescendantsMap) {
-        if (TrackedRendererListHashSet* descendants = gPercentHeightDescendantsMap->get(this)) {
-            TrackedRendererListHashSet::iterator end = descendants->end();
-            for (TrackedRendererListHashSet::iterator it = descendants->begin(); it != end; ++it) {
-                RenderBox* box = *it;
-                while (box != this) {
-                    if (box->normalChildNeedsLayout())
-                        break;
-                    box->setChildNeedsLayout(true, MarkOnlyThis);
-                    box = box->containingBlock();
-                    ASSERT(box);
-                    if (!box)
-                        break;
-                }
-            }
+    if (!gPercentHeightDescendantsMap)
+        return;
+
+    TrackedRendererListHashSet* descendants = gPercentHeightDescendantsMap->get(this);
+    if (!descendants)
+        return;
+    
+    TrackedRendererListHashSet::iterator end = descendants->end();
+    for (TrackedRendererListHashSet::iterator it = descendants->begin(); it != end; ++it) {
+        RenderBox* box = *it;
+        while (box != this) {
+            if (box->normalChildNeedsLayout())
+                break;
+            box->setChildNeedsLayout(true, MarkOnlyThis);
+            
+            // If the width of an image is affected by the height of a child (e.g., an image with an aspect ratio),
+            // then we have to dirty preferred widths, since even enclosing blocks can become dirty as a result.
+            // (A horizontal flexbox that contains an inline image wrapped in an anonymous block for example.)
+            if (box->hasAspectRatio()) 
+                box->setPreferredLogicalWidthsDirty(true);
+            
+            box = box->containingBlock();
+            ASSERT(box);
+            if (!box)
+                break;
         }
     }
+}
+
+void RenderBlock::layoutBlockChildren(bool relayoutChildren, LayoutUnit& maxFloatLogicalBottom)
+{
+    dirtyForLayoutFromPercentageHeightDescendants();
 
     LayoutUnit beforeEdge = borderAndPaddingBefore();
     LayoutUnit afterEdge = borderAndPaddingAfter() + scrollbarLogicalHeight();
index 33ec1c9..8bbe157 100644 (file)
@@ -1094,6 +1094,8 @@ private:
     static void repaintDirtyFloats(Vector<FloatWithRect>& floats);
 
 protected:
+    void dirtyForLayoutFromPercentageHeightDescendants();
+    
     void determineLogicalLeftPositionForChild(RenderBox* child, ApplyLayoutDeltaMode = DoNotApplyLayoutDelta);
 
     // Pagination routines.
index c005c7d..3106fc9 100644 (file)
@@ -331,6 +331,8 @@ void RenderDeprecatedFlexibleBox::layoutBlock(bool relayoutChildren, LayoutUnit)
     ChildFrameRects oldChildRects;
     appendChildFrameRects(this, oldChildRects);
     
+    dirtyForLayoutFromPercentageHeightDescendants();
+
     if (isHorizontal())
         layoutHorizontalBox(relayoutChildren);
     else
@@ -456,7 +458,7 @@ void RenderDeprecatedFlexibleBox::layoutHorizontalBox(bool relayoutChildren)
         // our box's intrinsic height.
         LayoutUnit maxAscent = 0, maxDescent = 0;
         for (RenderBox* child = iterator.first(); child; child = iterator.next()) {
-            if (relayoutChildren || (child->isReplaced() && (child->style()->width().isPercent() || child->style()->height().isPercent())))
+            if (relayoutChildren)
                 child->setChildNeedsLayout(true, MarkOnlyThis);
 
             if (child->isOutOfFlowPositioned())
@@ -758,7 +760,7 @@ void RenderDeprecatedFlexibleBox::layoutVerticalBox(bool relayoutChildren)
         size_t childIndex = 0;
         for (RenderBox* child = iterator.first(); child; child = iterator.next()) {
             // Make sure we relayout children if we need it.
-            if (!haveLineClamp && (relayoutChildren || (child->isReplaced() && (child->style()->width().isPercent() || child->style()->height().isPercent()))))
+            if (!haveLineClamp && relayoutChildren)
                 child->setChildNeedsLayout(true, MarkOnlyThis);
 
             if (child->isOutOfFlowPositioned()) {
index a1ea476..c95e37e 100644 (file)
@@ -349,6 +349,8 @@ void RenderFlexibleBox::layoutBlock(bool relayoutChildren, LayoutUnit)
 
     RenderBlock::startDelayUpdateScrollInfo();
 
+    dirtyForLayoutFromPercentageHeightDescendants();
+
     Vector<LineContext> lineContexts;
     OrderHashSet orderValues;
     computeMainAxisPreferredSizes(orderValues);
index 8080923..b5f77bf 100644 (file)
@@ -478,6 +478,8 @@ public:
     virtual bool isSVGResourceFilter() const { return false; }
     virtual bool isSVGResourceFilterPrimitive() const { return false; }
 
+    bool hasAspectRatio() const { return isReplaced() && (isImage() || isVideo() || isCanvas()); }
+    
     virtual RenderSVGResourceContainer* toRenderSVGResourceContainer();
 
     // FIXME: Those belong into a SVG specific base-class for all renderers (see above)
index de86a83..1df9e19 100644 (file)
@@ -248,12 +248,6 @@ bool RenderReplaced::hasReplacedLogicalHeight() const
     return false;
 }
 
-static inline bool rendererHasAspectRatio(const RenderObject* renderer)
-{
-    ASSERT(renderer);
-    return renderer->isImage() || renderer->isCanvas() || renderer->isVideo();
-}
-
 void RenderReplaced::computeAspectRatioInformationForRenderBox(RenderBox* contentRenderer, FloatSize& constrainedSize, double& intrinsicRatio, bool& isPercentageIntrinsicSize) const
 {
     FloatSize intrinsicSize;
@@ -266,7 +260,7 @@ void RenderReplaced::computeAspectRatioInformationForRenderBox(RenderBox* conten
         if (!isPercentageIntrinsicSize)
             intrinsicSize.scale(style()->effectiveZoom());
 
-        if (rendererHasAspectRatio(this) && isPercentageIntrinsicSize)
+        if (hasAspectRatio() && isPercentageIntrinsicSize)
             intrinsicRatio = 1;
             
         // Update our intrinsic size to match what the content renderer has computed, so that when we
@@ -312,7 +306,7 @@ void RenderReplaced::computeIntrinsicRatioInformation(FloatSize& intrinsicSize,
     intrinsicSize = FloatSize(intrinsicLogicalWidth(), intrinsicLogicalHeight());
 
     // Figure out if we need to compute an intrinsic ratio.
-    if (intrinsicSize.isEmpty() || !rendererHasAspectRatio(this))
+    if (intrinsicSize.isEmpty() || !hasAspectRatio())
         return;
 
     intrinsicRatio = intrinsicSize.width() / intrinsicSize.height();