Closed Bug 726817 Opened 8 years ago Closed 8 years ago

MAPLE: Websites are cut off

Categories

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

ARM
Android
defect

Tracking

()

VERIFIED FIXED
Firefox 13

People

(Reporter: martijn.martijn, Assigned: cwiiis)

References

(Blocks 1 open bug)

Details

(Whiteboard: MAPLE mwc-demo)

Attachments

(4 files, 2 obsolete files)

Attached image 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.
After panning, the page gets cut off even more.
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.
Priority: -- → P1
Blocks: 726882
Summary: OMTC: Websites are cut off → MAPLE: Websites are cut off
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.
Whiteboard: MAPLE mwc-demo
This also affects Hahlo if you are logged in to it.
Looking at this.
Assignee: nobody → chrislord.net
Status: NEW → ASSIGNED
I see this on pages when you zoom in past 1.0 zoom, for reference.
Blocks: 728249
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)
s/would/wouldn't, sorry for the confusion.
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.
Fixed this, indeed by adding an option to turn off scroll position clamping. Patches incoming, when Mercurial decides to play ball.
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.
Attachment #598410 - Flags: review?(roc)
And of course immediately I realise that patch isn't quite right - another incoming.
Amended patch.
Attachment #598410 - Attachment is obsolete: true
Attachment #598410 - Flags: review?(roc)
Attachment #598433 - Flags: review?(roc)
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.
Attachment #598433 - Flags: review?(roc) → review+
Hopefully this addresses all your comments, but I'm not brave enough to assume so, so re-requesting review. Sorry!
Attachment #598433 - Attachment is obsolete: true
Attachment #598504 - Flags: review?(roc)
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
Attachment #598504 - Flags: review?(roc) → review+
This seems to have landed.
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Duplicate of this bug: 728207
Duplicate of this bug: 726882
(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
Target Milestone: --- → Firefox 13
I think I hvae to wait tomorrow to test on nightly due to bug 729922?
Verified fixed on:
Nightly Fennec 13.0a1 (2012-03-11)
Device: HTC Desire Z
OS: Android 2.3.3
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.