LayoutBox::mapContentsRectToVisibleRectInBorderBoxSpace()

third_party/WebKit/Source/core/layout/LayoutBox.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 940 matching lines @@
 }
 
 IntSize LayoutBox::scrolledContentOffset() const
 {
     ASSERT(hasOverflowClip());
     ASSERT(hasLayer());
     // FIXME: Return DoubleSize here. crbug.com/414283.
     return flooredIntSize(getScrollableArea()->scrollOffset());
 }
 
-void LayoutBox::mapScrollingContentsRectToBoxSpace(LayoutRect& rect) const
+bool LayoutBox::mapContentsRectToVisibleRectInBorderBoxSpace(LayoutRect& rect, VisibleRectFlags visibleRectFlags) const
 {
-    ASSERT(hasLayer());
-    ASSERT(hasOverflowClip());
+    if (!hasOverflowClip())
+        return true;
 
+    // Apply reverse of scroll offset.
     LayoutSize offset = LayoutSize(-scrolledContentOffset());
     if (UNLIKELY(hasFlippedBlocksWritingMode())) {
         if (isHorizontalWritingMode())
             offset.setHeight(-offset.height());
         else
             offset.setWidth(-offset.width());
     }
     rect.move(offset);
-}
 
-bool LayoutBox::applyOverflowClip(LayoutRect& rect, VisibleRectFlags visibleRectFlags) const
-{
-    ASSERT(hasLayer());
-    ASSERT(hasOverflowClip());
+    if (usesCompositedScrolling())

[chrishtr, 2016/03/28 16:34:31]

I don't think this isn't right. These methods should not refer to compositing
state.
Same goes for PaintInvalidationState.

[Xianzhu, 2016/03/28 17:35:21]

Why? I think we should treat composited scrolling box not clipped, otherwise
we'll invalidate contents of a composited scrolling box on each scroll.

[chrishtr, 2016/03/28 18:02:04]

Because the concept of a visible rect should not take into account compositing.
I looked elsewhere
and see we do have special-casing in PaintInvalidaitonState, so this is not a
new introduction.

For the use case you're describing, the space in which visible rects are
computed would be the scrolling
element, right? In which case we should not apply that clip since it's the
ancestor. Maybe right now the code does apply that clip, but should it? This is
similar to the code/discussion about root-layer-scrolls.

[Xianzhu, 2016/03/28 18:15:37]

Our current condition of applying clipping is "container != ancestor" (old code
line 1998). This works for composited-scrolling container, but not for
non-composited-scrolling container (crbug.com/597902).

Cases:
1. container == ancestor:
  1a. container is composited-scrolling: should not clip
  1b. container is not composited-scrolling: should clip
2. container != ancestor (implies not composited-scrolling): should clip

For now the problem is about 1b. Slow-path doesn't clip (using condition
container == ancestor) while fast-path clips (using condition
usesCompositedScrolling).

[chrishtr, 2016/03/28 19:28:53]

I don't think case 1b is required at this point. if a LayoutObject is composited
for some other reason then it gets accelerated composited scrolling as well,
even on a low-DPI screen. Is there another use case for it?

[Xianzhu, 2016/03/28 19:42:10]

The case is overflow:hidden which seems to to cause composited scrolling.

[Xianzhu, 2016/03/28 19:43:01]

The case is overflow:hidden which seems not to cause composited scrolling.

[chrishtr, 2016/03/28 20:38:58]

How about:

if (container == ancestor)
  if (container scrolls overflow)
    don't clip
  else
    clip
else
  clip

[Xianzhu, 2016/03/28 21:12:08]

It should work. It just looks weird to pass 'ancestor' parameter here unless we
give it a good meaning. Haven't got an idea for now.

[chrishtr, 2016/03/28 22:09:58]

So you think it's good to try that in this patch?

+        return true;
 
+    // Apply clip.
     flipForWritingMode(rect);
-
-    // size() is inaccurate if we're in the middle of a layout of this LayoutBox, so use the
-    // layer's size instead. Even if the layer's size is wrong, the layer itself will issue paint invalidations
-    // anyway if its size does change.
     LayoutRect clipRect(LayoutPoint(), LayoutSize(layer()->size()));
     bool doesIntersect;
     if (visibleRectFlags & EdgeInclusive) {
         doesIntersect = rect.inclusiveIntersect(clipRect);
     } else {
         rect.intersect(clipRect);
         doesIntersect = !rect.isEmpty();
     }
     if (doesIntersect)
         flipForWritingMode(rect);
