Closed
Bug 786120
Opened 12 years ago
Closed 12 years ago
There should be default action settings for each direction which can override current settings
Categories
(Core :: DOM: UI Events & Focus Handling, defect)
Core
DOM: UI Events & Focus Handling
Tracking
()
RESOLVED
FIXED
mozilla20
People
(Reporter: tomfyuri, Assigned: emk)
References
Details
(Keywords: regression)
Attachments
(1 file, 4 obsolete files)
14.64 KB,
patch
|
emk
:
review+
|
Details | Diff | Splinter Review |
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.
Severity: normal → minor
Keywords: regression
Assignee | ||
Comment 1•12 years ago
|
||
That's expected. Wheel related prefs are completely rewritten due to implementing D3E wheel event.
Comment 2•12 years ago
|
||
The new prefs are documented here: https://wiki.mozilla.org/Gecko:Mouse_Wheel_Scrolling#Preferences_for_customizing_delta_values_and_default_action
Comment 3•12 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•12 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.
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. :)
Assignee | ||
Comment 6•12 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 → ---
Comment 7•12 years ago
|
||
(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.
Updated•12 years ago
|
Component: Untriaged → Event Handling
Product: Firefox → Core
Version: 17 Branch → Trunk
Comment 8•12 years ago
|
||
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•12 years ago
|
||
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•12 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•12 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.
Comment 12•12 years ago
|
||
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
Comment 14•12 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•12 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•12 years ago
|
||
Using linux here :)
Assignee | ||
Comment 17•12 years ago
|
||
- 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•12 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•12 years ago
|
||
Works for me. (I also had to invert mousewheel.default.delta_multiplier_x to get the expected direction.)
Flags: needinfo?
Comment 20•12 years ago
|
||
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•12 years ago
|
||
(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•12 years ago
|
||
Comment 23•12 years ago
|
||
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•12 years ago
|
||
(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 25•12 years ago
|
||
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
Updated•12 years ago
|
Attachment #688979 -
Flags: review?(bugs) → review+
Comment 26•12 years ago
|
||
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•12 years ago
|
||
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 28•12 years ago
|
||
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•12 years ago
|
||
patch for check in
Attachment #689962 -
Attachment is obsolete: true
Attachment #690028 -
Flags: review+
Assignee | ||
Updated•12 years ago
|
Keywords: checkin-needed
Comment 30•12 years ago
|
||
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
Comment 31•12 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 12 years ago → 12 years ago
Resolution: --- → FIXED
Updated•12 years ago
|
Comment 32•12 years ago
|
||
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•12 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•12 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/
Updated•6 years ago
|
Component: Event Handling → User events and focus handling
Updated•6 years ago
|
Type: enhancement → defect
You need to log in
before you can comment on or make changes to this bug.
Description
•