1399086 - Mouse swipe gesture is broken/intermittent since switch to macOS 10.11 SDK

Status: RESOLVED FIXED

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

Description

[Henrik Skupin [:whimboo][⌚️UTC+1]]

The mouse gestures eg. the swipe for navigating back and forward are no longer working. As the regression range has been shown this is caused by bug 1324892 which upgraded to the MacOS 10.11 SDK:

15:42.49 INFO: Narrowed inbound regression window from [62346a54, 8a67124d] (4 builds) to [a85670ee, 8a67124d] (2 builds) (~1 steps left)
15:42.49 INFO: No more inbound revisions, bisection finished.
15:42.49 INFO: Last good revision: a85670ee5e959299556b6ad0ef1370743debc43d
15:42.49 INFO: First bad revision: 8a67124d36d9ed0c6fa9111ae33620aeddc4ce4d
15:42.49 INFO: Pushlog:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=a85670ee5e959299556b6ad0ef1370743debc43d&tochange=8a67124d36d9ed0c6fa9111ae33620aeddc4ce4d

The only way it works is to leave the fingers on the mouse and swipe back and forth really fast.

This is a major regression so it should block the 57.0 release.

Comment 1

[Henrik Skupin [:whimboo][⌚️UTC+1]]

For postery I'm using Mozilla/5.0 (Macintosh; Intel Mac OS X 10.12; rv:57.0) Gecko/20100101 Firefox/57.0

Comment 2

[Markus Stange [:mstange]]

I can still swipe normally using the latest build on 10.12.6.

