techcrunch and scroll anchoring don't play nicely |
|||||
Issue description1. On a phone, go to http://techcrunch.com/2016/04/05/will-an-apple-a-day-keep-webrtc-away/. 2. Scroll down so the "read full article" button is visible. 3. Click that button. The part of the page that's anchored is the ads at the bottom instead of the main article content.
,
Apr 8 2016
I see two issues with the techcrunch page. The first one is that when you scroll past the point when the top nav hides, there is this judder which goes away once you scroll past a certain point. The second issue is that clicking the "read full article" scrolls (adjusts?) the page to the bottom, which is incorrect because new content was loaded in the viewport not above. Is this the issue you are referring to in this bug? I investigated the second issue and it looks like we're anchoring the <article> element not the button. I haven't gotten into why we choose the <article> as the anchor when AFAICT from the code, we try to find the deepest element in the visible rect of the scroller (which should be the text inside the <article>. In any case, I think the right thing to do is clear the anchor element if it's changed. WDYT?
,
Apr 8 2016
Yes, there are two bugs. You're correct that I was filing this bug for the second issue. Mind filing a separate bug for the first issue so we can make sure to fix that as well? At the moment it's not clear to me if the design where we can anchor to any edge is the root problem or if we should find a way to make that design work better (e.g. by clearing the anchor element). Can you make this into a minimal, reduced test case? We'll need that anyways so we add a test with the patch and looking at a reduced test case will make it easier to figure out what the right solution is.
,
Apr 8 2016
,
Apr 11 2016
Filed issue 601906 for the navbar bug. Minimized test case: https://jsbin.com/gukicun To repro: 1) Scroll until only the green div intersects with the viewport. 2) Click on the green div We pick the bottom left corner of the div as the anchor position and re-sizing the div means that our anchor position has moved and we need to adjust the offset. This is what happens in the techcrunch case. https://codereview.chromium.org/1875943004/ fixes this by not selecting the node that may change due to layout. Can you think of any cases where the anchor node that may change is the correct anchor node to pick? >>At the moment it's not clear to me if the design where we can anchor to any edge is the root problem I think you would still need this otherwise anchoring won't work when we can't find an appropriate anchor (because the only viable option is an edge of the div).
,
Apr 11 2016
Looking at needsLayout doesn't work. You could anchor to that node and then it could get the needsLayout bit later. Also, you don't know why it needs layout. I think we just need to pick a better anchor. Do you remember the cases that caused Steve to think we should anchor to bottom edge of the element? Maybe we should find a better solution for those cases. I've been using this flag as my default recently and it's becoming more and more clear to me that it's whatever that's at the top of the page that I expect to stay anchored. So, at the moment, I'm thinking we should switch back to finding the deepest, visible element at the top of the page (skipping positioned elements as we do now). But, before we do that, we should look at Steve's latest patch to see why he changed that.
,
Apr 17 2016
Here's another case where anchoring to the bottom doesn't work. 1. https://www.twitch.tv/directory/all/ps4 2. Scroll to the bottom. Once the spinner finishes and more content is loaded, we anchor to the bottom of the page.
,
Apr 26 2016
Issue 606504 has been merged into this issue.
,
May 10 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/d91478719a0603374c12efbd79d9cd248770e7ad commit d91478719a0603374c12efbd79d9cd248770e7ad Author: ymalik <ymalik@chromium.org> Date: Tue May 10 23:43:01 2016 Always anchor to top of the anchor element Anchoring to the bottom corner results in unnecessary adjustment content is inserted above the anchor position but within the viewport. Always anchoring to the top has the problem that we wont adjust when content is inserted below the anchor position but above the viewport. The latter (not anchoring at all) is better than anchoring to the wrong position. This CL anchors to the top left for ltr and vertical-lr writing mode and top right for rtl and vertical-rl writing mode. BUG= 600891 , 594877 Review-Url: https://codereview.chromium.org/1958973004 Cr-Commit-Position: refs/heads/master@{#392774} [modify] https://crrev.com/d91478719a0603374c12efbd79d9cd248770e7ad/third_party/WebKit/Source/core/layout/ScrollAnchor.cpp [modify] https://crrev.com/d91478719a0603374c12efbd79d9cd248770e7ad/third_party/WebKit/Source/core/layout/ScrollAnchor.h [modify] https://crrev.com/d91478719a0603374c12efbd79d9cd248770e7ad/third_party/WebKit/Source/core/layout/ScrollAnchorTest.cpp
,
May 12 2016
Anchoring to only the top corner fixes this.
,
Jun 21 2016
|
|||||
►
Sign in to add a comment |
|||||
Comment 1 by ymalik@chromium.org
, Apr 8 2016