950488 - Application is not repainted correctly when an element is going fullscreen

Status: RESOLVED FIXED

(Core :: Panning and Zooming, defect)

Description

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

Steps to reproduce:
 - Open UI tests
 - Go to the mozFullScreen section
 - Click the 'Test' button
 - Observe that the app is fullscreen an correctly rendered.
 - Click the Cancel button
 - Scroll a little while
 - Click again on the 'Test' button

Expected result:
 - The same rendering as the first click on the test button

Actual result:
 - There is a little bit of white at the end of the page

If you try to scroll on the app then it repaints correctly. I dumped a few infos and it seems like the displayPort is positionned at some y position, and resetted once the user scroll.

Comment 1

[Milan Sreckovic [:milan] (needinfo for best results)]

This happens in "real world", not just in our UI tests?

Comment 2

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

(In reply to Milan Sreckovic [:milan] from comment #1)
> This happens in "real world", not just in our UI tests?

Likely. It seems to happens only once you have scrolled and a website/app is using the fullscreen api. So it may happen for things where there are videos I would say.

Comment 3

[Chris Lord [:cwiiis]]

Finally, I can reproduce this (took me a while to sort out this inari device) - looking at it now.

Comment 4

[Chris Lord [:cwiiis]]

In this case, the mScrollOffset of the FrameMetrics that gets sent to UpdateSubFrame isn't changing when going into fullscreen - I'd have thought we'd get an update from content on this, as the scroll position is changing(?), but I'm not entirely sure how DOM fullscreen mode works exactly. Will dig further.

Comment 5

[Chris Lord [:cwiiis]]

Strange, we're getting told by layout to change the scroll offset to what it ends up as when using APZC - but the same scroll offset is always 0,0, as it should be, when APZC is disabled. I guess we're doing something when APZC is enabled that's causing the scroll position not to clamp correctly... ClampingScrollPortSize perhaps?

Still investigating.

Comment 6

[Chris Lord [:cwiiis]]

Strange, the scrollable rect and composition bounds are correct, so the scroll offset is just plain invalid - yet it comes from layout... Getting closer, I hope.

Comment 7

[Chris Lord [:cwiiis]]

(In reply to Chris Lord [:cwiiis] from comment #6)
> Strange, the scrollable rect and composition bounds are correct, so the
> scroll offset is just plain invalid - yet it comes from layout... Getting
> closer, I hope.

ok, so I've found that actually, layout wise, the behaviour is identical with and without APZC, the only problem is with APZC, we respect the bogus scroll position and set a displayport accordingly.

At this point, I'm really not sure what's going on - the scroll frame in question has a scrolled rect that allows the scroll position at the time of setting it (in ScrollToImpl), yet given the FrameMetrics shows a scroll port that wouldn't allow the scroll position, at some point after the scroll position is set, its scroll port changes?

In case this sounds familiar at all, cc'ing some layout people. I still have lots to dig into, but this is starting to reach the edges of my comfort zone :)

Comment 8

[Chris Lord [:cwiiis]]

So I'm pretty sure this is happening because we're ending up with an APZC on a fixed-position layer. Except layers aren't marked as fixed position anymore on b2g (bug 950993) - I suspect that fixing that may just fix this, but if it doesn't, it's a simple check to not create APZCs on fixed-position layers.

Comment 9

[Benoit Girard (:BenWa)]

I check the layer tree and I don't see a problem with fixed position layers but the displayport value is wrong:

Before touch (bug showing):
displayport=(x=0.000000, y=0.000000, w=600.000000, h=678.000000)

After touch (bug gone):
displayport=(x=0.000000, y=0.000000, w=600.000000, h=960.000000)

If we change the displayport to be relative pixel paddings then I think this bug will go away.

I tested without a displayport and this bug doesn't appear.

Comment 10

[Benoit Girard (:BenWa)]

Attachment: Work arround

Alright the problem is when we go to fullscreen we reset the scroll position. The element already has an APZC so we reuse the one and we never sync scroll position with the client but in this case the scroll position has to replace the original.

Comment 11

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

This patch by itself will cause much jittering when scrolling around, because we'll move the scroll offset on every paint, which in turn happens as we scroll around.

I think the more correct approach would be farther up the call stack, to set the scroll origin when we go into full screen so that the code at [1] sets the mUpdateScrollOffset flag on the FrameMetrics. That will will cause the mScrollOffset to update just a few lines down from where you modified the code in your patch.

[1] http://mxr.mozilla.org/mozilla-central/source/layout/base/nsDisplayList.cpp?rev=6e20a8ceebb7#640

Comment 12

[Benoit Girard (:BenWa)]

Attachment: patch

Comment 13

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

Comment on attachment 8358555 [details] [diff] [review]
patch

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

Would also like tn to review since this is layout/ code.

::: layout/generic/nsGfxScrollFrame.cpp
@@ +1538,5 @@
>    , mOuter(aOuter)
>    , mAsyncScroll(nullptr)
> +  // Initialize to non-APZC to reset the scroll position
> +  // on the compositor back to 0, 0 if this frame helper
> +  // is handled by an existing APZC.

I would reword this comment a bit, to something like this:

Initialize this such that the scroll position in the compositor is updated, because we may create new ScrollFrameHelpers for the same content, implicitly changing the scroll position.

Comment 14

[Timothy Nikkel (:tnikkel)]

Comment on attachment 8358555 [details] [diff] [review]
patch

So the scroll frame associated with a given APZC changes? And this is the only thing that goes wrong when that happens?

This change is fine. But I would be surprised if the scroll frame associated with an APZC changing doesn't cause us more problems in the future.

Comment 15

[Benoit Girard (:BenWa)]

I was running without APZC by accident. This doesn't fix the problem fully. Back to investigating this :(

Comment 16

[Benoit Girard (:BenWa)]

If I backout bug 942995 locally the problem disappears.

Comment 17

[Benoit Girard (:BenWa)]

I think bug 942995 isn't really responsible but it does make the problem show. I'm going to undo the analysis because I still don't know if Cwiiis' debugging was correct or not.

I discussed a bit with cpearce and putting something fullscreen doesn't modify the DOM but it does apply these styles:
http://mxr.mozilla.org/mozilla-central/source/layout/style/ua.css#236

I'm going to verify that these styles are properly handled by APZC.

Comment 18

[Timothy Nikkel (:tnikkel)]

Do we handle having an APZC inside a regular (non-fullscreen) position fixed element well?

Comment 19

[Botond Ballo [:botond]]

Attachment: Adjust scroll rect's origin when squishing for overflow:hidden

Comment 20

[Chris Lord [:cwiiis]]

I still think that this could be as simple as just short-circuiting a lot of stuff if Layer->IsFixed is true (i.e. unset displayport, don't set scroll position). I didn't realise in my comments above that disabling APZC would also disable hit-detection, so I guess that isn't an option.

Comment 21

[Benoit Girard (:BenWa)]

Attachment: Better patch

I still want to check tomorrow how it behave you have an APZC on a frame that can be scaled by has overflow:hidden on one/both axis.

Comment 22

[Botond Ballo [:botond]]

Comment on attachment 8360795 [details] [diff] [review]
Better patch

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

r- for the overscroll handoff issue, see inline comment.

Also note that Axis has a member named 'mScrollingDisabled' which is used for axis locking. We may want to evaluate whether we can reuse this flag for overflow:hidden, or if not then rename or comment things to ensure there is no confusion between that flag and the very similarly-named FrameMetrics::GetDisableScrolling[X|Y]().

::: gfx/layers/FrameMetrics.h
@@ +302,5 @@
>    // if the APZC receiving this metrics should update its local copy.
>    bool mUpdateScrollOffset;
> +
> +public:
> +  bool GetDisableScrollingX() const

Let's add a comment saying something along the lines of "When adding new fields to FrameMetrics, please make them private and write a getter/setter for them. The existing public fields are a legacy thing and should be converted eventually." Otherwise people reading the code might wonder why some fields are public and other are private with getters/setters.

::: gfx/layers/ipc/AsyncPanZoomController.cpp
@@ +1006,5 @@
> +
> +  if (mFrameMetrics.GetDisableScrollingX() && mFrameMetrics.GetDisableScrollingY()) {
> +    return;
> +  }
> +

This won't handle overscroll handoff correctly. If we have an overflow:hidden subframe inside a scrollable parent frame, we still want a pan gesture over the subframe to pan the parent frame.

For this to work, we need to reach the call to CallDispatchScroll() near the bottom of this function, because that's what will scroll the parent frame. That means we can't return early here, nor can we set the displacement to zero. Instead, we need to check the scrolling-disabled flag in Axis::AdjustDisplacement(), and if it's set then make the entire displacement be overscroll.

@@ +1496,5 @@
>    }
>  
> +  mFrameMetrics.SetDisableScrollingX(aLayerMetrics.GetDisableScrollingX());
> +  mFrameMetrics.SetDisableScrollingY(aLayerMetrics.GetDisableScrollingY());
> +

Please move these into the 'else' block below. In the 'if' block we take aLayerMetrics wholesale.

Comment 23

[Benoit Girard (:BenWa)]

Attachment: patch v2

Comment 24

[Botond Ballo [:botond]]

Comment on attachment 8361871 [details] [diff] [review]
patch v2

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

I have one remaining major concern: marking an element overflow:hidden should prevent it from beeing zoomable as well as scrollable.

For zooming _out_, squishing the scroll rect took care of this, because there is logic in AsyncPanZoomController::OnScale() that prevents us from zooming out further than the entire scroll rect [1]. This patch as-is would regress that, hence the r-.

For zooming _in_, we currently get this wrong already.

We can easily fix both cases by adding logic to OnScale() that checks the mDisableScrolling[X|Y] flags and adjusts the min/max zoom accordingly.

The remaining comments are just nits.

It would have made for nicer history to have the "scrolling disabled" -> "axis locked" terminology change in a separate patch, but let's not spend time doing that now.

[1] http://mxr.mozilla.org/mozilla-central/source/gfx/layers/ipc/AsyncPanZoomController.cpp#815

::: gfx/layers/FrameMetrics.h
@@ +323,5 @@
> +    mDisableScrollingY = aDisableScrollingY;
> +  }
> +
> +private:
> +  // New field from now on should be made private and old fields should

s/New field/New fields

::: gfx/layers/ipc/Axis.cpp
@@ +136,4 @@
>  }
>  
> +float Axis::AdjustDisplacement(float aDisplacement, float& aOverscrollAmountOut,
> +                               bool aScrollingDisable) {

s/aScrollingDisable/aScrollingDisabled

@@ +154,5 @@
>    }
>  
>    float accelerationFactor = GetAccelerationFactor();
>    float displacement = aDisplacement * accelerationFactor;
> +

