907275 - DOMMouseScroll doesn't fire on Mac trackpad with slow movements

Status: RESOLVED WONTFIX

(Core :: DOM: Events, defect)

Description

[Ian Gilman [:iangilman]]

This was working fine with Firefox 22 and before; it broke with Firefox 23. To reproduce:

1. Go to http://lab.cubiq.org/iscroll/examples/simple/ with a Mac that has a trackpad (tested on Mac Pro Mountain Lion with Magic Trackpad).
2. Do the two-finger scroll gesture on the trackpad slowly. Note that nothing happens.
3. Do it more quickly; note that it does move (though not much).

You can see the error in http://comic-viewer-dev.appspot.com/view/brad/ (my site) as well.

If you use a normal mouse scroll wheel on Mac Firefox 23, or the Mac trackpad on another browser, the sites work fine.

Also normal scrolling pages work fine with the Mac trackpad on Firefox 23... clearly the scroll message is getting through; it's just that the DOMMouseScroll event isn't firing properly.

Here's an interesting case: http://openseadragon.github.io/ The zooming image on the page is using DOMMouseScroll, but the page itself is also tall enough that there's room for vertical scrolling. In this case the DOMMouseScroll seems to be getting through properly.

My theory is that there's some "optimization" where the DOMMouseScroll event doesn't fire when the page knows there's not enough room for vertical scrolling. This is bad news for sites like http://comic-viewer-dev.appspot.com/view/brad/ that use the scroll gesture for zooming.

Please let me know if you need additional information!

Comment 1

[Ian Gilman [:iangilman]]

I've created these bare-bones test pages to show the bug in action:

http://iangilman.com/comics/scroll-broken.html
http://iangilman.com/comics/scroll-kind-of-working.html

You can see clearly that DOMMouseScroll is tied to the page's available space for scrolling, which it shouldn't be.

Comment 2

[Markus Stange [:mstange]]

Stephen, can you check whether this was caused by bug 673875?

Comment 3

[Stephen A Pohl [:spohl]]

Bug 673875 isn't currently turned on due to bug 860493, which is waiting for bug 817700 to be fixed... Or did you mean a different bug number?

Comment 4

[Markus Stange [:mstange]]

I actually do mean bug 673875, because I have the feeling that the widget changes from it still have an impact even when the feature is disabled.

Comment 5

[Stephen A Pohl [:spohl]]

Hmm, that'd be bad. Let me check.

Comment 6

[Stephen A Pohl [:spohl]]

Indeed, looks like bug 673875 is the culprit. I'll check if this is an easy fix. Otherwise, I'll revert the patches in bug 673875 for now, since the bounce behavior isn't turned on yet.

Comment 7

[Ian Gilman [:iangilman]]

Glad to know the culprit has been found! Will we be able to get the fix in version 24 (or sooner) or does it have to ride through the whole release cycle? Sorry for the naive question... I'm out of touch with the process these days.

Also, in the meantime, any thoughts on how a webapp might work around this issue?

Thank you :-)

Comment 8

[Stephen A Pohl [:spohl]]

Bug 673875 has just been backed out on inbound. After a few days, I'll request approval for uplift. Since this is a backout, I'm hopeful that we can uplift it to Fx24.

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/ce98deb07663
https://hg.mozilla.org/mozilla-central/rev/7c78f04b8640

Comment 10

[Ian Gilman [:iangilman]]

Awesome, thanks for the quick response! Having this in Fx24 would be great. I see it's currently marked for Fx26... I assume that's because you haven't gotten uplift approval yet.

Comment 11

[Stephen A Pohl [:spohl]]

(In reply to Ian Gilman [:iangilman] from comment #10)
> I assume that's because you haven't gotten uplift approval yet.

Your assumption is correct. :-)

Comment 12

[Stephen A Pohl [:spohl]]

Attachment: Part 1: Revert fd6c7792b048

This patch was r+'d in bug 673875. Moving it here to get approvals for aurora and beta and for better tracking.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Bug 673875
User impact if declined: Scrolling can stop prematurely and feel broken on OSX.
Testing completed (on m-c, etc.): Has been on m-c for ~3 days. Local testing shows that the issue is fixed.
Risk to taking this patch (and alternatives if risky): low (backout)
String or IDL/UUID changes made by this patch: none

Comment 13

[Stephen A Pohl [:spohl]]

Attachment: Part 2: Revert 366e6a39d71a

This patch was r+'d in bug 673875. Moving it here to get approvals for aurora and beta and for better tracking.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Bug 673875
User impact if declined: Scrolling can stop prematurely and feel broken on OSX.
Testing completed (on m-c, etc.): Has been on m-c for ~3 days. Local testing shows that the issue is fixed.
Risk to taking this patch (and alternatives if risky): low (backout)
String or IDL/UUID changes made by this patch: none

