410112 - Opt-click in scroll well should toggle Next Page/Jump to Here behavior, not Shift-Click

Status: RESOLVED FIXED

(Core :: Widget: Cocoa, defect, P1)

Description

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

On Mac OS X using native scrollbars, Opt-click in the scroll well toggles (reverses) the Next Page/Jump to Here behavior.  Branch builds and other Mac apps behave this way.

Currently, fake scrollbars use Shift-click in the scroll well to do this, which is incorrect; shift should have no effect (i.e., shift-click = click).

(Note that due to bug 377181, this shortcut is currently the only way to get "jump to here" behavior.  Also note that if bug 377181 is fixed before anyone gets to  this, you'll need to quit/restart the app to test any changes to the System Prefs setting, unless bug 389775 has also been fixed.)

Comment 1

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

At first I thought this would be a simple ifdef MACOSX-style fix, but it looks like the code is hard-wired to always treat shift as jump-to-here, whereas on Mac OS X option is a toggle to the current system prefs setting, so the code would have to check the state of the System Prefs, too :(

Comment 2

[Stan Shebs]

I have a patch for this, but when jumping to here, it goes to the right position, then immediately jumps down a half-page, not clear why changing the decision process changes the behavior.

Comment 3

[Stan Shebs]

Attachment: Correct but ugly fix

In case of laptop/plane crash, here is a correct but inelegant fix. It needs to be abstracted further, so that the whole shift vs option is per-platform, not visible at this level.

Comment 4

[Stan Shebs]

Attachment: Improved version of patch

Upon further study, this is about as elegant as it's going to get; it would be a bunch of extra machinery to send events down to widget-land just to make the one Mac-specific test, and the three slightly different tests are each sufficiently readable once the basic preference check is abstracted into a helper.

Comment 5

[Josh Aas]

Comment on attachment 301610 [details] [diff] [review]
Improved version of patch

+              (static_cast<nsMouseEvent*>(aEvent)->isAlt ? !GetScrollToClick() : GetScrollToClick() ) ) ||

Extra spaces between the last ") ) )"

+  if (((nsMouseEvent *)aEvent)->isAlt ? !GetScrollToClick() : GetScrollToClick() )

Extra space at the end here too

Comment 6

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Why don't you pass an event parameter to GetScrollToClick and put the #ifdef MAC inversion in there instead?

Comment 7

[Stan Shebs]

There is some impedance mismatch - the first and third callsites use nsMouseEvent, and the middle one uses an nsIDOMEvent. Additionally, the first and second sites include a test for middle button, but the third does not (although oddly, Linux middle-button doesn't seem to exhibit the jumpiness fixed by the third callsite). So while in theory things could be simplified further, in practice it seems like it would be safer to post it as a post-fx3 analysis/cleanup bug.

Comment 8

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 301610 [details] [diff] [review]
Improved version of patch

unifying DOM and widget events would definitely help.

+              (static_cast<nsMouseEvent*>(aEvent)->isAlt ? !GetScrollToClick() : GetScrollToClick() ) ) ||

GetScrollToClick() != isAlt
(occurs twice)

+  scrollToClick = GetScrollToClick();
+  if (invertScrollToClick)
+    scrollToClick = !scrollToClick;

scrollToClick = GetScrollToClick() != invertScrollToClick;

Comment 9

[Stan Shebs]

Attachment: Further improved patch

Improved per suggestion.

Comment 10

[Josh Aas]

landed on trunk