950487 - Overzoom when rotating an application from portrait to landscape

Status: RESOLVED FIXED

(Core :: Panning and Zooming, defect)

Description

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

The zooming scale is recalculated when an application rotate. I fixed that in bug 938312 for Poppit but it was working mostly because Poppit use a max-scale value. For apps that just use <meta name="viewport" content="width=device-width, initial-scale=1, user-scale=no"> then it breaks the app rendering.

Comment 1

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: orientation.wrong.zoom.patch

I'm not sure to understand why we are trying to fix the scale when the app is rotated ?

Comment 2

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Just a little explanation why I'm removing this code. 
My understanding of all this is that this code it is useless since when the content size will change, the child will be updated in HandlePossibleViewportChange and we will receive everything we need throught some calls to AsyncPanZoomController::NotifyLayersUpdated. 

I have even thought of removing the call to RequestContentRepaint().

Comment 3

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

I believe the reason this code exists is so that when you rotate, you immediately get the resized content view without having to wait for Gecko to repaint. Without this code, if you rotate, you will keep the same zoom level as before which means that either you will end up with extra content visible (going from portrait -> landscape) or less content visible (landscape -> portrait) and then after Gecko does the next repaint it will jump.

Comment 4

[Botond Ballo [:botond]]

My understanding is that this behaviour (zooming in/out to keep the width of the content being viewed constant across an orientation change) is a feature.

Note that up until recently, there was a bug (bug 937896) which un-did this zoom. That was fixed recently.

Comment 5

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

