New issue
Advanced search Search tips

Issue 821191 link

Starred by 3 users

Issue metadata

Status: Assigned
Owner:
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Android
Pri: 2
Type: Bug-Regression



Sign in to add a comment

Scroll offsets jump after navigation

Project Member Reported by pdr@chromium.org, Mar 12 2018

Issue description

Chrome Version: 67.0.3367.0/Canary
OS: Android

1) Go to https://www.bogleheads.org
2) Click on a post that has a few comments (so you can scroll).
3) Scroll halfway down the page.
4) Press back, you should now be on https://www.bogleheads.org again.
5) Use the dots menu and navigate forward, you should now be scrolled halfway down a post.
6) Scroll up and down slowly, notice the scroll offset jumps to 0,0.

Test team, could you bisect this please?
 
Cc: pnangunoori@chromium.org
Labels: -Needs-Bisect hasbisect-per-revision M-67 Target-67 FoundIn-67 M-65 M-66 RegressedIn-65 Needs-triage-Mobile FoundIn-66 Triaged-Mobile Target-65 FoundIn-65 Target-66
Owner: szager@chromium.org
Status: Assigned (was: Untriaged)
Tested the issue in Android and able to reproduce the issue. 

Steps Followed:
1. Launch Chrome.
2. Navigate to any URL which is long. Eg.: https://www.bogleheads.org
3. Tap on a post that has a few comments
4. Scroll halfway down the page.
5. Tap on device back button, you should now be on https://www.bogleheads.org again.
6. Tap on Chrome menu and tap on "Forward" button. Observed that user is scrolled halfway down a post.
7. Scroll up and down slowly.
8. Observed that user is pointed at the top of the page. (Notice the scroll offset jumps to 0,0.)

Chrome versions tested:
65.0.3325.109(Stable), 67.0.3368.0(Canary)

OS:
Android 8.1.0

Android Devices:
Pixel

Using the per-revision bisect providing the bisect results,
Good Build - 65.0.3284.0 (521227)
Bad Build - 65.0.3286.0 (521956)

You are looking for a change made after 521606(GOOD), but before 521607(BAD).

CHANGELOG URL:
The script might not always return single CL as suspect as some perf builds might get missing due to failure.
https://chromium.googlesource.com/chromium/src/+/5577210e596c6e879e12a5d67aff967a0951f53c

From the CL above, assigning the issue to the owner concerned.

@szager:  Could you please look into the issue, pardon me if it has nothing to do with your changes and if possible please assign it to owner concerned.

Please navigate to below link for log's and video--
go/chrome-androidlogs/821191

Note: 
1. This issue is not observed in Desktop.
2. Issue is not observed in FireFox mobile version.

Thanks!

Comment 3 by szager@chromium.org, Mar 15 2018

If it did, that's awesome, but I would be surprised.  I think the bug is related to when/how GetFrame().Loader().RestoreScrollPositionAndViewState() gets called, which changed in my suspect CL:

https://chromium.googlesource.com/chromium/src/+/bc3e6fa5756624cf270d4c7667d6b58a10d3fc3d%5E%21/#F2
Cc: ligim...@chromium.org
Labels: -M-67 ReleaseBlock-Stable
The patch mentioned in #2 landed in 67.0.3372.0, Praphulla could you please test in latest canary and update the thread.

If the bug still exist, it would be great to have a fix in M66 since its a recent regression. Tagging as RBS for tracking purpose.
Tested on Pixel Android 8.1 using the latest Chrome Canary #67.0.3374.0 and was not able to reproduce the issue.

Screen cast is provided here: go/chrome-androidlogs/821191
File Name - 821191-67.0.3374.0.mp4  

Thanks!
Is it confirmed that https://chromium.googlesource.com/chromium/src/+/bc3e6fa5756624cf270d4c7667d6b58a10d3fc3d is indeed what fixed the issue?

Comment 8 by cmasso@google.com, Mar 23 2018

Additional ping!
Ping - Stefan? I can still reproduce this bug.
Labels: -Pri-2 Pri-1
I'm only able to reproduce this on chrishtr@'s old phone; I can't reproduce it on my Nexus 5X.

I don't have root cause yet, and it's slow going, so I don't have an ETA.

