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

VERIFIED FIXED in Firefox 63

Status

()

defect
VERIFIED FIXED
7 months ago
5 months ago

People

(Reporter: julienw, Assigned: botond)

Tracking

({regression})

Trunk
mozilla65
ARM
Android
Points:
---

Firefox Tracking Flags

(firefox-esr60 unaffected, firefox62 unaffected, firefox63+ verified, firefox64+ verified, firefox65+ verified)

Details

Attachments

(7 attachments)

(Reporter)

Description

7 months ago
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.
(Reporter)

Comment 1

7 months ago
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.
(Reporter)

Comment 2

7 months ago
Once we're in this state, this is very easy to get this issue by simply scrolling.
(Reporter)

Comment 3

7 months ago
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)
(Reporter)

Comment 5

7 months ago
This is happening on Android 6 on a Z3C.
Flags: needinfo?(felash)
(Reporter)

Comment 6

7 months ago
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
Last Resolved: 7 months ago
Flags: needinfo?(jnicol)
Resolution: --- → DUPLICATE
Duplicate of bug: 1490789
(Reporter)

Comment 8

7 months ago
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 → ---
(Reporter)

Comment 10

7 months ago
Great :) Then I change the bug title to reflect this.
Blocks: 1465616
See Also: → 1490789
Summary: Invalidation and painting error on pages with position: fixed → Flickering of (some) sticky elements on Twitter and other websites
(Assignee)

Comment 11

7 months ago
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)
(Reporter)

Comment 12

7 months ago
Yeah this seems to fix it for me indeed :)
Flags: needinfo?(felash)
So is this a dupe, then?
Flags: needinfo?(botond)
(Assignee)

Comment 14

7 months ago
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)

Comment 16

7 months ago
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
(Reporter)

Comment 17

6 months ago
Flags: needinfo?(felash)
(Reporter)

Comment 19

6 months ago
I still see the issues in latest nightly.
(Assignee)

Comment 20

6 months ago
(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)
(Reporter)

Comment 22

6 months ago
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)
(Reporter)

Comment 23

6 months ago
This is still quite easy to reach such a situation.
(Reporter)

Comment 24

6 months ago
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!
(Assignee)

Comment 26

6 months ago
(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
(Assignee)

Updated

6 months ago
See Also: → 1495055
(Assignee)

Comment 27

6 months ago
I believe I've tracked down the remaining issue here.
(Assignee)

Comment 28

6 months ago
APZ wants the *size* of the layout viewport from the main thread, but it
knows the position better.
(Reporter)

Comment 30

6 months ago
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)

Comment 31

6 months ago
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

Comment 32

6 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/1d3872dd20e1
Status: NEW → RESOLVED
Last Resolved: 7 months ago6 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla65
Flags: qe-verify+
(Assignee)

Comment 33

6 months ago
(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)
(Reporter)

Comment 34

6 months ago
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)
(Assignee)

Comment 35

6 months ago
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)
(Assignee)

Comment 36

6 months ago
(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!)
(Reporter)

Comment 37

6 months ago
I filed bug 1502515, not sure what the right component is.
(Reporter)

Comment 38

6 months ago
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.
(Assignee)

Comment 39

6 months ago
(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.
(Assignee)

Comment 40

6 months ago
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?
(Reporter)

Comment 41

6 months ago
> 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+
(Assignee)

Comment 45

6 months ago
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)
(Assignee)

Comment 47

6 months ago
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?
(Assignee)

Comment 48

6 months ago
^ 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.