729169 - [maple] Content area is not clipped correctly

Status: VERIFIED FIXED

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

Description

[(no longer active)]

STR: Open about:firefox and drag the page to the right.  The bullet points should not be painted outside of the content area.

Comment 1

[(no longer active)]

Another interesting test case is support.mozilla.org (drag the page down).

Comment 2

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

Attachment: Clip the display port to the page bounds

The problem here is that the displayport isn't being clipped to the page bounds. I'm not sure if the displayport should be clipped on setting (i.e. in browser.js, where it calls setDisplayPortForElement) or on reading (in nsLayoutUtils::PaintFrame and friends). I tried to clip it in browser.js - see attached WIP - and that sort of works but seems to introduce other weird behaviour. Or maybe just exposes other pre-existing weird behaviour. Not really sure.

Comment 3

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

Attachment: Clip the display port to the page bounds (v2)

So after thinking about it some more I think my WIP fix might actually be the correct solution. Cleaned it up a little and requesting review.

Comment 4

[Chris Lord [:cwiiis]]

Comment on attachment 600779 [details] [diff] [review]
Clip the display port to the page bounds (v2)

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

Excellent, though the comment below needs to be addressed before committing.

::: mobile/android/chrome/content/browser.js
@@ +1674,5 @@
>      if (aReset) {
>        if (!aZoomLevel)
>          aZoomLevel = this.getDefaultZoomLevel();
> +      if (this.setZoom(aZoomLevel))
> +        this.refreshDisplayPort();

This will possibly break things when combined with the patch in bug 730681 (see the comment on the review of the second patch). You may just want to set this._zoom = aZoomLevel and always call refreshDisplayPort.

I'd recommend testing, and if this is the case, do that (or something similar/better) and add a comment about it.

Comment 5

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

(In reply to Chris Lord [:cwiiis] from comment #4)
> This will possibly break things when combined with the patch in bug 730681
> (see the comment on the review of the second patch). You may just want to
> set this._zoom = aZoomLevel and always call refreshDisplayPort.
> 
> I'd recommend testing, and if this is the case, do that (or something
> similar/better) and add a comment about it.

Turns out that what you're suggesting turned out to be a better solution anyway because of how my future patches turned out (which will be on bug 730687 shortly).

Comment 6

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

Attachment: Clip the display port to the page bounds (v3)

This is what this patch looks like now. Carrying r+ since it basically implements what you suggested; the only other change is it doesn't bother breaking out setZoom into a new function since it's only used once.

Comment 7

[Chris Lord [:cwiiis]]

Comment on attachment 600905 [details] [diff] [review]
Clip the display port to the page bounds (v3)

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

Sorry, I missed this earlier.

::: mobile/android/chrome/content/browser.js
@@ +1573,5 @@
> +    let viewport = this.viewport;
> +    let leftMargin = Math.min(kBufferAmount, viewport.x);
> +    let topMargin = Math.min(kBufferAmount, viewport.y);
> +    let rightMargin = Math.min(kBufferAmount, viewport.pageWidth - (viewport.x + viewport.width));
> +    let bottomMargin = Math.min(kBufferAmount, viewport.pageHeight - (viewport.y + viewport.height));

I missed this in the earlier review - We can't do this, as this will end up resizing the display port, which will end up forcing invalidations and texture reallocations.

We need to alter the margin first to be Min(pagesize, viewportsize+margin) for each axis, then when one axis is being clipped, the extra space needs to be added to the other axis.

This has the benefit of not resizing the displayport, and also reducing the displayport size on slim/short pages, reducing our texture usage (though we may want to reallocate that extra space to the other axis like xul-fennec does).

Comment 8

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

(In reply to Chris Lord [:cwiiis] from comment #7)
> 
> I missed this in the earlier review - We can't do this, as this will end up
> resizing the display port, which will end up forcing invalidations and
> texture reallocations.
> 

Out of curiosity, do you know where that code is? If you don't know off the top of your head I can look for it.

> We need to alter the margin first to be Min(pagesize, viewportsize+margin)
> for each axis, then when one axis is being clipped, the extra space needs to
> be added to the other axis.
> 

Just to make sure I understand correctly, this means doing something like this, right?

let xBufferAmount = Math.min(kBufferAmount, (viewport.pageWidth - viewport.width) / 2);
let yBufferAmount = Math.min(kBufferAmount, (viewport.pageHeight - viewport.height) / 2);
let leftMargin = Math.min(xBufferAmount, viewport.x);
let topMargin = Math.min(yBufferAmount, viewport.y);
let rightMargin = Math.min(xBufferAmount, viewport.pageWidth - (viewport.x + viewport.width));
let bottomMargin = Math.min(yBufferAmount, viewport.pageHeight - (viewport.y + viewport.height));

> This has the benefit of not resizing the displayport, and also reducing the
> displayport size on slim/short pages, reducing our texture usage

The displayport will still be resized as we zoom though.

> (though we
> may want to reallocate that extra space to the other axis like xul-fennec
> does).

So by reallocating the extra space to other axis, do you mean doing this for the yBufferAmount initialization instead:

let yBufferAmount = Math.min(kBufferAmount + (kBufferAmount - xBufferAmount), (viewport.pageHeight - viewport.height) / 2);

This seems iffy to me because if you shave off 600 pixels from the horizontal margins and add it to the vertical margins you could end up adding more pixels to the area. e.g. take a 1920x1080 screen which has a display port of 2520x1680 (=4233600 pixels) and move the horizontal margins to vertical, which gives you a display port of 1920x2280 (=4377600 pixels). With more off-square aspect ratios the increase in pixels gets higher.

Comment 9

[Chris Lord [:cwiiis]]

(In reply to Kartikaya Gupta (:kats) from comment #8)
> (In reply to Chris Lord [:cwiiis] from comment #7)
> > 
> > I missed this in the earlier review - We can't do this, as this will end up
> > resizing the display port, which will end up forcing invalidations and
> > texture reallocations.
> > 
> 
> Out of curiosity, do you know where that code is? If you don't know off the
> top of your head I can look for it.

ShadowBufferOGL, resize texture with surface-size mismatch:
http://hg.mozilla.org/projects/maple/file/f8b4a83be351/gfx/layers/opengl/ThebesLayerOGL.cpp#l907

BasicTextureImage resize function:
http://hg.mozilla.org/projects/maple/file/f8b4a83be351/gfx/gl/GLContext.cpp#l934

This is particularly bad because we're tiling, so we're reallocating a lot of textures when resizing.

I can't point to more than this off the top of my head, and it goes into complicated areas that I don't have much of a clue about, being honest...

> > We need to alter the margin first to be Min(pagesize, viewportsize+margin)
> > for each axis, then when one axis is being clipped, the extra space needs to
> > be added to the other axis.
> > 
> 
> Just to make sure I understand correctly, this means doing something like
> this, right?
> 
> let xBufferAmount = Math.min(kBufferAmount, (viewport.pageWidth -
> viewport.width) / 2);
> let yBufferAmount = Math.min(kBufferAmount, (viewport.pageHeight -
> viewport.height) / 2);
> let leftMargin = Math.min(xBufferAmount, viewport.x);
> let topMargin = Math.min(yBufferAmount, viewport.y);
> let rightMargin = Math.min(xBufferAmount, viewport.pageWidth - (viewport.x +
> viewport.width));
> let bottomMargin = Math.min(yBufferAmount, viewport.pageHeight - (viewport.y
> + viewport.height));

Right.

> > This has the benefit of not resizing the displayport, and also reducing the
> > displayport size on slim/short pages, reducing our texture usage
> 
> The displayport will still be resized as we zoom though.

Yeah, we can't avoid that, but we don't do draws during the zoom, so that's not too big deal (I don't think).

> > (though we
> > may want to reallocate that extra space to the other axis like xul-fennec
> > does).
> 
> So by reallocating the extra space to other axis, do you mean doing this for
> the yBufferAmount initialization instead:
> 
> let yBufferAmount = Math.min(kBufferAmount + (kBufferAmount -
> xBufferAmount), (viewport.pageHeight - viewport.height) / 2);
> 
> This seems iffy to me because if you shave off 600 pixels from the
> horizontal margins and add it to the vertical margins you could end up
> adding more pixels to the area. e.g. take a 1920x1080 screen which has a
> display port of 2520x1680 (=4233600 pixels) and move the horizontal margins
> to vertical, which gives you a display port of 1920x2280 (=4377600 pixels).
> With more off-square aspect ratios the increase in pixels gets higher.

Right, doing something like that but perhaps done in a way so that that doesn't happen... So more like extraYSpace = (kBuffer - xBufferAmount) * height/width, or whatever.

Comment 10

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

Attachment: Clip the display port to the page bounds (v4)

Getting this just right turned out to be more complicated than I thought it would be.

Comment 11

[Chris Lord [:cwiiis]]

Comment on attachment 601281 [details] [diff] [review]
Clip the display port to the page bounds (v4)

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

Looks good.

::: mobile/android/chrome/content/browser.js
@@ +1580,5 @@
> +    // if we reduced the buffer amount on the horizontal axis, we should take that saved memory and
> +    // use it on the vertical axis
> +    let savedPixels = (kBufferAmount - xBufferAmount) * (viewport.height + kBufferAmount);
> +    let extraYAmount = Math.floor(savedPixels / (viewport.width + xBufferAmount));
> +    let yBufferAmount = Math.min(kBufferAmount + extraYAmount, viewport.pageHeight - viewport.height);

Might also be nice to cover the other case of a wide, short page too. Not nearly as common, but all the same.

Comment 12

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

(In reply to Chris Lord [:cwiiis] from comment #11)
> 
> Might also be nice to cover the other case of a wide, short page too. Not
> nearly as common, but all the same.

Did this in the patch I landed. I also found that sometimes pageWidth is less than width if you pinch zoom into overscroll so I added guards against that in the form of Math.max(0, ...) in some places.

https://hg.mozilla.org/projects/maple/rev/726cd11889e6

Comment 13

[Catalin Suciu [:csuciu]]

The issue that was initially reported is verified fixed on: 
Maple nightly 13.0a1 2012-03-12 (20120312155232)
Samsung Galaxy SII (Android 2.3.4).

As for the issue reported in comment #2, a new bug was logged (bug #735180)

Comment 14

[Catalin Suciu [:csuciu]]

Closing per above comment.