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

Issue 606356 link

Starred by 3 users

Issue metadata

Status: Verified
Owner:
Closed: Oct 2016
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Android
Pri: 2
Type: Bug



Sign in to add a comment

Go back to the same place in snippets

Project Member Reported by rachelis@chromium.org, Apr 25 2016

Issue description

Hitting system back should take the user back to the same place on the NTP. Currently, they need to scroll down to view the stories again. 
 
Labels: zine-mr-untriaged

Comment 2 by fi...@chromium.org, Apr 26 2016

Cc: nepper@chromium.org
Do you think this should be part of the MVP?
Not sure. Nepper?

Comment 4 by fi...@chromium.org, Apr 26 2016

 Issue 605044  has been merged into this issue.

Comment 5 by nepper@chromium.org, Apr 26 2016

Cc: -nepper@chromium.org
Labels: -zine-mr-untriaged zine-mr-mile-MVP
Status: Available (was: Untriaged)
Yes, would be great to have, but not an MVP launch blocker.

Comment 6 by fi...@chromium.org, May 3 2016

Labels: -Pri-2 Pri-3

Comment 7 by fi...@chromium.org, May 10 2016

Blocking: 584266

Comment 8 by dgn@chromium.org, Jun 3 2016

Blocking: 617109
Labels: M-54
Blocking: -617109
Labels: zine-mr-MVP

Comment 12 by finkm@google.com, Jul 1 2016

Labels: -zine-mr-mvp
Labels: zine-mr-MVP
Labels: -zine-mr-mile-mvp -zine-mr-mvp zine-client-v1
Labels: zine-16-07-11
Owner: mvanouwe...@chromium.org
Status: Started (was: Available)
Status: Assigned (was: Started)

Comment 17 by fi...@chromium.org, Jul 29 2016

Labels: -zine-client-v1 zine-articles-v1 zine-triaged
Cc: ainslie@chromium.org
(+1 from me)
Labels: -Pri-3 Pri-2
Elevating priority to make this a cherry pick after launch blockers are done.
Cherry pick as in merge into the release branch after branch point? I think that would need to be P1 in that case.
Sorry for the ambiguity. I meant let's try to fix this before BP once our P1s are done.
Labels: -zine-articles-v1 zine-client-ux-v1
Labels: zine-ux
Labels: zine-16-08-29
Status: Started (was: Assigned)
Blocking: -584266
Labels: zine-16-09-05
Labels: -M-54 M-55
Labels: zine-16-09-12
Labels: zine-16-09-19
Project Member

Comment 29 by bugdroid1@chromium.org, Sep 21 2016

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

commit 22b18a9a6d4f421ddb6d4f74280a571bf7dca4f6
Author: mvanouwerkerk <mvanouwerkerk@chromium.org>
Date: Wed Sep 21 16:15:23 2016

Ntp: restore scroll position.

* The scroll position is stored as extra data on the NavigationEntry.
* The main use case handled is when the user clicks on a suggested
article and then back to view more suggestions. Maintaining scroll position helps maintain context and flow here.
* It is the RecyclerView Adapter position that is stored and restored, so if the device is rotated in the meantime, scroll restore will still present the same content as long as the underlying data has not changed.
* Because the underlying data is subject to change a few times per day, the scroll position is not persisted. It would be confusing the restore to an old position that now shows different content.

