1447196 - Pointer Event preventDefault should not block scrollbar clicks

Status: VERIFIED FIXED

(Core :: DOM: Events, defect, P2)

Description

[Bob]

User Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:59.0) Gecko/20100101 Firefox/59.0
Build ID: 20180310025718

Steps to reproduce:

Tested on a Windows 10 v1709 device with no touch input.

1. Open test case: https://codepen.io/Bob_Vul/pen/WzREVK (scrollable div with a handler on pointerdown that just preventDefault()s it)

2. Try to click on the scrollbar arrows or empty space to scroll.


Actual results:

Nothing happens when clicking on the scrollbar arrows or empty space.

Dragging the scroll handle still works.


Expected results:

Scrolling by clicking on the scrollbar should still work even with preventDefault() called on pointerdown.

Note that preventDefault() on mousedown does *not* block scrollbar clicks. Also, preventDefault() on pointerdown does not have this behaviour on Chrome 64 or Edge 41 (EdgeHTML 14), i.e. Firefox is the outlier here.

Comment 1

[Bob]

For an example of impact, this will affect anyone using Grids in the Ext JS framework, which consolidates all input event handlers under pointer events where available, and uses preventDefault to block clicks on the scrollbar from activating the underlying element while still allowing scrollbar usage.

It has already been reported to them by one other person: https://www.sencha.com/forum/showthread.php?469777-Firefox-59-0-1-Scroller-does-not-scroll-grid

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

Hmm, I haven't figured out the minimal testcase here yet, since if I export from codepen, the issue doesn't happen.

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

And even on codepen the issue happens only occasionally, at least on linux.

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

oh, I see, on linux it is just because the scrollbar is a tad different to windows. On Windows this is easier to notice.

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

Attachment: pointerdown_preventdefault.diff

remote: View your change here:
remote:   https://hg.mozilla.org/try/rev/8ced7db1588a6dbe663502e84e559fbf4577665e
remote: 
remote: Follow the progress of your build on Treeherder:
remote:   https://treeherder.mozilla.org/#/jobs?repo=try&revision=8ced7db1588a6dbe663502e84e559fbf4577665e
remote: recorded changegroup in replication log in 0.092s

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

Attachment: test

remote: View your change here:
remote:   https://hg.mozilla.org/try/rev/9e10d65e38d2d64678bd0adf73e8f58fe81e4976
remote: 
remote: Follow the progress of your build on Treeherder:
remote:   https://treeherder.mozilla.org/#/jobs?repo=try&revision=9e10d65e38d2d64678bd0adf73e8f58fe81e4976
remote: recorded changegroup in replication log in 0.072s

Comment 7

[Olli Pettay [:smaug][[e-mail]]]

Attachment: pointerdown_preventdefault_with_test.diff

we do need the default handling here, especially, we do need nsIFrame::HandleEvent.
But I think in general we want system group handling for mouse events, just
have the prevent default flag set.

hopefully this version of the test passes on all the desktop platforms.
(smooth scrolling was causing some trouble)

remote: View your change here:
remote:   https://hg.mozilla.org/try/rev/208dccee47d8319069dd52ad6e054d226258de1f
remote: 
remote: Follow the progress of your build on Treeherder:
remote:   https://treeherder.mozilla.org/#/jobs?repo=try&revision=208dccee47d8319069dd52ad6e054d226258de1f
remote: recorded changegroup in replication log in 0.064s

Comment 8

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Hmm, is the orange of dom/events/test/pointerevents/test_bug1303704.html a regression of your patch?

Comment 9

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8961296 [details] [diff] [review]
pointerdown_preventdefault_with_test.diff

Review of attachment 8961296 [details] [diff] [review]:
-----------------------------------------------------------------

Hmm, but looks good to me. Please check the orange before landing.

::: dom/events/test/pointerevents/test_bug1303704.html
@@ +92,5 @@
> +  var offsetX = rect.width - 4;
> +  var offsetY = rect.height - 4
> +  synthesizeMouse(scrollable, offsetX, offsetY,
> +                  { type: "mousedown",
> +                     inputSource: MouseEvent.MOZ_SOURCE_MOUSE });

nit: looks like odd indent.

@@ +96,5 @@
> +                     inputSource: MouseEvent.MOZ_SOURCE_MOUSE });
> +
> +  synthesizeMouse(scrollable, offsetX, offsetY,
> +                  { type: "mousemove",
> +                     inputSource: MouseEvent.MOZ_SOURCE_MOUSE });

ditto.

@@ +100,5 @@
> +                     inputSource: MouseEvent.MOZ_SOURCE_MOUSE });
> +
> +  synthesizeMouse(scrollable, offsetX, offsetY,
> +                  { type: "mouseup",
> +                     inputSource: MouseEvent.MOZ_SOURCE_MOUSE });

ditto.

Comment 10

[Olli Pettay [:smaug][[e-mail]]]

yes, because that is what I'm modifying, adding a new test there. I'm using tryserver as way to figure out how to make my changes pass on Mac and Windows

Comment 11

[Olli Pettay [:smaug][[e-mail]]]

Ah, now I looked at this stuff on Mac. We don't show the scrollbar there by default, so the test can't work.

Comment 12

[Olli Pettay [:smaug][[e-mail]]]

Attachment: pointerdown_preventdefault_with_test_2.diff

