|
|
Created:
4 years, 1 month ago by denniskempin Modified:
4 years ago CC:
chromium-reviews, mlamouri+watch-content_chromium.org, dtapuska+blinkwatch_chromium.org, tdresser+watch_chromium.org, eae+blinkwatch, sof, jam, blink-reviews-dom_chromium.org, dglazkov+blink, darin-cc_chromium.org, blink-reviews, kalyank, kinuko+watch, ozone-reviews_chromium.org, rwlbuis, jdufault, xiaoyinh(OOO Sep 11-29) Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionUse touch events to report stylus events
UI review has decided that a stylus should behave like touch and not like
mouse. e.g. It should scroll when dragging, not select text.
This change will switch stylus events back to use TouchEvents instead
of MouseEvents.
Besides adjusting the evdev converter to report TouchEvents, this change
also updates the routing of touch events to carry along pointer type
information.
BUG=665499
TEST=Test with rbyers eventTest and paint app
Committed: https://crrev.com/1f1f36a66ac46e957542f51e3c74bfcae70a5f36
Cr-Commit-Position: refs/heads/master@{#435479}
Patch Set 1 #
Total comments: 4
Patch Set 2 : fixed unit test #
Total comments: 4
Patch Set 3 : only pass pointer type through plugin api #Patch Set 4 : fixed conversion of pointer type for touch end events #Patch Set 5 : fixed nit #
Total comments: 1
Patch Set 6 : fixed nits #Patch Set 7 : Added check for pointerType in WebInputEventConversionTest #Patch Set 8 : fixed nit #Patch Set 9 : I should pay attention... #Messages
Total messages: 39 (15 generated)
denniskempin@google.com changed reviewers: + bradnelson@chromium.org, esprehn@chromium.org, sadrul@chromium.org, spang@chromium.org
Hi guys! please have a look at this change and let me know what you think. Elliott, I'll copy you on the email thread with details on the context. Thanks!
nzolghadr@chromium.org changed reviewers: + nzolghadr@chromium.org
https://codereview.chromium.org/2507503002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/dom/Touch.cpp (right): https://codereview.chromium.org/2507503002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/dom/Touch.cpp:59: m_pointerProperties(properties) { It seems that everytime we create a WebTouchPoint (using toWebTouchPoint) from these we also have a TouchEvent object which will have bunch of these Touch objects. Presumably all these Touch objects within a TouchEvent will have the same pointerType. So can we just store that type in the TouchEvent class instead of storing all those extra properties of WebPointerProperties in each and every Touch object? https://codereview.chromium.org/2507503002/diff/1/third_party/WebKit/Source/w... File third_party/WebKit/Source/web/WebInputEventConversion.cpp (right): https://codereview.chromium.org/2507503002/diff/1/third_party/WebKit/Source/w... third_party/WebKit/Source/web/WebInputEventConversion.cpp:683: point.pointerType = WebPointerProperties::PointerType::Touch; So I guess this line will be redundant? Or is it okay to move the new line over here as id and pointerType are sometimes used together later on. Also are you sure that in every path that a Touch object is created it has a pointerType set? Otherwise at this point WebTouchPoint will have PointerType::Unknown type instead of Touch (or the new pen type).
On 2016/11/15 18:53:38, Navid Zolghadr wrote: > https://codereview.chromium.org/2507503002/diff/1/third_party/WebKit/Source/c... > File third_party/WebKit/Source/core/dom/Touch.cpp (right): > > https://codereview.chromium.org/2507503002/diff/1/third_party/WebKit/Source/c... > third_party/WebKit/Source/core/dom/Touch.cpp:59: m_pointerProperties(properties) > { > It seems that everytime we create a WebTouchPoint (using toWebTouchPoint) from > these we also have a TouchEvent object which will have bunch of these Touch > objects. Presumably all these Touch objects within a TouchEvent will have the > same pointerType. So can we just store that type in the TouchEvent class instead > of storing all those extra properties of WebPointerProperties in each and every > Touch object? Sounds good to me. Theoretically there could be a device having both pen and touch pointer types at the same time. But no such device exists right now, so I'm ok with this. > > https://codereview.chromium.org/2507503002/diff/1/third_party/WebKit/Source/w... > File third_party/WebKit/Source/web/WebInputEventConversion.cpp (right): > > https://codereview.chromium.org/2507503002/diff/1/third_party/WebKit/Source/w... > third_party/WebKit/Source/web/WebInputEventConversion.cpp:683: point.pointerType > = WebPointerProperties::PointerType::Touch; > So I guess this line will be redundant? Or is it okay to move the new line over > here as id and pointerType are sometimes used together later on. > Also are you sure that in every path that a Touch object is created it has a > pointerType set? Otherwise at this point WebTouchPoint will have > PointerType::Unknown type instead of Touch (or the new pen type). I think Unknown would be more appropriate since it would correctly describe the pointer, right? I wouldn't mind defaulting to touch if you think that's a better solution.
Please write some logs based tests. Something is really wrong if you can make a major like change like this without changing events_unittests. https://codereview.chromium.org/2507503002/diff/1/ui/events/ozone/evdev/touch... File ui/events/ozone/evdev/touch_event_converter_evdev.cc (right): https://codereview.chromium.org/2507503002/diff/1/ui/events/ozone/evdev/touch... ui/events/ozone/evdev/touch_event_converter_evdev.cc:457: // todo(denniskempin): Report buttons via pointer events All caps - TODO() https://codereview.chromium.org/2507503002/diff/1/ui/events/ozone/evdev/touch... ui/events/ozone/evdev/touch_event_converter_evdev.cc:481: ReportButton(BTN_LEFT, event->btn_left.down, *event, timestamp); This doesn't seem right to me, even considering the TODO. Touches don't have left clicks. Can you just remove all of these no-ops?
On 2016/11/17 18:07:12, denniskempin wrote: > On 2016/11/15 18:53:38, Navid Zolghadr wrote: > > > https://codereview.chromium.org/2507503002/diff/1/third_party/WebKit/Source/c... > > File third_party/WebKit/Source/core/dom/Touch.cpp (right): > > > > > https://codereview.chromium.org/2507503002/diff/1/third_party/WebKit/Source/c... > > third_party/WebKit/Source/core/dom/Touch.cpp:59: > m_pointerProperties(properties) > > { > > It seems that everytime we create a WebTouchPoint (using toWebTouchPoint) from > > these we also have a TouchEvent object which will have bunch of these Touch > > objects. Presumably all these Touch objects within a TouchEvent will have the > > same pointerType. So can we just store that type in the TouchEvent class > instead > > of storing all those extra properties of WebPointerProperties in each and > every > > Touch object? > > Sounds good to me. Theoretically there could be a device having both pen and > touch pointer types > at the same time. But no such device exists right now, so I'm ok with this. > > > > > > https://codereview.chromium.org/2507503002/diff/1/third_party/WebKit/Source/w... > > File third_party/WebKit/Source/web/WebInputEventConversion.cpp (right): > > > > > https://codereview.chromium.org/2507503002/diff/1/third_party/WebKit/Source/w... > > third_party/WebKit/Source/web/WebInputEventConversion.cpp:683: > point.pointerType > > = WebPointerProperties::PointerType::Touch; > > So I guess this line will be redundant? Or is it okay to move the new line > over > > here as id and pointerType are sometimes used together later on. > > Also are you sure that in every path that a Touch object is created it has a > > pointerType set? Otherwise at this point WebTouchPoint will have > > PointerType::Unknown type instead of Touch (or the new pen type). > > I think Unknown would be more appropriate since it would correctly describe the > pointer, right? I wouldn't mind defaulting to touch if you think that's a better > solution. As long as every place that it was originated from a real touch it is initialized with PointerType::Touch it should be okay.
mustaq@chromium.org changed reviewers: + mustaq@chromium.org
Sorry Dennis, I missed reviewing your CL yesterday as I had planned. Could you please elaborate on the motivation a bit? What's the longer term plan? May be you already have it (design doc?), I just couldn't find it. The bug says nothing :( The reason I am looking for a plan is that TouchEvent seems inherently incapable of representing stylus: there is no notion of a hovering touch, so it's not clear how we could handle hovering stylus moves easily. May be we don't have a hovering h/w in this platform now but I believe it will be there in near future given that many Android phones already support it. On a related note, stylus events in Android also follows touch event path but /mostly/---there is special route only to dispatch hovering moves, as mouse events! I am sure we won't want to add the same hack for another platform. https://codereview.chromium.org/2507503002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/dom/Touch.h (right): https://codereview.chromium.org/2507503002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/Touch.h:112: const WebPointerProperties&); I agree with Navid here: it's better to remove the repeated redundant fields (id, pointerType) from here to the "enclosing class" Source/core/events/TouchEvent.h. May be even all the fields because in the stylus case a TouchEvent instance will contain only a single Touch instance. Multiple Touch instances in TouchEvent are for a multi-touch. https://codereview.chromium.org/2507503002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/web/WebInputEventConversion.cpp (right): https://codereview.chromium.org/2507503002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/web/WebInputEventConversion.cpp:693: point.pointerType = touch->pointerProperties().pointerType; Need to set point.button from pointerProperties().
On 2016/11/17 21:24:57, mustaq wrote: > Sorry Dennis, I missed reviewing your CL yesterday as I had planned. > > Could you please elaborate on the motivation a bit? What's the > longer term plan? May be you already have it (design doc?), I just > couldn't find it. The bug says nothing :( > > The reason I am looking for a plan is that TouchEvent seems > inherently incapable of representing stylus: there is no notion of a > hovering touch, so it's not clear how we could handle hovering stylus > moves easily. May be we don't have a hovering h/w in this platform > now but I believe it will be there in near future given that many > Android phones already support it. UI review made the decision that a stylus should behave like touch in pretty much every case. Not being able to support hover is an unfortunate side-effect of using TouchEvents. Folks on the UI side explicitly do not want to react to hover, so that's a drawback we can live with. Eventually we want apps to consume hover events via APIs, but that's not as important. > On a related note, stylus events in Android also follows touch event > path but /mostly/---there is special route only to dispatch hovering > moves, as mouse events! I am sure we won't want to add the same > hack for another platform. This might be a temporary solution. Ideally we would just generate PointerEvents from the beginning, but I am afraid that this will have a huge rats tail of work to handle PointerEvents at necessary event handlers all across Chrome UI / Blink / etc. > > https://codereview.chromium.org/2507503002/diff/20001/third_party/WebKit/Sour... > File third_party/WebKit/Source/core/dom/Touch.h (right): > > https://codereview.chromium.org/2507503002/diff/20001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/dom/Touch.h:112: const WebPointerProperties&); > I agree with Navid here: it's better to remove the repeated redundant > fields (id, pointerType) from here to the "enclosing class" > Source/core/events/TouchEvent.h. May be even all the fields because > in the stylus case a TouchEvent instance will contain only a single > Touch instance. Multiple Touch instances in TouchEvent are for a > multi-touch. > > https://codereview.chromium.org/2507503002/diff/20001/third_party/WebKit/Sour... > File third_party/WebKit/Source/web/WebInputEventConversion.cpp (right): > > https://codereview.chromium.org/2507503002/diff/20001/third_party/WebKit/Sour... > third_party/WebKit/Source/web/WebInputEventConversion.cpp:693: point.pointerType > = touch->pointerProperties().pointerType; > Need to set point.button from pointerProperties().
https://codereview.chromium.org/2507503002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/dom/Touch.h (right): https://codereview.chromium.org/2507503002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/Touch.h:112: const WebPointerProperties&); On 2016/11/17 21:24:57, mustaq wrote: > I agree with Navid here: it's better to remove the repeated redundant > fields (id, pointerType) from here to the "enclosing class" > Source/core/events/TouchEvent.h. May be even all the fields because > in the stylus case a TouchEvent instance will contain only a single > Touch instance. Multiple Touch instances in TouchEvent are for a > multi-touch. Besides not having hardware that supports this, there is not really anything preventing us from having multiple styli or stylus combined with touch in a single event. Which would be represented very well by the PointerEvents API. If we stay closer to that the transition to PointerEvents is probably going to be easier. How about just adding a single field pointer type to Touch.h? That would get rid of redundant fields. I don't feel strongly about this, if you would prefer to add that to just TouchEvent.h and have it implicitly applied to all enclosed touches (which would be just one) I am ok that. Let me know. https://codereview.chromium.org/2507503002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/web/WebInputEventConversion.cpp (right): https://codereview.chromium.org/2507503002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/web/WebInputEventConversion.cpp:693: point.pointerType = touch->pointerProperties().pointerType; On 2016/11/17 21:24:57, mustaq wrote: > Need to set point.button from pointerProperties(). Done.
I understand it is meant to be a temporary change but I am not convinced that we can get rid of it quickly because a WebPointerEvent is not something we will have anytime soon. I am personally passionate about it but don't expect to reach the finish line in a quarter or two because of both complication & being low in priority. This uncertainty in timeline makes your little change here on the Blink side look questionable: we won't want people to start using the extra fields in dom Touch event but we can't guarantee it in our open code. I tried to locate the "users" of the changes in dom Touch event here. Apparently only plugins would rely on this but I couldn't find any plugins that could potentially benefit from stylus being touch-or-mouse-like in our internal plumbing. Is there any specific plugin that relies on stylus specific properties (pressure, tilt etc) as opposed to just its mouse-like properties (coordinates, clicks)? Another thing: stylus behavior is already "split" among platforms, even outside plugins: a stylus drag selects text in Windows (mouse-like) but scrolls in Android (touch-like). I can't see why a plugin would care about this distinction, and why current (before your change) pepper/event_conversion.cc cares so much about it. I am not an expert in plugins, just wanted to find a concrete use-case that would justify such a change in Blink. I failed to find any, sorry. Please correct me if I missed any non-plugin use case (for the changes in dom Touch event).
We need stylus events in PPAPI, and I've been trying to follow the flow of events to get the pointer type information to PPAPI. It seems to be a plugin and go through the TouchEvent class so I have been trying to pass the information through. If there are other ways to pass this information please let me know. I'm still a little confused with the different event types withing chrome so maybe I missed a different path. Other events of that plugin API (Mouse and Keyboard) optionally carry the underlying PlatformEvent that was used to create the event. This would be another option we can go with. On Mon, Nov 28, 2016, 1:13 PM <mustaq@chromium.org> wrote: > I understand it is meant to be a temporary change but I am not convinced > that we > can get rid of it quickly because a WebPointerEvent is not something we > will > have anytime soon. I am personally passionate about it but don't expect to > reach > the finish line in a quarter or two because of both complication & being > low in > priority. This uncertainty in timeline makes your little change here on the > Blink side look questionable: we won't want people to start using the extra > fields in dom Touch event but we can't guarantee it in our open code. > > I tried to locate the "users" of the changes in dom Touch event here. > Apparently > only plugins would rely on this but I couldn't find any plugins that could > potentially benefit from stylus being touch-or-mouse-like in our internal > plumbing. Is there any specific plugin that relies on stylus specific > properties > (pressure, tilt etc) as opposed to just its mouse-like properties > (coordinates, > clicks)? > > Another thing: stylus behavior is already "split" among platforms, even > outside > plugins: a stylus drag selects text in Windows (mouse-like) but scrolls in > Android (touch-like). I can't see why a plugin would care about this > distinction, and why current (before your change) > pepper/event_conversion.cc > cares so much about it. > > I am not an expert in plugins, just wanted to find a concrete use-case that > would justify such a change in Blink. I failed to find any, sorry. > > Please correct me if I missed any non-plugin use case (for the changes in > dom > Touch event). > > > https://codereview.chromium.org/2507503002/ > -- You received this message because you are subscribed to the Google Groups "Blink Reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to blink-reviews+unsubscribe@chromium.org.
We need stylus events in PPAPI, and I've been trying to follow the flow of events to get the pointer type information to PPAPI. It seems to be a plugin and go through the TouchEvent class so I have been trying to pass the information through. If there are other ways to pass this information please let me know. I'm still a little confused with the different event types withing chrome so maybe I missed a different path. Other events of that plugin API (Mouse and Keyboard) optionally carry the underlying PlatformEvent that was used to create the event. This would be another option we can go with. On Mon, Nov 28, 2016, 1:13 PM <mustaq@chromium.org> wrote: > I understand it is meant to be a temporary change but I am not convinced > that we > can get rid of it quickly because a WebPointerEvent is not something we > will > have anytime soon. I am personally passionate about it but don't expect to > reach > the finish line in a quarter or two because of both complication & being > low in > priority. This uncertainty in timeline makes your little change here on the > Blink side look questionable: we won't want people to start using the extra > fields in dom Touch event but we can't guarantee it in our open code. > > I tried to locate the "users" of the changes in dom Touch event here. > Apparently > only plugins would rely on this but I couldn't find any plugins that could > potentially benefit from stylus being touch-or-mouse-like in our internal > plumbing. Is there any specific plugin that relies on stylus specific > properties > (pressure, tilt etc) as opposed to just its mouse-like properties > (coordinates, > clicks)? > > Another thing: stylus behavior is already "split" among platforms, even > outside > plugins: a stylus drag selects text in Windows (mouse-like) but scrolls in > Android (touch-like). I can't see why a plugin would care about this > distinction, and why current (before your change) > pepper/event_conversion.cc > cares so much about it. > > I am not an expert in plugins, just wanted to find a concrete use-case that > would justify such a change in Blink. I failed to find any, sorry. > > Please correct me if I missed any non-plugin use case (for the changes in > dom > Touch event). > > > https://codereview.chromium.org/2507503002/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
After an offline discussion, I realized that the plugin we are considering here only needs to distinguish incoming events from touch vs stylus. Let's do a minimal change in Blink to support this for now (until we have end-to-end pointer events in the long term): add only a pointerType field in dom Touch event. Please also add a comment saying this is an ad-hoc solution, to discourage new usage.
Mustaq, let me know what you think! Michael and Elliott, could you have a look for owner review as well? Thanks!
On 2016/11/30 00:31:36, denniskempin wrote: > Mustaq, let me know what you think! > > Michael and Elliott, could you have a look for owner review as well? Thanks! The changes in WebKit/Source looks reasonable to me. LGTM.
lgtm https://codereview.chromium.org/2507503002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/web/tests/WebInputEventConversionTest.cpp (right): https://codereview.chromium.org/2507503002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/web/tests/WebInputEventConversionTest.cpp:153: WebPointerProperties::PointerType::Touch); nit: I know it is just a simple getter/setter. But that would be good to just test for the value of pointerType at least once in these tests.
lgtm
Can you explain more in the description why this is desirable? The change lgtm, but it'snot clear why we want to do this, and if it needs an intent to ship?
Description was changed from ========== Use touch events to report stylus events This change will switch stylus events back to use TouchEvents instead of MouseEvents. Besides adjusting the evdev converter to report TouchEvents, this change also updates the routing of touch events to carry along pointer type information. BUG=665499 TEST=Test with rbyers eventTest and paint app ========== to ========== Use touch events to report stylus events UI review has decided that a stylus should behave like touch and not like mouse. e.g. It should scroll when dragging, not select text. This change will switch stylus events back to use TouchEvents instead of MouseEvents. Besides adjusting the evdev converter to report TouchEvents, this change also updates the routing of touch events to carry along pointer type information. BUG=665499 TEST=Test with rbyers eventTest and paint app ==========
The CQ bit was checked by denniskempin@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from nzolghadr@chromium.org, mustaq@chromium.org, esprehn@chromium.org, spang@chromium.org Link to the patchset: https://codereview.chromium.org/2507503002/#ps120001 (title: "Added check for pointerType in WebInputEventConversionTest")
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
Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by denniskempin@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from esprehn@chromium.org, nzolghadr@chromium.org, mustaq@chromium.org, spang@chromium.org Link to the patchset: https://codereview.chromium.org/2507503002/#ps140001 (title: "fixed nit")
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
Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by denniskempin@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from esprehn@chromium.org, nzolghadr@chromium.org, mustaq@chromium.org, spang@chromium.org Link to the patchset: https://codereview.chromium.org/2507503002/#ps160001 (title: "I should pay attention...")
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": 160001, "attempt_start_ts": 1480543209213870, "parent_rev": "dae15af742118dfe06aa4fe718dab87a97ebe419", "commit_rev": "1e2e1477bc23a38f337fa4d3a967120f9769f591"}
Message was sent while issue was closed.
Description was changed from ========== Use touch events to report stylus events UI review has decided that a stylus should behave like touch and not like mouse. e.g. It should scroll when dragging, not select text. This change will switch stylus events back to use TouchEvents instead of MouseEvents. Besides adjusting the evdev converter to report TouchEvents, this change also updates the routing of touch events to carry along pointer type information. BUG=665499 TEST=Test with rbyers eventTest and paint app ========== to ========== Use touch events to report stylus events UI review has decided that a stylus should behave like touch and not like mouse. e.g. It should scroll when dragging, not select text. This change will switch stylus events back to use TouchEvents instead of MouseEvents. Besides adjusting the evdev converter to report TouchEvents, this change also updates the routing of touch events to carry along pointer type information. BUG=665499 TEST=Test with rbyers eventTest and paint app ==========
Message was sent while issue was closed.
Committed patchset #9 (id:160001)
Message was sent while issue was closed.
Description was changed from ========== Use touch events to report stylus events UI review has decided that a stylus should behave like touch and not like mouse. e.g. It should scroll when dragging, not select text. This change will switch stylus events back to use TouchEvents instead of MouseEvents. Besides adjusting the evdev converter to report TouchEvents, this change also updates the routing of touch events to carry along pointer type information. BUG=665499 TEST=Test with rbyers eventTest and paint app ========== to ========== Use touch events to report stylus events UI review has decided that a stylus should behave like touch and not like mouse. e.g. It should scroll when dragging, not select text. This change will switch stylus events back to use TouchEvents instead of MouseEvents. Besides adjusting the evdev converter to report TouchEvents, this change also updates the routing of touch events to carry along pointer type information. BUG=665499 TEST=Test with rbyers eventTest and paint app Committed: https://crrev.com/1f1f36a66ac46e957542f51e3c74bfcae70a5f36 Cr-Commit-Position: refs/heads/master@{#435479} ==========
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/1f1f36a66ac46e957542f51e3c74bfcae70a5f36 Cr-Commit-Position: refs/heads/master@{#435479} |