Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1826853007:
LayoutBox::mapContentsRectToVisibleRectInBorderBoxSpace() (Closed)
|
Created:
4 years, 9 months ago by Xianzhu Modified:
4 years, 8 months ago CC:
blink-reviews, blink-reviews-layout_chromium.org, chromium-reviews, eae+blinkwatch, jchaffraix+rendering, leviw+renderwatch, pdr+renderingwatchlist_chromium.org, szager+layoutwatch_chromium.org, zoltan1 Base URL:
https://chromium.googlesource.com/chromium/src.git@pi Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionLayoutBox::mapContentsRectToVisibleRectInBorderBoxSpace()
Called in several mapToVisibleRectInAncestor() implementations, to
map a rect in contents space of the container to the border-box space
of the container, before calling container->mapToVisibleRectInAncestor().
This ensures that input rect to mapToVisibleRectInAncestor() is in the
local space (border-box space for LayoutBox) of the object.
This fixes two issues:
- Unnecessary flipping for writing mode when mapping the own rect of
a LayoutBox having flippedBlocksWritingMode().
- Missing clipping of the ancestor (non-composited-scrolling).
BUG=597965, 597902
Patch Set 1 #Patch Set 2 : #Patch Set 3 : Enable comparison, DO NOT CQ #
Total comments: 22
Messages
Total messages: 14 (1 generated)
wangxianzhu@chromium.org changed reviewers: + chrishtr@chromium.org, trchen@chromium.org
https://codereview.chromium.org/1826853007/diff/2/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/LayoutBox.cpp (right): https://codereview.chromium.org/1826853007/diff/2/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/LayoutBox.cpp:976: if (usesCompositedScrolling()) I don't think this isn't right. These methods should not refer to compositing state. Same goes for PaintInvalidationState. https://codereview.chromium.org/1826853007/diff/2/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/LayoutBox.cpp:1987: if (containerBox == ancestor || containerBox->isWritingModeRoot()) Why also if containerBox == ancestor? Why does this flipping have to happen here at all if it's (sometimes?) done in mapContentsRectToVisibleRectInBorderBoxSpace?
https://codereview.chromium.org/1826853007/diff/2/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/LayoutBox.cpp (right): https://codereview.chromium.org/1826853007/diff/2/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/LayoutBox.cpp:976: if (usesCompositedScrolling()) On 2016/03/28 16:34:31, chrishtr wrote: > I don't think this isn't right. These methods should not refer to compositing > state. > Same goes for PaintInvalidationState. Why? I think we should treat composited scrolling box not clipped, otherwise we'll invalidate contents of a composited scrolling box on each scroll. https://codereview.chromium.org/1826853007/diff/2/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/LayoutBox.cpp:1987: if (containerBox == ancestor || containerBox->isWritingModeRoot()) On 2016/03/28 16:34:31, chrishtr wrote: > Why also if containerBox == ancestor? Why does this flipping have to happen here > at all if it's > (sometimes?) done in mapContentsRectToVisibleRectInBorderBoxSpace? 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?)
https://codereview.chromium.org/1826853007/diff/2/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/LayoutBox.cpp (right): https://codereview.chromium.org/1826853007/diff/2/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/LayoutBox.cpp:976: if (usesCompositedScrolling()) On 2016/03/28 at 17:35:21, Xianzhu wrote: > On 2016/03/28 16:34:31, chrishtr wrote: > > I don't think this isn't right. These methods should not refer to compositing > > state. > > Same goes for PaintInvalidationState. > > Why? I think we should treat composited scrolling box not clipped, otherwise we'll invalidate contents of a composited scrolling box on each scroll. 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. https://codereview.chromium.org/1826853007/diff/2/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/LayoutBox.cpp:1937: // offset corner for the enclosing container). This allows for a fully RL or BT document to issue paint invalidations 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. https://codereview.chromium.org/1826853007/diff/2/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/LayoutBox.cpp:1987: if (containerBox == ancestor || containerBox->isWritingModeRoot()) On 2016/03/28 at 17:35:21, Xianzhu wrote: > On 2016/03/28 16:34:31, chrishtr wrote: > > Why also if containerBox == ancestor? Why does this flipping have to happen here > > at all if it's > > (sometimes?) done in mapContentsRectToVisibleRectInBorderBoxSpace? > > 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?) Makes sense, thanks for explaining.
https://codereview.chromium.org/1826853007/diff/2/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/LayoutBox.cpp (right): https://codereview.chromium.org/1826853007/diff/2/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/LayoutBox.cpp:976: if (usesCompositedScrolling()) On 2016/03/28 18:02:04, chrishtr wrote: > On 2016/03/28 at 17:35:21, Xianzhu wrote: > > On 2016/03/28 16:34:31, chrishtr wrote: > > > I don't think this isn't right. These methods should not refer to > compositing > > > state. > > > Same goes for PaintInvalidationState. > > > > Why? I think we should treat composited scrolling box not clipped, otherwise > we'll invalidate contents of a composited scrolling box on each scroll. > > 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. 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). https://codereview.chromium.org/1826853007/diff/2/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/LayoutBox.cpp:1987: if (containerBox == ancestor || containerBox->isWritingModeRoot()) On 2016/03/28 18:02:04, chrishtr wrote: > On 2016/03/28 at 17:35:21, Xianzhu wrote: > > On 2016/03/28 16:34:31, chrishtr wrote: > > > Why also if containerBox == ancestor? Why does this flipping have to happen > here > > > at all if it's > > > (sometimes?) done in mapContentsRectToVisibleRectInBorderBoxSpace? > > > > 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?) > > Makes sense, thanks for explaining. 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.).
https://codereview.chromium.org/1826853007/diff/2/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/LayoutBox.cpp (right): https://codereview.chromium.org/1826853007/diff/2/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/LayoutBox.cpp:976: if (usesCompositedScrolling()) On 2016/03/28 at 18:15:37, Xianzhu wrote: > On 2016/03/28 18:02:04, chrishtr wrote: > > On 2016/03/28 at 17:35:21, Xianzhu wrote: > > > On 2016/03/28 16:34:31, chrishtr wrote: > > > > I don't think this isn't right. These methods should not refer to > > compositing > > > > state. > > > > Same goes for PaintInvalidationState. > > > > > > Why? I think we should treat composited scrolling box not clipped, otherwise > > we'll invalidate contents of a composited scrolling box on each scroll. > > > > 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. > > 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). 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?
https://codereview.chromium.org/1826853007/diff/2/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/LayoutBox.cpp (right): https://codereview.chromium.org/1826853007/diff/2/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/LayoutBox.cpp:976: if (usesCompositedScrolling()) On 2016/03/28 19:28:53, chrishtr wrote: > On 2016/03/28 at 18:15:37, Xianzhu wrote: > > On 2016/03/28 18:02:04, chrishtr wrote: > > > On 2016/03/28 at 17:35:21, Xianzhu wrote: > > > > On 2016/03/28 16:34:31, chrishtr wrote: > > > > > I don't think this isn't right. These methods should not refer to > > > compositing > > > > > state. > > > > > Same goes for PaintInvalidationState. > > > > > > > > Why? I think we should treat composited scrolling box not clipped, > otherwise > > > we'll invalidate contents of a composited scrolling box on each scroll. > > > > > > 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. > > > > 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). > > 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? The case is overflow:hidden which seems to to cause composited scrolling.
https://codereview.chromium.org/1826853007/diff/2/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/LayoutBox.cpp (right): https://codereview.chromium.org/1826853007/diff/2/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/LayoutBox.cpp:976: if (usesCompositedScrolling()) On 2016/03/28 19:42:10, Xianzhu wrote: > On 2016/03/28 19:28:53, chrishtr wrote: > > On 2016/03/28 at 18:15:37, Xianzhu wrote: > > > On 2016/03/28 18:02:04, chrishtr wrote: > > > > On 2016/03/28 at 17:35:21, Xianzhu wrote: > > > > > On 2016/03/28 16:34:31, chrishtr wrote: > > > > > > I don't think this isn't right. These methods should not refer to > > > > compositing > > > > > > state. > > > > > > Same goes for PaintInvalidationState. > > > > > > > > > > Why? I think we should treat composited scrolling box not clipped, > > otherwise > > > > we'll invalidate contents of a composited scrolling box on each scroll. > > > > > > > > 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. > > > > > > 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). > > > > 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? > > The case is overflow:hidden which seems to to cause composited scrolling. The case is overflow:hidden which seems not to cause composited scrolling.
On 2016/03/28 at 19:43:01, wangxianzhu wrote: > https://codereview.chromium.org/1826853007/diff/2/third_party/WebKit/Source/c... > File third_party/WebKit/Source/core/layout/LayoutBox.cpp (right): > > https://codereview.chromium.org/1826853007/diff/2/third_party/WebKit/Source/c... > third_party/WebKit/Source/core/layout/LayoutBox.cpp:976: if (usesCompositedScrolling()) > On 2016/03/28 19:42:10, Xianzhu wrote: > > On 2016/03/28 19:28:53, chrishtr wrote: > > > On 2016/03/28 at 18:15:37, Xianzhu wrote: > > > > On 2016/03/28 18:02:04, chrishtr wrote: > > > > > On 2016/03/28 at 17:35:21, Xianzhu wrote: > > > > > > On 2016/03/28 16:34:31, chrishtr wrote: > > > > > > > I don't think this isn't right. These methods should not refer to > > > > > compositing > > > > > > > state. > > > > > > > Same goes for PaintInvalidationState. > > > > > > > > > > > > Why? I think we should treat composited scrolling box not clipped, > > > otherwise > > > > > we'll invalidate contents of a composited scrolling box on each scroll. > > > > > > > > > > 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. > > > > > > > > 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). > > > > > > 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? > > > > The case is overflow:hidden which seems to to cause composited scrolling. > > The case is overflow:hidden which seems not to cause composited scrolling. Oh right, there is that...
https://codereview.chromium.org/1826853007/diff/2/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/LayoutBox.cpp (right): https://codereview.chromium.org/1826853007/diff/2/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/LayoutBox.cpp:976: if (usesCompositedScrolling()) On 2016/03/28 at 19:43:01, Xianzhu wrote: > On 2016/03/28 19:42:10, Xianzhu wrote: > > On 2016/03/28 19:28:53, chrishtr wrote: > > > On 2016/03/28 at 18:15:37, Xianzhu wrote: > > > > On 2016/03/28 18:02:04, chrishtr wrote: > > > > > On 2016/03/28 at 17:35:21, Xianzhu wrote: > > > > > > On 2016/03/28 16:34:31, chrishtr wrote: > > > > > > > I don't think this isn't right. These methods should not refer to > > > > > compositing > > > > > > > state. > > > > > > > Same goes for PaintInvalidationState. > > > > > > > > > > > > Why? I think we should treat composited scrolling box not clipped, > > > otherwise > > > > > we'll invalidate contents of a composited scrolling box on each scroll. > > > > > > > > > > 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. > > > > > > > > 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). > > > > > > 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? > > > > The case is overflow:hidden which seems to to cause composited scrolling. > > The case is overflow:hidden which seems not to cause composited scrolling. How about: if (container == ancestor) if (container scrolls overflow) don't clip else clip else clip https://codereview.chromium.org/1826853007/diff/2/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/LayoutBox.h (right): https://codereview.chromium.org/1826853007/diff/2/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/LayoutBox.h:846: bool mapContentsRectToVisibleRectInBorderBoxSpace(LayoutRect&, VisibleRectFlags) const; This is not a visual rect right? Because it doesn't flip for writing mode, for example? https://codereview.chromium.org/1826853007/diff/2/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/LayoutView.cpp (left): https://codereview.chromium.org/1826853007/diff/2/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/LayoutView.cpp:494: if (style()->isFlippedBlocksWritingMode()) { This is handled by LayoutBox somehow? Is this tested? Add a unittest?
https://codereview.chromium.org/1826853007/diff/2/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/LayoutBox.cpp (right): https://codereview.chromium.org/1826853007/diff/2/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/LayoutBox.cpp:976: if (usesCompositedScrolling()) On 2016/03/28 20:38:58, chrishtr wrote: > On 2016/03/28 at 19:43:01, Xianzhu wrote: > > On 2016/03/28 19:42:10, Xianzhu wrote: > > > On 2016/03/28 19:28:53, chrishtr wrote: > > > > On 2016/03/28 at 18:15:37, Xianzhu wrote: > > > > > On 2016/03/28 18:02:04, chrishtr wrote: > > > > > > On 2016/03/28 at 17:35:21, Xianzhu wrote: > > > > > > > On 2016/03/28 16:34:31, chrishtr wrote: > > > > > > > > I don't think this isn't right. These methods should not refer to > > > > > > compositing > > > > > > > > state. > > > > > > > > Same goes for PaintInvalidationState. > > > > > > > > > > > > > > Why? I think we should treat composited scrolling box not clipped, > > > > otherwise > > > > > > we'll invalidate contents of a composited scrolling box on each > scroll. > > > > > > > > > > > > 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. > > > > > > > > > > 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). > > > > > > > > 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? > > > > > > The case is overflow:hidden which seems to to cause composited scrolling. > > > > The case is overflow:hidden which seems not to cause composited scrolling. > > How about: > > if (container == ancestor) > if (container scrolls overflow) > don't clip > else > clip > else > clip 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. https://codereview.chromium.org/1826853007/diff/2/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/LayoutBox.cpp:1937: // offset corner for the enclosing container). This allows for a fully RL or BT document to issue paint invalidations On 2016/03/28 18:02:04, chrishtr wrote: > 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. Acknowledged. https://codereview.chromium.org/1826853007/diff/2/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/LayoutBox.h (right): https://codereview.chromium.org/1826853007/diff/2/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/LayoutBox.h:846: bool mapContentsRectToVisibleRectInBorderBoxSpace(LayoutRect&, VisibleRectFlags) const; On 2016/03/28 20:38:59, chrishtr wrote: > This is not a visual rect right? Because it doesn't flip for writing mode, for > example? Right. I'm going to modify mapToVisibleRectInAncestorSpace() to accept visual rect (flipped for writing mode), so that this function can also always accept and output visible rect. https://codereview.chromium.org/1826853007/diff/2/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/LayoutView.cpp (left): https://codereview.chromium.org/1826853007/diff/2/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/LayoutView.cpp:494: if (style()->isFlippedBlocksWritingMode()) { On 2016/03/28 20:38:59, chrishtr wrote: > This is handled by LayoutBox somehow? > > Is this tested? Add a unittest? Can a LayoutView have writing-mode?
https://codereview.chromium.org/1826853007/diff/2/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/LayoutBox.cpp (right): https://codereview.chromium.org/1826853007/diff/2/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/LayoutBox.cpp:976: if (usesCompositedScrolling()) On 2016/03/28 at 21:12:08, Xianzhu wrote: > On 2016/03/28 20:38:58, chrishtr wrote: > > On 2016/03/28 at 19:43:01, Xianzhu wrote: > > > On 2016/03/28 19:42:10, Xianzhu wrote: > > > > On 2016/03/28 19:28:53, chrishtr wrote: > > > > > On 2016/03/28 at 18:15:37, Xianzhu wrote: > > > > > > On 2016/03/28 18:02:04, chrishtr wrote: > > > > > > > On 2016/03/28 at 17:35:21, Xianzhu wrote: > > > > > > > > On 2016/03/28 16:34:31, chrishtr wrote: > > > > > > > > > I don't think this isn't right. These methods should not refer to > > > > > > > compositing > > > > > > > > > state. > > > > > > > > > Same goes for PaintInvalidationState. > > > > > > > > > > > > > > > > Why? I think we should treat composited scrolling box not clipped, > > > > > otherwise > > > > > > > we'll invalidate contents of a composited scrolling box on each > > scroll. > > > > > > > > > > > > > > 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. > > > > > > > > > > > > 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). > > > > > > > > > > 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? > > > > > > > > The case is overflow:hidden which seems to to cause composited scrolling. > > > > > > The case is overflow:hidden which seems not to cause composited scrolling. > > > > How about: > > > > if (container == ancestor) > > if (container scrolls overflow) > > don't clip > > else > > clip > > else > > clip > > 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. So you think it's good to try that in this patch? https://codereview.chromium.org/1826853007/diff/2/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/LayoutView.cpp (left): https://codereview.chromium.org/1826853007/diff/2/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/LayoutView.cpp:494: if (style()->isFlippedBlocksWritingMode()) { On 2016/03/28 at 21:12:08, Xianzhu wrote: > On 2016/03/28 20:38:59, chrishtr wrote: > > This is handled by LayoutBox somehow? > > > > Is this tested? Add a unittest? > > Can a LayoutView have writing-mode? I think so. IIRC I added this code a month or so ago.
https://codereview.chromium.org/1826853007/diff/2/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/LayoutView.cpp (left): https://codereview.chromium.org/1826853007/diff/2/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/LayoutView.cpp:494: if (style()->isFlippedBlocksWritingMode()) { On 2016/03/28 22:09:58, chrishtr wrote: > On 2016/03/28 at 21:12:08, Xianzhu wrote: > > On 2016/03/28 20:38:59, chrishtr wrote: > > > This is handled by LayoutBox somehow? > > > > > > Is this tested? Add a unittest? > > > > Can a LayoutView have writing-mode? > > I think so. IIRC I added this code a month or so ago. Yes, LayoutView back-inherits the writing mode from html/body. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
