Overzoom when rotating an application from portrait to landscape

RESOLVED FIXED in Firefox 28

Status

()

defect
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: vingtetun, Assigned: kats)

Tracking

Trunk
mozilla29
ARM
Gonk (Firefox OS)
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking-b2g:1.3+, firefox27 wontfix, firefox28 fixed, firefox29 fixed, b2g-v1.3 fixed)

Details

(Whiteboard: [apz_testrun])

Attachments

(1 attachment, 1 obsolete attachment)

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.
Posted patch orientation.wrong.zoom.patch (obsolete) — Splinter Review
I'm not sure to understand why we are trying to fix the scale when the app is rotated ?
Attachment #8347763 - Flags: review?(bugmail.mozilla)
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().
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.
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.
(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
(In reply to Kartikaya Gupta (email:kats@mozilla.com) 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?
(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 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.
Attachment #8347763 - Flags: review?(bugmail.mozilla) → review+
(In reply to Kartikaya Gupta (email:kats@mozilla.com) 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?
(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?
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.
> (In reply to Kartikaya Gupta (email:kats@mozilla.com) 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?
(In reply to Botond Ballo [:botond] from comment #12)
> > (In reply to Kartikaya Gupta (email:kats@mozilla.com) 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.
Blocks the gaia-apzc bug which is blocking 1.3.
blocking-b2g: --- → 1.3?
Assignee: nobody → bugmail.mozilla
blocking-b2g: 1.3? → 1.3+
Comment on attachment 8348763 [details] [diff] [review]
Kill UpdateCompositionBounds

botond is sick, moving review to Cwiiis
Attachment #8348763 - Flags: review?(botond) → review?(chrislord.net)
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.
Attachment #8348763 - Flags: review?(chrislord.net) → review+
https://hg.mozilla.org/mozilla-central/rev/8a8ecfe64d03
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
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?
Duplicate of this bug: 951447
Whiteboard: [apz_testrun]
You need to log in before you can comment on or make changes to this bug.