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)
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)
(deleted),
image/png
|
Details | |
(deleted),
image/png
|
Details | |
(deleted),
image/png
|
Details | |
(deleted),
image/png
|
Details | |
(deleted),
image/png
|
Details | |
(deleted),
image/png
|
Details | |
(deleted),
text/x-phabricator-request
|
RyanVM
:
approval-mozilla-beta+
pascalc
:
approval-mozilla-release+
|
Details |
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•6 years 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•6 years ago
|
||
Once we're in this state, this is very easy to get this issue by simply scrolling.
Reporter | ||
Comment 3•6 years 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).
Updated•6 years ago
|
OS: Unspecified → Android
Reporter | ||
Comment 6•6 years ago
|
||
Hey Jamie, Nical told me you might be the right person to ping here.
Flags: needinfo?(jnicol)
Comment 7•6 years ago
|
||
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
Reporter | ||
Comment 8•6 years 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)
Comment 9•6 years ago
|
||
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•6 years ago
|
||
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
Updated•6 years ago
|
Keywords: regression
Assignee | ||
Comment 11•6 years 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•6 years ago
|
||
Yeah this seems to fix it for me indeed :)
Flags: needinfo?(felash)
Assignee | ||
Comment 14•6 years 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)
Comment 15•6 years ago
|
||
Bug 1495055 should be in today's fennec nightly, can you re-test?
Flags: needinfo?(felash)
Comment 16•6 years 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
Updated•6 years ago
|
QA Contact: svoisen
Reporter | ||
Comment 17•6 years ago
|
||
Flags: needinfo?(felash)
Reporter | ||
Comment 18•6 years ago
|
||
Reporter | ||
Comment 19•6 years ago
|
||
I still see the issues in latest nightly.
Assignee | ||
Comment 20•6 years 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.
Updated•6 years ago
|
Updated•6 years ago
|
Comment 21•6 years ago
|
||
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 years 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 years ago
|
||
This is still quite easy to reach such a situation.
Reporter | ||
Comment 24•6 years ago
|
||
I see this on Twitter too. But I think it's more difficult to trigger it, at least on Twitter.
Comment 25•6 years ago
|
||
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 years 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 years ago
|
Assignee | ||
Comment 27•6 years ago
|
||
I believe I've tracked down the remaining issue here.
Assignee | ||
Comment 28•6 years ago
|
||
APZ wants the *size* of the layout viewport from the main thread, but it
knows the position better.
Assignee | ||
Comment 29•6 years ago
|
||
Julien, any chance you could give this build [1] (from this Try push [2]) a spin?
[1] https://queue.taskcluster.net/v1/task/BaM-7UBgS9SZAXjNOokyNw/runs/0/artifacts/public/build/target.apk
[2] https://treeherder.mozilla.org/#/jobs?repo=try&revision=a8ebd816471732931de5929fd9cd966d13305f55
Flags: needinfo?(felash)
Reporter | ||
Comment 30•6 years 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 years 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 years ago
|
||
bugherder |
Status: NEW → RESOLVED
Closed: 6 years ago → 6 years ago
status-firefox65:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla65
Updated•6 years ago
|
Flags: qe-verify+
Assignee | ||
Comment 33•6 years 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 years 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 years 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 years 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 years ago
|
||
I filed bug 1502515, not sure what the right component is.
Reporter | ||
Comment 38•6 years 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 years 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 years 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 years 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.
Comment 42•6 years ago
|
||
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 43•6 years 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
[Triage Comment]
Fixes flickering while scrolling on some pages, approved for 64.0b5.
Attachment #9019184 -
Flags: approval-mozilla-beta? → approval-mozilla-beta+
Comment 44•6 years ago
|
||
bugherder uplift |
Updated•6 years ago
|
Flags: qe-verify+
Assignee | ||
Comment 45•6 years 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)
Comment 46•6 years ago
|
||
Sure, I don't see much harm in requesting release approval. The patch seems safe enough.
Flags: needinfo?(kats)
Assignee | ||
Comment 47•6 years 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 years 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.
Comment 49•6 years ago
|
||
Changing 63 status as we will evaluate it for a dot release.
Comment 50•6 years ago
|
||
Device:
- Google Pixel C (Android 8.0.0)
Verified as fixed in the latest Beta build 64b.0b5 (2018-10-29).
Flags: qe-verify+
Updated•6 years ago
|
tracking-firefox64:
+ → ---
Updated•6 years ago
|
tracking-firefox64:
--- → +
tracking-firefox65:
--- → +
Comment 51•6 years 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
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+
Comment 52•6 years ago
|
||
bugherder uplift |
Updated•6 years ago
|
Flags: qe-verify+
Comment 53•6 years ago
|
||
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.
Description
•