BUG= 606356 
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Review-Url: https://codereview.chromium.org/2327083002
Cr-Commit-Position: refs/heads/master@{#420075}

[modify] https://crrev.com/22b18a9a6d4f421ddb6d4f74280a571bf7dca4f6/chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java
[modify] https://crrev.com/22b18a9a6d4f421ddb6d4f74280a571bf7dca4f6/chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java
[modify] https://crrev.com/22b18a9a6d4f421ddb6d4f74280a571bf7dca4f6/chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageRecyclerView.java
[modify] https://crrev.com/22b18a9a6d4f421ddb6d4f74280a571bf7dca4f6/chrome/android/javatests/src/org/chromium/chrome/browser/NavigationPopupTest.java
[modify] https://crrev.com/22b18a9a6d4f421ddb6d4f74280a571bf7dca4f6/content/browser/frame_host/navigation_controller_android.cc
[modify] https://crrev.com/22b18a9a6d4f421ddb6d4f74280a571bf7dca4f6/content/browser/frame_host/navigation_controller_android.h
[modify] https://crrev.com/22b18a9a6d4f421ddb6d4f74280a571bf7dca4f6/content/public/android/java/src/org/chromium/content/browser/framehost/NavigationControllerImpl.java
[modify] https://crrev.com/22b18a9a6d4f421ddb6d4f74280a571bf7dca4f6/content/public/android/java/src/org/chromium/content_public/browser/NavigationController.java

Project Member

Comment 30 by bugdroid1@chromium.org, Sep 21 2016

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

commit 651d3cce5637e5b1875468534b66325fc9f5dc92
Author: hush <hush@chromium.org>
Date: Wed Sep 21 20:18:07 2016

Revert of Ntp: restore scroll position. (patchset #9 id:160001 of https://codereview.chromium.org/2327083002/ )

Reason for revert:
Broke org.chromium.chrome.browser.ntp.NewTabPageTest.*

Original issue's description:
> Ntp: restore scroll position.
>
> * The scroll position is stored as extra data on the NavigationEntry.
> * The main use case handled is when the user clicks on a suggested
> article and then back to view more suggestions. Maintaining scroll position helps maintain context and flow here.
> * It is the RecyclerView Adapter position that is stored and restored, so if the device is rotated in the meantime, scroll restore will still present the same content as long as the underlying data has not changed.
> * Because the underlying data is subject to change a few times per day, the scroll position is not persisted. It would be confusing the restore to an old position that now shows different content.
>
> BUG= 606356 
> CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
>
> Committed: https://crrev.com/22b18a9a6d4f421ddb6d4f74280a571bf7dca4f6
> Cr-Commit-Position: refs/heads/master@{#420075}

TBR=bauerb@chromium.org,tedchoc@chromium.org,clamy@chromium.org,mvanouwerkerk@chromium.org
# Skipping CQ checks because original CL landed less than 1 days ago.
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG= 606356 

Review-Url: https://codereview.chromium.org/2363483002
Cr-Commit-Position: refs/heads/master@{#420146}

[modify] https://crrev.com/651d3cce5637e5b1875468534b66325fc9f5dc92/chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java
[modify] https://crrev.com/651d3cce5637e5b1875468534b66325fc9f5dc92/chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java
[modify] https://crrev.com/651d3cce5637e5b1875468534b66325fc9f5dc92/chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageRecyclerView.java
[modify] https://crrev.com/651d3cce5637e5b1875468534b66325fc9f5dc92/chrome/android/javatests/src/org/chromium/chrome/browser/NavigationPopupTest.java
[modify] https://crrev.com/651d3cce5637e5b1875468534b66325fc9f5dc92/content/browser/frame_host/navigation_controller_android.cc
[modify] https://crrev.com/651d3cce5637e5b1875468534b66325fc9f5dc92/content/browser/frame_host/navigation_controller_android.h
[modify] https://crrev.com/651d3cce5637e5b1875468534b66325fc9f5dc92/content/public/android/java/src/org/chromium/content/browser/framehost/NavigationControllerImpl.java
[modify] https://crrev.com/651d3cce5637e5b1875468534b66325fc9f5dc92/content/public/android/java/src/org/chromium/content_public/browser/NavigationController.java

Project Member

Comment 31 by bugdroid1@chromium.org, Sep 21 2016

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

commit 651d3cce5637e5b1875468534b66325fc9f5dc92
Author: hush <hush@chromium.org>
Date: Wed Sep 21 20:18:07 2016

Revert of Ntp: restore scroll position. (patchset #9 id:160001 of https://codereview.chromium.org/2327083002/ )

Reason for revert:
Broke org.chromium.chrome.browser.ntp.NewTabPageTest.*

Original issue's description:
> Ntp: restore scroll position.
>
> * The scroll position is stored as extra data on the NavigationEntry.
> * The main use case handled is when the user clicks on a suggested
> article and then back to view more suggestions. Maintaining scroll position helps maintain context and flow here.
> * It is the RecyclerView Adapter position that is stored and restored, so if the device is rotated in the meantime, scroll restore will still present the same content as long as the underlying data has not changed.
> * Because the underlying data is subject to change a few times per day, the scroll position is not persisted. It would be confusing the restore to an old position that now shows different content.
>
> BUG= 606356 
> CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
>
> Committed: https://crrev.com/22b18a9a6d4f421ddb6d4f74280a571bf7dca4f6
> Cr-Commit-Position: refs/heads/master@{#420075}

TBR=bauerb@chromium.org,tedchoc@chromium.org,clamy@chromium.org,mvanouwerkerk@chromium.org
# Skipping CQ checks because original CL landed less than 1 days ago.
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG= 606356 

Review-Url: https://codereview.chromium.org/2363483002
Cr-Commit-Position: refs/heads/master@{#420146}

[modify] https://crrev.com/651d3cce5637e5b1875468534b66325fc9f5dc92/chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java
[modify] https://crrev.com/651d3cce5637e5b1875468534b66325fc9f5dc92/chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java
[modify] https://crrev.com/651d3cce5637e5b1875468534b66325fc9f5dc92/chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageRecyclerView.java
[modify] https://crrev.com/651d3cce5637e5b1875468534b66325fc9f5dc92/chrome/android/javatests/src/org/chromium/chrome/browser/NavigationPopupTest.java
[modify] https://crrev.com/651d3cce5637e5b1875468534b66325fc9f5dc92/content/browser/frame_host/navigation_controller_android.cc
[modify] https://crrev.com/651d3cce5637e5b1875468534b66325fc9f5dc92/content/browser/frame_host/navigation_controller_android.h
[modify] https://crrev.com/651d3cce5637e5b1875468534b66325fc9f5dc92/content/public/android/java/src/org/chromium/content/browser/framehost/NavigationControllerImpl.java
[modify] https://crrev.com/651d3cce5637e5b1875468534b66325fc9f5dc92/content/public/android/java/src/org/chromium/content_public/browser/NavigationController.java

Labels: zine-16-09-26
Hey Michael - how's this piece coming along? We didn't have a chance to go over it in the weekly today. Any pending questions?
My CL was reverted, but I could not reproduce the test failures locally, so I'm going to reland it.
Project Member

Comment 35 by bugdroid1@chromium.org, Sep 26 2016

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

commit 2c78ced6c8ce46eec8c9f7100b14b3414b7cbc07
Author: mvanouwerkerk <mvanouwerkerk@chromium.org>
Date: Mon Sep 26 15:53:28 2016

Reland: Ntp: restore scroll position.

* The scroll position is stored as extra data on the NavigationEntry.
* The main use case handled is when the user clicks on a suggested
article and then back to view more suggestions. Maintaining scroll position helps maintain context and flow here.
* It is the RecyclerView Adapter position that is stored and restored, so if the device is rotated in the meantime, scroll restore will still present the same content as long as the underlying data has not changed.
* Because the underlying data is subject to change a few times per day, the scroll position is not persisted. It would be confusing the restore to an old position that now shows different content.

BUG= 606356 
TBR=clamy,bauerb,tedchoc
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Committed: https://crrev.com/22b18a9a6d4f421ddb6d4f74280a571bf7dca4f6
Cr-Commit-Position: refs/heads/master@{#420075}

patch from issue 2327083002 at patchset 160001 (http://crrev.com/2327083002#ps160001)

Review-Url: https://codereview.chromium.org/2365313002
Cr-Commit-Position: refs/heads/master@{#420896}

[modify] https://crrev.com/2c78ced6c8ce46eec8c9f7100b14b3414b7cbc07/chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java
[modify] https://crrev.com/2c78ced6c8ce46eec8c9f7100b14b3414b7cbc07/chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java
[modify] https://crrev.com/2c78ced6c8ce46eec8c9f7100b14b3414b7cbc07/chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageRecyclerView.java
[modify] https://crrev.com/2c78ced6c8ce46eec8c9f7100b14b3414b7cbc07/chrome/android/javatests/src/org/chromium/chrome/browser/NavigationPopupTest.java
[modify] https://crrev.com/2c78ced6c8ce46eec8c9f7100b14b3414b7cbc07/content/browser/frame_host/navigation_controller_android.cc
[modify] https://crrev.com/2c78ced6c8ce46eec8c9f7100b14b3414b7cbc07/content/browser/frame_host/navigation_controller_android.h
[modify] https://crrev.com/2c78ced6c8ce46eec8c9f7100b14b3414b7cbc07/content/public/android/java/src/org/chromium/content/browser/framehost/NavigationControllerImpl.java
[modify] https://crrev.com/2c78ced6c8ce46eec8c9f7100b14b3414b7cbc07/content/public/android/java/src/org/chromium/content_public/browser/NavigationController.java

Project Member

Comment 36 by bugdroid1@chromium.org, Sep 26 2016

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

commit 14b97a44b845781fed2b493e948fd7d6509586df
Author: khushalsagar <khushalsagar@chromium.org>
Date: Mon Sep 26 19:35:07 2016

Revert of Reland: Ntp: restore scroll position. (patchset #2 id:20001 of https://codereview.chromium.org/2365313002/ )

Reason for revert:
Breaks AndroidTests(Dbg: https://build.chromium.org/p/chromium.linux/builders/Android%20Tests%20%28dbg%29/builds/36381

Original issue's description:
> Reland: Ntp: restore scroll position.
>
> * The scroll position is stored as extra data on the NavigationEntry.
> * The main use case handled is when the user clicks on a suggested
> article and then back to view more suggestions. Maintaining scroll position helps maintain context and flow here.
> * It is the RecyclerView Adapter position that is stored and restored, so if the device is rotated in the meantime, scroll restore will still present the same content as long as the underlying data has not changed.
> * Because the underlying data is subject to change a few times per day, the scroll position is not persisted. It would be confusing the restore to an old position that now shows different content.
>
> BUG= 606356 
> TBR=clamy,bauerb,tedchoc
> CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
>
> Committed: https://crrev.com/22b18a9a6d4f421ddb6d4f74280a571bf7dca4f6
> Cr-Commit-Position: refs/heads/master@{#420075}
>
> patch from issue 2327083002 at patchset 160001 (http://crrev.com/2327083002#ps160001)
>
> Committed: https://crrev.com/2c78ced6c8ce46eec8c9f7100b14b3414b7cbc07
> Cr-Commit-Position: refs/heads/master@{#420896}

TBR=tedchoc@chromium.org,clamy@chromium.org,bauerb@chromium.org,mvanouwerkerk@chromium.org
# Skipping CQ checks because original CL landed less than 1 days ago.
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG= 606356 

Review-Url: https://codereview.chromium.org/2370023002
Cr-Commit-Position: refs/heads/master@{#420963}

[modify] https://crrev.com/14b97a44b845781fed2b493e948fd7d6509586df/chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java
[modify] https://crrev.com/14b97a44b845781fed2b493e948fd7d6509586df/chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java
[modify] https://crrev.com/14b97a44b845781fed2b493e948fd7d6509586df/chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageRecyclerView.java
[modify] https://crrev.com/14b97a44b845781fed2b493e948fd7d6509586df/chrome/android/javatests/src/org/chromium/chrome/browser/NavigationPopupTest.java
[modify] https://crrev.com/14b97a44b845781fed2b493e948fd7d6509586df/content/browser/frame_host/navigation_controller_android.cc
[modify] https://crrev.com/14b97a44b845781fed2b493e948fd7d6509586df/content/browser/frame_host/navigation_controller_android.h
[modify] https://crrev.com/14b97a44b845781fed2b493e948fd7d6509586df/content/public/android/java/src/org/chromium/content/browser/framehost/NavigationControllerImpl.java
[modify] https://crrev.com/14b97a44b845781fed2b493e948fd7d6509586df/content/public/android/java/src/org/chromium/content_public/browser/NavigationController.java

Project Member

Comment 37 by bugdroid1@chromium.org, Sep 27 2016

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

commit 5b1519e7304f1a6d2f1c3e50c2d8a933e21f2806
Author: mvanouwerkerk <mvanouwerkerk@chromium.org>
Date: Tue Sep 27 15:47:46 2016

Reland: Ntp: restore scroll position.

* The scroll position is stored as extra data on the NavigationEntry.
* The main use case handled is when the user clicks on a suggested
article and then back to view more suggestions. Maintaining scroll position helps maintain context and flow here.
* It is the RecyclerView Adapter position that is stored and restored, so if the device is rotated in the meantime, scroll restore will still present the same content as long as the underlying data has not changed.
* Because the underlying data is subject to change a few times per day, the scroll position is not persisted. It would be confusing the restore to an old position that now shows different content.

BUG= 606356 
TBR=clamy,bauerb,tedchoc
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Committed: https://crrev.com/22b18a9a6d4f421ddb6d4f74280a571bf7dca4f6
Cr-Commit-Position: refs/heads/master@{#420075}

patch from issue 2327083002 at patchset 160001 (http://crrev.com/2327083002#ps160001)

Committed: https://crrev.com/2c78ced6c8ce46eec8c9f7100b14b3414b7cbc07
Review-Url: https://codereview.chromium.org/2365313002
Cr-Original-Commit-Position: refs/heads/master@{#420896}
Cr-Commit-Position: refs/heads/master@{#421223}

[modify] https://crrev.com/5b1519e7304f1a6d2f1c3e50c2d8a933e21f2806/chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java
[modify] https://crrev.com/5b1519e7304f1a6d2f1c3e50c2d8a933e21f2806/chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java
[modify] https://crrev.com/5b1519e7304f1a6d2f1c3e50c2d8a933e21f2806/chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageRecyclerView.java
[modify] https://crrev.com/5b1519e7304f1a6d2f1c3e50c2d8a933e21f2806/chrome/android/javatests/src/org/chromium/chrome/browser/NavigationPopupTest.java
[modify] https://crrev.com/5b1519e7304f1a6d2f1c3e50c2d8a933e21f2806/content/browser/frame_host/navigation_controller_android.cc
[modify] https://crrev.com/5b1519e7304f1a6d2f1c3e50c2d8a933e21f2806/content/browser/frame_host/navigation_controller_android.h
[modify] https://crrev.com/5b1519e7304f1a6d2f1c3e50c2d8a933e21f2806/content/public/android/java/src/org/chromium/content/browser/framehost/NavigationControllerImpl.java
[modify] https://crrev.com/5b1519e7304f1a6d2f1c3e50c2d8a933e21f2806/content/public/android/java/src/org/chromium/content_public/browser/NavigationController.java

Labels: -zine-ux
Removing the Zine UX tag since it looks like this doesn't currently need my input.
Labels: zine-16-10-03 zine-ux
Rachel: please review the current behavior, it is present in the Dev channel. If this is good enough for now, let's close it as Fixed. For later refinements we can open a new bug.
Thanks! This LGTM.
Labels: -zine-ux
Status: Fixed (was: Started)
Status: Verified (was: Fixed)
Verified on latest builds
Labels: zine-ux

Sign in to add a comment