820302 - Full-screen landscape -> portrait wrong canvas relocation

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[ilyalin]

User Agent: Opera/9.80 (Windows NT 6.1; WOW64) Presto/2.12.388 Version/12.10

Steps to reproduce:

Google Nexus 7

1. Enter on this page: http://dl.dropbox.com/u/55896607/rect.htm
2. Set device in landscape mode
3. Touch on the top left area to enter fullscreen 
4. Changed the orientation to landscape




Actual results:

1. The canvas(page) is in the right bottom part of the screen.
2. The top left area of the canvas is untouchable, in order to touch it user should touch the left top area of the screen instead (the area where the top left area of the canvas must be displayed, in reality).


Expected results:

1.The canvas(page) occupies full screen.
Of course, the canvas makes resize after orientation changing, but it makes it absolutely correct.

Comment 1

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

I was able to reproduce this on all channels, but it doesn't happen all the time. Most often I saw it when the N7 was in portrait and I entered full-screen, it wouldn't occupy the entire available space. On one occasion it ended up in a state where only the bottom-right quarter of the screen was being used but I only saw that happen once.

Comment 2

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

Attachment: Screenshot of not-quite-fullscreen

This is what it looks like; note the top and left margins where nothing appears.

Comment 3

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

Attachment: Copy of testcase

Copy of the test case from comment #0.

Comment 4

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

This appears to be caused by us receiving the MozScrollAreaChanged in the middle of the full screen action. This triggers a abortPanZoomAnimation which grabs some in-between-an-animation viewport and resets it as the "new" viewport after the full-screen is done. I have a fix that compares the viewport that comes in as a result of the MozScrollAreaChanged and doesn't bother with the abortPanZoomAnimation if they're the same. However there are additional floating-point rounding divergences that are complicating the patch.

Comment 5

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

Attachment: Patch

This fixes it for me. I tried a few fixes for the floating-point divergence problem and this seemed to be the least impactful. The problem with the current implementation of fuzzyEquals is that if you take two values 1.0000001 and 1.0000000, fuzzyEquals will say they are equal (which is fine). But then multiply both by 1000000 and you get 1000000.1 and 1000000.0 which fuzzyEquals says are not equal. This was happening with page width values of 980.0000 and 980.0001 - fuzzyEquals was declaring them unequal even though really they should have been declared equal.

Ideally we should update that function to check if the different between the values is less than 1e-6 *as a proportion of the values being compared*. However I'm afraid that will have a larger impact on the code and possibly break other things so I'd rather leave that the way it is for now.

Comment 6

[Chris Lord [:cwiiis]]

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

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

Looks fine to me. Good comment in ImmutableViewportMetrics.

::: mobile/android/base/gfx/GeckoLayerClient.java
@@ +301,5 @@
>              default:
>              case UPDATE:
>                  // Keep the old viewport size
>                  metrics = messageMetrics.setViewportSize(oldMetrics.getWidth(), oldMetrics.getHeight());
> +                if (!oldMetrics.fuzzyEquals(metrics)) {

This is kind of unrelated, but nice to have, right? (just need to make sure I understand the fix)

Comment 7

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

(In reply to Chris Lord [:cwiiis] from comment #6)
> 
> This is kind of unrelated, but nice to have, right? (just need to make sure
> I understand the fix)

No, this part is the actual meat of the fix. Ideally this would be all that is required to fix the problem, but because of the false-negatives in fuzzyEquals this doesn't work by itself.

To reiterate the problem, we get a MozScrollAreaChanged event in browser.js while we're doing the full-screening animation. This triggers an abortPanZoomAnimation even nothing has really changed and we shouldn't be aborting anything. The abortPanZoom code triggers another Java->JS viewport update (with a viewport from the aborted animation) that gets run after the fullscreen stuff is complete. This bad viewport gets kept as the final viewport.

The fix prevents us from aborting the animation and queueing the bad viewport send when we shouldn't really be doing that. Does that make sense?

Comment 8

[Chris Lord [:cwiiis]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #7)
> (In reply to Chris Lord [:cwiiis] from comment #6)
> > 
> > This is kind of unrelated, but nice to have, right? (just need to make sure
> > I understand the fix)
> 
> No, this part is the actual meat of the fix. Ideally this would be all that
> is required to fix the problem, but because of the false-negatives in
> fuzzyEquals this doesn't work by itself.
> 
> To reiterate the problem, we get a MozScrollAreaChanged event in browser.js
> while we're doing the full-screening animation. This triggers an
> abortPanZoomAnimation even nothing has really changed and we shouldn't be
> aborting anything. The abortPanZoom code triggers another Java->JS viewport
> update (with a viewport from the aborted animation) that gets run after the
> fullscreen stuff is complete. This bad viewport gets kept as the final
> viewport.
> 
> The fix prevents us from aborting the animation and queueing the bad
> viewport send when we shouldn't really be doing that. Does that make sense?

Ah ok, thanks - my r+ stands.

Comment 9

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

Cool, thanks.

https://hg.mozilla.org/integration/mozilla-inbound/rev/3a086666f645

Comment 10

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

https://hg.mozilla.org/mozilla-central/rev/3a086666f645

Comment 11

[Ioana Chiorean]

Firefox 20.0a1 (2013-01-03)
Device: Galaxy Nexus / Galaxy Tab 2
OS: Android 4.1.1 / 4.0.4

Using the link from comment#0  I an not able to reproduce this on these 2 devices. If this reproduces on Nexus 7 please reopen. 

Marking bug as Verified Fixed.