Closed Bug 1493742 Opened 6 years ago Closed 6 years ago

Flickering of (some) sticky elements on Twitter and other websites

Categories

(Core :: Layout: Positioned, defect)

ARM
Android
defect
Not set
normal

Tracking

()

VERIFIED FIXED
mozilla65
Tracking Status
firefox-esr60 --- unaffected
firefox62 --- unaffected
firefox63 + verified
firefox64 + verified
firefox65 + verified

People

(Reporter: julienw, Assigned: botond)

References

Details

(Keywords: regression)

Attachments

(7 files)

For some weeks I see some flickering and invalidating issue on some pages that use position:fixed (and maybe position: sticky too), while scrolling. I see this especially on these pages: * the header on https://lemonde.fr => this is especially visible when there's some dynamic update in the same time, eg some ads showing up in an article. * https://leboncoin.fr: try to open any ad and scroll On these pages this isn't _so_ annoying because we see a flickering but the "end state" is usually correct -- although that on "le monde" I've seen sometimes the header some pixels below its intended position. But this is especially annyoing on Twitter, where we can clearly see an invalidation issue, with part of the page is not being redrawn, and as a result we could see some tweet on top of another tweet. This happens for about 2 or 3 weeks. More importantly this happens on Beta. I didn't try Release yet. I didn't file earlier because I couldn't find a very reliable STR, but nical convinced me to file one nevertheless.
Attached image good example on Twitter (deleted) —
Here is a reproduction on Twitter in today's nightly. I think this happens if I scroll while twitter loads the time-line and adds content that would cause a reflow and move some elements down.
Attached image another issue in twittee (deleted) —
Once we're in this state, this is very easy to get this issue by simply scrolling.
I reproduce quite easily by scrolling down with momentum, then stopping abruptly when it's white (I don't know if the white part is due to Twitter or Firefox).
OS: Unspecified → Android
Which device and android version?
Flags: needinfo?(felash)
This is happening on Android 6 on a Z3C.
Flags: needinfo?(felash)
Hey Jamie, Nical told me you might be the right person to ping here.
Flags: needinfo?(jnicol)
Looks like a dup of bug 1490789, which I'm investigating now. You should be able to work around it by setting layout.display-list.flatten-transform=false (you'll need to add a new pref). Please let me know if that doesn't work.
Status: NEW → RESOLVED
Closed: 6 years ago
Flags: needinfo?(jnicol)
Resolution: --- → DUPLICATE
Attached image screenshot with the pref set to false (deleted) —
I can't reproduce the other issue but I still issues like this one on titter, or flickering on other websites.
Flags: needinfo?(jnicol)
Oh yeah, sorry. The flickering of the sticky elements is indeed a separate issue from the invalidation one on twitter. Let's reopen this bug for the sticky elements, since I was planning on opening a new one for that anyway. This is a regression from bug 1465616. cc Botond.
Status: RESOLVED → REOPENED
Flags: needinfo?(jnicol) → needinfo?(botond)
Resolution: DUPLICATE → ---
Great :) Then I change the bug title to reflect this.
Blocks: 1465616
Summary: Invalidation and painting error on pages with position: fixed → Flickering of (some) sticky elements on Twitter and other websites
Julien, could you try disabling full-screen browsing (in Settings -> General), and see if you're still experiencing the flickering? If that fixes it, then it's probably the same underlying issue as bug 1495055.
Flags: needinfo?(botond) → needinfo?(felash)
Yeah this seems to fix it for me indeed :)
Flags: needinfo?(felash)
So is this a dupe, then?
Flags: needinfo?(botond)
It seems that way. A fix for bug 1495055 should be landing today, I'll ask Julien to confirm after that makes it into a nightly.
Flags: needinfo?(botond)
Bug 1495055 should be in today's fennec nightly, can you re-test?
Flags: needinfo?(felash)
I was not able to reproduce the issue in the latest Nightly 64.0a1 (2018-10-04) and Beta 63.0b11 on the following devices: -Sony Xperia X (Android 6.0.1) -Sony Xperia XZ (Android 7.0) I tried to repro both on Twitter and LeMonde websites.
QA Contact: svoisen
QA Contact: svoisen
Attached image screenshot twitter in nightly 20181006100032 (deleted) —
Flags: needinfo?(felash)
I still see the issues in latest nightly.
(In reply to Julien Wajsberg [:julienw] from comment #19) > I still see the issues in latest nightly. Bug 1495055 has been backed out, so the question has become moot. Will ask again after bug 1495055 relands.
Julien, bug 1495055 relanded a few hours ago, could you test if you still have the bug with the next nightly build? Thanks
Flags: needinfo?(felash)
This is far from perfect and this will need to be further fixed, but I think Botond is aware of that given bug 1495055 comment 16. This is especially visible on websites such as https://www.leboncoin.fr/ameublement/1507238756.htm/ or https://www.leboncoin.fr/ameublement/offres/ile_de_france/.
Flags: needinfo?(felash)
Attached image example on leboncoin in nightly 20181014100136 (deleted) —
This is still quite easy to reach such a situation.
I see this on Twitter too. But I think it's more difficult to trigger it, at least on Twitter.
Marking as wontfix for 63 as I don't intend to take more low-level patches mitigating this issue than the one in bug 1495055 into the release as we are in RC week now. Thanks Julien for the testing!
(In reply to Julien Wajsberg [:julienw] from comment #22) > This is far from perfect and this will need to be further fixed, but I think > Botond is aware of that given bug 1495055 comment 16. > > This is especially visible on websites such as > https://www.leboncoin.fr/ameublement/1507238756.htm/ or > https://www.leboncoin.fr/ameublement/offres/ile_de_france/. Ok, let's use this bug to track the remaining issues.
Assignee: nobody → botond
Status: REOPENED → NEW
I believe I've tracked down the remaining issue here.
APZ wants the *size* of the layout viewport from the main thread, but it knows the position better.
It doesn't install on my phone, maybe because I already have another "Firefox Nightly" installed from the store (which is my normal browser, so I can't really uninstall it :/)? (I enabled "unknown sources" before of course). Indeed when I "run" the apk, it asks me about installing an update for this application... and then it fails after I start the process...
Flags: needinfo?(felash)
Pushed by bballo@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/1d3872dd20e1 When accepting a layout viewport update from the main thread, constrain its position to enclose the visual viewport. r=kats
Status: NEW → RESOLVED
Closed: 6 years ago6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla65
Flags: qe-verify+
(In reply to Julien Wajsberg [:julienw] from comment #30) > It doesn't install on my phone, maybe because I already have another > "Firefox Nightly" installed from the store (which is my normal browser, so I > can't really uninstall it :/)? (I enabled "unknown sources" before of > course). > > Indeed when I "run" the apk, it asks me about installing an update for this > application... and then it fails after I start the process... Hmm, not sure what's up with that. Anyway, the patch is in nightly now - are things behaving better?
Flags: needinfo?(felash)
Yep, the scroll issues seem to have disappeared and fixed elements behave as expected. But something else just appeared in the same build, so it could be because of this patch. When the keyboard is displayed, the viewport used to shrink vertically so that the input was always visible. But this changed: now the viewport isn't shrunk anymore, and as a result we can't see the input while typing when it's low in the page (eg answering a tweet on Mobile Twitter). Do you want me to file a different issue, or could that be related to this patch?
Flags: needinfo?(felash) → needinfo?(botond)
New bug, please. I don't think it could be caused by this patch, although it could conceivably have been caused by bug 1423011 or bug 1465616.
Flags: needinfo?(botond)
(I can repro your issue, and I confirmed that backing out this patch locally does not fix it. Please file a new bug and ask for a regression range, and we can take it from there. Thanks!)
I filed bug 1502515, not sure what the right component is.
Botond, I believe you should request an uplift to beta for this patch? > although it could conceivably have been caused by bug 1423011 or bug 1465616. The issue I see isn't in Beta and just appeared, so I don't think this is one of these bugs.
(In reply to Julien Wajsberg [:julienw] from comment #38) > > although it could conceivably have been caused by bug 1423011 or bug 1465616. > > The issue I see isn't in Beta and just appeared, so I don't think this is > one of these bugs. Ok. (Phew :)) Must be something else that landed recently, then.
Comment on attachment 9019184 [details] Bug 1493742 - When accepting a layout viewport update from the main thread, constrain its position to enclose the visual viewport. r?kats [Beta/Release Uplift Approval Request] Feature/Bug causing the regression: Bug 1465616 User impact if declined: When scrolling a page such that toolbar transitions are triggered, elements that are attached to the screen (e.g. position:fixed) can briefly flicker (i.e. move to an incorrect position and back). Is this code covered by automated tests?: No Has the fix been verified in Nightly?: Yes Needs manual test from QE?: No If yes, steps to reproduce: List of other uplifts needed: None Risk to taking this patch: Low Why is the change risky/not risky? (and alternatives if risky): This is a very straightforward change that's obvious in hindsight. Note also that an automated test is planned in bug 1501777. String changes made/needed:
Attachment #9019184 - Flags: approval-mozilla-beta?
> elements that are attached to the screen (e.g. position:fixed) can briefly flicker (i.e. move to an incorrect position and back). I'd like to highlight that in some occasions (not frequent, but not rare) it doesn't move back to the right position and stays in the incorrect position.
Device: - Nexus 5 (Android 6.0.1) Verified as fixed in the latest Nightly (2018-10-29).
Status: RESOLVED → VERIFIED
Flags: qe-verify+
Hardware: Unspecified → ARM
Comment on attachment 9019184 [details] Bug 1493742 - When accepting a layout viewport update from the main thread, constrain its position to enclose the visual viewport. r?kats [Triage Comment] Fixes flickering while scrolling on some pages, approved for 64.0b5.
Attachment #9019184 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Flags: qe-verify+
Kats, do you think we should consider release approval for this fix, to ride along with a potential future dot-release? From a risk management point of view, the patch seems very low risk: the call to RecalculateViewportOffset() that was added has the effect of causing us to maintain an invariant (the layout viewport encloses the visual viewport) that should basically hold at all times. Even if, hypothetically, it turns out we made the condition for making this call broader than necessary, it's hard to imagine any negative side effects. (To put it in other words, there's no such thing as calling RecalculateViewportOffset() "too often".)
Flags: needinfo?(kats)
Sure, I don't see much harm in requesting release approval. The patch seems safe enough.
Flags: needinfo?(kats)
Comment on attachment 9019184 [details] Bug 1493742 - When accepting a layout viewport update from the main thread, constrain its position to enclose the visual viewport. r?kats [Beta/Release Uplift Approval Request] Feature/Bug causing the regression: Bug 1465616 User impact if declined: When scrolling a page such that toolbar transitions are triggered, elements that are attached to the screen (e.g. position:fixed) can briefly flicker (i.e. move to an incorrect position and back). Occasionally, they can also get stuck in the incorrect position until further user input. Is this code covered by automated tests?: No Has the fix been verified in Nightly?: Yes Needs manual test from QE?: Yes If yes, steps to reproduce: List of other uplifts needed: None Risk to taking this patch: Low Why is the change risky/not risky? (and alternatives if risky): For reasons set out in comment 45, I believe this to be a very low risk patch. String changes made/needed:
Attachment #9019184 - Flags: approval-mozilla-release?
^ Note: I am proposing this for uplift to 63 only as a ride-along to any future dot release we might have. I don't believe the regression is severe enough to trigger a dot release itself.
Changing 63 status as we will evaluate it for a dot release.
Device: - Google Pixel C (Android 8.0.0) Verified as fixed in the latest Beta build 64b.0b5 (2018-10-29).
Flags: qe-verify+
Comment on attachment 9019184 [details] Bug 1493742 - When accepting a layout viewport update from the main thread, constrain its position to enclose the visual viewport. r?kats Fix verified in Beta, minimal patch evaluated by the author as being low risk, approved for our next 63 dot release for Fennec. Thanks.
Attachment #9019184 - Flags: approval-mozilla-release? → approval-mozilla-release+
Flags: qe-verify+
Verified in the latest version of release as well. Everything works as expected and the issue could no longer be reproduced.
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: