732016 - Maple: scrollIntoView does not clamp to the page size

Status: VERIFIED FIXED

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

Description

[Benoit Girard (:BenWa)]

When an element is scrolled into view the position isn't clamped to the page size.

STR:
1) Go to google.com.
2) Notice the input field auto selected.
3) Dismiss the onscreen keyboard.
4) The page position is now outside the edge of the page.

Comment 1

[Benoit Girard (:BenWa)]

Similarly it is possible to use window.scrollTo(x, y); to scroll the page outside the visible region.

Comment 3

[Tony Chung [:tchung]]

nom for triage team.  should be fixed before the merge

Comment 4

[Chris Lord [:cwiiis]]

Note, scrollTo purposefully doesn't clamp so that we can scroll to the outer extremes of the page (our zoom isn't taking into account by the scroll containers, so zooming in makes some area of the page to the lower-right inaccessible). scrollIntoView not clamping is a bug though.

Comment 5

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: patch

Kats, the blame for this shows the change comes merging maple to m-c

http://hg.mozilla.org/mozilla-central/rev/0742dd20781e

Comment 6

[Chris Lord [:cwiiis]]

Had a look at this... While I think a simple fix would be to change scrollToFocusedInput in browser.js to just bounds check scrollX/scrollY before setting them, it would really be much much nicer if the root scroll frame could be aware of the set resolution and modify things accordingly.

This would also fix problems with fixed position content becoming inaccessible when zooming in (possibly). The clampScrollPosition property could subsequently be removed.

Doing this properly would be a layout task. The quick fix ought to be simple, but I'd be entirely unsurprised to see this break again in either the same, or more subtle ways.

Changing the title of the bug as we explicitly set scrollTo *not* to clamp - that isn't a bug.

Comment 7

[Chris Lord [:cwiiis]]

Resetting bits that I must've accidentally changed... Not assigning to myself just yet :)

Comment 8

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

I don't think it's sufficient to fix the scrollIntoView case; javascript on the page can also call scrollTo with negative numbers and Gecko will obligingly scroll into overscroll. This was a problem in bug 733580, which was duped to here. While I do like gecko allowing us to scroll into that extra zoom area without having to use viewportExcess crud, I think just disabling the clamping altogether is probably a bad idea. We really want gecko to have a better idea of what the zoom is and how much scrolling should be allowed.

Comment 9

[Brad Lassey [:blassey] (use needinfo?)]

in that case I'm not the best assignee. Jet, can you recommend someone?

Comment 10

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

Comment on attachment 607061 [details] [diff] [review]
patch

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

Thanks, that explains why this is happening at least. However this fix just trades one regression for another, as it will prevent us from drawing and setting gecko's scroll position into the bottom-right corner when the zoom > 1.0. I'd like to fix this properly by making the scroll clamping aware of the zoom we're at somehow.

Comment 11

[Brad Lassey [:blassey] (use needinfo?)]

Just to note, this bug will be trivial to fix when bug 732971  lands, but it cannot be fixed until then

Comment 12

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

I wouldn't say "trivial". To fix this properly, we need to do three things:
1) Allow setting a resolution on content-window presshells
2) Have gecko take the resolution into account when clamping scroll coordinates
3) Turn off the disableScrollClamping thing in browser.js

I think bug 732971 covers step 1, and step 3 is trivial. But step 2 may or may not be trivial.

Comment 13

[Brad Lassey [:blassey] (use needinfo?)]

When I looked at the code a couple weeks ago it looked like clamping did take resolution into account when set on the content presshell, so number 2 should be taken care of.

Comment 16

[Damon Sicore (:damons)]

Who owns this?

Comment 17

[JP Rosevear [:jpr]]

Its dependent on bug 732971.

Comment 19

[Martijn Wargers (dead)]

Note to self and QA, once this lands, we need to check all dependancies and duplicates.

Comment 20

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

(In reply to Kartikaya Gupta (:kats) from comment #12)
> I wouldn't say "trivial". To fix this properly, we need to do three things:
> 1) Allow setting a resolution on content-window presshells
> 2) Have gecko take the resolution into account when clamping scroll
> coordinates
> 3) Turn off the disableScrollClamping thing in browser.js
> 
> I think bug 732971 covers step 1, and step 3 is trivial. But step 2 may or
> may not be trivial.

Note that currently when I set the resolution on the content-window presShell and disable scroll clamping, it lets me scroll into the bottom-right corner of the page as expected, but it doesn't paint anything in that area. the area of the page that gets painted is dependent on the zoom level, with more of it getting painted the more I zoom out.

Comment 21

[Timothy Nikkel (:tnikkel)]