Comment 14

[bhavana bajaj [:bajaj]]

(In reply to Ian Gilman [:iangilman] from comment #10)
> Awesome, thanks for the quick response! Having this in Fx24 would be great.
> I see it's currently marked for Fx26... I assume that's because you haven't
> gotten uplift approval yet.

Can you please help confirm the issue you reported is fixed now ? Thanks for you report and help on this issue :)

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/7be7bbac0064
https://hg.mozilla.org/releases/mozilla-aurora/rev/e6b7411e9b1f

https://hg.mozilla.org/releases/mozilla-beta/rev/782006611b52
https://hg.mozilla.org/releases/mozilla-beta/rev/58c80ab3c0e0

Comment 16

[Ian Gilman [:iangilman]]

(In reply to bhavana bajaj [:bajaj] from comment #14)
> Can you please help confirm the issue you reported is fixed now ? Thanks for
> you report and help on this issue :)

Just tested in nightly... yes, fixed! Thank you :-)

Comment 17

[Markus Stange [:mstange]]

Isn't Firefox 23 actually affected by this bug, too?

Comment 18

[Stephen A Pohl [:spohl]]

(In reply to Markus Stange [:mstange] from comment #17)
> Isn't Firefox 23 actually affected by this bug, too?

Right, Firefox 23 is affected.

Comment 19

[Stephen A Pohl [:spohl]]

Spoke with :bajaj and this is wontfix for Fx23, unfortunately. The fix will be released with Fx24.

Comment 21

[Bogdan Maris, Desktop Test Engineering]

We don`t have a Mac with trackpad here so can you please help us verify that this issue is fixed on latest Aurora and latest Nightly?
http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly//latest-mozilla-aurora/
http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly//latest-mozilla-central/

Comment 22

[Ian Gilman [:iangilman]]

I have just now verified that it is fixed on the latest Nightly and Aurora, and that it is fixed in Firefox 24.

Comment 23

[Bogdan Maris, Desktop Test Engineering]

(In reply to Ian Gilman [:iangilman] from comment #22)
> I have just now verified that it is fixed on the latest Nightly and Aurora,
> and that it is fixed in Firefox 24.
Thanks a lot Ian.
If you have some sphere time and want to verify this on Firefox 25 beta 6 as well, just to properly close this bug it would be much appreciated :)
http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly//25.0b6-candidates/build1/mac/en-US/

Comment 24

[Ian Gilman [:iangilman]]

I have now verified that it is fixed in the Firefox beta you linked to.

Comment 25

[Bogdan Maris, Desktop Test Engineering]

Thanks again Ian.

Comment 26

[Stephen A Pohl [:spohl]]

The latest patches for bug 673875 have reintroduced this bug, but it turns out that we may end up WONTFIX-ing it. Bringing the discussion over here from bug 926274, where I started the conversation with Masayuki.

From bug 926274:
(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from bug 926274 comment #8)
> (In reply to Stephen Pohl [:spohl] from bug 926274 comment #7)
> > I have one concern however: We will inevitably break what's been reported in
> > bug 907275 again. On the test page [1], when scrolling the list,
> > ComputeScrollTarget returns NULL, which we take to mean that the ViewPort
> > has been overscrolled. This means that we stop the vertical scroll and cause
> > the page to bounce instead. I've confirmed that Safari does the same, with
> > the difference that while the page is bouncing, the item list is scrolling
> > too. We can't do both (bounce and scroll) at the same time, since we're
> > currently using snapshots for the bounce effect until we've implemented the
> > AsyncPanZoomController for OSX (which will take some time). Masayuki, can
> > you tell me if this is an acceptable breakage?
> 
> I don't understand well your question. The testcase looks like not working
> fine with current Nightly build. Additionally, on my environment, swipe
> animation isn't performed. Do I need to change something in about:config?
> # The iscroll.js should handle wheel event rather than DOMMouseScroll event,
> though.

Right, the latest patches for bug 673875 have reintroduced this problem, so the testcase is broken on current Nightly. Swipe animations (and bounce behavior) can be turned on by setting browser.snapshots.limit to a value above 0 in about config (the default will be 5, see bug 860493).

Basically, the question right now is if we expect this test case to work in the future, keeping in mind that it also misbehaves in Safari, or whether the content should be changed to handle wheel events instead (would this fix it?). The difference in behavior between Safari and Firefox is explained in my earlier comment above (lack of AsyncPanZoomController on OSX).

Comment 27

[Stephen A Pohl [:spohl]]

On closer inspection it seems like iscroll.js handles both DOMMouseScroll and mousewheel events. Removing DOMMouseScroll did not have any effect that I could see.

Comment 28

[Markus Stange [:mstange]]

Maybe it's just missing an event.preventDefault() call?

Comment 29

[Stephen A Pohl [:spohl]]

(In reply to Markus Stange [:mstange] from comment #28)
> Maybe it's just missing an event.preventDefault() call?

I thought I've checked that, but I had to realize that the e.preventDefault() in the onBeforeScrollStart in iscroll.js is seemingly not being called when it should. Calling e.preventDefault() at the top of the _wheel function works well however. Thanks Markus!

Based on the fact that Safari exhibits a similar problem and that this can be fixed with a call to e.preventDefault() in the _wheel function I'm going to close this as WONTFIX for now. Please feel free to reopen if I'm missing anything here.

Comment 30

[Ian Gilman [:iangilman]]

Should this bug really be WONTFIX? The original bug has indeed been fixed... the only thing that's WONTFIX is the little bit of discussion at the bottom here. 

Also, regarding that discussion, let me know if there's anything I can do to verify that things are still working properly. I have a couple of projects which use the scrollwheel in this way, such as http://driftory.com.

Comment 31

[Stephen A Pohl [:spohl]]

(In reply to Ian Gilman [:iangilman] from comment #30)
> Should this bug really be WONTFIX? The original bug has indeed been fixed...
> the only thing that's WONTFIX is the little bit of discussion at the bottom
> here.

Ever since bug 673875 has landed again, the original issue has resurfaced. The thing is that it hasn't propagated to aurora, beta and release yet. If you check nightly [1] you will see the original issue again.

> Also, regarding that discussion, let me know if there's anything I can do to
> verify that things are still working properly. I have a couple of projects
> which use the scrollwheel in this way, such as http://driftory.com.

This appears to be broken right now (at least intermittently). You will want to follow the advice in comment 28 and comment 29  and call event.preventDefault(). This will make sure that the page doesn't bounce when scrolling.

[1] http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/latest-trunk/firefox-27.0a1.en-US.mac.dmg

Comment 32

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

(In reply to Stephen Pohl [:spohl] from comment #31)
> (In reply to Ian Gilman [:iangilman] from comment #30)
> > Also, regarding that discussion, let me know if there's anything I can do to
> > verify that things are still working properly. I have a couple of projects
> > which use the scrollwheel in this way, such as http://driftory.com.
> 
> This appears to be broken right now (at least intermittently). You will want
> to follow the advice in comment 28 and comment 29  and call
> event.preventDefault(). This will make sure that the page doesn't bounce
> when scrolling.

I guess that it's not enough. The script should listen D3E "wheel" event rather than legacy "DOMMouseScroll" event. Firefox 17 ESR has already implemented D3E wheel event. So, it's available since over a year. Note that Google Chrome will support it soon, IE 8 and later have already supported it.

Comment 33

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

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #32)
> (In reply to Stephen Pohl [:spohl] from comment #31)
> > (In reply to Ian Gilman [:iangilman] from comment #30)
> > > Also, regarding that discussion, let me know if there's anything I can do to
> > > verify that things are still working properly. I have a couple of projects
> > > which use the scrollwheel in this way, such as http://driftory.com.
> > 
> > This appears to be broken right now (at least intermittently). You will want
> > to follow the advice in comment 28 and comment 29  and call
> > event.preventDefault(). This will make sure that the page doesn't bounce
> > when scrolling.
> 
> I guess that it's not enough. The script should listen D3E "wheel" event
> rather than legacy "DOMMouseScroll" event. Firefox 17 ESR has already
> implemented D3E wheel event. So, it's available since over a year. Note that
> Google Chrome will support it soon, IE 8 and later have already supported it.

Sorry, IE 9 and later. IE 8 only supports "mousewheel".

Comment 34

[Markus Stange [:mstange]]

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #32)
> (In reply to Stephen Pohl [:spohl] from comment #31)
> > (In reply to Ian Gilman [:iangilman] from comment #30)
> > > Also, regarding that discussion, let me know if there's anything I can do to
> > > verify that things are still working properly. I have a couple of projects
> > > which use the scrollwheel in this way, such as http://driftory.com.
> > 
> > This appears to be broken right now (at least intermittently). You will want
> > to follow the advice in comment 28 and comment 29  and call
> > event.preventDefault(). This will make sure that the page doesn't bounce
> > when scrolling.
> 
> I guess that it's not enough. The script should listen D3E "wheel" event
> rather than legacy "DOMMouseScroll" event.

Why is it not enough? I remember that, when I implemented MozMousePixelScroll, we tried to ensure that calling preventDefault only on the DOMMouseScroll event would still be enough to stop all scrolling default handling. Is that no longer the case?

Comment 35

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

(In reply to Markus Stange [:mstange] from comment #34)
> (In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #32)
> > (In reply to Stephen Pohl [:spohl] from comment #31)
> > > (In reply to Ian Gilman [:iangilman] from comment #30)
> > > > Also, regarding that discussion, let me know if there's anything I can do to
> > > > verify that things are still working properly. I have a couple of projects
> > > > which use the scrollwheel in this way, such as http://driftory.com.
> > > 
> > > This appears to be broken right now (at least intermittently). You will want
> > > to follow the advice in comment 28 and comment 29  and call
> > > event.preventDefault(). This will make sure that the page doesn't bounce
> > > when scrolling.
> > 
> > I guess that it's not enough. The script should listen D3E "wheel" event
> > rather than legacy "DOMMouseScroll" event.
> 
> Why is it not enough? I remember that, when I implemented
> MozMousePixelScroll, we tried to ensure that calling preventDefault only on
> the DOMMouseScroll event would still be enough to stop all scrolling default
> handling. Is that no longer the case?

DOMMouseScroll is fired when accumulated scroll amount becomes over than 1 line. I.e., only wheel event and MozMousePixelScroll may be fired if user tries to scroll very slowly. It causes bug 681200.

Comment 36

[Markus Stange [:mstange]]

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #35)
> DOMMouseScroll is fired when accumulated scroll amount becomes over than 1
> line. I.e., only wheel event and MozMousePixelScroll may be fired if user
> tries to scroll very slowly. It causes bug 681200.

Can we instead fire it before the first pixel scroll event? I think this bug also causes slow scrolling on Google Docs (e.g. https://docs.google.com/spreadsheet/ccc?key=0Asj8iLTl0K0UdDcxemJEWVVXWTlKek1wR09iS2FsOXc&pli=1#gid=2 ) to fail... Or is this a different bug?

Comment 37

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

(In reply to Markus Stange [:mstange] from comment #36)
> (In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #35)
> > DOMMouseScroll is fired when accumulated scroll amount becomes over than 1
> > line. I.e., only wheel event and MozMousePixelScroll may be fired if user
> > tries to scroll very slowly. It causes bug 681200.
> 
> Can we instead fire it before the first pixel scroll event? I think this bug
> also causes slow scrolling on Google Docs (e.g.
> https://docs.google.com/spreadsheet/
> ccc?key=0Asj8iLTl0K0UdDcxemJEWVVXWTlKek1wR09iS2FsOXc&pli=1#gid=2 ) to
> fail... Or is this a different bug?

I've not tested it yet, I guess that when DOMMouseScroll event is fired is not matter. Even if we dispatch it before first MozMousePixelScroll, following events will be only wheel and MozMousePixelScroll. All web apps which handle mouse wheel should handle new wheel event instead of legacy events.

Comment 38

[Markus Stange [:mstange]]

I agree, but that's easy for us to say... it still means we break existing apps, which we should try to avoid.

Comment 39

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

(In reply to Markus Stange [:mstange] from comment #38)
> I agree, but that's easy for us to say... it still means we break existing
> apps, which we should try to avoid.

We've not changed the behavior except we supported high resolution scroll on Mac. I have no idea to avoid breaking compatibility on some web apps which were broken by the change.

Comment 40

[Ian Gilman [:iangilman]]

Why does DOMMouseScroll have to break? Isn't it possible to keep it working and phase it out gracefully? 

At any rate, I've updated http://driftory.com to use "wheel" and preventDefault, so it's working now in both Nightly and Release. That all seems to work. 

There's a good deal of legacy code out there that uses DOMMouseScroll, though; it would be great if we could avoid breaking them if possible.

Comment 41

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

(In reply to Ian Gilman [:iangilman] from comment #40)
> Why does DOMMouseScroll have to break? Isn't it possible to keep it working
> and phase it out gracefully? v

DOMMouseScroll event cannot represent small amount scroll less than 1 line. It was enough for old (slow) machines because high resolution scroll was too expensive since it needs higher frequency repaint. In these days, the cost of high frequency repaint doesn't matter because of faster machines. Therefore, Mac OS X 10.5 (IIRC) and Windows Vista supported high resolution wheel event for smoother scroll. We implemented it with MozMousePixelScroll which indicates scroll amount in pixels. At this time, DOMMouseScroll was stayed for compatibility. However, its preventDefault() *cannot* prevent default actions of MozMousePixelScroll between 2 DOMMouseScroll events. This is the reason why old web application had to be broken by the change.

> it would be great if we could avoid breaking them if possible.

I have no idea to avoid the issues because it's too difficult to decide which MozMousePixelScroll event is killed by a call of DOMMouseScroll event.

Anyway, using new wheel event improves web applications. It indicates the scroll amount in lines, pixels or pages (see deltaMode attribute) clearly (Legacy mousewheel event doesn't indicate the good scroll amount). Additionally, all web browsers will support wheel event soon. So, web application's code will be simpler than now.

Comment 42

[Ian Gilman [:iangilman]]

Okay, thank you for the explanation. I certainly am glad to be moving to a more standard scrollwheel event!