Disabling the test on Mac too. Handling of scrollable areas on mac is a bit weird.

Comment 13

[Olli Pettay [:smaug][[e-mail]]]

Attachment: pointerdown_preventdefault_with_test_2.diff

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/36c52265e4a1
canceling pointerdown should not affect to scrollbar handling, r=masayuki

Comment 15

[Andreea Pavel [:apavel]]

Backed out for failing mochitest headless at dom/events/test/pointerevents/test_bug1303704.html 

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=36c52265e4a18cb02d01af022cc9238aed0c8a94

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=169660480&repo=mozilla-inbound&lineNumber=2792

Backout: https://hg.mozilla.org/integration/mozilla-inbound/rev/d1960bf6a6175852ba23e9544a8bdacfb32d19e7

Comment 16

[Olli Pettay [:smaug][[e-mail]]]

ok, then disabling on headless too, somehow...

Comment 17

[Olli Pettay [:smaug][[e-mail]]]

Attachment: pointerdown_preventdefault_with_test_3.diff

And more try runs.

remote: 
remote: Follow the progress of your build on Treeherder:
remote:   https://treeherder.mozilla.org/#/jobs?repo=try&revision=6905c916ddc76f62e12a16153f8db18245524189
remote: recorded changegroup in replication log in 0.044s

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b4f1e88f8b6e
canceling pointerdown should not affect to scrollbar handling, r=masayuki

Comment 19

[Eliza Balazs [:ebalazs_]]

Backed out changeset b4f1e88f8b6e (bug 1447196) for 2 failures in dom/events/test/pointerevents/test_bug1303704.html
Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=b4f1e88f8b6e7f15c37990722d7e8491a74b729f&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=runnable&filter-resultStatus=running&filter-resultStatus=pending&filter-resultStatus=success&filter-classifiedState=unclassified&selectedJob=169801642
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=169801642&repo=mozilla-inbound&lineNumber=4134
Backout: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=7ce6b5f442f03c0f6fc886a8543ef823061c745b&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=runnable&filter-resultStatus=running&filter-resultStatus=pending&filter-resultStatus=success

Comment 20

[Olli Pettay [:smaug][[e-mail]]]

gah, I can't make the test stable.

Comment 21

[Olli Pettay [:smaug][[e-mail]]]

Attachment: pointerdown_preventdefault_with_test_4.diff

it would be nice to be able to reproduce locally.
Anyhow, modifying the test a bit.

remote: Follow the progress of your build on Treeherder:
remote:   https://treeherder.mozilla.org/#/jobs?repo=try&revision=df68ebf51d522c4638eee787421239999e661f0b
remote: recorded changegroup in replication log in 0.084s

Comment 22

[Olli Pettay [:smaug][[e-mail]]]

remote: Follow the progress of your build on Treeherder:
remote:   https://treeherder.mozilla.org/#/jobs?repo=try&revision=ce68e8430b7d59ef515d57d42631fc63dd4512bf
remote: recorded changegroup in replication log in 0.070s

Comment 23

[Bob]

(In reply to Olli Pettay [:smaug] (only webcomponents and event handling reviews, please) from comment #21)

> it would be nice to be able to reproduce locally.

If it helps, I can give you remote access to a Windows VM?

Comment 24

[Olli Pettay [:smaug][[e-mail]]]

thanks, but looks like the latest version is quite stable on try.

Comment 25

[Olli Pettay [:smaug][[e-mail]]]

Tryserver looks good, so retrying again.

Comment 26

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/fe95ca481981
canceling pointerdown should not affect to scrollbar handling, r=masayuki

Comment 27

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/mozilla-central/rev/fe95ca481981

Comment 28

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8961712 [details] [diff] [review]
pointerdown_preventdefault_with_test_4.diff

Approval Request Comment
[Feature/Bug causing the regression]: bug 1411467
[User impact if declined]: some web sites may not work the expected way
[Is this code covered by automated tests?]: yes
[Has the fix been verified in Nightly?]: yes
[Needs manual test from QE? If yes, steps to reproduce]: see comment 0 
[List of other uplifts needed for the feature/fix]: Just this patch
[Is the change risky?]: It is a tad risky since pointer events is a new addition and some parts of the spec are still shaky.
[Why is the change risky/not risky?]: see above
[String changes made/needed]: NA

Comment 29

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8961712 [details] [diff] [review]
pointerdown_preventdefault_with_test_4.diff

Simple webcompat fix which includes new automated test coverage. Approved for 60.0b9.

Comment 30

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/3954e94774f9

Comment 31

[Bob]

Thanks!

Tested working in 60.0b9/Win10 with both the simplified test case and the Ext JS Grid widget this was discovered in.

Comment 32

[Alexandru Simonca [:asimonca], Release Desktop QA]

Hi,

Tested on 60.b13 and the latest 61.0a1 on Windows 10 x64, Ubuntu 14.04 LTS x64 and MacOS 10.13.5. The issue is VERIFIED FIXED.

Comment 33

[James B]

Hello anybody still watching, it looks like changeset fe95ca481981946094bacf5191342b135c79f9e6 introduced bug #1463354.  If you'd like to test that bug, it's pretty simple (if you have a touch-capable Windows PC) -- just go to Google Maps and try to do a two-finger zoom.  Let me know if I can provide additional assistance.