Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1540443003:
Fix LayoutObject::mapToVisibleRectInContainerSpace() (Closed)
|
Created:
5 years ago by Xianzhu Modified:
5 years 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@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionFix LayoutObject::mapToVisibleRectInContainerSpace()
The original code missed scroll offset adjustment when the paint
invalidation container has overflow clip.
BUG=567875
Committed: https://crrev.com/c42f3abcd2c3c45380c21b751d057b46c21cf534
Cr-Commit-Position: refs/heads/master@{#366178}
Patch Set 1 #
Total comments: 5
Patch Set 2 : #
Total comments: 2
Patch Set 3 : #
Dependent Patchsets: Messages
Total messages: 21 (7 generated)
wangxianzhu@chromium.org changed reviewers: + chrishtr@chromium.org
The issue didn't cause many problems because LayoutInline's and LayoutBox's versions of mapToVisibleRectInContainerSpace() are correct. The issue affects selection invalidation of texts directly under a composited scrolling container.
wangxianzhu@chromium.org changed reviewers: + pdr@chromium.org
https://codereview.chromium.org/1540443003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/LayoutBox.h (right): https://codereview.chromium.org/1540443003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/LayoutBox.h:801: void applyCachedClipAndScrollOffsetForPaintInvalidationIfNeeded(const LayoutBoxModelObject* paintInvalidationContainer, LayoutRect& paintRect) const; Document that clips are not applied for the paintInvalidationContainer. Also, let's rename this method since it is not really paint invalidation container, but container. https://codereview.chromium.org/1540443003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/LayoutObject.cpp (right): https://codereview.chromium.org/1540443003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/LayoutObject.cpp:1372: LayoutSize inverseOffset= LayoutSize(toLayoutBox(&paintInvalidationContainer)->scrolledContentOffset()); space before =
szager@chromium.org changed reviewers: + szager@chromium.org
https://codereview.chromium.org/1540443003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/LayoutObject.cpp (left): https://codereview.chromium.org/1540443003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/LayoutObject.cpp:1604: if (o != paintInvalidationContainer && o->hasOverflowClip()) { Doh! Thanks for fixing this.
chrishtr@ is on vacation. szager@ and pdr@ can you ptal the updated patch? https://codereview.chromium.org/1540443003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/LayoutBox.h (right): https://codereview.chromium.org/1540443003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/LayoutBox.h:801: void applyCachedClipAndScrollOffsetForPaintInvalidationIfNeeded(const LayoutBoxModelObject* paintInvalidationContainer, LayoutRect& paintRect) const; On 2015/12/18 01:58:07, chrishtr - on vacation wrote: > Document that clips are not applied for the paintInvalidationContainer. > Done. > Also, let's rename this method since it is not really paint invalidation > container, but container. Removed the paintInvalidationContainer, because the function can test if this object is a paint invalidation container by itself. https://codereview.chromium.org/1540443003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/LayoutObject.cpp (right): https://codereview.chromium.org/1540443003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/LayoutObject.cpp:1372: LayoutSize inverseOffset= LayoutSize(toLayoutBox(&paintInvalidationContainer)->scrolledContentOffset()); On 2015/12/18 01:58:07, chrishtr - on vacation wrote: > space before = An incomplete cleanup of debug code. Reverted.
https://codereview.chromium.org/1540443003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/LayoutBox.cpp (right): https://codereview.chromium.org/1540443003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutBox.cpp:956: if (isPaintInvalidationContainer()) This doesn't work for the IntersectionObserver use case (which was the motivation for my previous change). Somewhere, the logic needs to explicitly check against the provided (badly-named) paintInvalidationContainer. You might also want to rename it to something that makes it clear this code isn't only used for paint invalidation.
https://codereview.chromium.org/1540443003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/LayoutBox.cpp (right): https://codereview.chromium.org/1540443003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutBox.cpp:956: if (isPaintInvalidationContainer()) On 2015/12/18 18:49:21, szager1 wrote: > This doesn't work for the IntersectionObserver use case (which was the > motivation for my previous change). > > Somewhere, the logic needs to explicitly check against the provided > (badly-named) paintInvalidationContainer. You might also want to rename it to > something that makes it clear this code isn't only used for paint invalidation. Oh I didn't notice that this patch is to fix a recent change. I thought it were an old paint invalidation bug because crbug.com/567875 is a M48 regression. It's weird that this patch does fix the bug in M49. Will upload a new patch for M49, and investigate the bug for M48.
Description was changed from ========== Fix LayoutObject::mapToVisibleRectInContainerSpace() The original code missed scroll offset adjustment when the paint invalidation container has overflow clip. Also extracted similar code in LayoutObject, LayoutInline and LayoutBox into LayoutBox::applyCachedClipAndScrollOffsetForPaintInvalidationIfNeeded() BUG=567875 ========== to ========== Fix LayoutObject::mapToVisibleRectInContainerSpace() The original code missed scroll offset adjustment when the paint invalidation container has overflow clip. BUG=567875 ==========
Uploaded a much simpler patch. Will address the naming issues with later changes.
LGTM
The CQ bit was checked by wangxianzhu@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1540443003/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1540443003/40001
Message was sent while issue was closed.
Description was changed from ========== Fix LayoutObject::mapToVisibleRectInContainerSpace() The original code missed scroll offset adjustment when the paint invalidation container has overflow clip. BUG=567875 ========== to ========== Fix LayoutObject::mapToVisibleRectInContainerSpace() The original code missed scroll offset adjustment when the paint invalidation container has overflow clip. BUG=567875 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Fix LayoutObject::mapToVisibleRectInContainerSpace() The original code missed scroll offset adjustment when the paint invalidation container has overflow clip. BUG=567875 ========== to ========== Fix LayoutObject::mapToVisibleRectInContainerSpace() The original code missed scroll offset adjustment when the paint invalidation container has overflow clip. BUG=567875 Committed: https://crrev.com/c42f3abcd2c3c45380c21b751d057b46c21cf534 Cr-Commit-Position: refs/heads/master@{#366178} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/c42f3abcd2c3c45380c21b751d057b46c21cf534 Cr-Commit-Position: refs/heads/master@{#366178}
Message was sent while issue was closed.
lgtm, thanks |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