(In reply to Botond Ballo [:botond] from comment #4)
> My understanding is that this behaviour (zooming in/out to keep the width of
> the content being viewed constant across an orientation change) is a feature.

As Vivien pointed out, the code that is supposed to deal with this is at [1]. The code he's modifying the patch is supposed to do the same thing but for the period of time between the rotation and the gecko repaint. However, I just tested this behaviour in the B2G browser on recent master, and it appears to be broken. Loading mozilla.org and rotating doesn't update the zoom to preserve the visible content; instead the zoom is preserved on repaint so the amount of visible content changes. Vivien's patch appears to fix this, but I want to understand a bit better what is going on before r+'ing it.

[1] http://mxr.mozilla.org/mozilla-central/source/dom/ipc/TabChild.cpp?rev=b6f9dbc91dc4#632

Comment 6

[Botond Ballo [:botond]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #5)
> (In reply to Botond Ballo [:botond] from comment #4)
> > My understanding is that this behaviour (zooming in/out to keep the width of
> > the content being viewed constant across an orientation change) is a feature.
> 
> As Vivien pointed out, the code that is supposed to deal with this is at
> [1]. The code he's modifying the patch is supposed to do the same thing but
> for the period of time between the rotation and the gecko repaint.

Interesting. If there is logic in TabChild to do the zoom adjustment, why do
we need to do a RequestContentRepaint() after we do the zoom adjustment in APZC?

Comment 7

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

(In reply to Botond Ballo [:botond] from comment #6)
> Interesting. If there is logic in TabChild to do the zoom adjustment, why do
> we need to do a RequestContentRepaint() after we do the zoom adjustment in
> APZC?

Beats me. If anything that should be a ScheduleComposite() rather than a RequestContentRepaint(). But I just tried with that and it doesn't really look good either, because the browser URL bar doesn't move until gecko does a repaint, so we end up with a weird behaviour where the content area changes zoom right upon rotation even though nothing else changes, and then Gecko repaints and it looks normal again. So I think Vivien's patch is correct, although it can remove even more code.

Comment 8

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

Comment on attachment 8347763 [details] [diff] [review]
orientation.wrong.zoom.patch

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

::: gfx/layers/ipc/AsyncPanZoomController.cpp
@@ +1502,3 @@
>    ScreenIntRect oldCompositionBounds = mFrameMetrics.mCompositionBounds;
> +  metrics.mCompositionBounds = aCompositionBounds;
> +  mFrameMetrics = metrics;

I don't think there's any point to creating this local "metrics" object. You can just leave it as mFrameMetrics.mCompositionBounds = aCompositionBounds.

Comment 9

[Botond Ballo [:botond]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #7)
> (In reply to Botond Ballo [:botond] from comment #6)
> > Interesting. If there is logic in TabChild to do the zoom adjustment, why do
> > we need to do a RequestContentRepaint() after we do the zoom adjustment in
> > APZC?
> 
> Beats me. If anything that should be a ScheduleComposite() rather than a
> RequestContentRepaint(). But I just tried with that and it doesn't really
> look good either, because the browser URL bar doesn't move until gecko does
> a repaint, so we end up with a weird behaviour where the content area
> changes zoom right upon rotation even though nothing else changes, and then
> Gecko repaints and it looks normal again. So I think Vivien's patch is
> correct, although it can remove even more code.

I see, that makes sense. Shouldn't we remove the call to RequestContentRepaint() too then?

Comment 10

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

(In reply to Botond Ballo [:botond] from comment #9)
> I see, that makes sense. Shouldn't we remove the call to
> RequestContentRepaint() too then?

I think that should be doable, yes. I wasn't sure earlier because I wasn't sure if the composition bounds would get propagated properly or not. But now I'm not sure if we even need the UpdateCompositionBounds function at all. The next layers update we get from gecko should have the new composition bounds so there should be no need to update mFrameMetrics out-of-band like we're doing now. What do you think?

Comment 11

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

Metro also currently doesn't call UpdateCompositionBounds at all, and deals with rotations just fine. So I think we can rip out all that code.

Comment 12

[Botond Ballo [:botond]]

> (In reply to Kartikaya Gupta (email:[e-mail]) from comment #7)
> > (In reply to Botond Ballo [:botond] from comment #6)
> > > Interesting. If there is logic in TabChild to do the zoom adjustment, why do
> > > we need to do a RequestContentRepaint() after we do the zoom adjustment in
> > > APZC?
> > 
> > Beats me. If anything that should be a ScheduleComposite() rather than a
> > RequestContentRepaint(). But I just tried with that and it doesn't really
> > look good either, because the browser URL bar doesn't move until gecko does
> > a repaint, so we end up with a weird behaviour where the content area
> > changes zoom right upon rotation even though nothing else changes, and then
> > Gecko repaints and it looks normal again. So I think Vivien's patch is
> > correct, although it can remove even more code.
> 
> I see, that makes sense.

Actually, on second thought I'm not so sure.

Couldn't the asynchronous zooming behaviour still be useful for non-browser apps where there is no chrome URL bar that doesn't change asynchronously?

Comment 13

[Botond Ballo [:botond]]

(In reply to Botond Ballo [:botond] from comment #12)
> > (In reply to Kartikaya Gupta (email:[e-mail]) from comment #7)
> > > (In reply to Botond Ballo [:botond] from comment #6)
> > > > Interesting. If there is logic in TabChild to do the zoom adjustment, why do
> > > > we need to do a RequestContentRepaint() after we do the zoom adjustment in
> > > > APZC?
> > > 
> > > Beats me. If anything that should be a ScheduleComposite() rather than a
> > > RequestContentRepaint(). But I just tried with that and it doesn't really
> > > look good either, because the browser URL bar doesn't move until gecko does
> > > a repaint, so we end up with a weird behaviour where the content area
> > > changes zoom right upon rotation even though nothing else changes, and then
> > > Gecko repaints and it looks normal again. So I think Vivien's patch is
> > > correct, although it can remove even more code.
> > 
> > I see, that makes sense.
> 
> Actually, on second thought I'm not so sure.
> 
> Couldn't the asynchronous zooming behaviour still be useful for non-browser
> apps where there is no chrome URL bar that doesn't change asynchronously?

Discussed this with Kats on IRC. Even for other apps, the notification bar would remain in the original orientation until the repaint, so it would look weird if the app's content changed orientation sooner.

In light of that, I think we can get rid of UpdateCompositionBounds altogether.

Comment 14

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

Attachment: Kill UpdateCompositionBounds

Pushed to try at https://tbpl.mozilla.org/?tree=Try&rev=b91e2c7e650d

Comment 15

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

Blocks the gaia-apzc bug which is blocking 1.3.

Comment 18

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

Comment on attachment 8348763 [details] [diff] [review]
Kill UpdateCompositionBounds

botond is sick, moving review to Cwiiis

Comment 19

[Chris Lord [:cwiiis]]

Comment on attachment 8348763 [details] [diff] [review]
Kill UpdateCompositionBounds

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

Love all this code getting removed. We should probably land this on aurora too, but perhaps let this one sit on central for a day or two first.

Comment 20

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

https://hg.mozilla.org/integration/b2g-inbound/rev/8a8ecfe64d03

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/8a8ecfe64d03

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/d913ebd1569f

Comment 23

[KTucker [:KTucker][Inactive 3/4/2016]]

If the user is on a webpage, taps the share button to share the website and then changes the orientation of the phone immediately, half the webpage will be shown on the screen when the user returns to that website. The bottom half of the phone screen will be black.

Is this related to this issue or should it be filed as a separate bug?