scroll anchoring gets stuck at top of https://keenhome.io/smart-vent |
||||||
Issue descriptionChrome Version : 55.0.2842.0 OS Version: OS X 10.11.6 What steps will reproduce the problem? 1. Go to https://keenhome.io/smart-vent 2. Scroll down *slowly*. Scroll anchoring keeps scrolling you back to the top of the screen.
,
Aug 29 2016
Looking into this.
,
Aug 29 2016
,
Aug 29 2016
The page has a header and JS that fixes the header in the viewport using position:fixed. The header looks like this: <div class="header"> <ol class="headerContent"> </ol> <div> When the header is fixed, the height of #header is shrunk and the content inside #headerContent is changed. Changing the positioning of the header causes layout and we anchor to some element below the header. After layout, because the header is no longer in-flow and no longer adds to the height of its container, the content jumps up, triggering an erroneous adjustment. This is different from issue 601906 , issue 603246 in that the webpage doesn't change a parent to compensate for the jump, which the basis of disabling scroll anchoring in the SANACLAP proposal. I can imagine there being more sites on the web that do position:fixed this way rather than adding changes to prevent the jump? Speculatively, bounce suppression should fix this, but I am worried that we may follow the same path as before of adding bad hacks. Ojan/Elliott/Steve, any suggestions?
,
Aug 29 2016
I think we knew SANACLAP wouldn't completely prevent scroll handler feedback loops, but didn't realize it was as simple as pulling a header out-of-flow without any compensation. I think we will need to resurrect bounce suppression, or do some kind of suppression triggered by "position" changes (which seems even more hacky).
,
Aug 30 2016
I think most of the breakages here are caused by authors fixing things to the viewport. What if we modify "findAnchor" to anchor to elements just above the viewport rather than inside the viewport. I think that will fix this case? The disadvantage is of course that we don't anchor if something loads under the anchor point and above the viewport.
,
Aug 30 2016
Why would that fix this? I thought we were not anchoring to the header. If we were anchoring to the header then SANACLAP should have fixed it.
,
Aug 30 2016
When a header changes from non-fixed to fixed, only the elements below it, while it was in-flow, would shift up. Is this not true?
,
Aug 30 2016
Yes, but the viewport may be an arbitrary distance below that by the time this occurs.
,
Aug 30 2016
Ah right! Yeah, my proposal doesn't work.
,
Aug 30 2016
Hmm, I'm not a big fan of the bounce suppression, I'll have to think more about this.
,
Aug 30 2016
,
Aug 30 2016
Could you make a reduced test case so we can think about this with an example? :)
,
Aug 30 2016
I think we should try the following addition to SANACLAP: If any of the aunts/uncles of the scroll anchor up to the scrolling root changes from in-flow to out-of-flow (or vice versa), then we don't scroll anchor that frame. I think that's more general than the bounce suppression. It doesn't address all the possible cases (e.g. removing an element from the DOM), but I think that's actually what we want. I think there are three general cases of things that break: -Dragging: solved by SANACLAP as far as we know. -Infinite scroll: Need to dig into the devtools case to understand it better. -Sticky headers: SANACLAP solves most of the known issues. The rest of the known issues involve changing the position CSS property. There may be other cases, but I think we should try this fix. Fix the devtools thing. Then see how we do on Canary. Sound OK?
,
Aug 30 2016
Though not in this case, the header could potentially be a sibling of the anchor element? Also, is it efficient to walk up the DOM and check aunts/uncles every layout?
,
Aug 30 2016
Ah, right. Siblings and aunts/uncles need to be checked. It's not efficient to walk up the DOM, but during style recalc, if the in-flow-ness of an element changes, we can setScrollAnchorDisablingStyleChanged on it's ancestor.
,
Aug 31 2016
I could be convinced that special-casing "position" changes is better than bounce suppression. I'm not sure it makes sense to treat aunts/uncles differently from cousins. On this page the header is an immediate child of <body>, but it would be trivial to wrap it in a <div>, breaking the proposed fix. Maybe any "position" change within a scroller should suppress scroll anchoring?
,
Aug 31 2016
skobes: Good point! I think we should try what you suggested of having any position change within a scroller suppress scroll anchoring. Although, I think we only need to have a change of in-flow state suppress it. Changing static->relative or absolute->fixed doesn't need to suppress. SG?
,
Aug 31 2016
Sounds good. Note we should be careful not to suppress on insertion of new DOM content, regardless of its position. That might impose some complexity (versus naively hooking LayoutObject::styleDidChange).
,
Sep 1 2016
re #16, it's still not clear to me how to do this efficiently. How high in the parent chain do you go up to setScrollAnchorDisablingStyleChanged? It should probably be every parent up until the root element, but then how do you differentiate this bit being set on an element by something above the anchor node from below the anchor node? Also, it sounds like if we're special casing the in-flow-ness, we will need to add a new bit to differentiate this from any other SANACLAP bit set on the elements above the anchor?
,
Sep 1 2016
Minimized repro: http://jsbin.com/nekije/quiet
,
Sep 1 2016
I think we should be able to reuse the SANACLAP bit. We just need position changes to set the bit on the nearest containing scroller.
,
Sep 1 2016
Yeah, that works if we want to disable anchoring for any position changes within the scroller. I thought that we should only disable anchoring if the position change is above the anchor because its probably the one that will cause the anchor to move. But I suppose just disabling for any position changes is simpler and what Ojan was referring to in #18.
,
Sep 7 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/a6268a949ace32beb1ac82409aa551d1b135dd92 commit a6268a949ace32beb1ac82409aa551d1b135dd92 Author: ymalik <ymalik@chromium.org> Date: Wed Sep 07 22:08:37 2016 Disable scroll anchoring is an element within the scroll changes its in-flow state BUG= 641814 Review-Url: https://codereview.chromium.org/2310223002 Cr-Commit-Position: refs/heads/master@{#417077} [modify] https://crrev.com/a6268a949ace32beb1ac82409aa551d1b135dd92/third_party/WebKit/Source/core/layout/LayoutObject.cpp [modify] https://crrev.com/a6268a949ace32beb1ac82409aa551d1b135dd92/third_party/WebKit/Source/core/layout/LayoutObject.h [modify] https://crrev.com/a6268a949ace32beb1ac82409aa551d1b135dd92/third_party/WebKit/Source/core/layout/ScrollAnchorTest.cpp
,
Sep 7 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/a6268a949ace32beb1ac82409aa551d1b135dd92 commit a6268a949ace32beb1ac82409aa551d1b135dd92 Author: ymalik <ymalik@chromium.org> Date: Wed Sep 07 22:08:37 2016 Disable scroll anchoring is an element within the scroll changes its in-flow state BUG= 641814 Review-Url: https://codereview.chromium.org/2310223002 Cr-Commit-Position: refs/heads/master@{#417077} [modify] https://crrev.com/a6268a949ace32beb1ac82409aa551d1b135dd92/third_party/WebKit/Source/core/layout/LayoutObject.cpp [modify] https://crrev.com/a6268a949ace32beb1ac82409aa551d1b135dd92/third_party/WebKit/Source/core/layout/LayoutObject.h [modify] https://crrev.com/a6268a949ace32beb1ac82409aa551d1b135dd92/third_party/WebKit/Source/core/layout/ScrollAnchorTest.cpp
,
Sep 12 2016
verified fixed in canary,
,
Sep 27 2016
|
||||||
►
Sign in to add a comment |
||||||
Comment 1 Deleted