Comment 12 by pdr@chromium.org, Mar 28 2018

I'm not able to reproduce this any more. Can you check the version of Chrome on chrishtr's phone?
I reproduced it on chrishtr's phone using a custom chromium build.  Using the identical build, I could not reproduce on my nexus 5x.  Maybe it's a race condition that affects older/slower phones more.
Just a note that this bug doesn't rely on navigating back and then forwards.  Just navigating to a page and scrolling down will cause the jump.  Appears to be related to hiding the URL bar.
Project Member

Comment 15 by bugdroid1@chromium.org, Mar 30 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/d71dca6d566f55004c7af6a4f4b18651287932ab

commit d71dca6d566f55004c7af6a4f4b18651287932ab
Author: Stefan Zager <szager@chromium.org>
Date: Fri Mar 30 20:27:54 2018

Fix frame scroll anchoring on resize.

ResizeViewportAnchor::ResizeFrameView is sometimes called when there is
a live ResizeScope; and sometimes called when there isn't.  When the
latter happens, drift_ may become non-zero; in particular,
ResizeFrameView calls into LocalFrameView::SetFrameRect, which calls
GetFrame().Loader().RestoreScrollPositionAndViewState(), which may
change the RootFrameViewport's scroll offset.

That doesn't cause any immediate side effect, but the next time a
ResizeScope goes into and then out of scope, the lingering non-zero
drift_ will be applied, causing a sudden jump in the scroll offset.
Typically, this happens when browser controls are shown or hidden.

drift_ only makes sense in the context of a live ResizeScope, so the
appropriate fix is to not modify it if there is no live ResizeScope.

I haven't yet figured out how to reproduce this sequence in a test,
but as this bug is a release blocker, I think it's worth landing
as-is.

BUG=821191

