Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2310223002:
Disable scroll anchoring is an element within the scroll changes its in-flow state (Closed)
|
Created:
4 years, 3 months ago by ymalik Modified:
4 years, 3 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 Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionDisable scroll anchoring is an element within the scroll changes its in-flow state
BUG=641814
Committed: https://crrev.com/a6268a949ace32beb1ac82409aa551d1b135dd92
Cr-Commit-Position: refs/heads/master@{#417077}
Patch Set 1 #Patch Set 2 : Fix presubmit #
Total comments: 12
Patch Set 3 : apply review comments #
Total comments: 2
Patch Set 4 : s\do while\while #
Messages
Total messages: 21 (8 generated)
ymalik@chromium.org changed reviewers: + ojan@chromium.org, skobes@chromium.org
https://codereview.chromium.org/2310223002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/LayoutObject.cpp (right): https://codereview.chromium.org/2310223002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutObject.cpp:1715: void LayoutObject::setScrollAnchorDisablingStyleChangedOnAncestor() It's not clear to me that this is sufficient for nested scrollers. Can an anchor node be the anchor for multiple scrolling ancestors? This probably deserves a test. https://codereview.chromium.org/2310223002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutObject.cpp:1747: && ((oldStyle->position() == StaticPosition || m_style->position() == StaticPosition) Why do you need to check for StaticPosition? That's a subset of in-flow position, right?
https://codereview.chromium.org/2310223002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/LayoutObject.cpp (right): https://codereview.chromium.org/2310223002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutObject.cpp:1724: bool hasScrollableOverflow = block->hasScrollableOverflowX() || block->hasScrollableOverflowY(); I don't think we should use this since it depends on layout (presence of overflow). Would it work to pick the first block that hasOverflowClip()? This seems safe because if a block clips overflow but doesn't actually have overflow, position changes in its descendants still won't affect things outside it. (I suppose there are exceptions to this.)
https://codereview.chromium.org/2310223002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/LayoutObject.cpp (right): https://codereview.chromium.org/2310223002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutObject.cpp:1715: void LayoutObject::setScrollAnchorDisablingStyleChangedOnAncestor() On 2016/09/06 23:36:36, ojan wrote: > It's not clear to me that this is sufficient for nested scrollers. Can an anchor > node be the anchor for multiple scrolling ancestors? I think the answer to this is "yes, but that's ok". ScrollAnchor looks for the SANACLAP bit on anything in the ancestor path from the anchor node, so setting it on the inner scroller is sufficient to suppress both scrollers.
Ojan / Steve, question for you :) https://codereview.chromium.org/2310223002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/LayoutObject.cpp (right): https://codereview.chromium.org/2310223002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutObject.cpp:1715: void LayoutObject::setScrollAnchorDisablingStyleChangedOnAncestor() On 2016/09/07 00:23:06, skobes wrote: > On 2016/09/06 23:36:36, ojan wrote: > > It's not clear to me that this is sufficient for nested scrollers. Can an > anchor > > node be the anchor for multiple scrolling ancestors? > > I think the answer to this is "yes, but that's ok". ScrollAnchor looks for the > SANACLAP bit on anything in the ancestor path from the anchor node, so setting > it on the inner scroller is sufficient to suppress both scrollers. Yes, that case is covered, but another question is, can a position change in the inner scroller move things around on the outer scroller. This can probably happen in some cases but rare? For example, say that the inner scroller has an element that goes from static -> absolute, and is now positioned relative to the outer scroller. Say that this change impacts the height of the inner scroller (auto height and horizontal scrolling?) and the user is scrolled past the inner scroller. Now the anchor node for the outer scroller (somewhere below the inner scroller) will move, and we won't suppress anchoring. It's probably more complete to suppress all scrollers on the path to root, but the rarity of the scenarios where it will help may make this optimization worth it? WDYT Ojan/Steve? https://codereview.chromium.org/2310223002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutObject.cpp:1724: bool hasScrollableOverflow = block->hasScrollableOverflowX() || block->hasScrollableOverflowY(); On 2016/09/07 00:02:11, skobes wrote: > I don't think we should use this since it depends on layout (presence of > overflow). Would it work to pick the first block that hasOverflowClip()? > > This seems safe because if a block clips overflow but doesn't actually have > overflow, position changes in its descendants still won't affect things outside > it. (I suppose there are exceptions to this.) I agree that we shouldn't depend on layout during layout. Will make this change. https://codereview.chromium.org/2310223002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutObject.cpp:1747: && ((oldStyle->position() == StaticPosition || m_style->position() == StaticPosition) On 2016/09/06 23:36:36, ojan wrote: > Why do you need to check for StaticPosition? That's a subset of in-flow > position, right? For reasons that I don't understand, StaticPosition is not referred to as being in flow. https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/style/Com... There's probably a good reason for this?
https://codereview.chromium.org/2310223002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/LayoutObject.cpp (right): https://codereview.chromium.org/2310223002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutObject.cpp:1715: void LayoutObject::setScrollAnchorDisablingStyleChangedOnAncestor() On 2016/09/07 00:39:57, ymalik wrote: > It's probably more complete to suppress all scrollers on the path to root, but > the rarity of the scenarios where it will help may make this optimization worth > it? My gut feeling is we don't gain anything by suppressing all scrollers to the root. There are theoretical ways to break our fix, but that's always the case. If we encounter nested-scroller compat issues in the wild we can revisit.
l-g-t-m once you resolve skobes's comments and the out of flow position thing. https://codereview.chromium.org/2310223002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/LayoutObject.cpp (right): https://codereview.chromium.org/2310223002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutObject.cpp:1747: && ((oldStyle->position() == StaticPosition || m_style->position() == StaticPosition) On 2016/09/07 at 00:39:57, ymalik wrote: > On 2016/09/06 23:36:36, ojan wrote: > > Why do you need to check for StaticPosition? That's a subset of in-flow > > position, right? > > For reasons that I don't understand, StaticPosition is not referred to as being in flow. https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/style/Com... > > There's probably a good reason for this? Hmmm...I guess I'm misremembering how this code worked. I think we have a different set of checks in the layout code. In either case, I don't think you need all four of these checks. All you need to check is if oldStyle->hasOutOfFlowPosition() != m_style->hasOutOfFlowPosition(). You just want to know if the thing changes its out of flow position state, right?
@skobes, LTAL https://codereview.chromium.org/2310223002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/LayoutObject.cpp (right): https://codereview.chromium.org/2310223002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutObject.cpp:1715: void LayoutObject::setScrollAnchorDisablingStyleChangedOnAncestor() On 2016/09/07 00:49:31, skobes wrote: > On 2016/09/07 00:39:57, ymalik wrote: > > It's probably more complete to suppress all scrollers on the path to root, but > > the rarity of the scenarios where it will help may make this optimization > worth > > it? > > My gut feeling is we don't gain anything by suppressing all scrollers to the > root. There are theoretical ways to break our fix, but that's always the case. > If we encounter nested-scroller compat issues in the wild we can revisit. Acknowledged. https://codereview.chromium.org/2310223002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutObject.cpp:1724: bool hasScrollableOverflow = block->hasScrollableOverflowX() || block->hasScrollableOverflowY(); On 2016/09/07 00:39:57, ymalik wrote: > On 2016/09/07 00:02:11, skobes wrote: > > I don't think we should use this since it depends on layout (presence of > > overflow). Would it work to pick the first block that hasOverflowClip()? > > > > This seems safe because if a block clips overflow but doesn't actually have > > overflow, position changes in its descendants still won't affect things > outside > > it. (I suppose there are exceptions to this.) > > I agree that we shouldn't depend on layout during layout. Will make this change. Done. https://codereview.chromium.org/2310223002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutObject.cpp:1747: && ((oldStyle->position() == StaticPosition || m_style->position() == StaticPosition) On 2016/09/07 00:56:44, ojan wrote: > On 2016/09/07 at 00:39:57, ymalik wrote: > > On 2016/09/06 23:36:36, ojan wrote: > > > Why do you need to check for StaticPosition? That's a subset of in-flow > > > position, right? > > > > For reasons that I don't understand, StaticPosition is not referred to as > being in flow. > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/style/Com... > > > > There's probably a good reason for this? > > Hmmm...I guess I'm misremembering how this code worked. I think we have a > different set of checks in the layout code. > > In either case, I don't think you need all four of these checks. All you need to > check is if oldStyle->hasOutOfFlowPosition() != m_style->hasOutOfFlowPosition(). > > You just want to know if the thing changes its out of flow position state, > right? You're totally right. This works.
lgtm w/ nit https://codereview.chromium.org/2310223002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/LayoutObject.cpp (right): https://codereview.chromium.org/2310223002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutObject.cpp:1730: } while (object); This can be a normal while loop.
https://codereview.chromium.org/2310223002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/LayoutObject.cpp (right): https://codereview.chromium.org/2310223002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutObject.cpp:1730: } while (object); On 2016/09/07 19:26:27, skobes wrote: > This can be a normal while loop. 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/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
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/2310223002/#ps60001 (title: "s\do while\while")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Disable scroll anchoring is an element within the scroll changes its in-flow state BUG=641814 ========== to ========== Disable scroll anchoring is an element within the scroll changes its in-flow state BUG=641814 Committed: https://crrev.com/a6268a949ace32beb1ac82409aa551d1b135dd92 Cr-Commit-Position: refs/heads/master@{#417077} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/a6268a949ace32beb1ac82409aa551d1b135dd92 Cr-Commit-Position: refs/heads/master@{#417077} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
