Closed Bug 709924 Opened 8 years ago Closed 8 years ago

Scrolling input fields into view is incorrect at zoom levels other than 1.0

Categories

(Firefox for Android :: General, defect, P1)

ARM
Android
defect

Tracking

()

VERIFIED FIXED
Firefox 12
Tracking Status
firefox11 --- verified
fennec 11+ ---

People

(Reporter: kats, Assigned: kats)

References

Details

Attachments

(1 file, 1 obsolete file)

On a Galaxy tab (or presumably any device with a VKB):

1. Go to http://people.mozilla.org/~kgupta/bug/696319.html
2. Zoom in by some noticeable amount (usually my zoom ends up being around 1.2 - 1.5)
3. Scroll down to the input fields near the bottom of the page and position the viewport so that the input fields are at the bottom of the visible area
4. Tap to give focus to the input field

Expected result: the input field scrolls into view
Actual result: the page scrolls but the input field is not in view

Note that this is definitely zoom-related, since skipping step #2 in the STR above results in the correct expected behaviour.

I also added some debug logging to see what the problem was. With the following in scrollToFocusedInput in browser.js:

+let tab = BrowserApp.getTabForBrowser(aBrowser);
+let win = aBrowser.contentWindow;
+dump("pre scrollY is " + aBrowser.contentWindow.scrollY + " scale is " + tab._viewport.zoom + " offsetY is " + tab._viewport.offsetY);
 focused.scrollIntoView(false);
+dump("post scrollY is " + aBrowser.contentWindow.scrollY + " scale is " + tab._viewport.zoom + " offsetY is " + tab._viewport.offsetY);

I get this output:

with zooming in:
E/GeckoConsole( 2865): pre scrollY is 4430 scale is 1.252200722694397 offsetY is 592
E/GeckoConsole( 2865): post scrollY is 4289 scale is 1.252200722694397 offsetY is 592

without zooming in:
E/GeckoConsole( 2865): pre scrollY is 3924 scale is 0.8163265306122449 offsetY is 592
E/GeckoConsole( 2865): post scrollY is 4289 scale is 0.8163265306122449 offsetY is 592

Note that in both cases the scrollY is set to 4289 by scrollIntoView, and therefore scrollIntoView is clearly ignoring the zoom factor, even though the pre-scroll scrollY values do take into account the zoom factor. This is likely due to the way we handle zooming the viewport in browser.js
Priority: -- → P1
Hardware: All → ARM
Just to note: the analysis I posted in comment 0 was somewhat wrong. The bug only manifests when we try to do a scrollIntoView() while zoomed in and with a viewportExcess != 0, because gecko doesn't take into account the shrunken viewport when calculating the max scroll position. It took me a few days to properly understand what was going on here, so let me know if there's anything I can comment better to make the review easier.
Comment on attachment 584099 [details] [diff] [review]
(1/2) Ensure gecko gets up-to-date viewport coordinates

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

r+, but you may want to get pcwalton's opinion - The commit message could be a bit clearer too. Prior to this, we sent updates when in the danger zone and when the user isn't touching the screen, now we've changed that to an or.
Attachment #584099 - Flags: review?(chrislord.net) → review+
Comment on attachment 584100 [details] [diff] [review]
Adjust viewportExcess to account for zoom

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

Looks good, and very well commented - r+ from me.
Attachment #584100 - Flags: review?(chrislord.net) → review+
Comment on attachment 584099 [details] [diff] [review]
(1/2) Ensure gecko gets up-to-date viewport coordinates

This is generally fine, now that we have gralloc and viewport updates aren't so painful. This will regress performance a bit on non-grallocable devices though, so we should try extra hard to get incremental/async texture upload in before release.
Attachment #584099 - Flags: feedback?(pwalton) → feedback+
Wait, does this mean that we render when a finger is down again?
(In reply to Patrick Walton (:pcwalton) from comment #7)
> Wait, does this mean that we render when a finger is down again?

Yes, but there are scenarios were this is desired. It's possible to pan more than screen dimensions while keeping a finger down at all times - pan with one finger, put down another finger and then lift the first finger, continue panning. So I think it makes sense to render if we're going to checkerboard regardless of whether a finger is down or not.
(In reply to Kartikaya Gupta (:kats) from comment #8)
> (In reply to Patrick Walton (:pcwalton) from comment #7)
> > Wait, does this mean that we render when a finger is down again?
> 
> Yes, but there are scenarios were this is desired. It's possible to pan more
> than screen dimensions while keeping a finger down at all times - pan with
> one finger, put down another finger and then lift the first finger, continue
> panning. So I think it makes sense to render if we're going to checkerboard
> regardless of whether a finger is down or not.

Ok, but can we not render when a finger is down during zooming at least? iOS does not render at all when zooming, even when checkerboarding, and it really helps its interactive performance.

Also, we definitely should not render when a finger is down unless we're *actually* checkerboarding, not just about to. Rendering when a finger is down should be treated as a last-resort measure, because any delay is really, really noticeable.
(To give a data point here, Andreas observed that our zoom performance is the absolute best-of-breed -- it feels like an iPhone, but on Android -- and I really don't want to regress it, especially when the iPhone and iPad get away with not rendering at all on zoom, even when it results in very noticeable checkerboards.)
Attachment #584099 - Flags: feedback+ → feedback-
Just to note, the ICS/Honeycomb browsers also don't render during zooming and feel great - they even do a bonus fade-in of the extra content when redrawing, but that's a separate bug :)

Not rendering at all during scale-changes (either via gesture or animation) sounds entirely reasonable to me.
I was investigating this further, and uncovered bug 713729, which was causing the finishAnimation() to not get run after a pan with no fling. With that fixed, it turns out that the forceRedraw thing kicks in exactly when I need it to (and when it's supposed to be happening), so I don't need the redrawHint() to change at all. I'll take that patch out, and if we want to change that behaviour we can do track it with another bug.
Attachment #584100 - Attachment description: (2/2) Adjust viewportExcess to account for zoom → Adjust viewportExcess to account for zoom
Try run showed green: https://tbpl.mozilla.org/?tree=Try&rev=d9f8a97679fa

Landed on m-i: https://hg.mozilla.org/integration/mozilla-inbound/rev/6e4452ca6da2
Status: NEW → ASSIGNED
Whiteboard: [inbound], [leave open after inbound merge]
Target Milestone: --- → Firefox 12
Attachment #584100 - Flags: approval-mozilla-aurora?
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Comment on attachment 584100 [details] [diff] [review]
Adjust viewportExcess to account for zoom

[Triage Comment]
Mobile only, approving for Aurora.
Attachment #584100 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
tracking-fennec: --- → 11+
Verified fixed on Firefox 11.0a2 (2012-01-10)
                          12.0a1 (2012-01-10)
Device: Samsung Galaxy SII (Android 2.3)
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.