787322 - Accelerometer page testing is zoomed when performing multiple device rotations

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: Toolbar, defect)

Description

[Paul Feher]

Aurora 17.0a2 2012-08-31
Devices: Samsung Galaxy Tab (Android 3.1), Samsung Galaxy Tab 2 7.0 (4.0.4)

Steps to reproduce:
1. Load http://people.mozilla.com/~nhirata/html_tp/accel_bug615597.html.
2. Rotate the device several times. 

Expected results:
The page displays black rectangle with a black dot in it that moves with the device motion. The page is not zoomed-in on rotations

Actual results:
The page is zoomed-in on every rotation and after several rotations a black artifact appears on the page.

Comment 1

[Aaron Train [:aaronmt]]

This might be expected.

Kats, why does resizing the viewport, and modify its zoom factor so that the page retains proportionally not work out well here?

Comment 2

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

After some investigation it looks like there are a couple of different bugs that have crept into the code here.

One is that we are now sending two Window:Resize events from java to browser.js for each rotation. The first one gets sent when the display metrics (screen size) change, and the other gets sent right after that when the view size (window size) changes. Since we are getting two calls into our surfaceChanged listener, this was likely introduced by bug 779152 which added an extra level of wrapping in the View hierarchy.

The other is that the updateViewportSize() code in browser.js that runs on Window:Resize seems to trigger layout code to run, which triggers the MozScrollAreaChanged event listener, which sends a viewport update back to Java in the midst of recalculating the new viewport size. This "incomplete" viewport update has the old zoom value, and Java might send this old zoom value back to browser.js, clobbering the new zoom value that gets calculated by the end of updateViewportSize(). This was likely introduced by some core Gecko change, but it's a legitimate code path that we need to handle better.

I'll need to investigate a bit more to verify all this and figure out how to fix it all.

Comment 3

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

(In reply to Kartikaya Gupta (:kats) from comment #2)
> One is that we are now sending two Window:Resize events from java to
> browser.js for each rotation.

Although annoying, I don't think there's anything I can do about this one. On the GN the view size actually does change twice. If I start in portrait with the view size at 720x1038 and rotate to landscape, the view first expands to 720x1054, and then catches up to the rotation and becomes 1196x590. So the two Window:Resize events are sort of justified, we'll just need to make sure we handle them properly.

Comment 4

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

Attachment: WIP

This works, but I want to think about it a bit more to see if there's a better way to do it.

Comment 5

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

Attachment: Patch

I like this solution a little bit better.

Comment 6

[Chris Lord [:cwiiis]]

Comment on attachment 657374 [details] [diff] [review]
Patch

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

Looks good to me.

Comment 7

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/e059c954ef75

Comment 8

[Ryan VanderMeulen [:RyanVM]]

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

Comment 9

[Paul Feher]

Verified on:
Firefox Nightly 18.0a1 (2012-09-17)
Device: Samsung Galaxy Tab
OS: Android 3.1