80.65 KB, image/png
Bug 1296887 - Prevent a second call to shiftLayerView with the same destination as the first from clearing the resize state prematurely.
58 bytes, text/x-review-board-request
1.80 KB, patch
|Details | Diff | Splinter Review|
1. Have Full Screen mode enabled in Fennec 2. Visit http://www.theverge.com/2016/8/19/12555052/google-shutting-down-chrome-apps 3. Scroll down to show the footer bar(share button to twitter/facebook). Notice the jump that occurs as the AwesomeBar is hidden as you scroll down. If you are scrolling down quickly you can see a pronounced white space on the Footer bar 4. Now have the AwesomeBar hidden and scroll up while having the Footer present. You'll again notice a jump on slow scrolling and a flash during a fast one with the pronounced white space at times. Nightly 51, moto g 2nd Gen MM 6.0
I frequently see this "white space" behavior in Fennec as well. My testing on Fennec on many websites exhibits somewhat similar behavior; when scrolling while the AwesomeBar is sliding into or out-of view, a white bar is seen at the bottom of the screen. If I cause the AwesomeBar to slide into or out-of view quickly, i.e. I scroll quickly, I see more vertical space being taken by the white bar during the show/hide animation (and only during the animation). Whereas if I scroll slowly to show or hide it, a lesser amount of vertical space at the bottom becomes a white bar. The behavior does not occur unless the AwesomeBar is sliding into or out of view. Disabling Full Screen Browsing causes scrolling to function normally and as expected, as does when scrolling downward continuously when the Awesomebar is already completely hidden. It seems to also affect desktop nightly (Win10x64-1607, Nightly x64-8/21) while in Full Screen mode (F11). Hovering the mouse cursor near the top of the screen to reveal the AwesomeBar may cause a white bar to be displayed at the bottom of the screen (of similar dimensions as the AwesomeBar itself?) while the slide-in/slide-out animation is occurring. I can't seem to consistently reproduce however, perhaps 10% of the time. For desktop nightly, it's easiest to reproduce when rapidly attempting to display/hide the AwesomeBar in a repeated fashion while in Full Screen mode, though it's no guarantee. I've been trying to find a good way to reproduce bug 1274597 on Fennec for the last few weeks, and I do believe that an occurrence of bug 1296887 may be related to or necessary for bug 1274597 to also occur, as that's usually when I'm most able to get that bug to occur.
This happens because when the dynamic toolbar goes in and out of view, we resize the content area. If the main thread is busy it can take a few seconds, during which the white bar is visible. That being said, it could either (a) show the background color rather than white and (b) show actual content because we have the displayport painted already. Both of those would be better than what it does now, and should actually be happening already. It's worth looking into why we end up showing a white bar.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Priority: -- → P2
The background color of the page is white so the white bar is what I would expect to see.
Ok, my super-duper-simple test page at https://mozilla.staktrace.com/bug/1296887.html doesn't demonstrate this problem. It has a resize listener that hogs the main thread for 2 seconds and I don't get a white bar at the bottom when the toolbar goes on and offscreen. So at least the code that's intended to handle this isn't completely broken. There's definitely some page-specific thing going on here, but we should be able to handle it better. Tagging wontfix for 49 since it's too late for beta, but I'll try and look into this some more.
(In reply to Kartikaya Gupta (email:email@example.com) from comment #4) > Ok, my super-duper-simple test page at > https://mozilla.staktrace.com/bug/1296887.html doesn't demonstrate this > problem. It has a resize listener that hogs the main thread for 2 seconds > and I don't get a white bar at the bottom when the toolbar goes on and > offscreen. So at least the code that's intended to handle this isn't > completely broken. > > There's definitely some page-specific thing going on here, but we should be > able to handle it better. Tagging wontfix for 49 since it's too late for > beta, but I'll try and look into this some more. Not sure if related - but on the main page of theverge.com this issue does happen without the footer
I dug around a bit. It looks like the white color (on theverge.com) is the actual background color of the page, and it's getting put in that bottom area because of the code at  - I checked this by hard-coding that color to nonwhite and verified that's what I ended up seeing on the screen. This means that the frame clear color is visible, which means nothing is painting on top of it. I think this is because the root scrollable layer has a clip on it, and that clip is the wrong size during the toolbar animation, so it's clipping stuff that it shouldn't be. The code at  is supposed to fix that, and it does seems to be working, but only while finger is still down - once the finger lifts, it seems to stop applying the clip-rect-expansion, and so that's when the bgcolor is visible. As far as I can tell it shouldn't be doing that, it should be applying the expansion all the way through to the repaint after the resize, but maybe something's broken there.  http://searchfox.org/mozilla-central/rev/3611a7607db7554b9d7065ebfef3d5111f7171b8/gfx/layers/composite/LayerManagerComposite.cpp#935  http://searchfox.org/mozilla-central/rev/3611a7607db7554b9d7065ebfef3d5111f7171b8/gfx/layers/composite/AsyncCompositionManager.cpp#783
The problem was that there were two calls to shiftLayerView in quick succession. Both tried to shift the layer view to the same place, but the second one was tripped up because of the state changes from the first one, and ended up getting mSnapRequired=false and so set mHeightDuringResize back to null. This cleared the temporary layer margins that were supposed to be applied between the end of the toolbar-hide animation and the resized repaint arriving at the compositor. Without those layer margins the clip remained un-expanded resulting in the bug. The patch basically throws away that second redundant call to shiftLayerView so that it doesn't clobber mHeightDuringResize.
Also I'm not confident enough in this fix to request uplift, so wontfixing for 50 as well.
Comment on attachment 8787382 [details] Bug 1296887 - Prevent a second call to shiftLayerView with the same destination as the first from clearing the resize state prematurely. https://reviewboard.mozilla.org/r/76162/#review74238
Attachment #8787382 - Flags: review?(rbarker) → review+
Pushed by firstname.lastname@example.org: https://hg.mozilla.org/integration/autoland/rev/1e4f5a991728 Prevent a second call to shiftLayerView with the same destination as the first from clearing the resize state prematurely. r=rbarker
This is still happening on sept 3 build on nightly
(In reply to bull500 from comment #13) > This is still happening on sept 3 build on nightly Does it look any better than before? I still see the footer glitch for a frame or two, but before it was in the wrong place for much longer (hundreds of milliseconds).
(If it's still bad, please file a new bug, or clone this one)
(In reply to Kartikaya Gupta (email:email@example.com) from comment #14) > (In reply to bull500 from comment #13) > > This is still happening on sept 3 build on nightly > > Does it look any better than before? I still see the footer glitch for a > frame or two, but before it was in the wrong place for much longer (hundreds > of milliseconds). Kats i agree the patch did fix the issue around 90% of it but it isn't smooth like the other browsers during hide and reveal of the AwesomeBar. There is still a jump when you try scrolling fast and a small jerk when you scroll slow I have no idea how to clone a bug, do you want me to file a new one? Its definitely way better than before, just that a complete fix with make its a breeze and improve the user experience Thank you for your time and hardwork!
(In reply to bull500 from comment #16) > Kats i agree the patch did fix the issue around 90% of it but it isn't > smooth like the other browsers during hide and reveal of the AwesomeBar. > There is still a jump when you try scrolling fast and a small jerk when you > scroll slow Ok, thanks. > I have no idea how to clone a bug, do you want me to file a new one? On bug pages if you search for the text "Clone This Bug" you should find a link in one of the bottom footers. Clicking that link will allow you to clone the bug. Or you can file a new one if you prefer. > Its definitely way better than before, just that a complete fix with make > its a breeze and improve the user experience > Thank you for your time and hardwork! Fair enough. I was actually discussing this with snorp and rbarker on Thursday, we might redo the dynamic toolbar (again) to try and fix these remaining issues, because they're very hard to eliminate entirely with the current architecture.
Approval Request Comment [Feature/regressing bug #]: the patch that landed in this bug introduced a regression (bug 1305969) that was worse than the thing it was trying to fix. I have a fix for the regression, but don't feel that it is safe to uplift to Aurora 51. Instead, I'd like to back this patch out from Aurora and then have both patches ride together on 52. [User impact if declined]: Sometimes Fennec gets stuck with a white bar along the bottom of the screen, and it doesn't go away except randomly. Rotating the device can make things even worse. [Describe test coverage new/current, TreeHerder]: manual testing only [Risks and why]: low risk, this is a backout patch. It restores the behaviour to what it was like in beta 50 and previous releases (it's been this way for a long time) [String/UUID change made/needed]: none
Attachment #8801485 - Flags: approval-mozilla-aurora?
Comment on attachment 8801485 [details] [diff] [review] Back out patch for Aurora 51 Backout code for 51. Take it in 51 aurora.
Attachment #8801485 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.