Closed
Bug 950488
Opened 7 years ago
Closed 7 years ago
Application is not repainted correctly when an element is going fullscreen
Categories
(Core :: Panning and Zooming, defect)
Core
Panning and Zooming
Tracking
()
People
(Reporter: vingtetun, Assigned: BenWa)
References
Details
Attachments
(2 files, 9 obsolete files)
22.32 KB,
patch
|
botond
:
review+
|
Details | Diff | Splinter Review |
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?
Updated•7 years ago
|
Assignee: nobody → chrislord.net
Reporter | ||
Comment 2•7 years ago
|
||
(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.
Reporter | ||
Updated•7 years ago
|
blocking-b2g: --- → 1.3?
Comment 3•7 years ago
|
||
Finally, I can reproduce this (took me a while to sort out this inari device) - looking at it now.
Comment 4•7 years ago
|
||
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•7 years ago
|
||
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•7 years ago
|
||
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•7 years ago
|
||
(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•7 years ago
|
||
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
Updated•7 years ago
|
blocking-b2g: 1.3? → 1.3+
Assignee | ||
Updated•7 years ago
|
Assignee: chrislord.net → bgirard
Assignee | ||
Comment 9•7 years ago
|
||
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.
Assignee | ||
Comment 10•7 years ago
|
||
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•7 years ago
|
||
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
Assignee | ||
Comment 12•7 years ago
|
||
Attachment #8358069 -
Attachment is obsolete: true
Attachment #8358555 -
Flags: review?(bugmail.mozilla)
Comment 13•7 years ago
|
||
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 14•7 years ago
|
||
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+
Assignee | ||
Comment 15•7 years ago
|
||
I was running without APZC by accident. This doesn't fix the problem fully. Back to investigating this :(
Assignee | ||
Comment 16•7 years ago
|
||
If I backout bug 942995 locally the problem disappears.
Assignee | ||
Comment 17•7 years ago
|
||
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•7 years ago
|
||
Do we handle having an APZC inside a regular (non-fullscreen) position fixed element well?
Comment 19•7 years ago
|
||
Comment 20•7 years ago
|
||
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.
Assignee | ||
Comment 21•7 years ago
|
||
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)
Comment 22•7 years ago
|
||
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-
Assignee | ||
Comment 23•7 years ago
|
||
Attachment #8360795 -
Attachment is obsolete: true
Attachment #8361871 -
Flags: review?(botond)
Comment 24•7 years ago
|
||
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-
Comment 25•7 years ago
|
||
(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•7 years ago
|
||
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
Assignee | ||
Comment 27•7 years ago
|
||
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)
Comment 28•7 years ago
|
||
(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•7 years ago
|
||
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•7 years ago
|
||
This patch disables zooming for overflow:hidden frames, thus resolving the issues mentioned in comment #28.
Attachment #8363028 -
Flags: review?(bugmail.mozilla)
Comment 31•7 years ago
|
||
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 32•7 years ago
|
||
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+
Comment 33•7 years ago
|
||
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 34•7 years ago
|
||
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 35•7 years ago
|
||
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 36•7 years ago
|
||
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
Comment 37•7 years ago
|
||
(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.
Comment 38•7 years ago
|
||
Update Part 1 patch with Kats' review comment addressed.
Attachment #8363061 -
Attachment is obsolete: true
Attachment #8363070 -
Flags: review+
Comment 39•7 years ago
|
||
(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+
Assignee | ||
Comment 40•7 years ago
|
||
Ok, pushing to try.
Assignee | ||
Comment 41•7 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=09a97bd8a55f
Assignee | ||
Comment 42•7 years ago
|
||
Inbound is closed. Will check again tonight.
Comment 43•7 years ago
|
||
Thanks BenWa
Assignee | ||
Comment 44•7 years ago
|
||
remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/28f8657d36e9 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/763a0f25391b
Comment 45•7 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/28f8657d36e9 https://hg.mozilla.org/mozilla-central/rev/763a0f25391b
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
Comment 46•7 years ago
|
||
https://hg.mozilla.org/releases/mozilla-aurora/rev/93fdb91a7e14 https://hg.mozilla.org/releases/mozilla-aurora/rev/7824e4c50fae
status-b2g-v1.3:
--- → fixed
status-b2g-v1.4:
--- → fixed
status-firefox27:
--- → wontfix
status-firefox28:
--- → fixed
status-firefox29:
--- → fixed
Comment 47•7 years ago
|
||
Bustage fixup: https://hg.mozilla.org/releases/mozilla-aurora/rev/c612bde70654
Updated•7 years ago
|
status-b2g-v1.3T:
--- → fixed
You need to log in
before you can comment on or make changes to this bug.
Description
•