New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 600891 link

Starred by 3 users

Issue metadata

Status: Fixed
Owner:
(currently inactive on Chromium)
Closed: May 2016
Cc:
EstimatedDays: ----
NextAction: ----
OS: Android
Pri: 3
Type: Bug

Blocking:
issue 558575



Sign in to add a comment

techcrunch and scroll anchoring don't play nicely

Project Member Reported by ojan@chromium.org, Apr 6 2016

Issue description

1. 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.
 
Status: Started (was: Available)
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?

Comment 3 by ojan@chromium.org, 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.

Cc: -ymalik@chromium.org
Owner: ymalik@chromium.org

Comment 5 by ymalik@chromium.org, 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).  
 

Comment 6 by ojan@chromium.org, 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.

Comment 7 by ojan@chromium.org, 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.

Comment 8 by ymalik@chromium.org, Apr 26 2016

Cc: ymalik@chromium.org kenjibaheux@chromium.org ojan@chromium.org
 Issue 606504  has been merged into this issue.
Project Member

Comment 9 by bugdroid1@chromium.org, 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

Status: Fixed (was: Started)
Anchoring to only the top corner fixes this.
Labels: Hotlist-Input-Dev

Sign in to add a comment