@@ skipping 936 matching lines @@
             return LayoutRect();
     }
 
     return visualOverflowRect();
 }
 
 bool LayoutBox::mapToVisibleRectInAncestorSpace(const LayoutBoxModelObject* ancestor, LayoutRect& rect, VisibleRectFlags visibleRectFlags) const
 {
     // The rect we compute at each step is shifted by our x/y offset in the parent container's coordinate space.
     // Only when we cross a writing mode boundary will we have to possibly flipForWritingMode (to convert into a more appropriate
     // offset corner for the enclosing container). This allows for a fully RL or BT document to issue paint invalidations

[chrishtr, 2016/03/28 18:02:04]

The sentence "This allows..." refers to invalidating during layout, which is no
longer done since
repaint-after-layout. Remove this sentence if you confirm it is obsolete.

[Xianzhu, 2016/03/28 21:12:08]

Acknowledged.

     // properly even during layout, since the rect remains flipped all the way until the end.
     //
-    // LayoutView::computeRectForPaintInvalidation then converts the rect to physical coordinates. We also convert to
-    // physical when we hit the ancestor. Therefore the final rect returned is always in the
+    // We convert to physical when we hit the ancestor. Therefore the final rect returned is always in the
     // physical coordinate space of the ancestor.
     const ComputedStyle& styleToUse = styleRef();
 
     EPosition position = styleToUse.position();
 
     // We need to inflate the paint invalidation rect before we use paintInvalidationState,
     // else we would forget to inflate it for the current layoutObject. FIXME: If these were
     // included into the visual overflow for paint invalidation, we wouldn't have this issue.
     inflatePaintInvalidationRectForReflectionAndFilter(rect);
 
-    if (ancestor == this) {
-        if (ancestor->style()->isFlippedBlocksWritingMode())
-            flipForWritingMode(rect);
+    if (ancestor == this)
         return true;
-    }
 
     bool containerSkipped;
     LayoutObject* container = this->container(ancestor, &containerSkipped);
     if (!container)
         return true;
 
-    if (isWritingModeRoot())
-        flipForWritingMode(rect);
-
     LayoutPoint topLeft = rect.location();
     topLeft.move(locationOffset());
 
     // We are now in our parent container's coordinate space.  Apply our transform to obtain a bounding box
     // in the parent's coordinate space that encloses us.
     if (hasLayer() && layer()->transform()) {
         rect = LayoutRect(layer()->transform()->mapRect(pixelSnappedIntRect(rect)));
         topLeft = rect.location();
         topLeft.move(locationOffset());
     }
 
     if (position == AbsolutePosition && container->isInFlowPositioned() && container->isLayoutInline()) {
         topLeft += toLayoutInline(container)->offsetForInFlowPositionedInline(*this);
     } else if (styleToUse.hasInFlowPosition() && layer()) {
         // Apply the relative position offset when invalidating a rectangle.  The layer
         // is translated, but the layout box isn't, so we need to do this to get the
         // right dirty rect.  Since this is called from LayoutObject::setStyle, the relative position
         // flag on the LayoutObject has been cleared, so use the one on the style().
         topLeft += layer()->offsetForInFlowPosition();
     }
 
-    // FIXME: We ignore the lightweight clipping rect that controls use, since if |o| is in mid-layout,
-    // its controlClipRect will be wrong. For overflow clip we use the values cached by the layer.
     rect.setLocation(topLeft);
-    if (container->hasOverflowClip()) {
+
+    if (container->isBox()) {
         LayoutBox* containerBox = toLayoutBox(container);
-        containerBox->mapScrollingContentsRectToBoxSpace(rect);
-        if (container != ancestor && !containerBox->applyOverflowClip(rect, visibleRectFlags))
+        if (!containerBox->mapContentsRectToVisibleRectInBorderBoxSpace(rect, visibleRectFlags))
             return false;
+
+        if (containerBox == ancestor || containerBox->isWritingModeRoot())

[chrishtr, 2016/03/28 16:34:31]

Why also if containerBox == ancestor? Why does this flipping have to happen here
at all if it's
(sometimes?) done in mapContentsRectToVisibleRectInBorderBoxSpace?

[Xianzhu, 2016/03/28 17:35:21]

1. The function is to map the rect in physical coordinates in ancestor, but
during recursion, if several boxes in the ancestor chain have the same writing
mode, we'll keep the rect unflipped until we meet a writing mode root (writing
mode changes) or the ancestor. This is mentioned in the comments at line
1935-1940 above.

2. mapContentsRectToVisibleRectInBorderBoxSpace() doesn't actually flip. It just
flips temporarily for clipping: it flips before applying clipping and flip again
to revert the flip after clipping.

Another method is to keep the rect always in physical coordinates during the
recursion. This requires us to add different functions which accept input rect
in different coordinates. (A question: what terms do we use to mention the
original and flipped coordinate spaces?)

[chrishtr, 2016/03/28 18:02:04]

Makes sense, thanks for explaining.

[Xianzhu, 2016/03/28 18:15:37]

I'd like to explore always-physical-coordinates way. One question is how to
mention the "natural coordinate space" that LayoutBox etc. are using (e.g. when
we say 'in local coordinates', 'in border box coordinates', etc., and the
coordinate space of location(), frameRect(), visualOverflowRect(), etc.).

+            containerBox->flipForWritingMode(rect);
     }
 
     if (containerSkipped) {
-        // If the paintInvalidationContainer is below o, then we need to map the rect into paintInvalidationContainer's coordinates.
+        // If the ancestor is below o, then we need to map the rect into paintInvalidationContainer's coordinates.
         LayoutSize containerOffset = ancestor->offsetFromAncestorContainer(container);
         rect.move(-containerOffset);
-        // If the paintInvalidationContainer is fixed, then the rect is already in its coordinates so doesn't need viewport-adjusting.
+        // If the ancestor is fixed, then the rect is already in its coordinates so doesn't need viewport-adjusting.
         if (ancestor->style()->position() != FixedPosition && container->isLayoutView() && position == FixedPosition)
             toLayoutView(container)->adjustOffsetForFixedPosition(rect);
         return true;
     }
 
     if (container->isLayoutView())
         return toLayoutView(container)->mapToVisibleRectInAncestorSpace(ancestor, rect, position == FixedPosition ? IsFixed : 0, visibleRectFlags);
     else
         return container->mapToVisibleRectInAncestorSpace(ancestor, rect, visibleRectFlags);
 }
@@ skipping 2686 matching lines @@
 
 void LayoutBox::clearPercentHeightDescendants()
 {
     for (LayoutObject* curr = slowFirstChild(); curr; curr = curr->nextInPreOrder(this)) {
         if (curr->isBox())
             toLayoutBox(curr)->removeFromPercentHeightContainer();
     }
 }
 
 } // namespace blink