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

Status: RESOLVED FIXED

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

Description

[Tom Fyuri]

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.

Comment 1

[Masatoshi Kimura [:emk]]

That's expected. Wheel related prefs are completely rewritten due to implementing D3E wheel event.

Comment 2

[Markus Stange [:mstange]]

The new prefs are documented here: https://wiki.mozilla.org/Gecko:Mouse_Wheel_Scrolling#Preferences_for_customizing_delta_values_and_default_action

Comment 3

[Konstantin Kletschke]

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

[Krzysztof Kotlenga]

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.

Comment 5

[Tom Fyuri]

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. :)

Comment 6

[Masatoshi Kimura [:emk]]

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.

Comment 7

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

(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.

Comment 8

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

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.

Comment 9

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

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

Comment 10

[Masatoshi Kimura [:emk]]

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

[Adam Smith]

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.

Comment 12

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

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.

Comment 14

[Konstantin Svist]

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?

Comment 15

[Masatoshi Kimura [:emk]]

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

[Konstantin Svist]

Using linux here :)

Comment 17

[Masatoshi Kimura [:emk]]

Attachment: 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.

Comment 18

[Masatoshi Kimura [:emk]]

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/[e-mail]-b87f99b1eae7/>
2. Set "mousewheel.default.action.override_x" to integer value 2.

Comment 19

[Krzysztof Kotlenga]

Works for me. (I also had to invert mousewheel.default.delta_multiplier_x to get the expected direction.)

Comment 20

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

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.

Comment 21

[Masatoshi Kimura [:emk]]

Attachment: 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.

Comment 22

[Masatoshi Kimura [:emk]]

https://tbpl.mozilla.org/?tree=Try&rev=709fe73be772

Comment 23

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

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.

Comment 24

[Masatoshi Kimura [:emk]]

Attachment: 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.

Comment 25

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

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

Comment 26

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

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.

Comment 27

[Masatoshi Kimura [:emk]]

Attachment: Implement mousewheel.*.action.override_x, v3

Folded two patches and added tests and comments.

Try result:
https://tbpl.mozilla.org/?tree=Try&rev=799e83255b38

Comment 28

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

Comment on attachment 689962 [details] [diff] [review]
Implement mousewheel.*.action.override_x, v3

Could you perhaps rename mActionsX to mOverriddenActionsX or mOverrideActionsX

Comment 29

[Masatoshi Kimura [:emk]]

Attachment: Implement mousewheel.*.action.override_x. r=smaug

patch for check in

Comment 30

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

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.

Comment 31

[(no longer active)]

https://hg.mozilla.org/mozilla-central/rev/15cf1f943bc9

Comment 32

[Lukas Blakk [:lsblakk] use ?needinfo]

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.

Comment 33

[Michael Tokarev]

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

[mephinet]

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/