726817 - MAPLE: Websites are cut off

Status: VERIFIED FIXED

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

Description

[Martijn Wargers (dead)]

Attachment: screenshot of bug

While testing latest trunk of the Maple branch I noticed that every page is cut off, see screenshot.
Latest maple branch can be found here: http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/latest-maple/

I also made a video out of this: http://www.youtube.com/watch?v=28GTYb5cFA4&feature=g-upl&context=G2a8c024AUAAAAAAAAAA

The screenshots and video are from the LG Optimus Black, but I see this on every device I tested with.

I've also seen cases where the page starts scrolling by itself.

Comment 1

[Martijn Wargers (dead)]

Attachment: Screenshot of bug after panning

After panning, the page gets cut off even more.

Comment 2

[Martijn Wargers (dead)]

I've seen this on several devices, it doesn't happen always, but it happens most of the time.
Also weird panning, zooming by itself.

It makes the browser very difficult/annoying to work with.

Comment 3

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

Attachment: Screenshot on load for ICS

Comment 4

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

Affects ICS and the webgl demo on land of the page.  Battery and Vibrartion Webapp look missing even though if you tap at certain spots you can bring them up.

Comment 5

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

This also affects Hahlo if you are logged in to it.

Comment 6

[Chris Lord [:cwiiis]]

Looking at this.

Comment 7

[Chris Lord [:cwiiis]]

I see this on pages when you zoom in past 1.0 zoom, for reference.

Comment 8

[Chris Lord [:cwiiis]]

This is happening because we no longer have any translation to reconcile the scroll position clamp when we zoom.

In Java compositor fennec, we would zoom with a CSS zoom, centred on the top-left. The problem with zooming is that it would alter the maximum scrollLeft/scrollTop (the page dimensions haven't changed, so why would it?) - we reconciled this by having an extra translation when we wanted to set scroll coordinates that extended beyond the allowed values.

Either we need to employ the same trick again, or we want some kind of flag to set in nsIDOMWindowUtils so we can violate the allowable scroll values.

ajuma points out the necessary code to change: http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsGfxScrollFrame.cpp#1811 (nsGfxScrollFrameInner::ClampAndRestrictToDevPixels)

Comment 9

[Chris Lord [:cwiiis]]

s/would/wouldn't, sorry for the confusion.

Comment 10

[Chris Lord [:cwiiis]]

Just to note, that although there may well be other problems, I've confirmed that this is *a* problem. Measuring viewport excess, you can see that the scroll position we set is not the one that the window ends up at...

I have a patch for turning off scroll position clamping, but it doesn't appear to be working yet.

Comment 11

[Chris Lord [:cwiiis]]

Fixed this, indeed by adding an option to turn off scroll position clamping. Patches incoming, when Mercurial decides to play ball.

Comment 12

[Chris Lord [:cwiiis]]

Attachment: Add nsIFrameLoader.clampScrollPosition

This and the browser.js change on top of it have already been pushed to Maple, but this changes layout so requires review.

It adds a new attribute to nsIFrameLoaderOwner called 'clampScrollPosition', in the same vein as clipSubdocument.

Comment 13

[Chris Lord [:cwiiis]]

And of course immediately I realise that patch isn't quite right - another incoming.

Comment 14

[Chris Lord [:cwiiis]]

Attachment: Add nsIFrameLoader.clampScrollPosition

Amended patch.

Comment 15

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

Comment on attachment 598433 [details] [diff] [review]
Add nsIFrameLoader.clampScrollPosition

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

r+ with those fixed.

::: content/base/public/nsIFrameLoader.idl
@@ +270,5 @@
> +   * If false, then the subdocument's scroll coordinates will not be clamped
> +   * to their scroll boundaries.
> +   * Defaults to true.
> +   */
> +  attribute boolean clampScrollPosition;

rev IID on nsIFrameLoader

::: layout/generic/nsGfxScrollFrame.cpp
@@ +1547,5 @@
>                                  nsIScrollableFrame::ScrollMode aMode)
>  {
> +  nsSubDocumentFrame* subdocFrame = static_cast<nsSubDocumentFrame*>
> +    (nsLayoutUtils::GetCrossDocParentFrame(mOuter->PresContext()->PresShell()->GetRootFrame()));
> +  if (!subdocFrame || subdocFrame->ShouldClampScrollPosition())

Abstract this test into nsGfxScrollFrameInner::ShouldClamp().

Currently, when clampScrollPosition is false, your code makes all scrollable areas in the subdocument not clamp. I'm guessing this is not what you want, especially because your code to start clamping in nsFrameLoader only fixes the scroll position for the root scroll frame. So add a check for mIsRoot here.

Comment 16

[Chris Lord [:cwiiis]]

Attachment: Add nsIFrameLoader.clampScrollPosition (revised)

Hopefully this addresses all your comments, but I'm not brave enough to assume so, so re-requesting review. Sorry!

Comment 17

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

Comment on attachment 598504 [details] [diff] [review]
Add nsIFrameLoader.clampScrollPosition (revised)

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

::: layout/generic/nsGfxScrollFrame.cpp
@@ +1548,5 @@
>  {
> +  if (ShouldClampScrollPosition())
> +    mDestination = ClampScrollPosition(aScrollPosition);
> +  else
> +    mDestination = aScrollPosition;

{} around if/else body

Comment 18

[Jeff Muizelaar [:jrmuizel]]

http://hg.mozilla.org/projects/maple/rev/a4af71f1bc1f

Comment 19

[Jeff Muizelaar [:jrmuizel]]

This seems to have landed.

Comment 22

[Ed Morley [:emorley]]

(In reply to Jeff Muizelaar [:jrmuizel] from comment #19)
> This seems to have landed.

It has now :-)

https://hg.mozilla.org/mozilla-central/rev/e4f5cf4a8f23

Comment 23

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

I think I hvae to wait tomorrow to test on nightly due to bug 729922?

Comment 24

[Paul Feher]

Verified fixed on:
Nightly Fennec 13.0a1 (2012-03-11)
Device: HTC Desire Z
OS: Android 2.3.3