Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2776183002:
NTP: Prevent conflict of dismiss animations and scroll position handling. (Closed)
|
Created:
3 years, 9 months ago by Michael van Ouwerkerk Modified:
3 years, 8 months ago Reviewers:
Bernhard Bauer CC:
chromium-reviews, noyau+watch_chromium.org, ntp-dev+reviews_chromium.org, agrieve+watch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionNTP: Prevent conflict of dismiss animations and scroll position handling.
When an item is removed, the RecyclerView fires onScrolled once with the end value, and animates the remaining items into place via translation, without firing intermediate scroll events. Calling computeVerticalScrollOffset returns the end scroll state, not any intermediate animated values, because it's not the property being animated.
We disable all focus change animations while the NewTabPageLayout is moved by the RecyclerView, and then update afterwards.
BUG=696972
Review-Url: https://codereview.chromium.org/2776183002
Cr-Commit-Position: refs/heads/master@{#460061}
Committed: https://chromium.googlesource.com/chromium/src/+/ce4d88aebc3c49570ad75ceb0877e74c72aa4a4d
Patch Set 1 #
Total comments: 4
Patch Set 2 : No need to post a delayed task for scroll handling. #Messages
Total messages: 19 (12 generated)
The CQ bit was checked by mvanouwerkerk@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
mvanouwerkerk@chromium.org changed reviewers: + bauerb@chromium.org
Bernhard, could you take a look please?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
I'm fine with the change itself, but I still don't understand why the bug is happening :-( https://codereview.chromium.org/2776183002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java (right): https://codereview.chromium.org/2776183002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java:201: // If |mNewTabPageLayout| is animated by the RecyclerView because an item below it Stupid question, why doesn't it work to call updateSearchBoxOnScroll in here if the animated item is the NewTabPageLayout? https://codereview.chromium.org/2776183002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java:228: mUpdateSearchBoxOnScrollRunnable, SCROLL_RUNNABLE_DELAY_MS); Is there a reason to use that particular delay (which is for snap scrolling)? Would 0 work?
Description was changed from ========== NTP: Prevent conflict of dismiss animations and scroll position handling. BUG=696972 ========== to ========== NTP: Prevent conflict of dismiss animations and scroll position handling. When an item is removed, the RecyclerView fires onScrolled once with the end value, and animates the remaining items into place via translation, without firing intermediate scroll events. Calling computeVerticalScrollOffset returns the end scroll state, not any intermediate animated values, because it's not the property being animated. We disable all focus change animations while the NewTabPageLayout is moved by the RecyclerView, and then update afterwards. BUG=696972 ==========
https://codereview.chromium.org/2776183002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java (right): https://codereview.chromium.org/2776183002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java:201: // If |mNewTabPageLayout| is animated by the RecyclerView because an item below it On 2017/03/27 17:36:54, Bernhard (OOO until Mar 27) wrote: > Stupid question, why doesn't it work to call updateSearchBoxOnScroll in here if > the animated item is the NewTabPageLayout? Because this method (animateMove) is called only once for each item than needs to be moved, in order to schedule its animation. There are no intermediate updates that would allow us to respond with causing jank, and we only see the end scroll state here. https://codereview.chromium.org/2776183002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java:228: mUpdateSearchBoxOnScrollRunnable, SCROLL_RUNNABLE_DELAY_MS); On 2017/03/27 17:36:54, Bernhard (OOO until Mar 27) wrote: > Is there a reason to use that particular delay (which is for snap scrolling)? > Would 0 work? As it turns out, there's no need to use postDelayed() here, simply post() is sufficient.
Ah yes, please take another look :-)
The CQ bit was checked by mvanouwerkerk@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm
The CQ bit was unchecked by mvanouwerkerk@chromium.org
The CQ bit was checked by mvanouwerkerk@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 20001, "attempt_start_ts": 1490699086557010,
"parent_rev": "662aa65677aabd2ee107e76a6928574cd167bc75", "commit_rev":
"ce4d88aebc3c49570ad75ceb0877e74c72aa4a4d"}
Message was sent while issue was closed.
Description was changed from ========== NTP: Prevent conflict of dismiss animations and scroll position handling. When an item is removed, the RecyclerView fires onScrolled once with the end value, and animates the remaining items into place via translation, without firing intermediate scroll events. Calling computeVerticalScrollOffset returns the end scroll state, not any intermediate animated values, because it's not the property being animated. We disable all focus change animations while the NewTabPageLayout is moved by the RecyclerView, and then update afterwards. BUG=696972 ========== to ========== NTP: Prevent conflict of dismiss animations and scroll position handling. When an item is removed, the RecyclerView fires onScrolled once with the end value, and animates the remaining items into place via translation, without firing intermediate scroll events. Calling computeVerticalScrollOffset returns the end scroll state, not any intermediate animated values, because it's not the property being animated. We disable all focus change animations while the NewTabPageLayout is moved by the RecyclerView, and then update afterwards. BUG=696972 Review-Url: https://codereview.chromium.org/2776183002 Cr-Commit-Position: refs/heads/master@{#460061} Committed: https://chromium.googlesource.com/chromium/src/+/ce4d88aebc3c49570ad75ceb0877... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/chromium/src/+/ce4d88aebc3c49570ad75ceb0877... |