Stray whitespace change.

@@ +200,4 @@
>    }
>  }
>  
> +bool Axis::IsAxisLocked() {

Let's keep this method named Scrollable() (we discussed this in person).

::: gfx/layers/ipc/Axis.h
@@ +78,5 @@
>     * is written to |aOverscrollAmountOut|.
>     * The adjusted displacement is returned.
> +   *
> +   * aScrollingDisable is used to indicate that no scrolling should happen
> +   * in this axis. This is used to implement overflow-axis: hidden;

I know what you mean by "overflow-axis: hidden" but it might not be clear to everyone. Let's just say "overflow: hidden", it should be clear that we are including "overflow-x: hidden" and "overflow-y: hidden".

@@ +83,3 @@
>     */
> +  float AdjustDisplacement(float aDisplacement, float& aOverscrollAmountOut,
> +                           bool aScrollingDisable);

s/aScrollingDisable/aScrollingDisabled (and in the comment too)

Comment 25

[Botond Ballo [:botond]]

(In reply to Botond Ballo [:botond] from comment #24)
> I have one remaining major concern: marking an element overflow:hidden
> should prevent it from beeing zoomable as well as scrollable.
> 
> For zooming _out_, squishing the scroll rect took care of this, because
> there is logic in AsyncPanZoomController::OnScale() that prevents us from
> zooming out further than the entire scroll rect [1]. This patch as-is would
> regress that, hence the r-.
> 
> For zooming _in_, we currently get this wrong already.
> 
> We can easily fix both cases by adding logic to OnScale() that checks the
> mDisableScrolling[X|Y] flags and adjusts the min/max zoom accordingly.

BenWa and I talked about this in person, summarizing for the record: 

An alternative is to still allow zooming in on a root scrollable element that is "overflow: hidden". One could make a case for this from the UX point of view, as the ability to zoom is often useful on a small screen, and there exists a mechanism to explicitly disable zooming (user-scalable:no in the meta viewport tag).

If we allow zooming in, we need to allow panning in the zoomed-in state (but still not beyond the originally visible bounds, to respect the "overflow: hidden"), and then zooming out (but again not beyond the initial zoom level). This is a bit more complicated to implement than disallowing zooming in.

I am OK with either approach. Since this bug is about fullscreen and not about our treatment of "overflow: hidden" elements, a reasonable choice is to take the simpler approach in this bug, and make any further changes in follow-up bugs.

Comment 26

[Botond Ballo [:botond]]

Note that if we disallow zooming in, we need to disallow double-tap-to-zoom as well. This can be done by modifying the condition here [1] to prevent GeckoContentController::HandleDoubleTap() from being called if the frame is overflow:hidden. (There is no need to add logic to AsyncPanZoomController::ZoomToRect(), and that is only called in response to HandleDoubleTap()).

[1] http://mxr.mozilla.org/mozilla-central/source/gfx/layers/ipc/AsyncPanZoomController.cpp#962

Comment 27

[Benoit Girard (:BenWa)]

Attachment: patch v3

I left the zooming behavior unchanged. As discussed we can defer this to another bug.

Comment 28

[Botond Ballo [:botond]]

(In reply to Benoit Girard (:BenWa) from comment #27)
> Created attachment 8362714 [details] [diff] [review]
> patch v3
> 
> I left the zooming behavior unchanged. As discussed we can defer this to
> another bug.

This patch does not leave the zooming behaviour unchanged. It regresses it in two ways for a page whose root element is marked "overflow:hidden" but does not specify "user-scalable=no" in its meta viewport tag:

 1) With this patch, zooming out is allowed, revealing areas of the
    page that that overflow:hidden was supposed to keep hidden.
    Without this patch, zooming out is disallowed.

 2) With this patch, zooming in is allowed, but once zoomed in you
    cannot pan around.
    Without this patch, zooming in is allowed and you can pan
    around within the originally viewable portion of the page.

These regressions can be addressed with a couple of lines of code in AsyncPanZoomController::OnScale() (to disallow pinch-to-zooming in or out) and AsyncPanZoomController::DoubleTap() (to disallow double-tap-to-zoom), as described in comment #24 and comment #26 respectively. If you're busy with other bugs I'm happy to write the code that addresses them.

Comment 29

[Botond Ballo [:botond]]

Comment on attachment 8362714 [details] [diff] [review]
patch v3

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

Also it looks like this patch needs to be rebased against the bug 795567 landing.

Comment 30

[Botond Ballo [:botond]]

Attachment: Part 2 - Disallow zooming for overflow:hidden frames

This patch disables zooming for overflow:hidden frames, thus resolving the issues mentioned in comment #28.

Comment 31

[Botond Ballo [:botond]]

Attachment: Part 1 - Reimplement APZ's handling of overflow:hidden

Rebased Part 1 patch to apply to recent trunk. Carrying r?

Comment 32

[Botond Ballo [:botond]]

Comment on attachment 8363040 [details] [diff] [review]
Part 1 - Reimplement APZ's handling of overflow:hidden

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

r+ with the early exit in AttemptScroll() removed. Please wait until the Part 2 patch is reviewed and land together.

::: gfx/layers/FrameMetrics.h
@@ +327,5 @@
> +  // New fields from now on should be made private and old fields should
> +  // be refactored to be private.
> +
> +  // Allow disabling scrolling in individual axis to support
> +  // |overflow-x: hidden|.

s/overflow-x: hidden/overflow: hidden

::: gfx/layers/ipc/AsyncPanZoomController.cpp
@@ +1117,5 @@
> +    displacement.x = 0;
> +  }
> +  if (mFrameMetrics.GetDisableScrollingY()) {
> +    displacement.y = 0;
> +  }

