Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1720023002:
Wheel scrolling with smooth scrolling enabled would cause an extra animation (Closed)
DescriptionWheel scrolling with smooth scrolling enabled would cause an extra animation
Call ScrollBegin/End on GestureScrollBegin; the real delta
co-ordinates will get updated in the GestureScrollUpdate that is
passed immediately afterwards. This avoids a kind of bubble effect that
is seen when scrolling one tick at a time.
BUG=588691
Committed: https://crrev.com/f2aceeade512172a12e829cd95d41f5a4b04574a
Cr-Commit-Position: refs/heads/master@{#376988}
Patch Set 1 #
Total comments: 4
Patch Set 2 : Change ScrollAnimated to ScrollBegin/End #Patch Set 3 : Fix failing test with adjusted scrollbegin/scroll end approach #
Messages
Total messages: 26 (11 generated)
Description was changed from ========== Wheel scrolling with smooth scrolling enabled would cause an extra animation Pass 0,0 to the ScrollAnimated on GestureScrollBegin; the real delta co-ordinates will get updated in the GestureScrollUpdate that is passed immediately afterwards. This avoids a kind of bubble effect that is seen when scrolling one tick at a time. BUG=588691 ========== to ========== Wheel scrolling with smooth scrolling enabled would cause an extra animation Pass 0,0 to the ScrollAnimated on GestureScrollBegin; the real delta co-ordinates will get updated in the GestureScrollUpdate that is passed immediately afterwards. This avoids a kind of bubble effect that is seen when scrolling one tick at a time. BUG=588691 ==========
dtapuska@chromium.org changed reviewers: + tdresser@chromium.org
https://codereview.chromium.org/1720023002/diff/1/ui/events/blink/input_handl... File ui/events/blink/input_handler_proxy.cc (right): https://codereview.chromium.org/1720023002/diff/1/ui/events/blink/input_handl... ui/events/blink/input_handler_proxy.cc:601: gfx::Point(gesture_event.x, gesture_event.y), gfx::Vector2dF()); Does this need to be a ScrollAnimated? Can we just always call input_handler_->ScrollBegin instead?
https://codereview.chromium.org/1720023002/diff/1/ui/events/blink/input_handl... File ui/events/blink/input_handler_proxy.cc (right): https://codereview.chromium.org/1720023002/diff/1/ui/events/blink/input_handl... ui/events/blink/input_handler_proxy.cc:601: gfx::Point(gesture_event.x, gesture_event.y), gfx::Vector2dF()); On 2016/02/22 20:14:49, tdresser wrote: > Does this need to be a ScrollAnimated? > Can we just always call input_handler_->ScrollBegin instead? I first attempted to do that and since a scroll animator isn't created it was failing.
tdresser@chromium.org changed reviewers: + ymalik@chromium.org
Hmmm, that's a pain. Yash, any thoughts on what the correct thing to do is here?
https://codereview.chromium.org/1720023002/diff/1/ui/events/blink/input_handl... File ui/events/blink/input_handler_proxy.cc (right): https://codereview.chromium.org/1720023002/diff/1/ui/events/blink/input_handl... ui/events/blink/input_handler_proxy.cc:601: gfx::Point(gesture_event.x, gesture_event.y), gfx::Vector2dF()); On 2016/02/22 20:15:59, dtapuska wrote: > On 2016/02/22 20:14:49, tdresser wrote: > > Does this need to be a ScrollAnimated? > > Can we just always call input_handler_->ScrollBegin instead? > Why do we need this branch to begin with if we're setting scroll delta to 0? > I first attempted to do that and since a scroll animator isn't created it was > failing. ScrollAnimated calls ScrollBegin, so where exactly does the call fail?
https://codereview.chromium.org/1720023002/diff/1/ui/events/blink/input_handl... File ui/events/blink/input_handler_proxy.cc (right): https://codereview.chromium.org/1720023002/diff/1/ui/events/blink/input_handl... ui/events/blink/input_handler_proxy.cc:601: gfx::Point(gesture_event.x, gesture_event.y), gfx::Vector2dF()); On 2016/02/22 20:35:32, ymalik1 wrote: > On 2016/02/22 20:15:59, dtapuska wrote: > > On 2016/02/22 20:14:49, tdresser wrote: > > > Does this need to be a ScrollAnimated? > > > Can we just always call input_handler_->ScrollBegin instead? > > > > Why do we need this branch to begin with if we're setting scroll delta to 0? > > > I first attempted to do that and since a scroll animator isn't created it was > > failing. > > ScrollAnimated calls ScrollBegin, so where exactly does the call fail? ScrollAnimated is successful; but the next ScrollAnimated call doesn't work; because it is already inside a scroll event. We need to know if the GestureScrollBegin can scroll or not; I guess I could call BeginScroll/EndScroll combination. Do you recommend that instead? Ideally I want to get into the ScrollAnimationCreate code path inside layer_tree_host_impl.cc
Description was changed from
==========
Wheel scrolling with smooth scrolling enabled would cause an extra animation
Pass 0,0 to the ScrollAnimated on GestureScrollBegin; the real delta
co-ordinates will get updated in the GestureScrollUpdate that is
passed immediately afterwards. This avoids a kind of bubble effect that
is seen when scrolling one tick at a time.
BUG=588691
==========
to
==========
Wheel scrolling with smooth scrolling enabled would cause an extra animation
Wheel scrolling with smooth scrolling enabled would cause an extra animation
Call ScrollBegin/End on GestureScrollBegin; the real delta
co-ordinates will get updated in the GestureScrollUpdate that is
passed immediately afterwards. This avoids a kind of bubble effect that
is seen when scrolling one tick at a time.
BUG=588691
==========
Description was changed from
==========
Wheel scrolling with smooth scrolling enabled would cause an extra animation
Wheel scrolling with smooth scrolling enabled would cause an extra animation
Call ScrollBegin/End on GestureScrollBegin; the real delta
co-ordinates will get updated in the GestureScrollUpdate that is
passed immediately afterwards. This avoids a kind of bubble effect that
is seen when scrolling one tick at a time.
BUG=588691
==========
to
==========
Wheel scrolling with smooth scrolling enabled would cause an extra animation
Call ScrollBegin/End on GestureScrollBegin; the real delta
co-ordinates will get updated in the GestureScrollUpdate that is
passed immediately afterwards. This avoids a kind of bubble effect that
is seen when scrolling one tick at a time.
BUG=588691
==========
On 2016/02/22 20:39:03, dtapuska wrote: > https://codereview.chromium.org/1720023002/diff/1/ui/events/blink/input_handl... > File ui/events/blink/input_handler_proxy.cc (right): > > https://codereview.chromium.org/1720023002/diff/1/ui/events/blink/input_handl... > ui/events/blink/input_handler_proxy.cc:601: gfx::Point(gesture_event.x, > gesture_event.y), gfx::Vector2dF()); > On 2016/02/22 20:35:32, ymalik1 wrote: > > On 2016/02/22 20:15:59, dtapuska wrote: > > > On 2016/02/22 20:14:49, tdresser wrote: > > > > Does this need to be a ScrollAnimated? > > > > Can we just always call input_handler_->ScrollBegin instead? > > > > > > > Why do we need this branch to begin with if we're setting scroll delta to 0? > > > > > I first attempted to do that and since a scroll animator isn't created it > was > > > failing. > > > > ScrollAnimated calls ScrollBegin, so where exactly does the call fail? > > ScrollAnimated is successful; but the next ScrollAnimated call doesn't work; > because it is already inside a scroll event. > > We need to know if the GestureScrollBegin can scroll or not; I guess I could > call BeginScroll/EndScroll combination. Do you recommend that instead? Ideally I > want to get into the ScrollAnimationCreate code path inside > layer_tree_host_impl.cc In talking with Yash; this is the desired approach. PTAL.
lgtm Thanks!
LGTM.
The CQ bit was checked by dtapuska@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1720023002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1720023002/20001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by dtapuska@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from ymalik@chromium.org, tdresser@chromium.org Link to the patchset: https://codereview.chromium.org/1720023002/#ps40001 (title: "Fix failing test with adjusted scrollbegin/scroll end approach")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1720023002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1720023002/40001
Message was sent while issue was closed.
Description was changed from
==========
Wheel scrolling with smooth scrolling enabled would cause an extra animation
Call ScrollBegin/End on GestureScrollBegin; the real delta
co-ordinates will get updated in the GestureScrollUpdate that is
passed immediately afterwards. This avoids a kind of bubble effect that
is seen when scrolling one tick at a time.
BUG=588691
==========
to
==========
Wheel scrolling with smooth scrolling enabled would cause an extra animation
Call ScrollBegin/End on GestureScrollBegin; the real delta
co-ordinates will get updated in the GestureScrollUpdate that is
passed immediately afterwards. This avoids a kind of bubble effect that
is seen when scrolling one tick at a time.
BUG=588691
==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from
==========
Wheel scrolling with smooth scrolling enabled would cause an extra animation
Call ScrollBegin/End on GestureScrollBegin; the real delta
co-ordinates will get updated in the GestureScrollUpdate that is
passed immediately afterwards. This avoids a kind of bubble effect that
is seen when scrolling one tick at a time.
BUG=588691
==========
to
==========
Wheel scrolling with smooth scrolling enabled would cause an extra animation
Call ScrollBegin/End on GestureScrollBegin; the real delta
co-ordinates will get updated in the GestureScrollUpdate that is
passed immediately afterwards. This avoids a kind of bubble effect that
is seen when scrolling one tick at a time.
BUG=588691
Committed: https://crrev.com/f2aceeade512172a12e829cd95d41f5a4b04574a
Cr-Commit-Position: refs/heads/master@{#376988}
==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/f2aceeade512172a12e829cd95d41f5a4b04574a Cr-Commit-Position: refs/heads/master@{#376988} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
