| Summary: | PointerEvent has 0 as pressure when the input device is not pressure-sensitive | ||||||
|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Akihiko Odaki <nekomanma> | ||||
| Component: | UI Events | Assignee: | Nobody <webkit-unassigned> | ||||
| Status: | NEW --- | ||||||
| Severity: | Normal | CC: | cdumez, darin, dbates, esprehn+autocc, ews-watchlist, graouts, kangil.han, thorton, webkit-bug-importer, wenson_hsieh | ||||
| Priority: | P2 | Keywords: | InRadar | ||||
| Version: | Safari 13 | ||||||
| Hardware: | Mac | ||||||
| OS: | macOS 10.15 | ||||||
| Attachments: |
|
||||||
|
Description
Akihiko Odaki
2020-01-14 00:08:59 PST
Created attachment 388210 [details]
Patch
Comment on attachment 388210 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=388210&action=review > Source/WebCore/dom/PointerEvent.cpp:115 > + , m_pressure(mouseEvent.buttons() ? 0.5 : 0) I am not 100% sure this is right. The specification calls for all pointerup events having pressure of 0. Are we guaranteed that buttons() will be 0 for *all* pointerup events? The test case covers only a simple case of a mouse with a single button. > LayoutTests/ChangeLog:11 > + * pointerevents/mouse/pointer-button-and-buttons-expected.txt: Removed. > + * pointerevents/mouse/pointer-button-and-buttons.html: Removed. > + * pointerevents/mouse/pointer-button-buttons-and-pressure-expected.txt: Added. > + * pointerevents/mouse/pointer-button-buttons-and-pressure.html: Added. Why are we renaming this test case? (In reply to Akihiko Odaki from comment #0) > Apparently, WebKit refers to pressure property of NSEvent. The documentation > states: > > For input devices that aren’t pressure-sensitive, the value is either 0.0 or 1.0. > https://developer.apple.com/documentation/appkit/nsevent/1534543-pressure Aside from the bug fixed here, this implies that on *Cocoa* platforms, for non-pressure-sensitive devices, WebKit is setting 1.0 as the pressure, where the standard instead requests/requires that we set it to 0.5. May be challenging to fix if NSEvent does not tell us whether the device is pressure-sensitive. It would be good if someone investigated this and created tests. (In reply to Darin Adler from comment #3) > Comment on attachment 388210 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=388210&action=review > > > Source/WebCore/dom/PointerEvent.cpp:115 > > + , m_pressure(mouseEvent.buttons() ? 0.5 : 0) > > I am not 100% sure this is right. The specification calls for all pointerup > events having pressure of 0. Are we guaranteed that buttons() will be 0 for > *all* pointerup events? The test case covers only a simple case of a mouse > with a single button. Maybe it would be better to use the provided event type to set the pressure value. (In reply to Darin Adler from comment #3) > Comment on attachment 388210 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=388210&action=review > > > Source/WebCore/dom/PointerEvent.cpp:115 > > + , m_pressure(mouseEvent.buttons() ? 0.5 : 0) > > I am not 100% sure this is right. The specification calls for all pointerup > events having pressure of 0. Are we guaranteed that buttons() will be 0 for > *all* pointerup events? The test case covers only a simple case of a mouse > with a single button. Yes, the specification states: > A user agent MUST fire a pointer event named pointerup when a pointer leaves the active buttons state. > > > LayoutTests/ChangeLog:11 > > + * pointerevents/mouse/pointer-button-and-buttons-expected.txt: Removed. > > + * pointerevents/mouse/pointer-button-and-buttons.html: Removed. > > + * pointerevents/mouse/pointer-button-buttons-and-pressure-expected.txt: Added. > > + * pointerevents/mouse/pointer-button-buttons-and-pressure.html: Added. > > Why are we renaming this test case? Because it is now testing pressure property too. The name simply represents tested properties. (In reply to Darin Adler from comment #4) > (In reply to Akihiko Odaki from comment #0) > > Apparently, WebKit refers to pressure property of NSEvent. The documentation > > states: > > > For input devices that aren’t pressure-sensitive, the value is either 0.0 or 1.0. > > https://developer.apple.com/documentation/appkit/nsevent/1534543-pressure > > Aside from the bug fixed here, this implies that on *Cocoa* platforms, for > non-pressure-sensitive devices, WebKit is setting 1.0 as the pressure, where > the standard instead requests/requires that we set it to 0.5. May be > challenging to fix if NSEvent does not tell us whether the device is > pressure-sensitive. It would be good if someone investigated this and > created tests. I wrote that statement before carefully looking at the code and it is misleading. In reality, pressure property of NSEvent is completely ignored because MouseEvent provided to PointerEvent to create a new instance does not have pressure information. (In reply to Antoine Quint from comment #5) > (In reply to Darin Adler from comment #3) > > Comment on attachment 388210 [details] > > Patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=388210&action=review > > > > > Source/WebCore/dom/PointerEvent.cpp:115 > > > + , m_pressure(mouseEvent.buttons() ? 0.5 : 0) > > > > I am not 100% sure this is right. The specification calls for all pointerup > > events having pressure of 0. Are we guaranteed that buttons() will be 0 for > > *all* pointerup events? The test case covers only a simple case of a mouse > > with a single button. > > Maybe it would be better to use the provided event type to set the pressure > value. The specification states it is guaranteed, so I do not think it makes difference. We can still add an assertion, but such an assertion should cover all (including one generated by pressure-sensitive device) PointerEvents and I think it is out of scope of this bug. |