Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2022843002:
Identity the mouse pointer type from low-level events for Mac (Closed)
|
Created:
4 years, 6 months ago by lanwei Modified:
4 years, 6 months ago CC:
chromium-reviews, yusukes+watch_chromium.org, shuchen+watch_chromium.org, jam, dtapuska+chromiumwatch_chromium.org, nona+watch_chromium.org, darin-cc_chromium.org, James Su Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionWe did not set the pointer type for WebMouseEvent, which is created from NSEvent in
WebMouseEventBuilder::Build. Now we set the pointer type based on the NSEvent's
type to see if it is a tablet event or subtype of a mouse event to see its input device.
Reference:
https://developer.apple.com/library/mac/documentation/Cocoa/Conceptual/EventOverview/EventObjectsTypes/EventObjectsTypes.html#//apple_ref/doc/uid/10000060i-CH4-SW4
BUG=615122
Committed: https://crrev.com/74a646d254f3836d7df802857b5a926adc4ec87b
Cr-Commit-Position: refs/heads/master@{#398635}
Patch Set 1 #
Total comments: 8
Patch Set 2 : Set stylus count and set id and pressure as well #
Total comments: 4
Patch Set 3 : Rename variable and reword comments #Patch Set 4 : Set pointer type to unknow for mouse enter and exit #Messages
Total messages: 26 (9 generated)
Description was changed from ========== mac pointer type BUG=615122 ========== to ========== We did not set the pointer type for WebMouseEvent, which is created from NSEvent in WebMouseEventBuilder::Build. Now we set the pointer type based on the NSEvent's type to see if it is a tablet event or subtype of a mouse event to see its input device. Reference: https://developer.apple.com/library/mac/documentation/Cocoa/Conceptual/EventO... BUG=615122 ==========
lanwei@chromium.org changed reviewers: + mustaq@chromium.org, nzolghadr@chromium.org, tdresser@chromium.org
Description was changed from ========== We did not set the pointer type for WebMouseEvent, which is created from NSEvent in WebMouseEventBuilder::Build. Now we set the pointer type based on the NSEvent's type to see if it is a tablet event or subtype of a mouse event to see its input device. Reference: https://developer.apple.com/library/mac/documentation/Cocoa/Conceptual/EventO... BUG=615122 ========== to ========== We did not set the pointer type for WebMouseEvent, which is created from NSEvent in WebMouseEventBuilder::Build. Now we set the pointer type based on the NSEvent's type to see if it is a tablet event or subtype of a mouse event to see its input device. Reference: https://developer.apple.com/library/mac/documentation/Cocoa/Conceptual/EventO... BUG=615122 ==========
https://codereview.chromium.org/2022843002/diff/1/content/browser/renderer_ho... File content/browser/renderer_host/render_widget_host_view_mac.mm (right): https://codereview.chromium.org/2022843002/diff/1/content/browser/renderer_ho... content/browser/renderer_host/render_widget_host_view_mac.mm:1994: // the pointer type should be pen, otherwise is mouse. Just a quick question for clarification. Does this comment implies that we are getting an event when the pen enters/leaves the range of the digitizer? What event type do we send to Blink in that case?
https://codereview.chromium.org/2022843002/diff/1/content/browser/renderer_ho... File content/browser/renderer_host/render_widget_host_view_mac.h (right): https://codereview.chromium.org/2022843002/diff/1/content/browser/renderer_ho... content/browser/renderer_host/render_widget_host_view_mac.h:189: BOOL stylusEnteringProximity_; Should this be stylusWithinProximity? With this patch, if a hold a stylus near the screen, and move my mouse around, we'll think it's a stylus, correct? We should avoid that if possible.
dtapuska@chromium.org changed reviewers: + dtapuska@chromium.org
https://codereview.chromium.org/2022843002/diff/1/content/browser/renderer_ho... File content/browser/renderer_host/render_widget_host_view_mac.mm (right): https://codereview.chromium.org/2022843002/diff/1/content/browser/renderer_ho... content/browser/renderer_host/render_widget_host_view_mac.mm:1996: stylusEnteringProximity_ Can we not always just use the subtype? It looks like you fill the type in always except for mouse enter/exit. Is it a problem with these events that mouse enter and exit don't have a subtype? Perhaps we should be listening to tabletPoint and tabletProximity events and generate appropriate events.
https://chromiumcodereview.appspot.com/2022843002/diff/1/content/browser/rend... File content/browser/renderer_host/render_widget_host_view_mac.h (right): https://chromiumcodereview.appspot.com/2022843002/diff/1/content/browser/rend... content/browser/renderer_host/render_widget_host_view_mac.h:189: BOOL stylusEnteringProximity_; On 2016/05/31 13:05:34, tdresser wrote: > Should this be stylusWithinProximity? > > With this patch, if a hold a stylus near the screen, and move my mouse around, > we'll think it's a stylus, correct? > > We should avoid that if possible. You are right, I should add some checks to the mouse events. https://chromiumcodereview.appspot.com/2022843002/diff/1/content/browser/rend... File content/browser/renderer_host/render_widget_host_view_mac.mm (right): https://chromiumcodereview.appspot.com/2022843002/diff/1/content/browser/rend... content/browser/renderer_host/render_widget_host_view_mac.mm:1994: // the pointer type should be pen, otherwise is mouse. On 2016/05/31 04:18:12, Navid Zolghadr wrote: > Just a quick question for clarification. Does this comment implies that we are > getting an event when the pen enters/leaves the range of the digitizer? What > event type do we send to Blink in that case? Even without actually touching the tablet screen, first a NSTabletProximity event, then MouseEnter, MouseMove events which have a subtype of NSTabletPointEventSubtype. MouseEnter and MouseExited do not have subtype. https://chromiumcodereview.appspot.com/2022843002/diff/1/content/browser/rend... content/browser/renderer_host/render_widget_host_view_mac.mm:1996: stylusEnteringProximity_ On 2016/05/31 13:31:05, dtapuska wrote: > Can we not always just use the subtype? It looks like you fill the type in > always except for mouse enter/exit. > > Is it a problem with these events that mouse enter and exit don't have a > subtype? > > Perhaps we should be listening to tabletPoint and tabletProximity events and > generate appropriate events. Sorry, I forgot to add the comment that MouseEnter and MouseExited do not have subtype. "Some event types might have subtypes, ..." They mentioned this in the doc I attached to this CL, and I tested, only MouseEnter and MouseExited do not have subtype. I am not sure why they design this way. If all mouse events have subtypes then we can easily decide what are stylus events, but now we have to use TabletProximity as well to decide the pointer types for MouseEnter and MouseExited events. I have not tried to listen to tabletPoint, " A pointer event is an NSEvent object of type NSTabletPoint or an object representing a mouse-down, mouse-dragged, or mouse-up event with a subtype of NSTabletPointEventSubtype." Maybe for stylus, we should listen to both NSTabletPoint and mouse events?
https://codereview.chromium.org/2022843002/diff/1/content/browser/renderer_ho... File content/browser/renderer_host/render_widget_host_view_mac.mm (right): https://codereview.chromium.org/2022843002/diff/1/content/browser/renderer_ho... content/browser/renderer_host/render_widget_host_view_mac.mm:1994: // the pointer type should be pen, otherwise is mouse. On 2016/05/31 14:33:35, lanwei wrote: > On 2016/05/31 04:18:12, Navid Zolghadr wrote: > > Just a quick question for clarification. Does this comment implies that we are > > getting an event when the pen enters/leaves the range of the digitizer? What > > event type do we send to Blink in that case? > > Even without actually touching the tablet screen, first a NSTabletProximity > event, then MouseEnter, MouseMove events which have a subtype of > NSTabletPointEventSubtype. MouseEnter and MouseExited do not have subtype. Navid: Blink gets move & leave respectively, defined in web_input_event_builders_mac.mm. That means the w3c test pointerevent_pointerleave_pen-manual should pass (before/after this CL, depending on dependece on pointerType) for mac, right? https://codereview.chromium.org/2022843002/diff/1/content/browser/renderer_ho... content/browser/renderer_host/render_widget_host_view_mac.mm:1996: stylusEnteringProximity_ On 2016/05/31 14:33:35, lanwei wrote: > On 2016/05/31 13:31:05, dtapuska wrote: > > Can we not always just use the subtype? It looks like you fill the type in > > always except for mouse enter/exit. > > > > Is it a problem with these events that mouse enter and exit don't have a > > subtype? > > > > Perhaps we should be listening to tabletPoint and tabletProximity events and > > generate appropriate events. > > Sorry, I forgot to add the comment that MouseEnter and MouseExited do not have > subtype. "Some event types might have subtypes, ..." They mentioned this in the > doc I attached to this CL, and I tested, only MouseEnter and MouseExited do not > have subtype. I am not sure why they design this way. > > If all mouse events have subtypes then we can easily decide what are stylus > events, but now we have to use TabletProximity as well to decide the pointer > types for MouseEnter and MouseExited events. > > I have not tried to listen to tabletPoint, " A pointer event is an NSEvent > object of type NSTabletPoint or an object representing a mouse-down, > mouse-dragged, or mouse-up event with a subtype of NSTabletPointEventSubtype." > Maybe for stylus, we should listen to both NSTabletPoint and mouse events? > The following link seems to suggest that subtype is "valid for mouse events", so MouseEnter & MouseExited not having this seems to contradict the spec. It would be great to have a second stylus device, this cold be device or driver specific problem. https://developer.apple.com/library/mac/documentation/Cocoa/Reference/Applica...
LGTM modulo a bit clearer var names & comments... https://codereview.chromium.org/2022843002/diff/20001/content/browser/rendere... File content/browser/renderer_host/input/web_input_event_builders_mac.mm (right): https://codereview.chromium.org/2022843002/diff/20001/content/browser/rendere... content/browser/renderer_host/input/web_input_event_builders_mac.mm:669: blink::WebPointerProperties::PointerType pointerType) { Please rename the |pointerType| param to |pointerTypeForEnterLeave| to make it clear that this is used in a special case. Then move the comment on Line 736 to above this function. https://codereview.chromium.org/2022843002/diff/20001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_view_mac.mm (right): https://codereview.chromium.org/2022843002/diff/20001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_mac.mm:1996: blink::WebPointerProperties::PointerType pointerType = As in the other file, rename |pointerType| to |pointerTypeForEnterLeave|, then clarify the comment: s/should be pen/assumed to be pen/.
https://codereview.chromium.org/2022843002/diff/20001/content/browser/rendere... File content/browser/renderer_host/input/web_input_event_builders_mac.mm (right): https://codereview.chromium.org/2022843002/diff/20001/content/browser/rendere... content/browser/renderer_host/input/web_input_event_builders_mac.mm:669: blink::WebPointerProperties::PointerType pointerType) { On 2016/06/02 15:39:12, mustaq wrote: > Please rename the |pointerType| param to |pointerTypeForEnterLeave| to make it > clear that this is used in a special case. Then move the comment on Line 736 to > above this function. Done. https://codereview.chromium.org/2022843002/diff/20001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_view_mac.mm (right): https://codereview.chromium.org/2022843002/diff/20001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_mac.mm:1996: blink::WebPointerProperties::PointerType pointerType = On 2016/06/02 15:39:12, mustaq wrote: > As in the other file, rename |pointerType| to |pointerTypeForEnterLeave|, then > clarify the comment: s/should be pen/assumed to be pen/. Done.
On 2016/06/03 14:13:15, lanwei wrote: > https://codereview.chromium.org/2022843002/diff/20001/content/browser/rendere... > File content/browser/renderer_host/input/web_input_event_builders_mac.mm > (right): > > https://codereview.chromium.org/2022843002/diff/20001/content/browser/rendere... > content/browser/renderer_host/input/web_input_event_builders_mac.mm:669: > blink::WebPointerProperties::PointerType pointerType) { > On 2016/06/02 15:39:12, mustaq wrote: > > Please rename the |pointerType| param to |pointerTypeForEnterLeave| to make it > > clear that this is used in a special case. Then move the comment on Line 736 > to > > above this function. > > Done. > > https://codereview.chromium.org/2022843002/diff/20001/content/browser/rendere... > File content/browser/renderer_host/render_widget_host_view_mac.mm (right): > > https://codereview.chromium.org/2022843002/diff/20001/content/browser/rendere... > content/browser/renderer_host/render_widget_host_view_mac.mm:1996: > blink::WebPointerProperties::PointerType pointerType = > On 2016/06/02 15:39:12, mustaq wrote: > > As in the other file, rename |pointerType| to |pointerTypeForEnterLeave|, then > > clarify the comment: s/should be pen/assumed to be pen/. > > Done. Thanks, still lgtm.
Patchset #4 (id:60001) has been deleted
LGTM, thanks.
LGTM
On 2016/06/08 14:36:53, Navid Zolghadr wrote: > LGTM lgtm
The CQ bit was checked by lanwei@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mustaq@chromium.org Link to the patchset: https://codereview.chromium.org/2022843002/#ps80001 (title: "Set pointer type to unknow for mouse enter and exit")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2022843002/80001
Message was sent while issue was closed.
Description was changed from ========== We did not set the pointer type for WebMouseEvent, which is created from NSEvent in WebMouseEventBuilder::Build. Now we set the pointer type based on the NSEvent's type to see if it is a tablet event or subtype of a mouse event to see its input device. Reference: https://developer.apple.com/library/mac/documentation/Cocoa/Conceptual/EventO... BUG=615122 ========== to ========== We did not set the pointer type for WebMouseEvent, which is created from NSEvent in WebMouseEventBuilder::Build. Now we set the pointer type based on the NSEvent's type to see if it is a tablet event or subtype of a mouse event to see its input device. Reference: https://developer.apple.com/library/mac/documentation/Cocoa/Conceptual/EventO... BUG=615122 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== We did not set the pointer type for WebMouseEvent, which is created from NSEvent in WebMouseEventBuilder::Build. Now we set the pointer type based on the NSEvent's type to see if it is a tablet event or subtype of a mouse event to see its input device. Reference: https://developer.apple.com/library/mac/documentation/Cocoa/Conceptual/EventO... BUG=615122 ========== to ========== We did not set the pointer type for WebMouseEvent, which is created from NSEvent in WebMouseEventBuilder::Build. Now we set the pointer type based on the NSEvent's type to see if it is a tablet event or subtype of a mouse event to see its input device. Reference: https://developer.apple.com/library/mac/documentation/Cocoa/Conceptual/EventO... BUG=615122 Committed: https://crrev.com/74a646d254f3836d7df802857b5a926adc4ec87b Cr-Commit-Position: refs/heads/master@{#398635} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/74a646d254f3836d7df802857b5a926adc4ec87b Cr-Commit-Position: refs/heads/master@{#398635} |
