CMD + swipe left to open previous location in new tab is broken with Magic Mouse

RESOLVED FIXED in Firefox 20

Status

()

defect
RESOLVED FIXED
7 years ago
6 years ago

People

(Reporter: julian.viereck, Assigned: masayuki)

Tracking

({regression})

Trunk
Firefox 20
x86
macOS
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox17 unaffected, firefox18 unaffected, firefox19 unaffected, firefox20+ fixed, firefox-esr10 unaffected, firefox-esr17 unaffected)

Details

Attachments

(1 attachment, 2 obsolete attachments)

Reporter

Description

7 years ago
This is on OSX:

Ways to reproduce:
- open a page A in a tab
- click on a link to open the address in the same tab
- let the new loaded tab be B
- once the tab is loaded, hold down CMD + swipe to the left using the trackpad

Expected result:
- a new tab should be opened and load page A

Actual result:
- The zoom factor changes

I'm currently on 20.0a1 (2012-12-06) where things are broken. For 19.0a1 (2012-11-14) it works.
Reporter

Comment 1

7 years ago
Seems to be caused by 814303.

If I change `mousewheel.with_meta.action` to `1` in my about:config, the bug goes away.
Depends on: 814303
Hmm, is that an intentional feature? I'll check it early next week.
Posted patch Patch (obsolete) — Splinter Review
Wheel event with a modifier key always prevents swipe gesture event if the action is zoom in/out or going back/forward in the history since the default action handlers don't set overflowDelta values even if they do nothing.

This isn't problem if the action is the "history" since the horizontal swipe is also the "history". So that we should not prevent swipe gesture by horizontal zoom action since users must try moving history at swiping horizontally.

First, we should kill the zoom action only on Mac with override_x prefs.

Next, ESM should set overflowDelta values if the action is none.

Then, Command + horizontal swipe and Control + horizontal swipe will behave same as before D3E WheelEvent implemented.
Assignee: nobody → masayuki
Status: NEW → ASSIGNED
Attachment #695993 - Flags: review?(smichaud)
Attachment #695993 - Flags: review?(bugs)
Comment on attachment 695993 [details] [diff] [review]
Patch

># HG changeset patch
># Parent d29b182e169ee1245729e2a45817e09b424df9de
>Bug 819252 WheelEvent along x-axis with a modifier key on Mac shouldn't cause zoom action for not preventing following horizontal swip event which will cause going back/forward in the history r=
>
>diff --git a/browser/app/profile/firefox.js b/browser/app/profile/firefox.js
>--- a/browser/app/profile/firefox.js
>+++ b/browser/app/profile/firefox.js
>@@ -532,16 +532,27 @@ pref("browser.gesture.tap", "cmd_fullZoo
> // scrolling to shift+wheel.
> pref("mousewheel.with_alt.action", 2);
> pref("mousewheel.with_shift.action", 1);
> // On MacOS X, control+wheel is typically handled by system and we don't
> // receive the event.  So, command key which is the main modifier key for
> // acceleration is the best modifier for zoom-in/out.  However, we should keep
> // the control key setting for backward compatibility.
> pref("mousewheel.with_meta.action", 3); // command key on Mac
>+// On MacOS X, left or right swipe gesture is used for going back or forward
>+// in the history.  That also works with modifier keys which will cause open
>+// the previous or next page in new tab or window.  However, the default action
>+// handlers for non-scroll action cause consuming all wheel events which will
>+// cause the swipe events.  So that we should not allow non-scroll action with
>+// vertical wheel operation.  However, horizontal wheel event with options key
>+// has been used for going back/forward in the history.  So that we should
>+// override the horizontal default action if the vertical default action is
>+// zoom in/out.

you have two spaces in few places in the comment when just one space would be enough.
"So that we should not allow non-scroll action with vertical wheel operation." sounds odd.
drop 'that'

>         case WheelPrefs::ACTION_HISTORY:
>+          // If this event doesn't cause NS_MOUSE_SCROLL event or the direction
>+          // is oblique, don't perform history back/forward.
>+          if (!wheelEvent->GetPreferredIntDelta()) {
>+            break;
>+          }
>           DoScrollHistory(wheelEvent->GetPreferredIntDelta());
>           break;
Looks a bit ugly.
Perhaps 
if ((int32_t intDelta = wheelEvent->GetPreferredIntDelta())) {
  DoScrollHistory(intDelta);
}
break;

Same also elsewhere.
Attachment #695993 - Flags: review?(bugs) → review-
Posted patch Patch (obsolete) — Splinter Review
> you have two spaces in few places in the comment when just one space would be enough.

Some people use two spaces after ".". Although, I'm not concerned with that.
Attachment #695993 - Attachment is obsolete: true
Attachment #695993 - Flags: review?(smichaud)
Attachment #696027 - Flags: review?(smichaud)
Attachment #696027 - Flags: review?(bugs)
Comment on attachment 696027 [details] [diff] [review]
Patch

>diff --git a/browser/app/profile/firefox.js b/browser/app/profile/firefox.js

>+// On MacOS X, left or right swipe gesture is used for going back or forward
>+// in the history. That also works with modifier keys which will cause open
>+// the previous or next page in new tab or window. However, the default action
>+// handlers for non-scroll action cause consuming all wheel events which will
>+// cause the swipe events. So we should not allow non-scroll action with
>+// vertical wheel operation. However, horizontal wheel event with options key
>+// has been used for going back/forward in the history. So that we should
>+// override the horizontal default action if the vertical default action is
>+// zoom in/out.
>+pref("mousewheel.with_control.action.override_x", 0);
>+pref("mousewheel.with_meta.action.override_x", 0);

This comment seems a little confusing. Is this alternative accurate?

// Disable control-/meta-modified horizontal mousewheel events, since
// those are used on Mac as part of modified swipe gestures (e.g.
// Left swipe+Cmd = go back in a new tab).
Posted patch PatchSplinter Review
Thanks, Gavin!
Attachment #696027 - Attachment is obsolete: true
Attachment #696027 - Flags: review?(smichaud)
Attachment #696027 - Flags: review?(bugs)
Attachment #696151 - Flags: review?(smichaud)
Attachment #696151 - Flags: review?(bugs)
Comment on attachment 696151 [details] [diff] [review]
Patch

(I don't have a mac to test this.)
Attachment #696151 - Flags: review?(bugs) → review+
This bug only happens with the Magic Mouse -- not the trackpad.
Summary: CMD + swipe left to open previous location in new tab is broken → CMD + swipe left to open previous location in new tab is broken with Magic Mouse
Comment on attachment 696151 [details] [diff] [review]
Patch

This looks fine to me, and works on my Mac.  (Though I almost never use a Magic Mouse, so I only did very minimal testing.)

I've started an all-platform tryserver build to test your test changes.  Once that's done (presuming I don't see any failures caused by your patch), I'll r+ the patch.
Comment on attachment 696151 [details] [diff] [review]
Patch

These all-platform tryserver builds take forever.

But it's better than 95% done, and there haven't been any non-spurious test failures.  That's good enough for me.
Attachment #696151 - Flags: review?(smichaud) → review+
https://hg.mozilla.org/mozilla-central/rev/888bf659fa9f
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 20
You need to log in before you can comment on or make changes to this bug.