Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1708393002:
Removed m_previousWheelScrolledNode from EventHandler (Closed)
DescriptionRemoved m_previousWheelScrolledNode from EventHandler
m_previousWheelScrolledNode gets nulled out at the beginning of each
wheel event so it had no global use. It's only use was to lock a
multi-axis scroll to the first scrolled element so I replaced it with
a local variable that does the same thing.
There should be no behavior change because of this CL.
BUG=
Committed: https://crrev.com/74fa2a601bd77a3b57a7de1d960b2ebebb888822
Cr-Commit-Position: refs/heads/master@{#378157}
Patch Set 1 #Patch Set 2 : Rebase #Patch Set 3 : Add back in local stopNode #Patch Set 4 : Rebase #
Messages
Total messages: 29 (12 generated)
bokan@chromium.org changed reviewers: + tdresser@chromium.org
ptal
Are you sure this doesn't do anything? It's an out parameter of EventHandler::scroll. I thought it should still prevent us from doing some extra work sometimes.
On 2016/02/19 14:28:29, tdresser wrote: > Are you sure this doesn't do anything? > > It's an out parameter of EventHandler::scroll. I thought it should still prevent > us from doing some extra work sometimes. Right, but it's cleared at each wheel event so at the least it could be replaced by a local, right? It seems strange to me though since a local effect like this wouldn't really be noticeable and it has a slight bias to horizontal scrolling so I suspect it's unintentional. I've checked that the behaviour on an osx track pad is correct for nested vertical strollers but I didn't check for nested boxes across both axes, I'll do that when I get back.
You're right, looks like it could be local, and should only effect cases with mixed direction. Biasing against scrolling multiple scrollers at the same time does seem like a reasonable property.
On 2016/02/19 15:36:08, tdresser wrote: > You're right, looks like it could be local, and should only effect cases with > mixed direction. > > Biasing against scrolling multiple scrollers at the same time does seem like a > reasonable property. I took a quick look and it looks like that's already accomplished using the RailsMode property of the event - at least on Mac. I'm not aware of other "two-way" wheel devices or how those might work. Using the "stopNode" in defaultWheelHandler meant that we find the first scrollable horizontal ancestor and we won't vertically scroll anything above that. So (assuming you removed the existing railing behaviuor) if you diagonally scroll on: A vertical scroller within a horizontal scrolling box: both will scroll. A horizontal scroller within a vertical scrolling box: only the horizontal box will scroll. It's not clear to me this is useful or intentional so I'm in favor of removing it altogether, especially since I don't think its actually getting used anyway (there's no CC equivalent behavior). WDYT?
On 2016/02/22 15:11:00, bokan wrote: > On 2016/02/19 15:36:08, tdresser wrote: > > You're right, looks like it could be local, and should only effect cases with > > mixed direction. > > > > Biasing against scrolling multiple scrollers at the same time does seem like a > > reasonable property. > > I took a quick look and it looks like that's already accomplished using the > RailsMode property of the event - at least on Mac. I'm not aware of other > "two-way" wheel devices or how those might work. > > Using the "stopNode" in defaultWheelHandler meant that we find the first > scrollable horizontal ancestor and we won't vertically scroll anything above > that. So (assuming you removed the existing railing behaviuor) if you diagonally > scroll on: > > A vertical scroller within a horizontal scrolling box: both will scroll. > A horizontal scroller within a vertical scrolling box: only the horizontal box > will scroll. > > It's not clear to me this is useful or intentional so I'm in favor of removing > it altogether, especially since I don't think its actually getting used anyway > (there's no CC equivalent behavior). WDYT? Agreed, let's rip this out. Can you clarify in the description what the functionality change is here? It might even be worth adding a test that for scrolling multiple scrollers (horiziontal in vertical and vertical in horizontal).
On 2016/02/22 15:21:25, tdresser wrote: > On 2016/02/22 15:11:00, bokan wrote: > > On 2016/02/19 15:36:08, tdresser wrote: > > > You're right, looks like it could be local, and should only effect cases > with > > > mixed direction. > > > > > > Biasing against scrolling multiple scrollers at the same time does seem like > a > > > reasonable property. > > > > I took a quick look and it looks like that's already accomplished using the > > RailsMode property of the event - at least on Mac. I'm not aware of other > > "two-way" wheel devices or how those might work. > > > > Using the "stopNode" in defaultWheelHandler meant that we find the first > > scrollable horizontal ancestor and we won't vertically scroll anything above > > that. So (assuming you removed the existing railing behaviuor) if you > diagonally > > scroll on: > > > > A vertical scroller within a horizontal scrolling box: both will scroll. > > A horizontal scroller within a vertical scrolling box: only the horizontal box > > will scroll. > > > > It's not clear to me this is useful or intentional so I'm in favor of removing > > it altogether, especially since I don't think its actually getting used anyway > > (there's no CC equivalent behavior). WDYT? > > Agreed, let's rip this out. > Can you clarify in the description what the functionality change is here? > > It might even be worth adding a test that for scrolling multiple scrollers > (horiziontal in vertical and vertical in horizontal). LGTM, though I think a test might be worthwhile.
On 2016/02/22 15:21:47, tdresser wrote: > On 2016/02/22 15:21:25, tdresser wrote: > > On 2016/02/22 15:11:00, bokan wrote: > > > On 2016/02/19 15:36:08, tdresser wrote: > > > > You're right, looks like it could be local, and should only effect cases > > with > > > > mixed direction. > > > > > > > > Biasing against scrolling multiple scrollers at the same time does seem > like > > a > > > > reasonable property. > > > > > > I took a quick look and it looks like that's already accomplished using the > > > RailsMode property of the event - at least on Mac. I'm not aware of other > > > "two-way" wheel devices or how those might work. > > > > > > Using the "stopNode" in defaultWheelHandler meant that we find the first > > > scrollable horizontal ancestor and we won't vertically scroll anything above > > > that. So (assuming you removed the existing railing behaviuor) if you > > diagonally > > > scroll on: > > > > > > A vertical scroller within a horizontal scrolling box: both will scroll. > > > A horizontal scroller within a vertical scrolling box: only the horizontal > box > > > will scroll. > > > > > > It's not clear to me this is useful or intentional so I'm in favor of > removing > > > it altogether, especially since I don't think its actually getting used > anyway > > > (there's no CC equivalent behavior). WDYT? > > > > Agreed, let's rip this out. > > Can you clarify in the description what the functionality change is here? > > > > It might even be worth adding a test that for scrolling multiple scrollers > > (horiziontal in vertical and vertical in horizontal). > > LGTM, though I think a test might be worthwhile. The multiple scroller case is still locked by the RailsMode though. Or did you mean something else?
Description was changed from ========== Removed m_previousWheelScrolledNode from EventHandler This doesn't seem to do anything anymore. BUG= ========== to ========== Removed m_previousWheelScrolledNode from EventHandler m_previousWheelScrolledNode gets nulled out at the beginning of each wheel event so it had no global use. It's only use was to lock a multi-axis scroll to the first scrolled element so I replaced it with a local variable that does the same thing. BUG= ==========
Description was changed from ========== Removed m_previousWheelScrolledNode from EventHandler m_previousWheelScrolledNode gets nulled out at the beginning of each wheel event so it had no global use. It's only use was to lock a multi-axis scroll to the first scrolled element so I replaced it with a local variable that does the same thing. BUG= ========== to ========== Removed m_previousWheelScrolledNode from EventHandler m_previousWheelScrolledNode gets nulled out at the beginning of each wheel event so it had no global use. It's only use was to lock a multi-axis scroll to the first scrolled element so I replaced it with a local variable that does the same thing. There should be no behavior change because of this CL. BUG= ==========
The CQ bit was checked by bokan@chromium.org to run a CQ dry run
bokan@chromium.org changed reviewers: + jbroman@chromium.org
Ok, tested on ChromeOS and I added back in the stopNode locally - the behavior is now unchanged. +jbroman@ for stamp
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1708393002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1708393002/40001
rs lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_chromium_gn_compile_dbg on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_chro...) cast_shell_linux on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...) chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) linux_chromium_compile_dbg_32_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_gn_chromeos_rel on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...) mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...) mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by bokan@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1708393002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1708393002/60001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by bokan@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tdresser@chromium.org, jbroman@chromium.org Link to the patchset: https://codereview.chromium.org/1708393002/#ps60001 (title: "Rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1708393002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1708393002/60001
Message was sent while issue was closed.
Description was changed from ========== Removed m_previousWheelScrolledNode from EventHandler m_previousWheelScrolledNode gets nulled out at the beginning of each wheel event so it had no global use. It's only use was to lock a multi-axis scroll to the first scrolled element so I replaced it with a local variable that does the same thing. There should be no behavior change because of this CL. BUG= ========== to ========== Removed m_previousWheelScrolledNode from EventHandler m_previousWheelScrolledNode gets nulled out at the beginning of each wheel event so it had no global use. It's only use was to lock a multi-axis scroll to the first scrolled element so I replaced it with a local variable that does the same thing. There should be no behavior change because of this CL. BUG= ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Removed m_previousWheelScrolledNode from EventHandler m_previousWheelScrolledNode gets nulled out at the beginning of each wheel event so it had no global use. It's only use was to lock a multi-axis scroll to the first scrolled element so I replaced it with a local variable that does the same thing. There should be no behavior change because of this CL. BUG= ========== to ========== Removed m_previousWheelScrolledNode from EventHandler m_previousWheelScrolledNode gets nulled out at the beginning of each wheel event so it had no global use. It's only use was to lock a multi-axis scroll to the first scrolled element so I replaced it with a local variable that does the same thing. There should be no behavior change because of this CL. BUG= Committed: https://crrev.com/74fa2a601bd77a3b57a7de1d960b2ebebb888822 Cr-Commit-Position: refs/heads/master@{#378157} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/74fa2a601bd77a3b57a7de1d960b2ebebb888822 Cr-Commit-Position: refs/heads/master@{#378157} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
