There should be default action settings for each direction which can override current settings

RESOLVED FIXED in Firefox 20

Status

()

Core
Event Handling
--
enhancement
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: Tom Fyuri, Assigned: emk)

Tracking

({regression})

Trunk
mozilla20
regression
Points:
---
Dependency tree / graph

Firefox Tracking Flags

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

Details

Attachments

(1 attachment, 4 obsolete attachments)

(Reporter)

Description

5 years ago
User Agent: Mozilla/5.0 (X11; Linux i686; rv:17.0) Gecko/17.0 Firefox/17.0
Build ID: 20120810012545

Steps to reproduce:

I tried downloading and installing every build that came out in august and any build after 12th august (including 12th august) has a regression with option mousewheel.horizscroll.withnokey.action for me, setting it in about:config makes no difference and I can't press mouse4/mouse5 and go back/next pages. So either I use last build and have to hold shift all time or i use build before 12th august, set mousewheel.horizscroll.withnokey.action (numlines,sysnumlines) to my likings and enjoy using mouse4 and mouse5 for go back/next pages.

I'm using Linux 32bit.
Regression between these builds:
11 august: https://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2012/08/2012-08-11-mozilla-central-debug/firefox-17.0a1.en-US.debug-linux-i686.tar.bz2
12 august: https://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2012/08/2012-08-12-mozilla-central-debug/firefox-17.0a1.en-US.debug-linux-i686.tar.bz2

This is my first firefox bug-report ever.


Actual results:

mousewheel.horizscroll.withnokey.action regression in firefox-nightly after 11th august 2012. It doesn't respond however value you change it to.


Expected results:

It was fine before 12th august 2012.
(Reporter)

Updated

5 years ago
Severity: normal → minor
Keywords: regression
(Assignee)

Comment 1

5 years ago
That's expected. Wheel related prefs are completely rewritten due to implementing D3E wheel event.
Blocks: 719320
Status: UNCONFIRMED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → INVALID
The new prefs are documented here: https://wiki.mozilla.org/Gecko:Mouse_Wheel_Scrolling#Preferences_for_customizing_delta_values_and_default_action

Comment 3

5 years ago
Hello,

I read the page with the documentation of the changes. I used 
mousewheel.withnokey.action set to 1 to scroll entire pages like page up and page down. Is this not possible anymore or can this emulated (kind of) with ne new setting scheme?
Kind Regards
Konsti

Comment 4

5 years ago
As I see it, in Firefox 17.0 it's now impossible to do history back/forward with the horizontal scroll wheel (or tilt wheel, which are fairly common). If it's still possible to have an effect of mousewheel.horizscroll.withnokey.action=2 with the new scheme, please tell us how. Otherwise it's a usability regression and the bug should be reopened.
(Reporter)

Comment 5

5 years ago
Running Firefox 17.0 and mousewheel.horizscroll.withnokey.action=2 doesn't work. It was like this all this time. Since I made this report...
Wasn't this like, new feature? In other words, horizscroll and vertiscroll CAN'T have separate actions now. I kinda got used to it now. :)
(Reporter)

Updated

5 years ago
Hardware: x86 → All
(Assignee)

Comment 6