Change-Id: Id8ff537faa5ba9e77b16b7a83cef8de70de7e412
Reviewed-on: https://chromium-review.googlesource.com/987386
Reviewed-by: Chris Harrelson <chrishtr@chromium.org>
Commit-Queue: Stefan Zager <szager@chromium.org>
Cr-Commit-Position: refs/heads/master@{#547258}
[modify] https://crrev.com/d71dca6d566f55004c7af6a4f4b18651287932ab/third_party/WebKit/Source/core/frame/ResizeViewportAnchor.cpp

Labels: Merge-Request-66
Project Member

Comment 17 by sheriffbot@chromium.org, Mar 30 2018

Labels: -Merge-Request-66 Merge-Review-66 Hotlist-Merge-Review
This bug requires manual review: M66 has already been promoted to the beta branch, so this requires manual review
Please contact the milestone owner if you have questions.
Owners: cmasso@(Android), cmasso@(iOS), josafat@(ChromeOS), abdulsyed@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Owner: chrishtr@chromium.org

Comment 19 by cmasso@google.com, Apr 2 2018

Please verify in canary and update 
Owner: pdr@chromium.org
Philip could you verify on canary and take care of the M66 merge? Stefan is out this
week, thanks.

Comment 21 by cmasso@google.com, Apr 3 2018

Ping! M66 stable is nearing

Comment 22 by pdr@chromium.org, Apr 3 2018

Labels: Merge-Request-66
I verified this is fixed in canary and would like to merge.
Project Member

Comment 23 by sheriffbot@chromium.org, Apr 3 2018

Labels: -Merge-Request-66
This bug requires manual review: We are only 13 days from stable.
Please contact the milestone owner if you have questions.
Owners: cmasso@(Android), cmasso@(iOS), josafat@(ChromeOS), abdulsyed@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

Comment 24 by cmasso@google.com, Apr 4 2018

Labels: -Hotlist-Merge-Review -Merge-Review-66 Merge-Approved-66
Project Member

Comment 25 by bugdroid1@chromium.org, Apr 4 2018

Labels: -merge-approved-66 merge-merged-3359
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/8cab1e475e9c717c5f31947bd59fa601acafc1fa

commit 8cab1e475e9c717c5f31947bd59fa601acafc1fa
Author: Stefan Zager <szager@chromium.org>
Date: Wed Apr 04 19:44:43 2018

Fix frame scroll anchoring on resize.

ResizeViewportAnchor::ResizeFrameView is sometimes called when there is
a live ResizeScope; and sometimes called when there isn't.  When the
latter happens, drift_ may become non-zero; in particular,
ResizeFrameView calls into LocalFrameView::SetFrameRect, which calls
GetFrame().Loader().RestoreScrollPositionAndViewState(), which may
change the RootFrameViewport's scroll offset.

That doesn't cause any immediate side effect, but the next time a
ResizeScope goes into and then out of scope, the lingering non-zero
drift_ will be applied, causing a sudden jump in the scroll offset.
Typically, this happens when browser controls are shown or hidden.

drift_ only makes sense in the context of a live ResizeScope, so the
appropriate fix is to not modify it if there is no live ResizeScope.

I haven't yet figured out how to reproduce this sequence in a test,
but as this bug is a release blocker, I think it's worth landing
as-is.

BUG=821191

Change-Id: Id8ff537faa5ba9e77b16b7a83cef8de70de7e412
Reviewed-on: https://chromium-review.googlesource.com/987386
Reviewed-by: Chris Harrelson <chrishtr@chromium.org>
Commit-Queue: Stefan Zager <szager@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#547258}(cherry picked from commit d71dca6d566f55004c7af6a4f4b18651287932ab)
Reviewed-on: https://chromium-review.googlesource.com/996293
Reviewed-by: Philip Rogers <pdr@chromium.org>
Cr-Commit-Position: refs/branch-heads/3359@{#579}
Cr-Branched-From: 66afc5e5d10127546cc4b98b9117aff588b5e66b-refs/heads/master@{#540276}
[modify] https://crrev.com/8cab1e475e9c717c5f31947bd59fa601acafc1fa/third_party/WebKit/Source/core/frame/ResizeViewportAnchor.cpp

Comment 26 by pdr@chromium.org, Apr 4 2018

Status: Fixed (was: Assigned)
Project Member

Comment 27 by bugdroid1@chromium.org, Apr 4 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/8cab1e475e9c717c5f31947bd59fa601acafc1fa

commit 8cab1e475e9c717c5f31947bd59fa601acafc1fa
Author: Stefan Zager <szager@chromium.org>
Date: Wed Apr 04 19:44:43 2018

Fix frame scroll anchoring on resize.

ResizeViewportAnchor::ResizeFrameView is sometimes called when there is
a live ResizeScope; and sometimes called when there isn't.  When the
latter happens, drift_ may become non-zero; in particular,
ResizeFrameView calls into LocalFrameView::SetFrameRect, which calls
GetFrame().Loader().RestoreScrollPositionAndViewState(), which may
change the RootFrameViewport's scroll offset.

That doesn't cause any immediate side effect, but the next time a
ResizeScope goes into and then out of scope, the lingering non-zero
drift_ will be applied, causing a sudden jump in the scroll offset.
Typically, this happens when browser controls are shown or hidden.

drift_ only makes sense in the context of a live ResizeScope, so the
appropriate fix is to not modify it if there is no live ResizeScope.

I haven't yet figured out how to reproduce this sequence in a test,
but as this bug is a release blocker, I think it's worth landing
as-is.

BUG=821191

Change-Id: Id8ff537faa5ba9e77b16b7a83cef8de70de7e412
Reviewed-on: https://chromium-review.googlesource.com/987386
Reviewed-by: Chris Harrelson <chrishtr@chromium.org>
Commit-Queue: Stefan Zager <szager@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#547258}(cherry picked from commit d71dca6d566f55004c7af6a4f4b18651287932ab)
Reviewed-on: https://chromium-review.googlesource.com/996293
Reviewed-by: Philip Rogers <pdr@chromium.org>
Cr-Commit-Position: refs/branch-heads/3359@{#579}
Cr-Branched-From: 66afc5e5d10127546cc4b98b9117aff588b5e66b-refs/heads/master@{#540276}
[modify] https://crrev.com/8cab1e475e9c717c5f31947bd59fa601acafc1fa/third_party/WebKit/Source/core/frame/ResizeViewportAnchor.cpp

Labels: -ReleaseBlock-Stable
Owner: szager@chromium.org
Status: Assigned (was: Fixed)
Still needs a test.
Labels: -Pri-1 Pri-2

Sign in to add a comment