Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2816923002:
change overlay scrollbar hover show to hover fade in (Closed)
Descriptionchange overlay scrollbar hover show to hover fade in
Currently, we have 2 ways to show the hidden scrollbars:
1. scroll position update
2. move mouse to a region near edge
In this patch, we only change the "move mouse to a region near edge" to
show scrollbars immediately to fade in scrollbars.
BUG=710543
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
Review-Url: https://codereview.chromium.org/2816923002
Cr-Commit-Position: refs/heads/master@{#466740}
Committed: https://chromium.googlesource.com/chromium/src/+/a47e7158e746c01b4ccbbd25d71e267d93347b94
Patch Set 1 #
Total comments: 10
Patch Set 2 : fix LayerTreeHostImplTest.ScrollbarVisibilityChangeCausesRedrawAndCommit #Patch Set 3 : bokan comment addressed #
Total comments: 4
Patch Set 4 : bokan comment addressed #
Total comments: 1
Patch Set 5 : merge fade-in and fade-out to same code method #
Total comments: 3
Patch Set 6 : merge FadeInDuration and FadeOutDuration to FadeDuration #
Total comments: 3
Patch Set 7 : merge kOverlayScrollbarFadeInDelay and kOverlayScrollbarFadeOutDelay to kOverlayScrollbarFadeDelay #
Total comments: 3
Patch Set 8 : rebase update #Patch Set 9 : fix for android #Patch Set 10 : combine fade_in_delay and fade_out_delay to fade_delay #Dependent Patchsets: Messages
Total messages: 69 (38 generated)
Description was changed from ========== change overlay scrollbar hover show to hover fade in BUG=710543 ========== to ========== change overlay scrollbar hover show to hover fade in BUG=710543 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ==========
Description was changed from ========== change overlay scrollbar hover show to hover fade in BUG=710543 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ========== to ========== change overlay scrollbar hover show to hover fade in BUG=710543 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ==========
chaopeng@chromium.org changed reviewers: + bokan@chromium.org
PTAL. Thank you.
The CQ bit was checked by chaopeng@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Patchset #2 (id:20001) has been deleted
The CQ bit was checked by chaopeng@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...
https://codereview.chromium.org/2816923002/diff/1/cc/input/scrollbar_animatio... File cc/input/scrollbar_animation_controller.cc (left): https://codereview.chromium.org/2816923002/diff/1/cc/input/scrollbar_animatio... cc/input/scrollbar_animation_controller.cc:362: delayed_scrollbar_show_.Cancel(); Why don't we want to cancel this now? If we scroll while the mouse is nearby, this will cause the scrollbars to show instantly - at that point, we don't need the "fade in due to hover" animation anymore, right? https://codereview.chromium.org/2816923002/diff/1/cc/input/scrollbar_animatio... File cc/input/scrollbar_animation_controller.cc (right): https://codereview.chromium.org/2816923002/diff/1/cc/input/scrollbar_animatio... cc/input/scrollbar_animation_controller.cc:134: delayed_scrollbar_show_.Reset( Rename delayed_scrollbar_show too. https://codereview.chromium.org/2816923002/diff/1/cc/input/scrollbar_animatio... cc/input/scrollbar_animation_controller.cc:190: } else { DCHECK above that animation_change_ isn't NONE https://codereview.chromium.org/2816923002/diff/1/cc/input/scrollbar_animatio... File cc/input/scrollbar_animation_controller_unittest.cc (right): https://codereview.chromium.org/2816923002/diff/1/cc/input/scrollbar_animatio... cc/input/scrollbar_animation_controller_unittest.cc:1031: time += kFadeInDuration; Please add a step in here before the animation finishes to ensure we're actually animating the fade in. https://codereview.chromium.org/2816923002/diff/1/cc/trees/layer_tree_host_im... File cc/trees/layer_tree_host_impl_unittest.cc (right): https://codereview.chromium.org/2816923002/diff/1/cc/trees/layer_tree_host_im... cc/trees/layer_tree_host_impl_unittest.cc:3248: // requested a redraw and a commit. This assumes the fade in is instant, you'll need to fix it to move time forward a bit.
Updated. PTAL. Thank you. https://codereview.chromium.org/2816923002/diff/1/cc/input/scrollbar_animatio... File cc/input/scrollbar_animation_controller.cc (left): https://codereview.chromium.org/2816923002/diff/1/cc/input/scrollbar_animatio... cc/input/scrollbar_animation_controller.cc:362: delayed_scrollbar_show_.Cancel(); On 2017/04/13 14:38:14, bokan wrote: > Why don't we want to cancel this now? If we scroll while the mouse is nearby, > this will cause the scrollbars to show instantly - at that point, we don't need > the "fade in due to hover" animation anymore, right? Yes, we should keep this. https://codereview.chromium.org/2816923002/diff/1/cc/input/scrollbar_animatio... File cc/input/scrollbar_animation_controller.cc (right): https://codereview.chromium.org/2816923002/diff/1/cc/input/scrollbar_animatio... cc/input/scrollbar_animation_controller.cc:134: delayed_scrollbar_show_.Reset( On 2017/04/13 14:38:14, bokan wrote: > Rename delayed_scrollbar_show too. Done. https://codereview.chromium.org/2816923002/diff/1/cc/input/scrollbar_animatio... cc/input/scrollbar_animation_controller.cc:190: } else { On 2017/04/13 14:38:14, bokan wrote: > DCHECK above that animation_change_ isn't NONE Done. https://codereview.chromium.org/2816923002/diff/1/cc/input/scrollbar_animatio... File cc/input/scrollbar_animation_controller_unittest.cc (right): https://codereview.chromium.org/2816923002/diff/1/cc/input/scrollbar_animatio... cc/input/scrollbar_animation_controller_unittest.cc:1031: time += kFadeInDuration; On 2017/04/13 14:38:14, bokan wrote: > Please add a step in here before the animation finishes to ensure we're actually > animating the fade in. Done. https://codereview.chromium.org/2816923002/diff/1/cc/trees/layer_tree_host_im... File cc/trees/layer_tree_host_impl_unittest.cc (right): https://codereview.chromium.org/2816923002/diff/1/cc/trees/layer_tree_host_im... cc/trees/layer_tree_host_impl_unittest.cc:3248: // requested a redraw and a commit. On 2017/04/13 14:38:14, bokan wrote: > This assumes the fade in is instant, you'll need to fix it to move time forward > a bit. Done.
https://codereview.chromium.org/2816923002/diff/60001/cc/input/scrollbar_anim... File cc/input/scrollbar_animation_controller.cc (right): https://codereview.chromium.org/2816923002/diff/60001/cc/input/scrollbar_anim... cc/input/scrollbar_animation_controller.cc:156: if (is_animating_) { Why'd you lose the `&& animation_change_ != NONE` here? We'll enter this function if the thinning controllers need animation so if we're not animating opacity we shouldn't do anything for opacity. https://codereview.chromium.org/2816923002/diff/60001/cc/input/scrollbar_anim... File cc/input/scrollbar_animation_controller.h (right): https://codereview.chromium.org/2816923002/diff/60001/cc/input/scrollbar_anim... cc/input/scrollbar_animation_controller.h:146: // show_delay_ is only for the case where the mouse hovers near the screen nit: update comment.
Patchset #4 (id:80001) has been deleted
Updated. PTAL. Thank you. https://codereview.chromium.org/2816923002/diff/60001/cc/input/scrollbar_anim... File cc/input/scrollbar_animation_controller.cc (right): https://codereview.chromium.org/2816923002/diff/60001/cc/input/scrollbar_anim... cc/input/scrollbar_animation_controller.cc:156: if (is_animating_) { On 2017/04/13 15:31:40, bokan wrote: > Why'd you lose the `&& animation_change_ != NONE` here? We'll enter this > function if the thinning controllers need animation so if we're not animating > opacity we shouldn't do anything for opacity. animation_change_ must not be NONE if is_animating_ is true. I add a DCHECK here and StartAnimation.
lgtm https://codereview.chromium.org/2816923002/diff/60001/cc/input/scrollbar_anim... File cc/input/scrollbar_animation_controller.cc (right): https://codereview.chromium.org/2816923002/diff/60001/cc/input/scrollbar_anim... cc/input/scrollbar_animation_controller.cc:156: if (is_animating_) { On 2017/04/13 15:56:08, chaopeng wrote: > On 2017/04/13 15:31:40, bokan wrote: > > Why'd you lose the `&& animation_change_ != NONE` here? We'll enter this > > function if the thinning controllers need animation so if we're not animating > > opacity we shouldn't do anything for opacity. > > animation_change_ must not be NONE if is_animating_ is true. I add a DCHECK here > and StartAnimation. Ah, got it. Thanks.
chaopeng@chromium.org changed reviewers: + weiliangc@chromium.org
weiliangc@chromium.org: PTAL. Thank you.
https://codereview.chromium.org/2816923002/diff/100001/cc/input/scrollbar_ani... File cc/input/scrollbar_animation_controller.cc (right): https://codereview.chromium.org/2816923002/diff/100001/cc/input/scrollbar_ani... cc/input/scrollbar_animation_controller.cc:132: DCHECK(delayed_scrollbar_fade_out_.IsCancelled()); Could this happen in the middle of FadeOut animation? Or vice versa? Would setting the animation_change_ directly here just change what the animation is?
On 2017/04/18 16:45:21, weiliangc wrote: > https://codereview.chromium.org/2816923002/diff/100001/cc/input/scrollbar_ani... > File cc/input/scrollbar_animation_controller.cc (right): > > https://codereview.chromium.org/2816923002/diff/100001/cc/input/scrollbar_ani... > cc/input/scrollbar_animation_controller.cc:132: > DCHECK(delayed_scrollbar_fade_out_.IsCancelled()); > Could this happen in the middle of FadeOut animation? Or vice versa? > > Would setting the animation_change_ directly here just change what the animation > is? I merge FadeOut and FadeIn together. PTAL. Thank you.
On 2017/04/19 at 02:50:51, chaopeng wrote: > On 2017/04/18 16:45:21, weiliangc wrote: > > https://codereview.chromium.org/2816923002/diff/100001/cc/input/scrollbar_ani... > > File cc/input/scrollbar_animation_controller.cc (right): > > > > https://codereview.chromium.org/2816923002/diff/100001/cc/input/scrollbar_ani... > > cc/input/scrollbar_animation_controller.cc:132: > > DCHECK(delayed_scrollbar_fade_out_.IsCancelled()); > > Could this happen in the middle of FadeOut animation? Or vice versa? > > > > Would setting the animation_change_ directly here just change what the animation > > is? > > I merge FadeOut and FadeIn together. PTAL. Thank you. What I am worried about is when is_animating FadeOut, mouse move and triggers posting a FadeIn delayed task, and animation_change_ is set right away, and when you RunAnimationFrame, your opacity calculation would be wrong because you are doing FadeIn calculation with FadeOut progress. The proper fix is probably only setting animation_change_ in the StartAnimation.
https://codereview.chromium.org/2816923002/diff/120001/cc/input/scrollbar_ani... File cc/input/scrollbar_animation_controller.cc (right): https://codereview.chromium.org/2816923002/diff/120001/cc/input/scrollbar_ani... cc/input/scrollbar_animation_controller.cc:322: } else { when is_animating FadeOut, mouse move would not post a FadeIn delayed task. We also has a test cover this case https://cs.chromium.org/chromium/src/cc/input/scrollbar_animation_controller_...
LGTM https://codereview.chromium.org/2816923002/diff/120001/cc/input/scrollbar_ani... File cc/input/scrollbar_animation_controller.cc (right): https://codereview.chromium.org/2816923002/diff/120001/cc/input/scrollbar_ani... cc/input/scrollbar_animation_controller.cc:322: } else { On 2017/04/19 at 16:12:18, chaopeng wrote: > when is_animating FadeOut, mouse move would not post a FadeIn delayed task. We also has a test cover this case https://cs.chromium.org/chromium/src/cc/input/scrollbar_animation_controller_... Ah I see so there is no animation to fade in when it's fading out. Thanks for pointing that out. Thanks.
The CQ bit was checked by chaopeng@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from bokan@chromium.org Link to the patchset: https://codereview.chromium.org/2816923002/#ps120001 (title: "merge fade-in and fade-out to same code method")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
chaopeng@chromium.org changed reviewers: + estade@chromium.org, kbr@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
estade@ PTAL at ui/native_theme/overlay_scrollbar_constants_aura.h kbr@ PTAL at content/renderer/gpu/render_widget_compositor.cc Thanks all.
https://codereview.chromium.org/2816923002/diff/120001/ui/native_theme/overla... File ui/native_theme/overlay_scrollbar_constants_aura.h (right): https://codereview.chromium.org/2816923002/diff/120001/ui/native_theme/overla... ui/native_theme/overlay_scrollbar_constants_aura.h:25: constexpr base::TimeDelta kOverlayScrollbarFadeOutDuration = why do fade in/out have their own constants if they're identical?
content/renderer/gpu/ lgtm with estade's question addressed.
On 2017/04/19 22:05:32, Evan Stade wrote: > https://codereview.chromium.org/2816923002/diff/120001/ui/native_theme/overla... > File ui/native_theme/overlay_scrollbar_constants_aura.h (right): > > https://codereview.chromium.org/2816923002/diff/120001/ui/native_theme/overla... > ui/native_theme/overlay_scrollbar_constants_aura.h:25: constexpr base::TimeDelta > kOverlayScrollbarFadeOutDuration = > why do fade in/out have their own constants if they're identical? Done. PTAL. Thank you.
https://codereview.chromium.org/2816923002/diff/140001/ui/native_theme/overla... File ui/native_theme/overlay_scrollbar_constants_aura.h (right): https://codereview.chromium.org/2816923002/diff/140001/ui/native_theme/overla... ui/native_theme/overlay_scrollbar_constants_aura.h:24: base::TimeDelta::FromMilliseconds(1000); Any reason to keep the fadein/out separate?
https://codereview.chromium.org/2816923002/diff/140001/ui/native_theme/overla... File ui/native_theme/overlay_scrollbar_constants_aura.h (right): https://codereview.chromium.org/2816923002/diff/140001/ui/native_theme/overla... ui/native_theme/overlay_scrollbar_constants_aura.h:24: base::TimeDelta::FromMilliseconds(1000); On 2017/04/20 02:40:30, Evan Stade wrote: > Any reason to keep the fadein/out separate? These two are different. FadeIn is by hovering a rect near screen edge. FadeOut is by no scroll action. I think PM may change them to different value.
https://codereview.chromium.org/2816923002/diff/140001/ui/native_theme/overla... File ui/native_theme/overlay_scrollbar_constants_aura.h (right): https://codereview.chromium.org/2816923002/diff/140001/ui/native_theme/overla... ui/native_theme/overlay_scrollbar_constants_aura.h:24: base::TimeDelta::FromMilliseconds(1000); On 2017/04/20 02:50:10, chaopeng wrote: > On 2017/04/20 02:40:30, Evan Stade wrote: > > Any reason to keep the fadein/out separate? > > These two are different. FadeIn is by hovering a rect near screen edge. FadeOut > is by no scroll action. I think PM may change them to different value. I would say we should use the same constant (and in other code, the same variable). If we need them to differ it won't be hard to allow for that.
On 2017/04/20 03:45:01, Evan Stade wrote: > https://codereview.chromium.org/2816923002/diff/140001/ui/native_theme/overla... > File ui/native_theme/overlay_scrollbar_constants_aura.h (right): > > https://codereview.chromium.org/2816923002/diff/140001/ui/native_theme/overla... > ui/native_theme/overlay_scrollbar_constants_aura.h:24: > base::TimeDelta::FromMilliseconds(1000); > On 2017/04/20 02:50:10, chaopeng wrote: > > On 2017/04/20 02:40:30, Evan Stade wrote: > > > Any reason to keep the fadein/out separate? > > > > These two are different. FadeIn is by hovering a rect near screen edge. > FadeOut > > is by no scroll action. I think PM may change them to different value. > > I would say we should use the same constant (and in other code, the same > variable). If we need them to differ it won't be hard to allow for that. Updated, PTAL. Thank you.
The CQ bit was checked by chaopeng@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
https://codereview.chromium.org/2816923002/diff/160001/cc/input/scrollbar_ani... File cc/input/scrollbar_animation_controller.h (right): https://codereview.chromium.org/2816923002/diff/160001/cc/input/scrollbar_ani... cc/input/scrollbar_animation_controller.h:56: base::TimeDelta fade_in_delay, I had envisioned you combining these as well, but I'll let code owners provide their preferences on that. https://codereview.chromium.org/2816923002/diff/160001/ui/native_theme/overla... File ui/native_theme/overlay_scrollbar_constants_aura.h (right): https://codereview.chromium.org/2816923002/diff/160001/ui/native_theme/overla... ui/native_theme/overlay_scrollbar_constants_aura.h:22: // the screen edge and no scroll action for a while. nit: the wording of this comment is a little confusing but I think the variable names are self-documenting enough so instead of rewording it we can just delete it.
The CQ bit was checked by chaopeng@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...
https://codereview.chromium.org/2816923002/diff/160001/cc/input/scrollbar_ani... File cc/input/scrollbar_animation_controller.h (right): https://codereview.chromium.org/2816923002/diff/160001/cc/input/scrollbar_ani... cc/input/scrollbar_animation_controller.h:56: base::TimeDelta fade_in_delay, On 2017/04/20 14:33:45, Evan Stade wrote: > I had envisioned you combining these as well, but I'll let code owners provide > their preferences on that. Now we have fade_in_delay, fade_out_delay and fade_out_resize_delay. They are same in Aura, but fade_out_delay and fade_out_resize_delay are different in Android. https://cs.chromium.org/chromium/src/cc/input/scrollbar_animation_controller_... Also we use different delay in test to recognize which task we post. I prefer to keep it. weiliangc@ WDYT?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by chaopeng@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
I think maybe fade_in_delay and fade_out_delay can be combined. Also please update the CL description to say that fade in animation is only triggered when scrollbars are currently hidden. Otherwise still LGTM
On 2017/04/21 20:36:16, weiliangc wrote: > I think maybe fade_in_delay and fade_out_delay can be combined. > > Also please update the CL description to say that fade in animation is only > triggered when scrollbars are currently hidden. > > Otherwise still LGTM lgtm with above addressed
Description was changed from ========== change overlay scrollbar hover show to hover fade in BUG=710543 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ========== to ========== change overlay scrollbar hover show to hover fade in Currently, we have 2 ways to show the hidden scrollbars: 1. scroll position update 2. move mouse to a region near edge In this patch, we only change the "move mouse to a region near edge" to show scrollbars immediately to fade in scrollbars. BUG=710543 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ==========
The CQ bit was checked by chaopeng@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...
chaopeng@chromium.org changed reviewers: + esprehn@chromium.org, sadrul@chromium.org
weiliangc and estade comments addressed. esprehn@chromium.org: Please review changes in content sadrul@chromium.org: Please review changes in ui Thank you.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm
Content/ lgtm
The CQ bit was checked by chaopeng@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from bokan@chromium.org, kbr@chromium.org, weiliangc@chromium.org, estade@chromium.org Link to the patchset: https://codereview.chromium.org/2816923002/#ps220001 (title: "combine fade_in_delay and fade_out_delay to fade_delay")
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": 220001, "attempt_start_ts": 1493060982840070,
"parent_rev": "9587eaf061cc8429228e1e1c5e494e8bf983fc9f", "commit_rev":
"a47e7158e746c01b4ccbbd25d71e267d93347b94"}
Message was sent while issue was closed.
Description was changed from ========== change overlay scrollbar hover show to hover fade in Currently, we have 2 ways to show the hidden scrollbars: 1. scroll position update 2. move mouse to a region near edge In this patch, we only change the "move mouse to a region near edge" to show scrollbars immediately to fade in scrollbars. BUG=710543 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ========== to ========== change overlay scrollbar hover show to hover fade in Currently, we have 2 ways to show the hidden scrollbars: 1. scroll position update 2. move mouse to a region near edge In this patch, we only change the "move mouse to a region near edge" to show scrollbars immediately to fade in scrollbars. BUG=710543 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel Review-Url: https://codereview.chromium.org/2816923002 Cr-Commit-Position: refs/heads/master@{#466740} Committed: https://chromium.googlesource.com/chromium/src/+/a47e7158e746c01b4ccbbd25d71e... ==========
Message was sent while issue was closed.
Committed patchset #10 (id:220001) as https://chromium.googlesource.com/chromium/src/+/a47e7158e746c01b4ccbbd25d71e... |