5 years ago
Reopening to ask an opinion.
Nakano-san, what do you think about this? Indeed both deltaX and deltaY can be non-zero about a device such as touchpad or trackball, it is not a reasonable assumption about a tilt-wheel IMO.
Status: RESOLVED → UNCONFIRMED
Resolution: INVALID → ---
(In reply to Masatoshi Kimura [:emk] from comment #6)
> Indeed both deltaX and deltaY can
> be non-zero about a device such as touchpad or trackball, it is not a
> reasonable assumption about a tilt-wheel IMO.

I'm not sure which problem do you worry about.

Current implementation uses widget::WheelEvent::GetPreferredIntDelta() for deciding which delta value should be used for history-go-back/forward and zoom-in/out.
http://mxr.mozilla.org/mozilla-central/source/widget/nsGUIEvent.h#1242

I'm not sure if this is the best algorithm for all devices all over the world.

But I think it doesn't make sense that we separate the action handling in XP level. And also, some touch pad devices may dispatch WM_MOUSEWHEEL and WM_MOUSEHWHEEL for an action (diagonal scroll operation). We cannot detect the differences.

If you have better idea of the method, let's use it.
Component: Untriaged → Event Handling
Product: Firefox → Core
Version: 17 Branch → Trunk
Ah, do you meant that we should separate the action prefs to x and y? If you have some good idea, I'd like to take it.
Tom:

Although, I'm not familiar with Linux, if you could map button8/9 to the operation only on Firefox, you could use horizontal wheel action as back/forward button:
http://mxr.mozilla.org/mozilla-central/source/widget/gtk2/nsWindow.cpp#2716
(Assignee)

Comment 10

5 years ago
We don't have to detect whether the device is touch pad. The settings will not be enabled unless users modify the about:config, and users will know whether their device is touch pad or not.

Comment 11

5 years ago
However it's done, I would be very grateful if this functionality is returned. I am not upgrading some of my computers specifically because of this issue.

Thank you.
Okay, I think we should add mousewheel.*.action.override_[xyz]. It can take same values as mousewheel.*.action and -1. If the value is -1, it does NOT override the current setting.

If I had time, we'd do this. But I have some more important work for now. If somebody wants to do this, feel free to take this. I will give some advice.
Severity: minor → enhancement
Status: UNCONFIRMED → NEW
Ever confirmed: true
OS: Linux → All
Summary: mousewheel.horizscroll.withnokey.action regression between 11th august and 12th august in firefox-nightly → There should be default action settings for each direction which can override current settings
Duplicate of this bug: 815415

Comment 14

5 years ago
Oh, so the problem that touchpad side scrolling uses same keycodes as mouse's wheel tilt?
That's pretty annoying... isn't there a way of telling if it's a mouse or touchpad?
(Assignee)

Comment 15

5 years ago
On Windows, it really depends on your mouse driver. Sometimes it generates keycodes, sometimes it generates scroll message, and sometimes it generates horizontal wheel message.

Comment 16

5 years ago
Using linux here :)
(Assignee)

Comment 17

5 years ago
Created attachment 688441 [details] [diff] [review]
Implement mousewheel.*.action.override_x

- I added only .override_x because I counldn't imageine a read-world use case
  of overrides for other directions. It looks a bit overgeneralization to me.
  Also, old prefs provide only settings for vertical/horizontal. Furthermore,
  the current ComputeActionFor function doesn't even consider deltaZ!
  If you disagree, please teach me why .override_[yz] is useful.
- Fixed an unspecified behavior. Implementations may remove the if-statement
  testing the range of Action values for similar reason to the following code:
    bool b = <an int value>;
    // The condition will never hold, so the if-statement may be removed.
    if (b != true && b != false) {
      ...
    }
- Using MOZ_ENUM_TYPE to save the space.
Attachment #688441 - Flags: feedback?(masayuki)
(Assignee)

Comment 18

5 years ago
https://tbpl.mozilla.org/?tree=Try&rev=b87f99b1eae7
Reporter or commenters:
Please report if the test build fixed the problem:
1. Install the test build from <http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/VYV03354@nifty.ne.jp-b87f99b1eae7/>
2. Set "mousewheel.default.action.override_x" to integer value 2.
Flags: needinfo?

Comment 19

5 years ago
Works for me. (I also had to invert mousewheel.default.delta_multiplier_x to get the expected direction.)
Flags: needinfo?
Comment on attachment 688441 [details] [diff] [review]
Implement mousewheel.*.action.override_x

Thank you, Kimura-san.

I think that your patch looks almost great. But:

> +  Action* actions = (aEvent->deltaY || aEvent->deltaZ) ? mActions : mActionsX;

This means that even if user wants to use this new pref on Mac, if the event's deltaY has a very little number when user wants to make horizontal wheel event, the override action won't work.

How about this?

bool deltaXPreferred = (NS_ABS(aEvent->deltaX) > NS_ABS(aEvent->deltaY) &&
                        NS_ABS(aEvent->deltaX) > NS_ABS(aEvent->deltaZ)); 
Action* actions = deltaXPreferred ? mActionsX: mActions;

And...

> -    return (mActions[INDEX_DEFAULT] == ACTION_SCROLL) ? ACTION_SCROLL :
> +    return (actions[INDEX_DEFAULT] == ACTION_SCROLL) ? ACTION_SCROLL :
>                                                          ACTION_NONE;

Please fix the indentation level of ACTION_NONE.
Attachment #688441 - Flags: feedback?(masayuki) → feedback+
(Assignee)

Comment 21

5 years ago
Created attachment 688727 [details] [diff] [review]
Implement mousewheel.*.action.override_x

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #20)
> > +  Action* actions = (aEvent->deltaY || aEvent->deltaZ) ? mActions : mActionsX;
> 
> This means that even if user wants to use this new pref on Mac, if the
> event's deltaY has a very little number when user wants to make horizontal
> wheel event, the override action won't work.
> 
> How about this?
> 
> bool deltaXPreferred = (NS_ABS(aEvent->deltaX) > NS_ABS(aEvent->deltaY) &&
>                         NS_ABS(aEvent->deltaX) > NS_ABS(aEvent->deltaZ)); 
> Action* actions = deltaXPreferred ? mActionsX: mActions;

