1892537 - The behavior of pen is different between Windows Chrome and Windows Firefox

Status: RESOLVED FIXED

(Core :: DOM: UI Events & Focus Handling, defect)

Description

[Kagami Rosylight [:saschanaz] (they/them)]

1. Open https://patrickhlauke.github.io/touch/pen-tracker/ on either browser on touch-capable device (Surface Pro 7 in my case)
2. Drag with pen on the page

Expected: The pen should appear in the screen surface and move on it, and should not move the surface itself.
Actual: The pen appears but dragging moves the surface instead of the pen.

Disabling dom.w3c_pointer_events.scroll_by_pen.enabled fixes it, but this flag is actually to mimic the Chrome behavior. The work item is to find the behavior difference when the pref is on.

(Found this while checking the Android webcompat report. The pen does not show up at all on Android.)

Comment 1

[BugBot [:suhaib / :marco/ :calixte]]

Set release status flags based on info from the regressing bug 1637259

Comment 2

[Kagami Rosylight [:saschanaz] (they/them)]

I see two issues:

1. Somehow the page gets pointerdown with isPrimary=false, so even on pen input it does not disable "orbit control" that pans the surface, which is handled by the following touchstart event.
2. Then it gets touchmove before pointermove, but pointermove should fire first. The page then assumes this touchmove is not from a pen and it calls e.preventDefault/stopPropagation, so pointermove never fires.

Edgar, do you have any quick idea where to start looking? At least the second issue sounds like some bug in EventStateManager.

Comment 3

[Kagami Rosylight [:saschanaz] (they/them)]

(And the Android issue is that GeckoView does not handle pens at all, so I filed bug 1894692. Surprising that there's no existing bug, or maybe my search skill is bad.)

Comment 4

[Kagami Rosylight [:saschanaz] (they/them)]

This is hard to understand. So each content process gives the proper event order for initial pen drawing: pointermove and then touchmove. But then the next pen drawing gets touchmove first. Refreshing the page doesn't fix it, but opening a new content process via duplicating the page fixes it. Duplicated tabs still becomes broken if it shares the same content process.

I wonder some global state for each process is affecting the behavior.

Comment 5

[Kagami Rosylight [:saschanaz] (they/them)]

Ah, the event order is just some page script behavior affected by isPrimary. The real problem is that isPrimary somehow becomes false after initial pen drawing.

Comment 6

[Kagami Rosylight [:saschanaz] (they/them)]

I'll check how this becomes false...

Comment 7

[Kagami Rosylight [:saschanaz] (they/them)]

Attachment: Bug 1892537 - Set mIsPrimary by pointer ID only for touch input r=edgar

It only make sense for touch input, as touch pointer ID always restarts from 0 while pen pointer ID always increments.

This won't work if there are multiple active pens, but that's rare enough and not straightforward to fix as somehow sActivePointersIds gets pointerId=0 instead of actual pointer ID for pens.

Comment 8

[Kagami Rosylight [:saschanaz] (they/them)]

Attachment: Bug 1892537 - Add test with sendTouchEventAsPen r=edgar

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4293bcd71f5b
Set mIsPrimary by pointer ID only for touch input r=edgar
https://hg.mozilla.org/integration/autoland/rev/ee40fe578b2c
Add test with sendTouchEventAsPen r=edgar

Comment 10

[Iulian Moraru]

https://hg.mozilla.org/mozilla-central/rev/4293bcd71f5b
https://hg.mozilla.org/mozilla-central/rev/ee40fe578b2c

Comment 11

[BugBot [:suhaib / :marco/ :calixte]]

Since nightly and release are affected, beta will likely be affected too.
For more information, please visit BugBot documentation.

Comment 12

[Andrej (:aglavic)]

Perfherder has detected a talos performance change from push ee40fe578b2cc3f10b18847f5e9c82b145cd73b0.

Improvements:

Ratio	Test	Platform	Options	Absolute values (old vs new)
8%	perf_reftest_singletons svg-text-getExtentOfChar-1.html	windows11-64-qr	e10s fission stylo webrender	3,655.43 -> 3,373.73
7%	perf_reftest_singletons getElementById-1.html	windows11-64-qr	e10s fission stylo webrender	23.77 -> 22.02
5%	perf_reftest_singletons window-named-property-get.html	windows11-64-qr	e10s fission stylo webrender	492.22 -> 466.90

Details of the alert can be found in the alert summary, including links to graphs and comparisons for each of the affected tests.

If you need the profiling jobs you can trigger them yourself from treeherder job view or ask a sheriff to do that for you.

You can run these tests on try with ./mach try perf --alert 1067

For more information on performance sheriffing please see our FAQ.

Comment 13

[Kagami Rosylight [:saschanaz] (they/them)]

.... I'm not sure how this can affect performance?

Comment 14

[Jens Stutte [:jstutte]]

It seems the original alert was on bug 1871378 and has been manually reassigned, not sure if that one is more likely, the graph shows quite some density of possible candidates around that time.

Comment 15

[Andrej (:aglavic)]

My apologies I mis-atrributed this, thanks for the need info, I've reassigned the improvements to the correct bug

Comment 16

[Ryan VanderMeulen [:RyanVM]]

Is this something we should uplift to ESR128? It grafts cleanly.

Comment 17

[Kagami Rosylight [:saschanaz] (they/them)]

Attachment: Bug 1892537 - Set mIsPrimary by pointer ID only for touch input r=edgar

It only make sense for touch input, as touch pointer ID always restarts from 0 while pen pointer ID always increments.

This won't work if there are multiple active pens, but that's rare enough and not straightforward to fix as somehow sActivePointersIds gets pointerId=0 instead of actual pointer ID for pens.

Original Revision: https://phabricator.services.mozilla.com/D214978

Comment 18

[Kagami Rosylight [:saschanaz] (they/them)]

Attachment: Bug 1892537 - Add test with sendTouchEventAsPen r=edgar

Original Revision: https://phabricator.services.mozilla.com/D215639

Comment 19

[Phabricator Automation]

esr128 Uplift Approval Request

• User impact if declined: Pen input may not work as expected on some websites
• Code covered by automated testing: yes
• Fix verified in Nightly: yes
• Needs manual QE test: no
• Steps to reproduce for manual QE testing: N/A
• Risk associated with taking this patch: low
• Explanation of risk level: Devices with multiple pen support may break in a different way (all pens would falsely get isPrimary=true instead of falsely getting almost always false), but that should be rare enough to justify this.
• String changes made/needed: N/A
• Is Android affected?: no

Comment 20

[Kagami Rosylight [:saschanaz] (they/them)]

Requests up 👍

Comment 21

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-esr128/rev/560485f317f9
https://hg.mozilla.org/releases/mozilla-esr128/rev/c045e853e0f4