Now that we are handling the GetDisableScrolling[X|Y]() in AdjustDisplacement(), we need to remove this early exit here. (I should have noticed and mentioned this in the previous round of review.)

Comment 33

[Botond Ballo [:botond]]

Attachment: Part 1 - Overhaul APZ handling of overflow:hidden

Reuploaded Part 1 patch with the correct author. While I was at it I addressed my review comments in comment #32, so the patch is good to land as-is (once part 2 is reviewed).

Comment 34

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

Comment on attachment 8363040 [details] [diff] [review]
Part 1 - Reimplement APZ's handling of overflow:hidden

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

::: gfx/ipc/GfxMessageUtils.h
@@ +586,5 @@
>      WriteParam(aMsg, aParam.mIsRoot);
>      WriteParam(aMsg, aParam.mHasScrollgrab);
>      WriteParam(aMsg, aParam.mUpdateScrollOffset);
> +    WriteParam(aMsg, aParam.GetDisableScrollingX());
> +    WriteParam(aMsg, aParam.GetDisableScrollingY());

I think these should still be using mDisableScrollingX and mDisableScrollingY rather than the accessor methods. The reason is that we want the object to be copied exactly, and if in the future the GetDisableScrollingX() function is changed to incorporate some other condition this will silently start breaking.

::: gfx/layers/FrameMetrics.h
@@ +324,5 @@
> +  }
> +
> +private:
> +  // New fields from now on should be made private and old fields should
> +  // be refactored to be private.

