Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2002623003:
Exclude position:absolute elements when picking an anchor for ScrollAnchoring (Closed)
|
Created:
4 years, 7 months ago by ymalik Modified:
4 years, 6 months ago Reviewers:
skobes CC:
chromium-reviews, szager+layoutwatch_chromium.org, zoltan1, blink-reviews-layout_chromium.org, pdr+renderingwatchlist_chromium.org, eae+blinkwatch, leviw+renderwatch, jchaffraix+rendering, blink-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionExclude position:absolute elements when picking an anchor for ScrollAnchoring
Only exclude ones that have top/left/right/bottom set to a non-zero value.
BUG=604996
Committed: https://crrev.com/556834f09ba3e4caf3ac682330df55c03bf0ffda
Cr-Commit-Position: refs/heads/master@{#397159}
Patch Set 1 #
Total comments: 4
Patch Set 2 : exclude only for non-zero top/left/bottom/right #
Total comments: 1
Patch Set 3 : nit + rebase #
Messages
Total messages: 25 (10 generated)
Description was changed from ========== [WIP]Exclude position:absolute elements when picking an anchor for ScrollAnchoring BUG=604996 ========== to ========== [WIP]Exclude position:absolute elements when picking an anchor for ScrollAnchoring BUG=604996 ==========
Description was changed from ========== [WIP]Exclude position:absolute elements when picking an anchor for ScrollAnchoring BUG=604996 ========== to ========== Exclude position:absolute elements when picking an anchor for ScrollAnchoring BUG=604996 ==========
ymalik@chromium.org changed reviewers: + skobes@chromium.org
We already have a unit test for position:absolute, maybe just update that? https://codereview.chromium.org/2002623003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/ScrollAnchor.cpp (right): https://codereview.chromium.org/2002623003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/ScrollAnchor.cpp:112: // Elements with absolute positioning are often used for social nav "social nav bars" is too specific, just say that these elements may stick to the viewport. https://codereview.chromium.org/2002623003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/ScrollAnchor.cpp:118: candidate->container(scrollerLayoutBox(scroller), &skippedByContainerLookup); I think we don't need this anymore?
Also we want to exclude only for non-zero left/top. This should hopefully let us continue anchoring on pages like https://newrepublic.com/article/133043/youve-got-hate-mail that use a high level position:absolute wrapper.
On 2016/05/28 00:20:05, skobes wrote: > Also we want to exclude only for non-zero left/top. > > This should hopefully let us continue anchoring on pages like > https://newrepublic.com/article/133043/youve-got-hate-mail that use a high level > position:absolute wrapper. Excluding only for non-zero left/top seems hacky. For example, if the wrapper just happened to have top:1, we would still exclude it. I think its more rational to exclude all absolute positioned elements. WDYT?
https://codereview.chromium.org/2002623003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/ScrollAnchor.cpp (right): https://codereview.chromium.org/2002623003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/ScrollAnchor.cpp:118: candidate->container(scrollerLayoutBox(scroller), &skippedByContainerLookup); On 2016/05/28 00:18:10, skobes wrote: > I think we don't need this anymore? You're right that we don't need this if we don't special case position:absolute with non-zero top/left. If we do end up only excluding absolute positioned elements with non-zero top/left, then we still need this because the candidate can be stuck to the viewport when top/left = 0.
On 2016/05/30 18:12:48, ymalik wrote: > Excluding only for non-zero left/top seems hacky. For example, if the wrapper > just happened to have top:1, we would still exclude it. I think its more > rational to exclude all absolute positioned elements. WDYT? I think anything we do here will be somewhat hacky, and there will always be hypothetical ways it can break. The concrete examples we have so far are: (1) pages like laist.com that emulate fixed by setting absolute + left + top (2) pages like newrepublic.com and plus.google.com that use absolute-positioned wrappers with left and top == 0 Excluding all absolute-positioned elements will fix (1) but break (2). We can handle both cases with minimal overhead by checking for non-zero left/top.
Description was changed from ========== Exclude position:absolute elements when picking an anchor for ScrollAnchoring BUG=604996 ========== to ========== Exclude position:absolute elements when picking an anchor for ScrollAnchoring Only exclude ones that have top/left/right/bottom set to a non-zero value. BUG=604996 ==========
@skobes PTAL :) https://codereview.chromium.org/2002623003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/ScrollAnchor.cpp (right): https://codereview.chromium.org/2002623003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/ScrollAnchor.cpp:112: // Elements with absolute positioning are often used for social nav On 2016/05/28 00:18:10, skobes wrote: > "social nav bars" is too specific, just say that these elements may stick to the > viewport. Done. https://codereview.chromium.org/2002623003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/ScrollAnchor.cpp:118: candidate->container(scrollerLayoutBox(scroller), &skippedByContainerLookup); On 2016/05/30 18:13:03, ymalik wrote: > On 2016/05/28 00:18:10, skobes wrote: > > I think we don't need this anymore? > > You're right that we don't need this if we don't special case position:absolute > with non-zero top/left. If we do end up only excluding absolute positioned > elements with non-zero top/left, then we still need this because the candidate > can be stuck to the viewport when top/left = 0. As per your comment below and our offline conversation, we decided to keep top/left, so we need this.
@skobes PTAL :) https://codereview.chromium.org/2002623003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/ScrollAnchor.cpp (right): https://codereview.chromium.org/2002623003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/ScrollAnchor.cpp:112: // Elements with absolute positioning are often used for social nav On 2016/05/28 00:18:10, skobes wrote: > "social nav bars" is too specific, just say that these elements may stick to the > viewport. Done.
The CQ bit was checked by ymalik@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2002623003/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2002623003/20001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm https://codereview.chromium.org/2002623003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/ScrollAnchor.cpp (right): https://codereview.chromium.org/2002623003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ScrollAnchor.cpp:116: if (!style->top().isZero()) nit: this would be more concise with ||
The CQ bit was checked by ymalik@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from skobes@chromium.org Link to the patchset: https://codereview.chromium.org/2002623003/#ps40001 (title: "nit + rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2002623003/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2002623003/40001
Message was sent while issue was closed.
Description was changed from ========== Exclude position:absolute elements when picking an anchor for ScrollAnchoring Only exclude ones that have top/left/right/bottom set to a non-zero value. BUG=604996 ========== to ========== Exclude position:absolute elements when picking an anchor for ScrollAnchoring Only exclude ones that have top/left/right/bottom set to a non-zero value. BUG=604996 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Exclude position:absolute elements when picking an anchor for ScrollAnchoring Only exclude ones that have top/left/right/bottom set to a non-zero value. BUG=604996 ========== to ========== Exclude position:absolute elements when picking an anchor for ScrollAnchoring Only exclude ones that have top/left/right/bottom set to a non-zero value. BUG=604996 Committed: https://crrev.com/556834f09ba3e4caf3ac682330df55c03bf0ffda Cr-Commit-Position: refs/heads/master@{#397159} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/556834f09ba3e4caf3ac682330df55c03bf0ffda Cr-Commit-Position: refs/heads/master@{#397159} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
