Closed Bug 732016 Opened 8 years ago Closed 8 years ago

Maple: scrollIntoView does not clamp to the page size

Categories

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

ARM
Android
defect

Tracking

()

VERIFIED FIXED
Firefox 15
Tracking Status
firefox14 --- fixed
blocking-fennec1.0 --- beta+

People

(Reporter: BenWa, Assigned: tnikkel)

References

Details

(Whiteboard: maple [layout])

Attachments

(3 files, 7 obsolete files)

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.
Similarly it is possible to use window.scrollTo(x, y); to scroll the page outside the visible region.
OS: Mac OS X → Android
Hardware: x86 → ARM
Whiteboard: maple
tracking-fennec: --- → ?
Summary: Maple: scrollIntoView does not clamp to the page size → Maple: scrollIntoView/scrollTo does not clamp to the page size
nom for triage team.  should be fixed before the merge
tracking-fennec: ? → ---
blocking-fennec1.0: --- → ?
blocking-fennec1.0: ? → beta+
Priority: -- → P2
Whiteboard: maple → maple [gfx]
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.
Attached patch patch (obsolete) — Splinter Review
Kats, the blame for this shows the change comes merging maple to m-c

http://hg.mozilla.org/mozilla-central/rev/0742dd20781e
Assignee: nobody → blassey.bugs
Attachment #607061 - Flags: review?(bugmail.mozilla)
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.
Assignee: blassey.bugs → chrislord.net
Status: NEW → ASSIGNED
Summary: Maple: scrollIntoView/scrollTo does not clamp to the page size → Maple: scrollIntoView does not clamp to the page size
Resetting bits that I must've accidentally changed... Not assigning to myself just yet :)
Assignee: chrislord.net → blassey.bugs
Status: ASSIGNED → NEW
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.
in that case I'm not the best assignee. Jet, can you recommend someone?
Assignee: blassey.bugs → nobody
Whiteboard: maple [gfx] → maple [layout]
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.
Attachment #607061 - Flags: review?(bugmail.mozilla) → review-
Blocks: 736008
Depends on: 732791
No longer depends on: 732274
Depends on: 732089, 732971
No longer depends on: 732791
Just to note, this bug will be trivial to fix when bug 732971  lands, but it cannot be fixed until then
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.
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.
Duplicate of this bug: 740005
Duplicate of this bug: 736008
Who owns this?
Its dependent on bug 732971.
Duplicate of this bug: 744769
Note to self and QA, once this lands, we need to check all dependancies and duplicates.
(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.
(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.
Making setResolution work on the content document will not help fix this bug in any way.
Duplicate of this bug: 735539
Duplicate of this bug: 735652
Assignee: nobody → tnikkel
Priority: P2 → P1
Attached patch wip (obsolete) — Splinter Review
This fixes the problem that I could see on duckduckgo (bug 741992).

Would be good to get some other testing done on it.
Keywords: qawanted
Testing as per #25 would be good.
No longer depends on: 737274
Depends on: 737274
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.
Adrian, thanks for testing.  We would also want to test this with the patch from this bug, which has not landed yet.
Whiteboard: maple [layout] → maple [layout][has wip patch]
Duplicate of this bug: 736439
Attached patch patch (obsolete) — Splinter Review
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).
Attachment #615553 - Attachment is obsolete: true
Attachment #615860 - Flags: review?(roc)
Attached patch patch (obsolete) — Splinter Review
A version that compiles, oops.
Attachment #615860 - Attachment is obsolete: true
Attachment #615872 - Flags: review?(roc)
Attachment #615860 - Flags: review?(roc)
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.
(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.
Attachment #615872 - Attachment is obsolete: true
Attachment #616063 - Flags: review?(roc)
Attachment #616063 - Flags: feedback?(bugmail.mozilla)
Attachment #615872 - Flags: review?(roc)
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.
Attachment #616063 - Flags: feedback?(bugmail.mozilla) → feedback+
Status: NEW → ASSIGNED
Attached patch Patch to test scroll port API (obsolete) — Splinter Review
I think this patch should allow testing of the scroll port API. I haven't tried it myself but it's pretty straightforward.
Comment on attachment 616133 [details] [diff] [review]
Patch to test scroll port API

This works well. Do you want to get review for this?
Attachment #616133 - Flags: feedback+
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 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?
Attachment #616063 - Flags: review?(roc) → review+
Would be good to have a test here. I think a mochitest-chrome test wouldn't be too hard.
Assumes the patch in attachment 616063 [details] [diff] [review] has been applied.
Attachment #616133 - Attachment is obsolete: true
Attachment #616439 - Flags: review?(chrislord.net)
Attachment #607061 - Attachment is obsolete: true
Comment 38 was intended for a different bug.
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"?
Attachment #616439 - Flags: review?(chrislord.net) → review+
Updated for review comments, carrying r+. :tn, feel free to land this when you land the other patch.
Attachment #616439 - Attachment is obsolete: true
Attachment #616561 - Flags: review+
Patch with the renamed API.
Attachment #616063 - Attachment is obsolete: true
I forgot the test! Oops! I'll try to come up with one.
https://hg.mozilla.org/mozilla-central/rev/492d12fa8b65
https://hg.mozilla.org/mozilla-central/rev/83faa1da3db8
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 14
Depends on: 747845
Depends on: 747680
Depends on: 747681
This caused a number of major regressions, see bug 747681 and bug 747680.
Depends on: 747842
Depends on: 747944
No longer depends on: 747944
No longer depends on: 747681
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.
Attachment #617660 - Flags: review?(roc)
Whiteboard: maple [layout][has wip patch] → maple [layout]
Depends on: 748046
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 on attachment 617660 [details] [diff] [review]
do it only for the root scroll frame

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

oops
Attachment #617660 - Flags: review?(roc) → review+
qawanted: verify bug 747681 and bug 747680 and bug 747944 are not reproducing on inbound build.
To avoid confusion, bug 747681 turned out to be caused by something else.
Whiteboard: maple [layout] → maple [layout][re-landed on inbound]
Comment on attachment 616561 [details] [diff] [review]
Use new scrollport API in fennec frontend

Will need aurora for mobile
Attachment #616561 - Flags: approval-mozilla-aurora?
Comment on attachment 617660 [details] [diff] [review]
do it only for the root scroll frame

Will need aurora for mobile
Attachment #617660 - Flags: approval-mozilla-aurora?
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)
(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.
Blocks: 748733
No longer blocks: 748733
Duplicate of this bug: 748733
QA still waiting for it to be checked into Central.
https://hg.mozilla.org/mozilla-central/rev/4f2dd5f1c565
https://hg.mozilla.org/mozilla-central/rev/73d5ffc47885
Status: REOPENED → RESOLVED
Closed: 8 years ago8 years ago
Resolution: --- → FIXED
Whiteboard: maple [layout][re-landed on inbound] → maple [layout]
Attachment #616561 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #617660 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
This is currently only in 15, thus the aurora flags.
Target Milestone: Firefox 14 → Firefox 15
Target Milestone: Firefox 14 → Firefox 15
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
Status: RESOLVED → VERIFIED
Clearing qawanted based on comment 68
Keywords: qawanted
You need to log in before you can comment on or make changes to this bug.