Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2250523003:
Implement SANACLAP (http://bit.ly/sanaclap). (Closed)
|
Created:
4 years, 4 months ago by skobes Modified:
4 years, 4 months ago CC:
blink-reviews, blink-reviews-layout_chromium.org, blink-reviews-style_chromium.org, chromium-reviews, eae+blinkwatch, jchaffraix+rendering, leviw+renderwatch, nainar, pdr+renderingwatchlist_chromium.org, rune, sashab, szager+layoutwatch_chromium.org, Tab Atkins, zoltan1 Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionImplement SANACLAP (http://bit.ly/sanaclap).
ScrollAnchor now suppresses anchoring if the anchor node or one of its ancestors
(up to the scroller) has just changed a layout-affecting CSS property.
Also removes bounce suppression, the 20-adjustment limit, and the exclusion of
absolute-positioned elements with non-zero left/top.
BUG=637626
Committed: https://crrev.com/98578a93aa90885ea8fc8a0d30c0823b7eb708ca
Cr-Commit-Position: refs/heads/master@{#413010}
Patch Set 1 #Patch Set 2 : kill AnchorPoint #
Total comments: 9
Patch Set 3 : rebase #Patch Set 4 : address review comments #
Total comments: 4
Patch Set 5 : address review comments #
Total comments: 2
Patch Set 6 : improve comment #Messages
Total messages: 45 (29 generated)
Patchset #1 (id:1) has been deleted
Description was changed from ========== Implement SANACLAP. BUG=637626 ========== to ========== Implement SANACLAP (http://bit.ly/sanaclap). ScrollAnchor now suppresses anchoring if an ancestor of the anchor node changed a layout-affecting CSS property. Also removes bounce suppression, the 20-adjustment limit, and the exclusion of absolute-positioned elements with non-zero left/top. BUG=637626 ==========
Patchset #1 (id:20001) has been deleted
Patchset #1 (id:40001) has been deleted
The CQ bit was checked by skobes@chromium.org to run a CQ dry run
Patchset #1 (id:60001) has been deleted
Patchset #1 (id:80001) has been deleted
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: Try jobs failed on following builders: linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by skobes@chromium.org to run a CQ dry run
Patchset #1 (id:100001) has been deleted
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 checked by skobes@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...
skobes@chromium.org changed reviewers: + bugsnash@chromium.org, ojan@chromium.org, ymalik@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
\o/ https://codereview.chromium.org/2250523003/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/layout/LayoutObject.h (right): https://codereview.chromium.org/2250523003/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/LayoutObject.h:1420: bool hasLayoutAffectingStyleChanged() { return m_bitfields.hasLayoutAffectingStyleChanged(); } <bikeshed>This name might be too general. It's not clear without reading all the code involved what the difference is from the needsLayout bits. Maybe we should just call this something very specific like scrollAnchorDisablingStyleChanged? https://codereview.chromium.org/2250523003/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/layout/ScrollAnchor.cpp (left): https://codereview.chromium.org/2250523003/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/ScrollAnchor.cpp:123: if (style->hasOutOfFlowPosition()) { I think we'll still need to exclude out of flow positioned elements from being anchors, but we can try this for now and see how it works on the web in practice. https://codereview.chromium.org/2250523003/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/layout/ScrollAnchor.cpp (right): https://codereview.chromium.org/2250523003/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/ScrollAnchor.cpp:189: bool ScrollAnchor::computeHasLayoutAffectingPropertyChanged() This is n^2 in the depth of the scrollable areas. I think that's probably fine in practice since there shouldn't be that many nested scrollable areas and we only do it for scrollers that are scrolled to a non-zero scroll offset. Just calling it out in case I'm missing some reason it's not fine. :) https://codereview.chromium.org/2250523003/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/style/ComputedStyle.cpp (right): https://codereview.chromium.org/2250523003/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/style/ComputedStyle.cpp:876: if (diff.needsLayout() || diff.transformChanged()) A couple thoughts on this. 1. You need to also check diff.needsPositionedMovementLayout. 2. Putting it in this function seems a bit strange to me. Maybe just put it directly in visualInvalidationDiff? 3. I think this is fine for now for us to test it in the wild, but the thing we spec/ship won't be able to use the needsLayout bit directly because it encodes our current logic for what needs a layout, not something about which properties changed. So, we probably need a TODO to that effect. For example: -When opacity changes, we only set the needsLayout bit if the element changed whether it was a stacking context, but probably want to always disable scroll anchoring? Or do we? https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/style/Com.... -When the textAutoSizingMultiplier changes we set this bit, but probably shouldn't disable scroll anchoring. I'm sure there are other examples. I just found those as two quick ones staring at the code. Bugs/Sasha/Elliott: Do you have opinions on the best way to incorporate this logic and/or whether we should always disable scroll anchoring on opacity changes or just when stacking context state changes? I suspect Tab has opinions on the latter too.
https://codereview.chromium.org/2250523003/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/style/ComputedStyle.cpp (right): https://codereview.chromium.org/2250523003/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/style/ComputedStyle.cpp:876: if (diff.needsLayout() || diff.transformChanged()) On 2016/08/16 at 23:54:32, ojan wrote: > A couple thoughts on this. > 1. You need to also check diff.needsPositionedMovementLayout. Isn't that already checked via diff.needsLayout()? > 2. Putting it in this function seems a bit strange to me. Maybe just put it directly in visualInvalidationDiff? Agree it doesn't seem 'property specific' to me > 3. I think this is fine for now for us to test it in the wild, but the thing we spec/ship won't be able to use the needsLayout bit directly because it encodes our current logic for what needs a layout, not something about which properties changed. So, we probably need a TODO to that effect. For example: > > -When opacity changes, we only set the needsLayout bit if the element changed whether it was a stacking context, but probably want to always disable scroll anchoring? Or do we? https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/style/Com.... > -When the textAutoSizingMultiplier changes we set this bit, but probably shouldn't disable scroll anchoring. > > I'm sure there are other examples. I just found those as two quick ones staring at the code. > > Bugs/Sasha/Elliott: Do you have opinions on the best way to incorporate this logic and/or whether we should always disable scroll anchoring on opacity changes or just when stacking context state changes? I suspect Tab has opinions on the latter too. Not sure. How does scroll anchoring currently decide what is a layout affecting change? Should be the same no?
On 2016/08/17 02:08:46, Bugs Nash wrote: > On 2016/08/16 at 23:54:32, ojan wrote: > > Bugs/Sasha/Elliott: Do you have opinions on the best way to incorporate this > logic and/or whether we should always disable scroll anchoring on opacity > changes or just when stacking context state changes? I suspect Tab has opinions > on the latter too. > > Not sure. How does scroll anchoring currently decide what is a layout affecting > change? Should be the same no? "Layout-affecting change" is a new concept being introduced here. I think the main criterion should be whether the change could affect the anchor node's position (as reported by relativeBounds in ScrollAnchor.cpp). Do stacking contexts affect positioning?
On 2016/08/17 at 04:12:49, skobes wrote: > On 2016/08/17 02:08:46, Bugs Nash wrote: > > On 2016/08/16 at 23:54:32, ojan wrote: > > > Bugs/Sasha/Elliott: Do you have opinions on the best way to incorporate this > > logic and/or whether we should always disable scroll anchoring on opacity > > changes or just when stacking context state changes? I suspect Tab has opinions > > on the latter too. > > > > Not sure. How does scroll anchoring currently decide what is a layout affecting > > change? Should be the same no? > > "Layout-affecting change" is a new concept being introduced here. > > I think the main criterion should be whether the change could affect the anchor node's position (as reported by relativeBounds in ScrollAnchor.cpp). Do stacking contexts affect positioning? They control what you're positioned with respect to. If you're position: absolute, and an ancestor becomes a stacking context (ex. position: relative) it can cause you to move. I'm not sure if we want to disable anchoring for that, we don't really know if things are going to move. Note that lots of things set these needsLayout() bits, ex. we go through layout for focus ring changes even though they don't really need to, because of how we track visual overflow. Using the bits like this will disable anchoring for tons of cases that will be hard to spec since it's just whatever our engine wants to do layout for (which is not always layout impacting properties).
Thanks for the review. On 2016/08/17 14:22:51, esprehn wrote: > They control what you're positioned with respect to. If you're position: > absolute, and an ancestor becomes a stacking context (ex. position: relative) it > can cause you to move. In that case I think we probably should disable anchoring. > Note that lots of things set > these needsLayout() bits, ex. we go through layout for focus ring changes even > though they don't really need to, because of how we track visual overflow. I agree focus ring changes shouldn't disable anchoring. Are there other examples that you know of? Maybe we should start building a list. > Using the bits like this will disable anchoring for tons of cases that will be hard to > spec since it's just whatever our engine wants to do layout for (which is not > always layout impacting properties). I certainly don't think we should write the spec based on "whatever our engine wants to do layout for". We should spec what makes sense based on "can this move the anchor node" and evolve the implementation to match. https://codereview.chromium.org/2250523003/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/layout/LayoutObject.h (right): https://codereview.chromium.org/2250523003/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/LayoutObject.h:1420: bool hasLayoutAffectingStyleChanged() { return m_bitfields.hasLayoutAffectingStyleChanged(); } On 2016/08/16 23:54:32, ojan wrote: > <bikeshed>This name might be too general. It's not clear without reading all the > code involved what the difference is from the needsLayout bits. Maybe we should > just call this something very specific like scrollAnchorDisablingStyleChanged? Done. https://codereview.chromium.org/2250523003/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/layout/ScrollAnchor.cpp (left): https://codereview.chromium.org/2250523003/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/ScrollAnchor.cpp:123: if (style->hasOutOfFlowPosition()) { On 2016/08/16 23:54:32, ojan wrote: > I think we'll still need to exclude out of flow positioned elements from being > anchors, but we can try this for now and see how it works on the web in > practice. Acknowledged. Either way we shouldn't special-case non-zero top/left so I think it makes sense for this change to remove the hack that was added for http://crbug.com/604996, then land a separate change to exclude all abspos if we decide to do that. I'm updating the WICG explainer to include a section on why I think we shouldn't exclude all abspos so we can have a public discussion about that. https://codereview.chromium.org/2250523003/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/layout/ScrollAnchor.cpp (right): https://codereview.chromium.org/2250523003/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/ScrollAnchor.cpp:189: bool ScrollAnchor::computeHasLayoutAffectingPropertyChanged() On 2016/08/16 23:54:32, ojan wrote: > This is n^2 in the depth of the scrollable areas. I think that's probably fine > in practice since there shouldn't be that many nested scrollable areas and we > only do it for scrollers that are scrolled to a non-zero scroll offset. Just > calling it out in case I'm missing some reason it's not fine. :) I agree this is probably fine. https://codereview.chromium.org/2250523003/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/style/ComputedStyle.cpp (right): https://codereview.chromium.org/2250523003/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/style/ComputedStyle.cpp:876: if (diff.needsLayout() || diff.transformChanged()) On 2016/08/16 23:54:32, ojan wrote: > 1. You need to also check diff.needsPositionedMovementLayout. As bugsnash noted, needsLayout includes this. > 2. Putting it in this function seems a bit strange to me. Maybe just put it > directly in visualInvalidationDiff? Done. > 3. I think this is fine for now for us to test it in the wild, but the thing we > spec/ship won't be able to use the needsLayout bit directly because it encodes > our current logic for what needs a layout, not something about which properties > changed. So, we probably need a TODO to that effect. Added TODO. Agree the spec should reference specific properties and not our needsLayout concept. Implementation-wise I wonder if it's better to start with needsLayout() and add exceptions for the cases where we set needsLayout but don't want to disable anchoring.
The CQ bit was checked by skobes@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 checked by skobes@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.
Description was changed from ========== Implement SANACLAP (http://bit.ly/sanaclap). ScrollAnchor now suppresses anchoring if an ancestor of the anchor node changed a layout-affecting CSS property. Also removes bounce suppression, the 20-adjustment limit, and the exclusion of absolute-positioned elements with non-zero left/top. BUG=637626 ========== to ========== Implement SANACLAP (http://bit.ly/sanaclap). ScrollAnchor now suppresses anchoring if the anchor node or one of its ancestors (up to the scroller) has just changed a layout-affecting CSS property. Also removes bounce suppression, the 20-adjustment limit, and the exclusion of absolute-positioned elements with non-zero left/top. BUG=637626 ==========
Per offline discussion with Ojan the current plan is: 1. Land this patch with the current logic of checking StyleDifference::needsLayout(). 2. Make a spreadsheet of all the properties / other things that cause needsLayout to be set. 3. Audit the spreadsheet for things we think shouldn't disable scroll anchoring, and link it from the explainer for WICG feedback. 4. Update the implementation based on the results of step 3. Separately we should decide what the policy is for future CSS properties that set needsLayout. We could - default to not-disabling since we do not have to worry about compat issues with existing websites, or - default to disabling for consistency with the principle of "disable for things that might move the anchor node". Does this sound ok to everyone?
On 2016/08/18 at 01:17:07, skobes wrote: > Per offline discussion with Ojan the current plan is: > > 1. Land this patch with the current logic of checking StyleDifference::needsLayout(). > > 2. Make a spreadsheet of all the properties / other things that cause needsLayout to be set. > > 3. Audit the spreadsheet for things we think shouldn't disable scroll anchoring, and link it from the explainer for WICG feedback. > > 4. Update the implementation based on the results of step 3. > > Separately we should decide what the policy is for future CSS properties that set needsLayout. We could > > - default to not-disabling since we do not have to worry about compat issues with existing websites, or > > - default to disabling for consistency with the principle of "disable for things that might move the anchor node". > > Does this sound ok to everyone? sgtm. Should there be a new layout test for this change?
lgtm https://codereview.chromium.org/2250523003/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/layout/ScrollAnchor.cpp (right): https://codereview.chromium.org/2250523003/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/ScrollAnchor.cpp:256: return; Will it be useful to have an UMA metric for the number of times we skip anchoring because of this? https://codereview.chromium.org/2250523003/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/ScrollAnchor.cpp:259: adjust(adjustment); nit: we don't seem to be using adjust elsewhere anymore. Move the code here?
On 2016/08/18 05:36:16, Bugs Nash wrote: > sgtm. Should there be a new layout test for this change? I think the new test cases in ScrollAnchorTest are sufficient to verify the functionality we are adding here? https://codereview.chromium.org/2250523003/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/layout/ScrollAnchor.cpp (right): https://codereview.chromium.org/2250523003/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/ScrollAnchor.cpp:256: return; On 2016/08/18 07:44:20, ymalik wrote: > Will it be useful to have an UMA metric for the number of times we skip > anchoring because of this? Added TODO. https://codereview.chromium.org/2250523003/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/ScrollAnchor.cpp:259: adjust(adjustment); On 2016/08/18 07:44:20, ymalik wrote: > nit: we don't seem to be using adjust elsewhere anymore. Move the code here? Done.
LGTM The immediate next step will be to audit the needsLayout setting bits and come to a conclusion about what the low-maintenance version of the bit setting should look like, so I think a TODO is OK for now given that we'd like to get some in the wild testing of the new behavior while we figure the above out. Regarding the specific case of opacity, stacking context doesn't imply containing block. Stacking context doesn't affect your position AFAIK, so changing opacity shouldn't matter here. IIRC the reason we force a layout is just because of technical debt in our codebase, but I don't fully remember. Test case http://plexode.com/eval3/#s=aekVQXANJVQMbAx1yAXgePQOEGwETUVkBhxwBUFFCREpVWhsR... This is a good example of what we're going to need to evaluate for each row in our spreadsheet. Most will be obvious though, e.g. changing width. https://codereview.chromium.org/2250523003/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/layout/ScrollAnchor.h (right): https://codereview.chromium.org/2250523003/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/ScrollAnchor.h:108: // has had a scroll-anchor-disabling style changed since the last layout. Nit: Might want to add a bit more to this so people don't have to go to the bit.ly link to understand. Just something like: Style changes on the node or one of it's ancestors that can cause the node to move disable scroll anchoring.
lgtm
https://codereview.chromium.org/2250523003/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/layout/ScrollAnchor.h (right): https://codereview.chromium.org/2250523003/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/ScrollAnchor.h:108: // has had a scroll-anchor-disabling style changed since the last layout. On 2016/08/18 18:39:48, ojan wrote: > Nit: Might want to add a bit more to this so people don't have to go to the > bit.ly link to understand. Just something like: > Style changes on the node or one of it's ancestors that can cause the node to > move disable scroll anchoring. Done.
The CQ bit was checked by skobes@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from ymalik@chromium.org, bugsnash@chromium.org, ojan@chromium.org Link to the patchset: https://codereview.chromium.org/2250523003/#ps220001 (title: "improve comment")
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.
Description was changed from ========== Implement SANACLAP (http://bit.ly/sanaclap). ScrollAnchor now suppresses anchoring if the anchor node or one of its ancestors (up to the scroller) has just changed a layout-affecting CSS property. Also removes bounce suppression, the 20-adjustment limit, and the exclusion of absolute-positioned elements with non-zero left/top. BUG=637626 ========== to ========== Implement SANACLAP (http://bit.ly/sanaclap). ScrollAnchor now suppresses anchoring if the anchor node or one of its ancestors (up to the scroller) has just changed a layout-affecting CSS property. Also removes bounce suppression, the 20-adjustment limit, and the exclusion of absolute-positioned elements with non-zero left/top. BUG=637626 ==========
Message was sent while issue was closed.
Committed patchset #6 (id:220001)
Message was sent while issue was closed.
Description was changed from ========== Implement SANACLAP (http://bit.ly/sanaclap). ScrollAnchor now suppresses anchoring if the anchor node or one of its ancestors (up to the scroller) has just changed a layout-affecting CSS property. Also removes bounce suppression, the 20-adjustment limit, and the exclusion of absolute-positioned elements with non-zero left/top. BUG=637626 ========== to ========== Implement SANACLAP (http://bit.ly/sanaclap). ScrollAnchor now suppresses anchoring if the anchor node or one of its ancestors (up to the scroller) has just changed a layout-affecting CSS property. Also removes bounce suppression, the 20-adjustment limit, and the exclusion of absolute-positioned elements with non-zero left/top. BUG=637626 Committed: https://crrev.com/98578a93aa90885ea8fc8a0d30c0823b7eb708ca Cr-Commit-Position: refs/heads/master@{#413010} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/98578a93aa90885ea8fc8a0d30c0823b7eb708ca Cr-Commit-Position: refs/heads/master@{#413010} |
