Closed Bug 950488 Opened 6 years ago Closed 6 years ago

Application is not repainted correctly when an element is going fullscreen

Categories

(Core :: Panning and Zooming, defect)

Product:
Core
Component:
Panning and Zooming
Type:
defect
Priority:
Not set

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla29
Project Flags:
blocking-b2g 1.3+
Tracking Flags:
Tracking Status
firefox27 --- wontfix
firefox28 --- fixed
firefox29 --- fixed
b2g-v1.3 --- fixed
b2g-v1.3T --- fixed
b2g-v1.4 --- fixed

People

(Reporter: vingtetun, Assigned: BenWa)

References

Details

Attachments

(2 files, 9 obsolete files)

Part 1 - Overhaul APZ handling of overflow:hidden
22.32 KB, patch
botond
: review+
Details | Diff | Splinter Review
Part 2 - Disallow zooming for overflow:hidden frames
4.45 KB, patch
botond
: review+
Details | Diff | Splinter Review 
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.
This happens in "real world", not just in our UI tests?
Assignee: nobody → chrislord.net
(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.
Finally, I can reproduce this (took me a while to sort out this inari device) - looking at it now.
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.
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.
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.
(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 :)
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.
Depends on: 950993
blocking-b2g: 1.3? → 1.3+
Assignee: chrislord.net → bgirard
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.
Attached file Work arround (obsolete) — 
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.
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
Attached patch patch (obsolete) — — Splinter Review
Attachment #8358069 - Attachment is obsolete: true
Attachment #8358555 - Flags: review?(bugmail.mozilla)
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.
Attachment #8358555 - Flags: review?(tnikkel)
Attachment #8358555 - Flags: review?(bugmail.mozilla)
Attachment #8358555 - Flags: review+
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.
Attachment #8358555 - Flags: review?(tnikkel) → review+
I was running without APZC by accident. This doesn't fix the problem fully. Back to investigating this :(
If I backout bug 942995 locally the problem disappears.
Depends on: 942995
No longer depends on: 950993
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.
Depends on: 950993
No longer depends on: 942995
Do we handle having an APZC inside a regular (non-fullscreen) position fixed element well?
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.
Attached patch Better patch (obsolete) — — Splinter Review 
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.
Attachment #8358555 - Attachment is obsolete: true
Attachment #8360135 - Attachment is obsolete: true
Attachment #8360795 - Flags: review?(botond)
Blocks: 925026
Blocks: 942267
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.
Attachment #8360795 - Flags: review?(botond) → review-
Attached patch patch v2 (obsolete) — — Splinter Review
Attachment #8360795 - Attachment is obsolete: true
Attachment #8361871 - Flags: review?(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)
Attachment #8361871 - Flags: review?(botond) → review-
(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.
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
Attached patch patch v3 (obsolete) — — Splinter Review 
I left the zooming behavior unchanged. As discussed we can defer this to another bug.
Attachment #8361871 - Attachment is obsolete: true
Attachment #8362714 - Flags: review?(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 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.
This patch disables zooming for overflow:hidden frames, thus resolving the issues mentioned in comment #28.
Attachment #8363028 - Flags: review?(bugmail.mozilla)
Rebased Part 1 patch to apply to recent trunk. Carrying r?
Attachment #8362714 - Attachment is obsolete: true
Attachment #8362714 - Flags: review?(botond)
Attachment #8363040 - Flags: review?(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.)
Attachment #8363040 - Flags: review?(botond) → review+
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).
Attachment #8363040 - Attachment is obsolete: true
Attachment #8363061 - Flags: review+
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
Attachment #8363040 - Attachment is obsolete: false
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.
Attachment #8363028 - Flags: review?(bugmail.mozilla) → review+
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.
Attachment #8363040 - Attachment is obsolete: true
(In reply to Kartikaya Gupta (email:kats@mozilla.com) 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.
Update Part 1 patch with Kats' review comment addressed.
Attachment #8363061 - Attachment is obsolete: true
Attachment #8363070 - Flags: review+
(In reply to Kartikaya Gupta (email:kats@mozilla.com) 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.
Attachment #8363028 - Attachment is obsolete: true
Attachment #8363088 - Flags: review+
Ok, pushing to try.
Inbound is closed. Will check again tonight.
Thanks BenWa
https://hg.mozilla.org/mozilla-central/rev/28f8657d36e9
https://hg.mozilla.org/mozilla-central/rev/763a0f25391b
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
Depends on: 975962
You need to log in before you can comment on or make changes to this bug.