Correct overflow propagation in BTT and RTL writing-modes

Source/core/rendering/RenderBox.cpp

 /*
  * Copyright (C) 1999 Lars Knoll (knoll@kde.org)
  *           (C) 1999 Antti Koivisto (koivisto@kde.org)
  *           (C) 2005 Allan Sandfeld Jensen (kde@carewolf.com)
  *           (C) 2005, 2006 Samuel Weinig (sam.weinig@gmail.com)
  * Copyright (C) 2005, 2006, 2007, 2008, 2009, 2010 Apple Inc. All rights reserved.
  * Copyright (C) 2013 Adobe Systems Incorporated. All rights reserved.
  *
  * This library is free software; you can redistribute it and/or
  * modify it under the terms of the GNU Library General Public
@@ skipping 4140 matching lines @@
     // overflow if we are clipping our own overflow.
     if (child->hasSelfPaintingLayer() || hasOverflowClip())
         return;
     LayoutRect childVisualOverflowRect = child->visualOverflowRectForPropagation(style());
     childVisualOverflowRect.move(delta);
     addVisualOverflow(childVisualOverflowRect);
 }
 
 void RenderBox::addLayoutOverflow(const LayoutRect& rect)
 {
-    LayoutRect clientBox = clientBoxRect();
+    LayoutRect clientBox = noOverflowRect();
     if (clientBox.contains(rect) || rect.isEmpty())
         return;
     
     // For overflow clip objects, we don't want to propagate overflow into unreachable areas.
     LayoutRect overflowRect(rect);
     if (hasOverflowClip() || isRenderView()) {
         // Overflow is in the block's coordinate space and thus is flipped for horizontal-bt and vertical-rl 
         // writing modes.  At this stage that is actually a simplification, since we can treat horizontal-tb/bt as the same
         // and vertical-lr/rl as the same.
         bool hasTopOverflow = !style()->isLeftToRightDirection() && !isHorizontalWritingMode();
@@ skipping 34 matching lines @@
     m_overflow->addLayoutOverflow(overflowRect);
 }
 
 void RenderBox::addVisualOverflow(const LayoutRect& rect)
 {
     LayoutRect borderBox = borderBoxRect();
     if (borderBox.contains(rect) || rect.isEmpty())
         return;
         
     if (!m_overflow)
-        m_overflow = adoptPtr(new RenderOverflow(clientBoxRect(), borderBox));
+        m_overflow = adoptPtr(new RenderOverflow(noOverflowRect(), borderBox));
     
     m_overflow->addVisualOverflow(rect);
 }
 
 void RenderBox::clearLayoutOverflow()
 {
     if (!m_overflow)
         return;
     
     if (visualOverflowRect() == borderBoxRect()) {
         m_overflow.clear();
         return;
     }
     
     m_overflow->setLayoutOverflow(borderBoxRect());

[jbroman, 2013/07/16 01:19:05]

This should be noOverflowRect() as well.

[mstensho (USE GERRIT), 2013/07/17 13:39:36]

Done.

 }
 
 inline static bool percentageLogicalHeightIsResolvable(const RenderBox* box)
 {
     return RenderBox::percentageLogicalHeightIsResolvableFromBlock(box->containingBlock(), box->isOutOfFlowPositioned());
 }
 
 bool RenderBox::percentageLogicalHeightIsResolvableFromBlock(const RenderBlock* containingBlock, bool isOutOfFlowPositioned)
 {
     // In quirks mode, blocks with auto height are skipped, and we keep looking for an enclosing
@@ skipping 158 matching lines @@
     // We are putting ourselves into our parent's coordinate space.  If there is a flipped block mismatch
     // in a particular axis, then we have to flip the rect along that axis.
     if (style()->writingMode() == RightToLeftWritingMode || parentStyle->writingMode() == RightToLeftWritingMode)
         rect.setX(width() - rect.maxX());
     else if (style()->writingMode() == BottomToTopWritingMode || parentStyle->writingMode() == BottomToTopWritingMode)
         rect.setY(height() - rect.maxY());
 
     return rect;
 }
 
+LayoutRect RenderBox::noOverflowRect() const
+{
+    // Because of the special coodinate system used for overflow rectangles (not quite logical, not
+    // quite physical), we need to flip the block progression coordinate in vertical-rl and
+    // horizontal-bt writing modes. Apart from that, this method does the same as clientBoxRect().
+
+    LayoutUnit left = borderLeft();
+    LayoutUnit top = borderTop();
+    LayoutUnit right = borderRight();
+    LayoutUnit bottom = borderBottom();
+    // Calculate physical padding box.

[Julien - ping for review, 2013/07/15 23:50:33]

I don't think this comment adds much.

[mstensho (USE GERRIT), 2013/07/17 13:39:36]

Done.

+    LayoutRect rect(left, top, width() - left - right, height() - top - bottom);

[Julien - ping for review, 2013/07/15 23:50:33]

This is very much:

LayoutRect physicalBorderBoxRect = this->borderBoxRect();
physicalBorderBoxRect.move(borderLeft(), borderTop());

(Note: I have no clue why borderBoxRect() doesn't start at (borderLeft(),
borderTop()) apart that it really looks like a bug).

[jbroman, 2013/07/16 01:19:05]

Comment-on-a-comment: the border box is the rect that surrounds the border, not
the rect surrounded by the border. This rect is the rect just within the border.

[mstensho (USE GERRIT), 2013/07/17 13:39:36]

A good method to use here would be paddingBoxRect(), but there are some
complications with scrollbars, due to
https://bugs.webkit.org/show_bug.cgi?id=76129 , so I think we're better off
building the rectangle on our own for now. See new patch regarding the
scrollbars; I've tried to explain it better.

+    // Flip block progression axis if writing mode is vertical-rl or horizontal-bt.

[Julien - ping for review, 2013/07/15 23:50:33]

Same comment about not adding much (it repeats what the code is doing). The
other comments are great though.

[mstensho (USE GERRIT), 2013/07/17 13:39:36]

Done.

+    flipForWritingMode(rect);
+    // Subtract space occupied by scrollbars. They are at their physical edge in this coordinate

[mstensho (USE GERRIT), 2013/07/17 13:39:36]

Raising an issue myself. :)
I didn't understand this comment myself, so I took a closer look at what's going
on (which is why it took me so long to follow up on the review). What we do here
is strange, but it's all due to https://bugs.webkit.org/show_bug.cgi?id=76129

+    // system, so order is important here: first flip, then subtract scrollbars.
+    rect.contract(verticalScrollbarWidth(), horizontalScrollbarHeight());
+    return rect;
+}
+
 LayoutRect RenderBox::overflowRectForPaintRejection() const
 {
     LayoutRect overflowRect = visualOverflowRect();
     if (!m_overflow || !usesCompositedScrolling())
         return overflowRect;
 
     overflowRect.unite(layoutOverflowRect());
     overflowRect.move(-scrolledContentOffset());
     return overflowRect;
 }
@@ skipping 174 matching lines @@
 
 void RenderBox::reportStaticMembersMemoryUsage(MemoryInstrumentation* memoryInstrumentation)
 {
     memoryInstrumentation->addRootObject(gOverrideHeightMap, WebCoreMemoryTypes::RenderingStructures);
     memoryInstrumentation->addRootObject(gOverrideWidthMap, WebCoreMemoryTypes::RenderingStructures);
     memoryInstrumentation->addRootObject(gOverrideContainingBlockLogicalHeightMap, WebCoreMemoryTypes::RenderingStructures);
     memoryInstrumentation->addRootObject(gOverrideContainingBlockLogicalWidthMap, WebCoreMemoryTypes::RenderingStructures);
 }
 
 } // namespace WebCore