While I agree with the goal here I think it's something that should be enforced via review rather than a comment in the file. I don't mind leaving it in though.

::: gfx/layers/ipc/AsyncPanZoomController.cpp
@@ +1590,1 @@
>  

double blank line

Comment 35

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

Comment on attachment 8363028 [details] [diff] [review]
Part 2 - Disallow zooming for overflow:hidden frames

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

::: gfx/layers/ipc/AsyncPanZoomController.cpp
@@ +1830,5 @@
>  
> +bool AsyncPanZoomController::AllowZoom() const {
> +  // In addition to looking at the zoom constraints, which comes from the meta
> +  // viewport tag, disallow zooming if we are overflow:hidden in either direction.
> +  return mZoomConstraints.mAllowZoom

I bet if you put a mMonitor.AssertCurrentThreadIn() call here it would probably fail on one of these code paths. That's probably not good, but hey, our threading situation is so horrible that this probably doesn't matter anyway.

If grabbing the monitor here doesn't result in some weird deadlock condition then it's probably worth doing but if it's deadlocky then I'd just leave it out and not worry about it for now.

Comment 36

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

Comment on attachment 8363040 [details] [diff] [review]
Part 1 - Reimplement APZ's handling of overflow:hidden

Whoops, unobsoleted this attachment. Please do address the GfxMessageUtils.h comment at least before landing.

Comment 37

[Botond Ballo [:botond]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #34)
> ::: gfx/layers/FrameMetrics.h
> @@ +324,5 @@
> > +  }
> > +
> > +private:
> > +  // New fields from now on should be made private and old fields should
> > +  // be refactored to be private.
> 
> While I agree with the goal here I think it's something that should be
> enforced via review rather than a comment in the file. I don't mind leaving
> it in though.

The intent of the comment is to explain to someone reading the code why
some fields are public while others are private and have getters, and 
to inform someone wanting to add new fields how they should go about it.
I agree that the policy should be enforced during review.

Comment 38

[Botond Ballo [:botond]]

Attachment: Part 1 - Overhaul APZ handling of overflow:hidden

Update Part 1 patch with Kats' review comment addressed.

Comment 39

[Botond Ballo [:botond]]

Attachment: Part 2 - Disallow zooming for overflow:hidden frames

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #35)
> Comment on attachment 8363028 [details] [diff] [review]
> Part 2 - Disallow zooming for overflow:hidden frames
> 
> Review of attachment 8363028 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/layers/ipc/AsyncPanZoomController.cpp
> @@ +1830,5 @@
> >  
> > +bool AsyncPanZoomController::AllowZoom() const {
> > +  // In addition to looking at the zoom constraints, which comes from the meta
> > +  // viewport tag, disallow zooming if we are overflow:hidden in either direction.
> > +  return mZoomConstraints.mAllowZoom
> 
> I bet if you put a mMonitor.AssertCurrentThreadIn() call here it would
> probably fail on one of these code paths. That's probably not good, but hey,
> our threading situation is so horrible that this probably doesn't matter
> anyway.
> 
> If grabbing the monitor here doesn't result in some weird deadlock condition
> then it's probably worth doing but if it's deadlocky then I'd just leave it
> out and not worry about it for now.

Good catch! Grabbing the monitor here should be OK by inspection (all the callers go on to grab the monitor shortly thereafter anyways). I also tested it a bit and did not run into deadlocks.

Updated patch to grab the monitor. Carrying r+.

BenWa, I believe these patches are now ready to land.

Comment 40

[Benoit Girard (:BenWa)]

Ok, pushing to try.

Comment 41

[Benoit Girard (:BenWa)]

https://tbpl.mozilla.org/?tree=Try&rev=09a97bd8a55f

Comment 42

[Benoit Girard (:BenWa)]

Inbound is closed. Will check again tonight.

Comment 43

[Preeti Raghunath(:Preeti)]

Thanks BenWa

Comment 44

[Benoit Girard (:BenWa)]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/28f8657d36e9
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/763a0f25391b

Comment 45

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/28f8657d36e9
https://hg.mozilla.org/mozilla-central/rev/763a0f25391b

Comment 46

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/93fdb91a7e14
https://hg.mozilla.org/releases/mozilla-aurora/rev/7824e4c50fae

Comment 47

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

Bustage fixup:

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