(In reply to Henrik Skupin (:whimboo) from comment #0)
> The only way it works is to leave the fingers on the mouse and swipe back
> and forth really fast.

Can you find out more about what makes it work vs what doesn't?

Comment 3

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Ok, another way to get it working is to very slowly perform the swipe. So it looks like a timing issue here, and maybe doing it in my normal speed doesn't trigger a swipe.

Comment 4

[Marcia Knous [:marcia]]

Do all the mouse gestures not work (https://support.apple.com/en-us/HT204895), or just the swipe?

Comment 5

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Hard to say because we don't seem to use all, or I don't have them all configured. At least what's also not working is the 3-finger swipe up/down to scroll to the top/bottom of the page.

Comment 6

[Stephen A Pohl [:spohl]]

I can't look into this until I have a system running macOS 10.13 natively (one is on the way). Keeping n-i flag set until then.

Comment 7

[Stephen A Pohl [:spohl]]

I just noticed that this was filed against 10.12. I'm unable to reproduce this and we don't seem to get a lot of reports of this issue. Is there anything else that's specific to your system (prefs/settings)?

Comment 8

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Stephen! I totally missed the point that when I'm at home I always work with an external monitor, keyboard, and mouse. So now that you have asked for system settings I tried with the integrated touchpad and it works great. But it fails when using my external Apple mouse. So this might be the missing info you need to get it reproduced.

Comment 9

[Stephen A Pohl [:spohl]]

So, I'm assuming you have "Swipe between pages" enabled in your mouse settings and you have it set to "Scroll left or right with one finger" for example? I have just tested this on a build compiled with the 10.7 SDK and one with the 10.11 SDK and it works as expected. Could you verify what your settings are?

Comment 10

[Henrik Skupin [:whimboo][⌚️UTC+1]]

That is correct. Two fingers for back/forward navigation and a single finger for scrolling. I just noticed that I haven't added the fact to the summary yet, that it works if the gesture is performed very slowly (as mentioned in comment 3). Sadly I cannot find a place where to adjust the speed.

Also all gesture related preference in Firefox are set to default values.

Comment 11

[Stephen A Pohl [:spohl]]

Unfortunately, there isn't enough here to go on yet. Summarizing the various comments, it works when swiping fast (comment 0) or swiping slowly (comment 3). If there wasn't a regression range (comment 0) I would say you might have a defective mouse. We will need to know much more specifically what triggers this such as STR, specific pages in browser history, at what point is the swipe initiated (while page is still loading or when the page is fully loaded) etc. Also, please compare with Firefox release (built with 10.7) one more time to make sure that this wasn't just a fluke. This continues to work as expected for me using an external Apple mouse and I haven't been able to find a noticeable difference between the 10.7 and 10.11 SDK.

Comment 12

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to Stephen A Pohl [:spohl] from comment #11)
> 3). If there wasn't a regression range (comment 0) I would say you might
> have a defective mouse. We will need to know much more specifically what
> triggers this such as STR, specific pages in browser history, at what point
> is the swipe initiated (while page is still loading or when the page is
> fully loaded) etc. Also, please compare with Firefox release (built with
> 10.7) one more time to make sure that this wasn't just a fluke. This

The mouse is working perfectly fine, and that not only in other applications but also with the latest 56.0 beta. The problem only manifests in 57.0a1, and that as the regression window shows.

I don't think that there are more steps necessary. Whenever you can navigate back or forth (means the history is filled in that direction) the misbehavior can be seen. It's really happening for every single website, also when using a fresh profile.

> Unfortunately, there isn't enough here to go on yet. Summarizing the various
> comments, it works when swiping fast (comment 0) or swiping slowly (comment

To note again... swiping fast only when I do that back and forth repeatedly, but not a single time.

Comment 13

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Just tried with the upcoming 57.0b1 developer edition and it is also happening there.

Is there any way to eg. log events as they are retrieved/processed by the event loop?

Comment 14

[Hsin-Yi Tsai (she/her) [:hsinyi]]

(In reply to Henrik Skupin (:whimboo) from comment #13)
> Just tried with the upcoming 57.0b1 developer edition and it is also
> happening there.
> 
> Is there any way to eg. log events as they are retrieved/processed by the
> event loop?

Hi Stone, could you please suggest how to log events for Henrik? Thanks.

Comment 16

[Stephen A Pohl [:spohl]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=57cfa8a21167a3d314b5a96b7d1b52452c240900

Comment 17

[Stephen A Pohl [:spohl]]

Attachment: Patch

Bug 1052253 introduced code to properly support gestures on macOS 10.11 and above when Firefox was built with an SDK of 10.11 or above, particularly       beginOrEndGestureForEventPhase:[1]. Unfortunately, this was always broken for swipeWithEvent:[2]. Swiping on the magic mouse sometimes sends the event with a phase of NSEventPhaseBegan, which would cause us to bail and not execute the swipe. Other times, we would get a phase of NSEventPhaseChanged, which executes the swipe because it isn't handled by beginOrEndGestureForEventPhase:.

The Logitech MX Master mouse always sends swipe events with a phase of NSEventPhaseEnded, which returns true from beginOrEndGestureForEventPhase: and this is why we never executed the swipe (see bug 1401837).


[1] https://hg.mozilla.org/mozilla-central/annotate/33b7b8e81b4befcba503c0e48cd5370aeb715085/widget/cocoa/nsChildView.mm#l4360
[2] https://hg.mozilla.org/mozilla-central/annotate/33b7b8e81b4befcba503c0e48cd5370aeb715085/widget/cocoa/nsChildView.mm#l4186

Comment 18

[Stephen A Pohl [:spohl]]

Attachment: Refactor code and comments

This refactors some comments and drops some Lion-only code to bring things up-to-date.

Comment 19

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Great to see progress on this bug Stephen! In case you need someone to test please submit a try build and I can have a go.

Comment 21

[Stephen A Pohl [:spohl]]

(In reply to Henrik Skupin (:whimboo) from comment #19)
> Great to see progress on this bug Stephen! In case you need someone to test
> please submit a try build and I can have a go.

A try build is available in comment 16. The direct link to the build is:
https://queue.taskcluster.net/v1/task/CL0zKOn1Q_2TIIv7uQM5tQ/runs/0/artifacts/public/build/target.dmg

Comment 22

[Stephen A Pohl [:spohl]]

Comment on attachment 8912345 [details] [diff] [review]
Refactor code and comments

This patch needs an update because bug 1374110 just landed on inbound.

Comment 23

[Stephen A Pohl [:spohl]]

Attachment: Refactor comments

Comment 24

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to Stephen A Pohl [:spohl] from comment #21)
> A try build is available in comment 16. The direct link to the build is:
> https://queue.taskcluster.net/v1/task/CL0zKOn1Q_2TIIv7uQM5tQ/runs/0/
> artifacts/public/build/target.dmg

Tested and it fixes the problem with swiping for me. Thanks!

Comment 25

[Mikkel Ziemer (:aaxa)]

Is this issue also affecting the issue with mouse button 4 and 5 on Logitech MX Master not working for navigating between next and previous page?

Comment 26

[Stephen A Pohl [:spohl]]

(In reply to Mikkel Ziemer (:aaxa) from comment #25)
> Is this issue also affecting the issue with mouse button 4 and 5 on Logitech
> MX Master not working for navigating between next and previous page?

Yes, see bug 1401837 comment 11.

Comment 27

[Mikkel Ziemer (:aaxa)]

(In reply to Stephen A Pohl [:spohl] from comment #26)
> (In reply to Mikkel Ziemer (:aaxa) from comment #25)
> > Is this issue also affecting the issue with mouse button 4 and 5 on Logitech
> > MX Master not working for navigating between next and previous page?
> 
> Yes, see bug 1401837 comment 11.

Cool, thank you!

Comment 28

[Markus Stange [:mstange]]

Comment on attachment 8912344 [details] [diff] [review]
Patch

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

Thanks for the detailed diagnosis.

Comment 29

[Stephen A Pohl [:spohl]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/006387a24bc91455ea66f2a37171a06817adb274
Bug 1399086: Ensure that swipes on magic mice and back/forward buttons on other mice work as expected on macOS after the SDK switch to 10.11. r=mstange

https://hg.mozilla.org/integration/mozilla-inbound/rev/50722cd6558f35512e4cd5597e2f815dfb1b6886
Bug 1399086 - Refactor comments and remove gesture support code for macOS 10.7. r=mstange

Comment 30

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/006387a24bc9
https://hg.mozilla.org/mozilla-central/rev/50722cd6558f

Comment 31

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Stephen, can you please request uplift to beta, so that we can have this fix in the next beta release? Thanks.

Comment 32

[Stephen A Pohl [:spohl]]

Comment on attachment 8912344 [details] [diff] [review]
Patch

Approval Request Comment
[Feature/Bug causing the regression]: Bug 1324892
[User impact if declined]: History back/forward buttons and swipes is broken on magic mice and third-party mice on macOS
[Is this code covered by automated tests?]: No; external hardware is required to trigger these code paths.
[Has the fix been verified in Nightly?]: Yes
[Needs manual test from QE? If yes, steps to reproduce]: No
[List of other uplifts needed for the feature/fix]: None
[Is the change risky?]: No
[Why is the change risky/not risky?]: This is a one-line change, has been investigated in depth and is well understood.
[String changes made/needed]: none

Comment 33

[Stephen A Pohl [:spohl]]

Note: We only need to uplift attachment 8912344 [details] [diff] [review] to fix the bug in beta.

Comment 34

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8912344 [details] [diff] [review]
Patch

Fix a recent regression on mac, taking it.

Comment 35

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/0e04c7294567

Comment 36

[Andrei Vaida [:avaida]]

(In reply to Stephen A Pohl [:spohl] from comment #32)
> [Is this code covered by automated tests?]: No; external hardware is
> required to trigger these code paths.
> [Has the fix been verified in Nightly?]: Yes
> [Needs manual test from QE? If yes, steps to reproduce]: No

Setting qe-verify- based on Stephen's assessment on manual testing needs.