Good idea. I've adopted it except for using std::abs instead of NS_ABS which has been replaced by bug 817574.
Attachment #688441 - Attachment is obsolete: true
Attachment #688727 - Flags: review?(bugs)
(Assignee)

Comment 22

5 years ago
https://tbpl.mozilla.org/?tree=Try&rev=709fe73be772
Oops, why don't you add the prefs to all.js? Such hidden prefs will probably cause similar bug reports since they won't find the prefs in about:config.
(Assignee)

Comment 24

5 years ago
Created attachment 688979 [details] [diff] [review]
Pref additions

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #23)
> Oops, why don't you add the prefs to all.js? Such hidden prefs will probably
> cause similar bug reports since they won't find the prefs in about:config.
Ugh, I saw only the top of attachment 650059 [details] [diff] [review] and misunderstand mousewheel.horizscroll.*.action wasn't defined in the pref.

Added .override_x prefs back to all.js.
Attachment #688979 - Flags: review?(bugs)
Comment on attachment 688727 [details] [diff] [review]
Implement mousewheel.*.action.override_x

This all needs some comments. It is pretty unclear how
mActionsX is different from mActions
Attachment #688979 - Flags: review?(bugs) → review+
Comment on attachment 688727 [details] [diff] [review]
Implement mousewheel.*.action.override_x

So, I'd like to see comments explaining why X is special and what mActionsX is about, and also some tests. No need for tons of tests, but some test
to prevent accidental regressions.
Attachment #688727 - Flags: review?(bugs) → review-
(Assignee)

Comment 27

5 years ago
Created attachment 689962 [details] [diff] [review]
Implement mousewheel.*.action.override_x, v3

Folded two patches and added tests and comments.

Try result:
https://tbpl.mozilla.org/?tree=Try&rev=799e83255b38
Attachment #688727 - Attachment is obsolete: true
Attachment #688979 - Attachment is obsolete: true
Attachment #689962 - Flags: review?(bugs)
Comment on attachment 689962 [details] [diff] [review]
Implement mousewheel.*.action.override_x, v3

Could you perhaps rename mActionsX to mOverriddenActionsX or mOverrideActionsX
Attachment #689962 - Flags: review?(bugs) → review+
(Assignee)

Comment 29

5 years ago
Created attachment 690028 [details] [diff] [review]
Implement mousewheel.*.action.override_x. r=smaug

patch for check in
Attachment #689962 - Attachment is obsolete: true
Attachment #690028 - Flags: review+
(Assignee)

Updated

5 years ago
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/15cf1f943bc9

Thank you, Kimura-san. This must help bug 819252.

Requesting tracking-firefox-esr17 ?. However, I'm not sure whether this should be fixed on ESR branch. If there are some users in this trouble, they feel inconvenience. If the environment is in a company or an organization, they may not be able to use non-ESR build. The patch's risk isn't high, I think.
Assignee: nobody → VYV03354
Blocks: 819252
Status: NEW → ASSIGNED
status-firefox-esr10: --- → unaffected
status-firefox17: --- → wontfix
status-firefox18: --- → wontfix
status-firefox19: --- → wontfix
status-firefox20: --- → affected
status-firefox-esr17: --- → affected
tracking-firefox-esr17: --- → ?
Keywords: checkin-needed
Target Milestone: --- → mozilla20
(Assignee)

Updated

5 years ago
Blocks: 819652
Depends on: 819766
https://hg.mozilla.org/mozilla-central/rev/15cf1f943bc9
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago5 years ago
Resolution: --- → FIXED

Updated

5 years ago
status-firefox20: affected → fixed
This doesn't meet criteria for ESR landings, if there were reports of significant user pain on ESR deployments we could consider for uplift but wouldn't do so without cause.
tracking-firefox-esr17: ? → -

Comment 33

5 years ago
Which reports of significant pain do you need? :)

I can report mine. I upgraded from esr10 to esr17 and had to go back, because esr17 was hardly usable for me due to exactly this issue, because this is a regression in esr17 compared with esr10.  But esr10 is not supported anymore, so I'm basically lost, together with all our users... ;)

Comment 34

5 years ago
For those of you stuck with esr17 on Linux: Avian's solution of remapping the pointer buttons works for me:
http://www.tablix.org/~avian/blog/archives/2012/12/tilt_wheel_actions_on_firefox_17/
You need to log in before you can comment on or make changes to this bug.