(In reply to Kartikaya Gupta (:kats) from comment #20)
> Note that currently when I set the resolution on the content-window
> presShell and disable scroll clamping, it lets me scroll into the
> bottom-right corner of the page as expected, but it doesn't paint anything
> in that area. the area of the page that gets painted is dependent on the
> zoom level, with more of it getting painted the more I zoom out.

Setting resolution on a non-root document is effectively a no-op as far as I can tell.

Comment 22

[Timothy Nikkel (:tnikkel)]

Making setResolution work on the content document will not help fix this bug in any way.

Comment 25

[Timothy Nikkel (:tnikkel)]

Attachment: wip

This fixes the problem that I could see on duckduckgo (bug 741992).

Would be good to get some other testing done on it.

Comment 26

[JP Rosevear [:jpr]]

Testing as per #25 would be good.

Comment 27

[Adrian Tamas (:AdrianT)]

Tested on Nightly/14.0a1 2012-04-16 using a Motorola Droid 2 ( Android 2.3) and HTC Desire (Android 2.2).

The original issue at google.com could not be reproduced. Also the issue could not be reproduced at duckduckgo.com(bug 741992). At maps.google.com (Bug 735652) there is a visible transition from the bottom to the top of the page.
The autocomplete suggestions are no longer detached (Bug 736008) but I am seeing a second tap in the same textfield having other targets (focusing other elements like links or buttons) at mail.yahoo.com (Bug 744769 or Bug 740005).

For more investigations on the clamping behavior please provide a link to a custom build described in https://bugzilla.mozilla.org/show_bug.cgi?id=741992#c11. Leaving the qawanted tag for further testing. Please remove if it is not needed anymore.

Comment 28

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

Adrian, thanks for testing.  We would also want to test this with the patch from this bug, which has not landed yet.

Comment 30

[Timothy Nikkel (:tnikkel)]

Attachment: patch

So current frontend code wants to always set the scroll top left to be (approximately) the top left of what it is showing on the screen.

Maybe we want to move to the way XUL fennec did it where it didn't do this, but where the scroll position followed what would be visible in the zoomed out view.

This patch allows the current way to work and turn on scroll clamping (because the complete lack of scroll clamping can cause problems).

Comment 31

[Timothy Nikkel (:tnikkel)]

Attachment: patch

A version that compiles, oops.

Comment 32

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

Comment on attachment 615872 [details] [diff] [review]
patch

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

::: layout/generic/nsGfxScrollFrame.cpp
@@ +1944,5 @@
>    if (aShouldClamp) {
> +#ifdef MOZ_JAVA_COMPOSITOR
> +    // If we are using the java compositor then we want to fake the scrollport
> +    // size for the purpose of clamping the scroll position so the java
> +    // compositor can scroll to what it is actually showing on screen.

This is really hacky :-(.

Do we need a new presshell API to set the scrollport? I think we probably do.

Comment 33

[Timothy Nikkel (:tnikkel)]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #32)
> This is really hacky :-(.

Yeah, you're right.

> Do we need a new presshell API to set the scrollport? I think we probably do.

Ok.

Comment 34

[Timothy Nikkel (:tnikkel)]

Attachment: Add a scroll port api and use it for clamping.

Comment 35

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

Comment on attachment 616063 [details] [diff] [review]
Add a scroll port api and use it for clamping.

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

Seems reasonable to me. I can put together a patch that makes the front-end call this.

Comment 36

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

Attachment: Patch to test scroll port API

I think this patch should allow testing of the scroll port API. I haven't tried it myself but it's pretty straightforward.

Comment 37

[Timothy Nikkel (:tnikkel)]

Comment on attachment 616133 [details] [diff] [review]
Patch to test scroll port API

This works well. Do you want to get review for this?

Comment 38

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

Personally I think a new virtual method on all display items is cleaner than checking display item types.

I wonder though, would it make more sense to just walk the frame tree to find images instead of building display lists? Is there much win from culling images that are covered by other stuff? If not, we should probably just walk the frame tree.

Comment 39

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

Comment on attachment 616063 [details] [diff] [review]
Add a scroll port api and use it for clamping.

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

::: layout/base/nsIPresShell.h
@@ +1268,5 @@
> +  void SetScrollPort(nscoord aWidth, nscoord aHeight);
> +  bool IsScrollPortSet() { return mScrollPortSet; }
> +  nsSize GetScrollPort() {
> +    NS_ASSERTION(mScrollPortSet, "asking for scroll port when its not set?");
> +    return mScrollPortSize;

These should be renamed. We're not actually changing the scroll port size, just the size that's used when clamping the scroll position. How about SetScrollPositionClampingScrollPortSize?

Comment 40

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

Would be good to have a test here. I think a mochitest-chrome test wouldn't be too hard.

Comment 41

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

Attachment: Use new scrollport API in fennec frontend

Assumes the patch in attachment 616063 [details] [diff] [review] has been applied.

Comment 42

[Timothy Nikkel (:tnikkel)]

Comment 38 was intended for a different bug.

Comment 43

[Chris Lord [:cwiiis]]

Comment on attachment 616439 [details] [diff] [review]
Use new scrollport API in fennec frontend

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

One nit, looks fine.

::: mobile/android/chrome/content/browser.js
@@ +1721,5 @@
>      // Transform coordinates based on zoom
>      let x = aViewport.x / aViewport.zoom;
>      let y = aViewport.y / aViewport.zoom;
>  
>      // Set scroll position

Alter this comment to something along the lines of "Set scroll-port and scroll position"?

Comment 44

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

Attachment: Use new scrollport API in fennec frontend

Updated for review comments, carrying r+. :tn, feel free to land this when you land the other patch.

Comment 45

[Timothy Nikkel (:tnikkel)]

Attachment: Add a scroll port api and use it for clamping.

Patch with the renamed API.

Comment 46

[Timothy Nikkel (:tnikkel)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/492d12fa8b65
https://hg.mozilla.org/integration/mozilla-inbound/rev/83faa1da3db8

Comment 47

[Timothy Nikkel (:tnikkel)]

I forgot the test! Oops! I'll try to come up with one.

Comment 48

[(no longer active)]

https://hg.mozilla.org/mozilla-central/rev/492d12fa8b65
https://hg.mozilla.org/mozilla-central/rev/83faa1da3db8

Comment 49

[Aaron Train [:aaronmt]]

This caused a number of major regressions, see bug 747681 and bug 747680.

Comment 50

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

Let's back this out.

Comment 51

[Joe Drew (not getting mail)]

Backed out:

https://hg.mozilla.org/integration/mozilla-inbound/rev/8238bffb52b4
https://hg.mozilla.org/integration/mozilla-inbound/rev/064655d7ea01

Comment 52

[Timothy Nikkel (:tnikkel)]

Attachment: do it only for the root scroll frame

We only want to apply this special clamping on the root scroll frame even when the API is used, not every scroll frame in the document.

I'll merge this into the main patch when relanding.

Comment 53

[(no longer active)]

Backout merged: https://hg.mozilla.org/mozilla-central/rev/8238bffb52b4
https://hg.mozilla.org/mozilla-central/rev/064655d7ea01

Comment 54

[Timothy Nikkel (:tnikkel)]

Comment on attachment 617660 [details] [diff] [review]
do it only for the root scroll frame

This should fix all of the regressions that were posted in bugs caused by this.

Comment 55

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

Comment on attachment 617660 [details] [diff] [review]
do it only for the root scroll frame

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

oops

Comment 56

[Timothy Nikkel (:tnikkel)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4f2dd5f1c565
https://hg.mozilla.org/integration/mozilla-inbound/rev/73d5ffc47885

Comment 57

[Tony Chung [:tchung]]

qawanted: verify bug 747681 and bug 747680 and bug 747944 are not reproducing on inbound build.

Comment 58

[Timothy Nikkel (:tnikkel)]

To avoid confusion, bug 747681 turned out to be caused by something else.

Comment 59

[JP Rosevear [:jpr]]

Comment on attachment 616561 [details] [diff] [review]
Use new scrollport API in fennec frontend

Will need aurora for mobile

Comment 60

[JP Rosevear [:jpr]]

Comment on attachment 617660 [details] [diff] [review]
do it only for the root scroll frame

Will need aurora for mobile

Comment 61

[Adrian Tamas (:AdrianT)]

1) Bug 747681 was Verified as Fixed on Nightly/15.0a1 2012-04-24 and the issue is also not reproducible on Inbound/14.0a1 2012-04-24.
2) Bug 747680 was Verified as Fixed on Nightly/15.0a1 2012-04-24 and the issue is also not reproducible on Inbound/14.0a1 2012-04-24.
3) Bug 747944 is not reproducible on Inbound/14.0a1 2012-04-24.

Tested on: 
Galaxy Nexus (Android 4.0.2), HTC Desire (Android 2.2.2)

Comment 62

[JP Rosevear [:jpr]]

(In reply to adrian tamas from comment #61)
> 1) Bug 747681 was Verified as Fixed on Nightly/15.0a1 2012-04-24 and the
> issue is also not reproducible on Inbound/14.0a1 2012-04-24.
> 2) Bug 747680 was Verified as Fixed on Nightly/15.0a1 2012-04-24 and the
> issue is also not reproducible on Inbound/14.0a1 2012-04-24.
> 3) Bug 747944 is not reproducible on Inbound/14.0a1 2012-04-24.

This patch would not be in the current nightly since its not on central yet.

Comment 64

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

QA still waiting for it to be checked into Central.

Comment 65

[(no longer active)]

https://hg.mozilla.org/mozilla-central/rev/4f2dd5f1c565
https://hg.mozilla.org/mozilla-central/rev/73d5ffc47885

Comment 66

[Joe Drew (not getting mail)]

This is currently only in 15, thus the aurora flags.

Comment 67

[Timothy Nikkel (:tnikkel)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/a38075d6e6d0
https://hg.mozilla.org/releases/mozilla-aurora/rev/e08b460bcb60

Comment 68

[Adrian Tamas (:AdrianT)]

Initial behavior described in the bug and behaviors from the bugs duped after this one are no longer reproducible. Verifying the issue as fixed.

Verified on:
Aurora 14.0a2 2012-04-29
Nightly 15.0a1 2012-04-30
Device: HTC Desire 
OS: Android 2.2

Comment 69

[Kevin Brosnan [Ex-Mozilla]]

Clearing qawanted based on comment 68