Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2049323002:
[Mac] Don't dispatch wheel events for PhaseMayBegin|Cancelled|Ended (Closed)
|
Created:
4 years, 6 months ago by chongz Modified:
4 years, 5 months ago CC:
chromium-reviews, dtapuska+blinkwatch_chromium.org, nzolghadr+blinkwatch_chromium.org, eae+blinkwatch, blink-reviews-events_chromium.org, dglazkov+blink, blink-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[Mac] Don't dispatch wheel events for PhaseMayBegin|Cancelled|Ended
Mac will dispatch wheel events with |PlatformWheelEventPhase|, e.g.
1. |PhaseMayBegin| -> |PhaseCancelled| for two finger touch and lift
2. |PhaseMayBegin| -> |PhaseBegan| -> |PhaseChanged|* -> |PhaseEnded| for two finger scroll
3. |PhaseNone| for real mouse wheel scroll
Other platforms don't have phase info and only have events similar to
|PhaseBegan| and |PhaseChanged|. This CL filters those extra events
and only dispatch |PhaseBegan| and |PhaseChanged| for platform interop.
Note that |PhaseBegan| usually has non-zero delta, so we want to keep them.
Interop:
Firefox and Safari doesn't fire events for these phases, so we will match them after CL.
BUG=553732
Committed: https://crrev.com/b92d711668477cf2fdbfc961bbefa61d9c7c2810
Cr-Commit-Position: refs/heads/master@{#403719}
Patch Set 1 : Add filter and LayoutTests #
Total comments: 2
Patch Set 2 : dtapuska's review #
Total comments: 2
Patch Set 3 : Use == instead of strcmp() #
Messages
Total messages: 44 (31 generated)
Description was changed from ========== [Mac] Don't dispatch extra Mac-specific wheel events BUG=553732 ========== to ========== [Mac] Don't dispatch extra Mac-specific wheel events Mac will dispatch wheel events with |PlatformWheelEventPhase|, e.g. 1. |..MayBegin| -> |..Cancelled| 2. |..MayBegin| -> |..Began| -> |..Changed|* -> |..Ended| But other platforms only has wheel changed events. This CL filters those extra events and only dispatch |..Began| and |..Changed| for platform interop. Note that |..Began| usually has non-zero delta, so we want to keep them. BUG=553732 ==========
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Description was changed from ========== [Mac] Don't dispatch extra Mac-specific wheel events Mac will dispatch wheel events with |PlatformWheelEventPhase|, e.g. 1. |..MayBegin| -> |..Cancelled| 2. |..MayBegin| -> |..Began| -> |..Changed|* -> |..Ended| But other platforms only has wheel changed events. This CL filters those extra events and only dispatch |..Began| and |..Changed| for platform interop. Note that |..Began| usually has non-zero delta, so we want to keep them. BUG=553732 ========== to ========== [Mac] Don't dispatch Mac-specific wheel events Mac will dispatch wheel events with |PlatformWheelEventPhase|, e.g. 1. |..MayBegin| -> |..Cancelled| 2. |..MayBegin| -> |..Began| -> |..Changed|* -> |..Ended| But other platforms only has wheel changed events. This CL filters those extra events and only dispatch |..Began| and |..Changed| for platform interop. Note that |..Began| usually has non-zero delta, so we want to keep them. TODO: EventSender is sending events with |PlatformWheelEventPhaseNone| so we cannot filter them now, should fix after all platform supports scroll phases. BUG=553732 ==========
Description was changed from ========== [Mac] Don't dispatch Mac-specific wheel events Mac will dispatch wheel events with |PlatformWheelEventPhase|, e.g. 1. |..MayBegin| -> |..Cancelled| 2. |..MayBegin| -> |..Began| -> |..Changed|* -> |..Ended| But other platforms only has wheel changed events. This CL filters those extra events and only dispatch |..Began| and |..Changed| for platform interop. Note that |..Began| usually has non-zero delta, so we want to keep them. TODO: EventSender is sending events with |PlatformWheelEventPhaseNone| so we cannot filter them now, should fix after all platform supports scroll phases. BUG=553732 ========== to ========== [Mac] Don't dispatch Mac-specific wheel events Mac will dispatch wheel events with |PlatformWheelEventPhase|, e.g. 1. |..MayBegin| -> |..Cancelled| 2. |..MayBegin| -> |..Began| -> |..Changed|* -> |..Ended| But other platforms only has wheel changed events. This CL filters those extra events and only dispatch |..Began| and |..Changed| for platform interop. Note that |..Began| usually has non-zero delta, so we want to keep them. TODO: EventSender is sending events with |PlatformWheelEventPhaseNone| so we cannot filter them. BUG=553732 ==========
Description was changed from ========== [Mac] Don't dispatch Mac-specific wheel events Mac will dispatch wheel events with |PlatformWheelEventPhase|, e.g. 1. |..MayBegin| -> |..Cancelled| 2. |..MayBegin| -> |..Began| -> |..Changed|* -> |..Ended| But other platforms only has wheel changed events. This CL filters those extra events and only dispatch |..Began| and |..Changed| for platform interop. Note that |..Began| usually has non-zero delta, so we want to keep them. TODO: EventSender is sending events with |PlatformWheelEventPhaseNone| so we cannot filter them. BUG=553732 ========== to ========== [Mac] Don't dispatch Mac-specific wheel events Mac will dispatch wheel events with |PlatformWheelEventPhase|, e.g. 1. |..MayBegin| -> |..Cancelled| 2. |..MayBegin| -> |..Began| -> |..Changed|* -> |..Ended| But other platforms only has wheel changed events. This CL filters those extra events and only dispatch |..Began| and |..Changed| for platform interop. Note that |..Began| usually has non-zero delta, so we want to keep them. BUG=553732 ==========
Patchset #1 (id:40001) has been deleted
Patchset #1 (id:60001) has been deleted
Patchset #1 (id:80001) has been deleted
Patchset #1 (id:100001) has been deleted
Patchset #1 (id:120001) has been deleted
Patchset #1 (id:140001) has been deleted
Patchset #1 (id:160001) has been deleted
Patchset #1 (id:180001) has been deleted
Description was changed from ========== [Mac] Don't dispatch Mac-specific wheel events Mac will dispatch wheel events with |PlatformWheelEventPhase|, e.g. 1. |..MayBegin| -> |..Cancelled| 2. |..MayBegin| -> |..Began| -> |..Changed|* -> |..Ended| But other platforms only has wheel changed events. This CL filters those extra events and only dispatch |..Began| and |..Changed| for platform interop. Note that |..Began| usually has non-zero delta, so we want to keep them. BUG=553732 ========== to ========== [Mac] Don't dispatch wheel events for PhaseMayBegin|Cancelled|Ended Mac will dispatch wheel events with |PlatformWheelEventPhase|, e.g. 1. |PhaseMayBegin| -> |PhaseCancelled| for two finger touch and lift 2. |PhaseMayBegin| -> |PhaseBegan| -> |PhaseChanged|* -> |PhaseEnded| for two finger scroll Other platforms don't have phase info and only have events similar to |PhaseBegan| and |PhaseChanged|. This CL filters those extra events and only dispatch |PhaseBegan| and |PhaseChanged| for platform interop. Note that |PhaseBegan| usually has non-zero delta, so we want to keep them. BUG=553732 ==========
Description was changed from ========== [Mac] Don't dispatch wheel events for PhaseMayBegin|Cancelled|Ended Mac will dispatch wheel events with |PlatformWheelEventPhase|, e.g. 1. |PhaseMayBegin| -> |PhaseCancelled| for two finger touch and lift 2. |PhaseMayBegin| -> |PhaseBegan| -> |PhaseChanged|* -> |PhaseEnded| for two finger scroll Other platforms don't have phase info and only have events similar to |PhaseBegan| and |PhaseChanged|. This CL filters those extra events and only dispatch |PhaseBegan| and |PhaseChanged| for platform interop. Note that |PhaseBegan| usually has non-zero delta, so we want to keep them. BUG=553732 ========== to ========== [Mac] Don't dispatch wheel events for PhaseMayBegin|Cancelled|Ended Mac will dispatch wheel events with |PlatformWheelEventPhase|, e.g. 1. |PhaseMayBegin| -> |PhaseCancelled| for two finger touch and lift 2. |PhaseMayBegin| -> |PhaseBegan| -> |PhaseChanged|* -> |PhaseEnded| for two finger scroll Other platforms don't have phase info and only have events similar to |PhaseBegan| and |PhaseChanged|. This CL filters those extra events and only dispatch |PhaseBegan| and |PhaseChanged| for platform interop. Note that |PhaseBegan| usually has non-zero delta, so we want to keep them. BUG=553732 ==========
Description was changed from ========== [Mac] Don't dispatch wheel events for PhaseMayBegin|Cancelled|Ended Mac will dispatch wheel events with |PlatformWheelEventPhase|, e.g. 1. |PhaseMayBegin| -> |PhaseCancelled| for two finger touch and lift 2. |PhaseMayBegin| -> |PhaseBegan| -> |PhaseChanged|* -> |PhaseEnded| for two finger scroll Other platforms don't have phase info and only have events similar to |PhaseBegan| and |PhaseChanged|. This CL filters those extra events and only dispatch |PhaseBegan| and |PhaseChanged| for platform interop. Note that |PhaseBegan| usually has non-zero delta, so we want to keep them. BUG=553732 ========== to ========== [Mac] Don't dispatch wheel events for PhaseMayBegin|Cancelled|Ended Mac will dispatch wheel events with |PlatformWheelEventPhase|, e.g. 1. |PhaseMayBegin| -> |PhaseCancelled| for two finger touch and lift 2. |PhaseMayBegin| -> |PhaseBegan| -> |PhaseChanged|* -> |PhaseEnded| for two finger scroll Other platforms don't have phase info and only have events similar to |PhaseBegan| and |PhaseChanged|. This CL filters those extra events and only dispatch |PhaseBegan| and |PhaseChanged| for platform interop. Note that |PhaseBegan| usually has non-zero delta, so we want to keep them. BUG=553732 ==========
Patchset #1 (id:200001) has been deleted
Patchset #1 (id:220001) has been deleted
Patchset #1 (id:240001) has been deleted
Patchset #1 (id:260001) has been deleted
Patchset #1 (id:280001) has been deleted
Patchset #1 (id:300001) has been deleted
Patchset #1 (id:320001) has been deleted
chongz@chromium.org changed reviewers: + dtapuska@chromium.org
dtapuska@ PTAL, thanks! There seems to be some hit test issue with browser_tests, so I'm doing it with LayoutTests.
https://codereview.chromium.org/2049323002/diff/330004/components/test_runner... File components/test_runner/event_sender.cc (right): https://codereview.chromium.org/2049323002/diff/330004/components/test_runner... components/test_runner/event_sender.cc:2554: if (!args->PeekNext().IsEmpty()) { This is kind of weird that we will cast a value from javascript into an enum. Usually there is some indirection like for modifiers it goes through a String -> flags conversion.
dtapuska@ I've updated as per your comments, PTAL again, thanks! https://codereview.chromium.org/2049323002/diff/330004/components/test_runner... File components/test_runner/event_sender.cc (right): https://codereview.chromium.org/2049323002/diff/330004/components/test_runner... components/test_runner/event_sender.cc:2554: if (!args->PeekNext().IsEmpty()) { On 2016/06/30 19:53:00, dtapuska wrote: > This is kind of weird that we will cast a value from javascript into an enum. > Usually there is some indirection like for modifiers it goes through a String -> > flags conversion. Done.
https://codereview.chromium.org/2049323002/diff/350001/components/test_runner... File components/test_runner/event_sender.cc (right): https://codereview.chromium.org/2049323002/diff/350001/components/test_runner... components/test_runner/event_sender.cc:335: if (!strcmp(characters, "phaseNone")) { Why strcmps? and not just a == operator?
dtapuska@ PTAL, thanks! https://codereview.chromium.org/2049323002/diff/350001/components/test_runner... File components/test_runner/event_sender.cc (right): https://codereview.chromium.org/2049323002/diff/350001/components/test_runner... components/test_runner/event_sender.cc:335: if (!strcmp(characters, "phaseNone")) { On 2016/07/04 14:15:40, dtapuska wrote: > Why strcmps? and not just a == operator? Sorry I was following existing code (GetKeyModifier()), changed to == operator.
On 2016/07/04 17:58:43, chongz wrote: > dtapuska@ PTAL, thanks! > > https://codereview.chromium.org/2049323002/diff/350001/components/test_runner... > File components/test_runner/event_sender.cc (right): > > https://codereview.chromium.org/2049323002/diff/350001/components/test_runner... > components/test_runner/event_sender.cc:335: if (!strcmp(characters, > "phaseNone")) { > On 2016/07/04 14:15:40, dtapuska wrote: > > Why strcmps? and not just a == operator? > > Sorry I was following existing code (GetKeyModifier()), changed to == operator. lgtm
Description was changed from ========== [Mac] Don't dispatch wheel events for PhaseMayBegin|Cancelled|Ended Mac will dispatch wheel events with |PlatformWheelEventPhase|, e.g. 1. |PhaseMayBegin| -> |PhaseCancelled| for two finger touch and lift 2. |PhaseMayBegin| -> |PhaseBegan| -> |PhaseChanged|* -> |PhaseEnded| for two finger scroll Other platforms don't have phase info and only have events similar to |PhaseBegan| and |PhaseChanged|. This CL filters those extra events and only dispatch |PhaseBegan| and |PhaseChanged| for platform interop. Note that |PhaseBegan| usually has non-zero delta, so we want to keep them. BUG=553732 ========== to ========== [Mac] Don't dispatch wheel events for PhaseMayBegin|Cancelled|Ended Mac will dispatch wheel events with |PlatformWheelEventPhase|, e.g. 1. |PhaseMayBegin| -> |PhaseCancelled| for two finger touch and lift 2. |PhaseMayBegin| -> |PhaseBegan| -> |PhaseChanged|* -> |PhaseEnded| for two finger scroll 3. |PhaseNone| for real mouse wheel scroll Other platforms don't have phase info and only have events similar to |PhaseBegan| and |PhaseChanged|. This CL filters those extra events and only dispatch |PhaseBegan| and |PhaseChanged| for platform interop. Note that |PhaseBegan| usually has non-zero delta, so we want to keep them. BUG=553732 ==========
chongz@chromium.org changed reviewers: + rbyers@chromium.org
rbyers@ PTAL at this Mac touch pad wheel event issue, thanks!
LGTM Note that there's some chance that some site/library has figured this behavior out and is depending on it (it can be useful to know when the fingers have lifted). Does Safari on MacOS have these events too? Regardless I agree it's not worth having Mac-specific behavior. But once we know phase information on Windows (when it's available from the OS) we may want to expose it via an explicit API.
Description was changed from ========== [Mac] Don't dispatch wheel events for PhaseMayBegin|Cancelled|Ended Mac will dispatch wheel events with |PlatformWheelEventPhase|, e.g. 1. |PhaseMayBegin| -> |PhaseCancelled| for two finger touch and lift 2. |PhaseMayBegin| -> |PhaseBegan| -> |PhaseChanged|* -> |PhaseEnded| for two finger scroll 3. |PhaseNone| for real mouse wheel scroll Other platforms don't have phase info and only have events similar to |PhaseBegan| and |PhaseChanged|. This CL filters those extra events and only dispatch |PhaseBegan| and |PhaseChanged| for platform interop. Note that |PhaseBegan| usually has non-zero delta, so we want to keep them. BUG=553732 ========== to ========== [Mac] Don't dispatch wheel events for PhaseMayBegin|Cancelled|Ended Mac will dispatch wheel events with |PlatformWheelEventPhase|, e.g. 1. |PhaseMayBegin| -> |PhaseCancelled| for two finger touch and lift 2. |PhaseMayBegin| -> |PhaseBegan| -> |PhaseChanged|* -> |PhaseEnded| for two finger scroll 3. |PhaseNone| for real mouse wheel scroll Other platforms don't have phase info and only have events similar to |PhaseBegan| and |PhaseChanged|. This CL filters those extra events and only dispatch |PhaseBegan| and |PhaseChanged| for platform interop. Note that |PhaseBegan| usually has non-zero delta, so we want to keep them. Interop: Firefox and Safari doesn't fire events for these phases, so we will match them after CL. BUG=553732 ==========
On 2016/07/04 20:13:37, Rick Byers wrote: > LGTM > > Note that there's some chance that some site/library has figured this behavior > out and is depending on it (it can be useful to know when the fingers have > lifted). Does Safari on MacOS have these events too? Regardless I agree it's Sorry I forgot to add to CL description: Firefox and Safari doesn't fire these events, so we are matching them after CL. (I'm not sure about macOS Sierra though) > not worth having Mac-specific behavior. But once we know phase information on > Windows (when it's available from the OS) we may want to expose it via an > explicit API. Yep we can expose them after we have the spec.
On 2016/07/04 20:35:32, chongz wrote: > On 2016/07/04 20:13:37, Rick Byers wrote: > > LGTM > > > > Note that there's some chance that some site/library has figured this behavior > > out and is depending on it (it can be useful to know when the fingers have > > lifted). Does Safari on MacOS have these events too? Regardless I agree it's > > Sorry I forgot to add to CL description: Firefox and Safari doesn't fire these > events, so we are matching them after CL. > (I'm not sure about macOS Sierra though) Great! Even more reason to fix this (and treat it as just a bug, not a web-platform facing change). > > not worth having Mac-specific behavior. But once we know phase information on > > Windows (when it's available from the OS) we may want to expose it via an > > explicit API. > > Yep we can expose them after we have the spec.
The CQ bit was checked by chongz@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== [Mac] Don't dispatch wheel events for PhaseMayBegin|Cancelled|Ended Mac will dispatch wheel events with |PlatformWheelEventPhase|, e.g. 1. |PhaseMayBegin| -> |PhaseCancelled| for two finger touch and lift 2. |PhaseMayBegin| -> |PhaseBegan| -> |PhaseChanged|* -> |PhaseEnded| for two finger scroll 3. |PhaseNone| for real mouse wheel scroll Other platforms don't have phase info and only have events similar to |PhaseBegan| and |PhaseChanged|. This CL filters those extra events and only dispatch |PhaseBegan| and |PhaseChanged| for platform interop. Note that |PhaseBegan| usually has non-zero delta, so we want to keep them. Interop: Firefox and Safari doesn't fire events for these phases, so we will match them after CL. BUG=553732 ========== to ========== [Mac] Don't dispatch wheel events for PhaseMayBegin|Cancelled|Ended Mac will dispatch wheel events with |PlatformWheelEventPhase|, e.g. 1. |PhaseMayBegin| -> |PhaseCancelled| for two finger touch and lift 2. |PhaseMayBegin| -> |PhaseBegan| -> |PhaseChanged|* -> |PhaseEnded| for two finger scroll 3. |PhaseNone| for real mouse wheel scroll Other platforms don't have phase info and only have events similar to |PhaseBegan| and |PhaseChanged|. This CL filters those extra events and only dispatch |PhaseBegan| and |PhaseChanged| for platform interop. Note that |PhaseBegan| usually has non-zero delta, so we want to keep them. Interop: Firefox and Safari doesn't fire events for these phases, so we will match them after CL. BUG=553732 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:370001)
Message was sent while issue was closed.
Description was changed from ========== [Mac] Don't dispatch wheel events for PhaseMayBegin|Cancelled|Ended Mac will dispatch wheel events with |PlatformWheelEventPhase|, e.g. 1. |PhaseMayBegin| -> |PhaseCancelled| for two finger touch and lift 2. |PhaseMayBegin| -> |PhaseBegan| -> |PhaseChanged|* -> |PhaseEnded| for two finger scroll 3. |PhaseNone| for real mouse wheel scroll Other platforms don't have phase info and only have events similar to |PhaseBegan| and |PhaseChanged|. This CL filters those extra events and only dispatch |PhaseBegan| and |PhaseChanged| for platform interop. Note that |PhaseBegan| usually has non-zero delta, so we want to keep them. Interop: Firefox and Safari doesn't fire events for these phases, so we will match them after CL. BUG=553732 ========== to ========== [Mac] Don't dispatch wheel events for PhaseMayBegin|Cancelled|Ended Mac will dispatch wheel events with |PlatformWheelEventPhase|, e.g. 1. |PhaseMayBegin| -> |PhaseCancelled| for two finger touch and lift 2. |PhaseMayBegin| -> |PhaseBegan| -> |PhaseChanged|* -> |PhaseEnded| for two finger scroll 3. |PhaseNone| for real mouse wheel scroll Other platforms don't have phase info and only have events similar to |PhaseBegan| and |PhaseChanged|. This CL filters those extra events and only dispatch |PhaseBegan| and |PhaseChanged| for platform interop. Note that |PhaseBegan| usually has non-zero delta, so we want to keep them. Interop: Firefox and Safari doesn't fire events for these phases, so we will match them after CL. BUG=553732 Committed: https://crrev.com/b92d711668477cf2fdbfc961bbefa61d9c7c2810 Cr-Commit-Position: refs/heads/master@{#403719} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/b92d711668477cf2fdbfc961bbefa61d9c7c2810 Cr-Commit-Position: refs/heads/master@{#403719} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
