Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1904023004:
[Android] Ignore scroll position when accommodating omnibox focus on the cards UI NTP (Closed)
|
Created:
4 years, 8 months ago by Bernhard Bauer Modified:
4 years, 8 months ago Reviewers:
PEConn CC:
chromium-reviews, mcwilliams+watch_chromium.org, dgn+watch_chromium.org, May Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[Android] Ignore scroll position when accommodating omnibox focus on the cards UI NTP
When the omnibox is focused on the New Tab Page, this effectively positions the
above-the-fold tiles where they would appear if it was scrolled all the way to
the top, even when it's scrolled further down.
Because of snap scrolling in the cards UI, the only steady state where tiles are
visible is scrolled all the way to the top, so in that position we match the
previous behavior, and if they are scrolled out of sight, they will now stay
hidden, which is the desired result.
Also, make |mSearchBoxView| a local variable, as it's only used during
initialization.
BUG=604371
Committed: https://crrev.com/fafb2f03ab60e7d762c22e08faa7906ec83ba88a
Cr-Commit-Position: refs/heads/master@{#389255}
Patch Set 1 #
Total comments: 2
Patch Set 2 : review #Patch Set 3 : comment #Messages
Total messages: 16 (8 generated)
Description was changed from ========== [Android][NTP] Ignore scroll position when setting vertical offset for the omnibox. This effectively positions the above-the-fold tiles where they would appear if the omnibox was focused while the New Tab Page was scrolled all the way to the top, even when it's scrolled further down. Because of snap scrolling, the only steady state where tiles are visible is scrolled all the way to the top, so in that position we match the previous behavior, and if they are scrolled out of sight, they will now stay hidden, which is the desired result. Also, make |mSearchBoxView| a local variable, as it's only used during initialization. BUG=604371 ========== to ========== [Android] Ignore scroll position when accommodating omnibox focus on the NTP This effectively positions the above-the-fold tiles where they would appear if the omnibox was focused while the New Tab Page was scrolled all the way to the top, even when it's scrolled further down. Because of snap scrolling, the only steady state where tiles are visible is scrolled all the way to the top, so in that position we match the previous behavior, and if they are scrolled out of sight, they will now stay hidden, which is the desired result. Also, make |mSearchBoxView| a local variable, as it's only used during initialization. BUG=604371 ==========
Description was changed from ========== [Android] Ignore scroll position when accommodating omnibox focus on the NTP This effectively positions the above-the-fold tiles where they would appear if the omnibox was focused while the New Tab Page was scrolled all the way to the top, even when it's scrolled further down. Because of snap scrolling, the only steady state where tiles are visible is scrolled all the way to the top, so in that position we match the previous behavior, and if they are scrolled out of sight, they will now stay hidden, which is the desired result. Also, make |mSearchBoxView| a local variable, as it's only used during initialization. BUG=604371 ========== to ========== [Android] Ignore scroll position when accommodating omnibox focus on the NTP When the omnibox is focused on the New Tab Page, this effectively positions the above-the-fold tiles where they would appear if it was scrolled all the way to the top, even when it's scrolled further down. Because of snap scrolling, the only steady state where tiles are visible is scrolled all the way to the top, so in that position we match the previous behavior, and if they are scrolled out of sight, they will now stay hidden, which is the desired result. Also, make |mSearchBoxView| a local variable, as it's only used during initialization. BUG=604371 ==========
bauerb@chromium.org changed reviewers: + peconn@chromium.org
Please review.
https://codereview.chromium.org/1904023004/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java (right): https://codereview.chromium.org/1904023004/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java:670: private void setUrlFocusChangeAnimationPercentInternal(float percent) { So this function is also used with the original scroll view, so do check that this doesn't cause the NTP to act funny when you don't have snippets enabled. Potentially you could just check to see if Most Visited is visible (maybe mRecyclerView.isFirstItemVisible() would work).
Description was changed from ========== [Android] Ignore scroll position when accommodating omnibox focus on the NTP When the omnibox is focused on the New Tab Page, this effectively positions the above-the-fold tiles where they would appear if it was scrolled all the way to the top, even when it's scrolled further down. Because of snap scrolling, the only steady state where tiles are visible is scrolled all the way to the top, so in that position we match the previous behavior, and if they are scrolled out of sight, they will now stay hidden, which is the desired result. Also, make |mSearchBoxView| a local variable, as it's only used during initialization. BUG=604371 ========== to ========== [Android] Ignore scroll position when accommodating omnibox focus on the cards UI NTP When the omnibox is focused on the New Tab Page, this effectively positions the above-the-fold tiles where they would appear if it was scrolled all the way to the top, even when it's scrolled further down. Because of snap scrolling in the cards UI, the only steady state where tiles are visible is scrolled all the way to the top, so in that position we match the previous behavior, and if they are scrolled out of sight, they will now stay hidden, which is the desired result. Also, make |mSearchBoxView| a local variable, as it's only used during initialization. BUG=604371 ==========
https://codereview.chromium.org/1904023004/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java (right): https://codereview.chromium.org/1904023004/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java:670: private void setUrlFocusChangeAnimationPercentInternal(float percent) { On 2016/04/21 16:45:00, PEConn1 wrote: > So this function is also used with the original scroll view, so do check that > this doesn't cause the NTP to act funny when you don't have snippets enabled. > > Potentially you could just check to see if Most Visited is visible (maybe > mRecyclerView.isFirstItemVisible() would work). I ended up checking for |mUseCardsUi|, to avoid a discontinuity when Most Likely is hidden. PTAL?
On 2016/04/22 12:42:31, Bernhard Bauer wrote: > https://codereview.chromium.org/1904023004/diff/1/chrome/android/java/src/org... > File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java > (right): > > https://codereview.chromium.org/1904023004/diff/1/chrome/android/java/src/org... > chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java:670: > private void setUrlFocusChangeAnimationPercentInternal(float percent) { > On 2016/04/21 16:45:00, PEConn1 wrote: > > So this function is also used with the original scroll view, so do check that > > this doesn't cause the NTP to act funny when you don't have snippets enabled. > > > > Potentially you could just check to see if Most Visited is visible (maybe > > mRecyclerView.isFirstItemVisible() would work). > > I ended up checking for |mUseCardsUi|, to avoid a discontinuity when Most Likely > is hidden. PTAL? LGTM, though I'd add a comment explaining the behaviour - with snippets enabled, getScrollY() will be 0 when above the fold, letting the transition work properly and when below the fold, the resulting Y translation will be negative enough to ensure Most Likely is off the screen. Since the result we want (which isn't explicit) is Most Likely working when above the fold, and for there to be no apparent transition when below the fold.
On 2016/04/22 14:32:34, PEConn1 wrote: > On 2016/04/22 12:42:31, Bernhard Bauer wrote: > > > https://codereview.chromium.org/1904023004/diff/1/chrome/android/java/src/org... > > File > chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java > > (right): > > > > > https://codereview.chromium.org/1904023004/diff/1/chrome/android/java/src/org... > > > chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java:670: > > private void setUrlFocusChangeAnimationPercentInternal(float percent) { > > On 2016/04/21 16:45:00, PEConn1 wrote: > > > So this function is also used with the original scroll view, so do check > that > > > this doesn't cause the NTP to act funny when you don't have snippets > enabled. > > > > > > Potentially you could just check to see if Most Visited is visible (maybe > > > mRecyclerView.isFirstItemVisible() would work). > > > > I ended up checking for |mUseCardsUi|, to avoid a discontinuity when Most > Likely > > is hidden. PTAL? > > LGTM, though I'd add a comment explaining the behaviour - with snippets enabled, > getScrollY() will be 0 when above the fold, letting the transition work properly > and when below the fold, the resulting Y translation will be negative enough to > ensure Most Likely is off the screen. > > Since the result we want (which isn't explicit) is Most Likely working when > above the fold, and for there to be no apparent transition when below the fold. Done.
The CQ bit was checked by bauerb@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from peconn@chromium.org Link to the patchset: https://codereview.chromium.org/1904023004/#ps40001 (title: "comment")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1904023004/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1904023004/40001
Message was sent while issue was closed.
Description was changed from ========== [Android] Ignore scroll position when accommodating omnibox focus on the cards UI NTP When the omnibox is focused on the New Tab Page, this effectively positions the above-the-fold tiles where they would appear if it was scrolled all the way to the top, even when it's scrolled further down. Because of snap scrolling in the cards UI, the only steady state where tiles are visible is scrolled all the way to the top, so in that position we match the previous behavior, and if they are scrolled out of sight, they will now stay hidden, which is the desired result. Also, make |mSearchBoxView| a local variable, as it's only used during initialization. BUG=604371 ========== to ========== [Android] Ignore scroll position when accommodating omnibox focus on the cards UI NTP When the omnibox is focused on the New Tab Page, this effectively positions the above-the-fold tiles where they would appear if it was scrolled all the way to the top, even when it's scrolled further down. Because of snap scrolling in the cards UI, the only steady state where tiles are visible is scrolled all the way to the top, so in that position we match the previous behavior, and if they are scrolled out of sight, they will now stay hidden, which is the desired result. Also, make |mSearchBoxView| a local variable, as it's only used during initialization. BUG=604371 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== [Android] Ignore scroll position when accommodating omnibox focus on the cards UI NTP When the omnibox is focused on the New Tab Page, this effectively positions the above-the-fold tiles where they would appear if it was scrolled all the way to the top, even when it's scrolled further down. Because of snap scrolling in the cards UI, the only steady state where tiles are visible is scrolled all the way to the top, so in that position we match the previous behavior, and if they are scrolled out of sight, they will now stay hidden, which is the desired result. Also, make |mSearchBoxView| a local variable, as it's only used during initialization. BUG=604371 ========== to ========== [Android] Ignore scroll position when accommodating omnibox focus on the cards UI NTP When the omnibox is focused on the New Tab Page, this effectively positions the above-the-fold tiles where they would appear if it was scrolled all the way to the top, even when it's scrolled further down. Because of snap scrolling in the cards UI, the only steady state where tiles are visible is scrolled all the way to the top, so in that position we match the previous behavior, and if they are scrolled out of sight, they will now stay hidden, which is the desired result. Also, make |mSearchBoxView| a local variable, as it's only used during initialization. BUG=604371 Committed: https://crrev.com/fafb2f03ab60e7d762c22e08faa7906ec83ba88a Cr-Commit-Position: refs/heads/master@{#389255} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/fafb2f03ab60e7d762c22e08faa7906ec83ba88a Cr-Commit-Position: refs/heads/master@{#389255} |
