Fixed: scroll wheel speed is way to low in firefox [+1] #2013

Merged
merged 3 commits into from Jul 23, 2014

Conversation

Projects
None yet
6 participants
Contributor

daboe01 commented Oct 29, 2013

I increased scroll wheel speed to a sensible value

Fixed: scroll wheel speed is way to low in firefox
I increased scroll wheel speed to a sensible value

cappbot commented Oct 30, 2013

Milestone: Someday. Label: #new. What's next? A reviewer should examine this issue.

Owner

primalmotion commented Nov 19, 2013

Do you have any test case that can reproduce the error?

@@ -913,7 +913,7 @@ var resizeTimer = nil;
{
// Find the scroll delta
var deltaX = _DOMScrollingElement.scrollLeft - 150,
- deltaY = (_DOMScrollingElement.scrollTop - 150) || (aDOMEvent.deltaY === undefined ? 0 : aDOMEvent.deltaY);
+ deltaY = (_DOMScrollingElement.scrollTop - 150) || (aDOMEvent.deltaY === undefined ? 0 : (aDOMEvent.deltaY*6));
@primalmotion

primalmotion Nov 19, 2013

Owner

Style is not correct. should be

(aDOMEvent.deltaY * 6)
Contributor

daboe01 commented Nov 19, 2013

scrolling by scrollwheel is universally too slow in firefox.
see ahankinsons comment in PR#1717 ..."with the patch they do (but very slowly)."

Contributor

BlairDuncan commented Nov 19, 2013

I just checked on a Windows FF and it does in fact scroll very slowly...
My mac browsers FF, Chrome, Safari scroll quite fast as it is.
Does this make THEM scroll even faster than they do now?
Just thinking that it could be a FF issue...
http://support.mozilla.org/en-US/questions/948356

Contributor

daboe01 commented Nov 20, 2013

in fact not all FF versions are affected. my patch makes sure that only the appropriate FF versions are speeded up.
the other browsers are not changed by my patch.
best greetings,
daniel

Contributor

ahankinson commented Feb 21, 2014

I'm sure others will have a comment on whether or not this should be adopted, but in a cursory code review, you should encode the factor in the patch ("*6") as a constant and use that instead of the magic number. Something like "FIREFOX_SCROLL_FACTOR".

-#new
+#needs-improvement
+bug

cappbot commented Feb 21, 2014

Milestone: Someday. Label: bug. What's next? A reviewer should examine this issue.

Contributor

daboe01 commented May 24, 2014

+1

@daboe01 daboe01 changed the title from Fixed: scroll wheel speed is way to low in firefox to Fixed: scroll wheel speed is way to low in firefox [+1] May 24, 2014

cappbot commented May 24, 2014

Milestone: Someday. Vote: 1. Label: bug. What's next? A reviewer should examine this issue.

Contributor

daboe01 commented May 24, 2014

this is a redone PR#1717 (merged one year ago) to fix a major shortcoming of capp on firefox.
could this please be merged?
best greetings and many thanks,
daniel

@@ -913,7 +915,7 @@ var resizeTimer = nil;
{
// Find the scroll delta
var deltaX = _DOMScrollingElement.scrollLeft - 150,
- deltaY = (_DOMScrollingElement.scrollTop - 150) || (aDOMEvent.deltaY === undefined ? 0 : aDOMEvent.deltaY);
+ deltaY = (_DOMScrollingElement.scrollTop - 150) || (aDOMEvent.deltaY === undefined ? 0 : (aDOMEvent.deltaY * FIREFOX_SCROLLWHEEL_FACTOR));
@aljungberg

aljungberg May 24, 2014

Owner

Wouldn't this be applied for every browser, not just Firefox, and therefore cause other browsers to scroll extremely fast?

And it this is intentionally applied to every browser, why is it called FIREFOX_SCROLL_FACTOR and not Y_SCROLL_FACTOR or something to reflect that it's a general thing?

Contributor

daboe01 commented May 24, 2014

Please let me explain why merging this is not so risky IMHO:

deltaY is defined in scrollwheel events in certain FF versions. delaY does not give coordinates (as scrollTop does) but "ticks" of arbitrary unit.

deltaY was never queried in pre-PR #1717 cappuccino. back at that time, scroll wheeling simply did not work in FF. This was because scrollTop has the undefined value in scrollwheel events there.

PR #1717 introduced code that coalesces a missing scrolling offset with the scroll ticks (deltaY) only when scrollTop is undefined (this is currently only in FF when scroll-wheeling). i unfortunately forgot to do the appropriate scaling from ticks to coordinates in #1717.

PR #2013 rectifies this by scaling ticks to pixels (factor 6 works best in my testing)

It does not change any other semantics in comparison to the (already merged) PR #1717!

I use #2013 in production for several months and scrolling is fine in FF, Safari and Chrome.

Best greetings,

Daniel

Owner

primalmotion commented Jul 19, 2014

@aljungberg what should we do with this one?

Owner

aljungberg commented Jul 23, 2014

@primalmotion let's merge it, I'll do it

+#accepted
assignee=aljungberg
milestone=0.9.8

cappbot commented Jul 23, 2014

Milestone: 0.9.8. Vote: 1. Labels: #accepted, bug. What's next? A reviewer should examine this issue.

Owner

aljungberg commented Jul 23, 2014

Hmm sorry, the fix doesn't seem to work well. Yes, it makes mouse-wheel scrolling faster with Firefox, but it also makes touch scrolling impossibly fast in Firefox.

To verify:

  1. Load the CPScrollView2 test app and enable "Big doc".
  2. Without this patch, use touch scrolling to go down in the main scroll view (using a Magic Mouse, Mac trackpad, probably iPad etc). Note it's smooth and goes at the expected speed.
  3. Without this patch, use mouse wheel scrolling. It's slow but it works.
  4. Now apply the patch and reload. Now mouse wheel scrolling is okay but when you use touch scrolling the view scrolls far too fast and is hard to control.
Owner

aljungberg commented Jul 23, 2014

(All of this in Firefox of course)

Owner

aljungberg commented Jul 23, 2014

Alright looks like the right solution here is to recognise that when receiving the "wheel" event, the deltaMode flag needs to be checked. For a discrete "stepping" mouse wheel, Firefox sets this to "1" indicating 1 deltaY unit is one "line" worth of scrolling. For a touch device, Firefox sets it to "0" to indicate deltaY is in pixels.

@aljungberg aljungberg merged commit b439e0c into cappuccino:master Jul 23, 2014

1 check passed

default The Travis CI build passed
Details

aljungberg added a commit that referenced this pull request Jul 23, 2014

Merge pull request #2013 from daboe01/fix-scrollwheel-speed-on-firefox
Fixed: scroll wheel speed is way to low in firefox [+1]

aljungberg added a commit that referenced this pull request Jul 23, 2014

aljungberg added a commit that referenced this pull request Jul 23, 2014

New: support line-by-line scrolling with old scroll wheel devices in …
…Firefox.

Without this change, using a traditional scroll wheel mouse with discrete steps would result in very slow scrolling in Firefox.

With this fix we handle these events like in Cocoa: as "[im]preciseScrollingDeltas", which `CPScrollView` in turn knows to apply the configurable line scroll amount for. For backwards compatibility, [event deltaX] and [event deltaY] are premultiplied with a suitable constant, while [event scrollingDeltaX] and [event scrollingDeltaY] show the true values needed.

Browsers other than Firefox seem to always send pixel scrolling information even for old style scrolling devices, at least on the Mac, so are not affected by this change.

Fixes #2013.

@aljungberg aljungberg self-assigned this Jul 23, 2014

Owner

aljungberg commented Jul 23, 2014

Okay fixed with a more accurate solution.

Thanks for the pull request.

+#fixed

cappbot commented Jul 23, 2014

Assignee: aljungberg. Milestone: 0.9.8. Vote: 1. Labels: #fixed, bug. What's next? This issue is considered successfully resolved.

Contributor

daboe01 commented Jul 23, 2014

cool! thank you so much!
best greetings,
daniel

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment