477468 - resizing window while panned to the right screws up WidgetStack

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Comment 1

[:Gavin Sharp [email: [e-mail]]]

Attachment: patch

This patch reworks how the widgetstack resets the viewport after pans, and fixes all of the resizing bugs I'm aware of. Currently includes the patch for bug 477435, because I didn't commit it before starting on this one, and haven't taken the time to split them cleanly yet. Also need to go through it and write up a description of the changes.

Comment 2

[:Gavin Sharp [email: [e-mail]]]

Attachment: better patch

The previous patch was broken for pans while zoomed.

The major change here is that I've gotten rid of _panRegionOffsets, in favor of the simpler strategy of always just snapping the viewport back to it's original position (though making sure to keep the viewportInnerBounds within the viewportBounds). The reason that my previous patch was busted was that I was ignoring pans that move the viewportInnerBounds outside of the viewport, but not those that moved them back in. Fixing that required keeping track of the offsets we're ignoring while panning away from the viewport, and making sure not to factor them in when panning back.

Other smaller changes:
-changed meaning of panBy parameters. for some reason it was moving the viewingRect in the opposite direction of the passed in values, and I don't think that makes any sense (i.e. panBy(-10, -10) now moves viewingRect to the top and left)
-changed some callers to deal with that (others were already expecting it, which explains some of the bustage)
-removed the panTo(0,0) workaround from zoomToElement/zoomFromElement, since they're no longer needed
-made rectTranslateConstrain deal with rects that are already outside the bounds, by allowing them to move further into the bounds. this lets us handle windows larger than the viewportBounds (i.e. viewingRects larger than the viewportBounds, removes need for hack from bug 477104)
-made panBy ignore 0,0 pans (discovered this issue because we were attempting to pan early during startup when our viewingRect was sized incorrectly - need to followup on that)

Comment 3

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #2)
> Other smaller changes:
> -changed meaning of panBy parameters

This means the tests in wsTests.js no longer work, presumably, but I'm not sure they did before. I'd like to get those working again, but will do that in a followup, I think.

Comment 4

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 361450 [details] [diff] [review]
better patch

this looks fine, I think, though I confess I didn't go through the math in a huge amount of detail.

The panby argument sign was because it was intended to simulate moving the "window" that's viewing into the content.. so grabbing that window and moving it up/left by 10 pixels would show you the same content as moving the content underneath the window down/right by 10.  Either way's fine, though; I don't think any other methods operate in that coordinate space.

Comment 5

[:Gavin Sharp [email: [e-mail]]]

https://hg.mozilla.org/mobile-browser/rev/d2b4f1c91ac9