732364 - Scrolling vertically is near impossible after scrolling horizontally in some pages

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Reuben Morais [:reuben]]

Samsung Galaxy S2, running CyanogenMod 7.1 and today's Maple nightly build.

STR:
1. Go to https://github.com/travis-ci/travis-hub/blob/master/lib/travis/hub.rb
2. Scroll down the code a bit.
3. Scroll the code to the left.
4. Try to scroll down or up again.

Actual results:
Scrolling vertically is almost impossible after the horizontal scroll.
This may be related to bug 705877.

Expected results:
Smooth scrolling :)

Comment 1

[Mark Finkle (:mfinkle) (use needinfo?)]

You might be hitting our axis locking. Are you lifting your finger after step #3?

Comment 2

[Mark Finkle (:mfinkle) (use needinfo?)]

I don't see this happening so not blocking

Comment 3

[Kartikaya Gupta (email:[e-mail])]

I was able to reproduce this on the Galaxy Nexus, and I don't think it's axis locking. The github source viewer has some sort of subdocument that is scrollable, but it isn't behaving like the normal iframe scrolling bugs or expected behaviour. This will need more investigation.

Comment 4

[Reuben Morais [:reuben]]

(In reply to Mark Finkle (:mfinkle) from comment #1)
> You might be hitting our axis locking. Are you lifting your finger after
> step #3?

Sorry, this got lost in bugmail. Yes, I am.

Comment 6

[Les Orchard [:lorchard]]

Just to copy my STR over from the bug I just duped:

Running Nightly on a Motorola Photon phone running Gingerbread.

1) Subscribe to a graphics-heavy feed like joystiq.com on reader.google.com

2) Visit reader.google.com/i on an Android device running Nightly

3) Tap on some feed entries to expand them.

3) Feed entries with wide graphics can be panned from side to side, but the page itself cannot be panned vertically unless I very carefully swipe on what feels like a single pixel gutter on the extreme left or right of the page.

FWIW, I lift my finger plenty of times. It just seems as if the horizontally-scrolling part swallows and ignores all vertical scrolling events altogether.

Comment 8

[(no longer active)]

This happens in Google Reader all the time.  I hear that's a popular web app!  ;-)

Comment 9

[(no longer active)]

STR: bug 742175 comment 0.

Comment 10

[Kartikaya Gupta (email:[e-mail])]

Attachment: (1/2) Fix _elementCanScroll

A few changes here:

1. the scroll coordinates coming in would sometimes be 1.99234 instead of 2.0, and they would get truncated, so make sure to round them first.

2. _elementCanScroll always checked both axes, even if the user was only scrolling on one axis. This meant that if the horizontal axis was scrollable and the vertical axis was not, and the requested scroll was something like (0, 5), _elementCanScroll would return true even though it was actually impossible to scroll in the direction the user was requesting. The new code only returns true if the axis is scrollable AND there is a nonzero scroll amount on that axis.

Comment 11

[Kartikaya Gupta (email:[e-mail])]

Attachment: (2/2) Apply axis locking on subdocument scrolling

Axis locking was being ignored on subdocument scrolling, so if you had a subdocument that was horizontally scrollable and not vertical scrollable, you basically had to move your finger in a strict vertical line for _elementCanScroll to return false and pan the main document. Applying axis locking fixes this problem because minute horizontal variations are filtered out. Also it makes the behaviour more consistent in general.

Comment 12

[Chris Lord [:cwiiis]]

Comment on attachment 620332 [details] [diff] [review]
(1/2) Fix _elementCanScroll

Review of attachment 620332 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good to me.

::: mobile/android/chrome/content/browser.js
@@ +2552,5 @@
>        // the user wanted, and neither can any non-root sub-frame, cancel the
>        // override so that Java can handle panning the main document.
>        let data = JSON.parse(aData);
> +      data.x = Math.round(data.x);
> +      data.y = Math.round(data.y);

Do we need to round here? A comment about it would be good.

@@ +2821,4 @@
>  
> +    if (x < 0 && elem.scrollLeft > 0) {
> +      scrollX = true;
> +    } else if (x > 0 && elem.scrollLeft < (elem.scrollWidth - elem.clientWidth)) {

I think this would still be readable as a single if statement, just putting the clauses in brackets on separate lines.

@@ +2826,5 @@
>      }
>  
> +    if (y < 0 && elem.scrollTop > 0) {
> +      scrollY = true;
> +    } else if (y > 0 && elem.scrollTop < (elem.scrollHeight - elem.clientHeight)) {

ditto here.

Comment 13

[Chris Lord [:cwiiis]]

Comment on attachment 620334 [details] [diff] [review]
(2/2) Apply axis locking on subdocument scrolling

Review of attachment 620334 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good to me.

Comment 14

[Kartikaya Gupta (email:[e-mail])]

Landed with review comments addresed.

https://hg.mozilla.org/mozilla-central/rev/08ea94cb6365
https://hg.mozilla.org/mozilla-central/rev/948a0d72d99f

Comment 15

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 620332 [details] [diff] [review]
(1/2) Fix _elementCanScroll

[Approval Request Comment]
Regression caused by (bug #): 
User impact if declined: scrolling on iframes/subdocuments behaves poorly when the element is scrollable on both axes
Testing completed (on m-c, etc.): locally, on m-c for a couple of days
Risk to taking this patch (and alternatives if risky): mobile-only, low risk
String changes made by this patch: none

Comment 16

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 620334 [details] [diff] [review]
(2/2) Apply axis locking on subdocument scrolling

[Approval Request Comment]

same as above

Comment 17

[Kartikaya Gupta (email:[e-mail])]

https://hg.mozilla.org/releases/mozilla-aurora/rev/4622e10cc004
https://hg.mozilla.org/releases/mozilla-aurora/rev/265161b4c581

Comment 18

[Les Orchard [:lorchard]]

FWIW, unless I'm imagining things, it looks like this fix landed in my Aurora on Android after I updated. And, it seems to work great. 

No more issues scrolling through Google Reader, so I am a happy camper. Thank you!

Comment 19

[Catalin Suciu [:csuciu]]

Verified fixed on Nightly 15.0a1 (2012-05-21)
                  Aurora 14.0a2 (2012-05-21)
Samsung Nexus (4.0.2)