Julien Wajsberg [:julienw] Reporter
	Description • 6 years 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.

	Julien Wajsberg [:julienw] 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.

	Julien Wajsberg [:julienw] Reporter
	Comment 2 • 6 years ago

Once we're in this state, this is very easy to get this issue by simply scrolling.

	Julien Wajsberg [:julienw] 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).

	Julien Cristau [:jcristau]
	Comment 4 • 6 years ago

Which device and android version?

	Julien Wajsberg [:julienw] Reporter
	Comment 5 • 6 years ago

This is happening on Android 6 on a Z3C.

	Julien Wajsberg [:julienw] Reporter
	Comment 6 • 6 years ago

Hey Jamie, Nical told me you might be the right person to ping here.

	Jamie Nicol [: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.

	Julien Wajsberg [:julienw] 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.

	Jamie Nicol [: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.

	Julien Wajsberg [:julienw] Reporter
	Comment 10 • 6 years ago

Great :) Then I change the bug title to reflect this.

	Botond Ballo [:botond] 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.

	Julien Wajsberg [:julienw] Reporter
	Comment 12 • 6 years ago

Yeah this seems to fix it for me indeed :)

	David Durst [:ddurst]
	Comment 13 • 6 years ago

So is this a dupe, then?

	Botond Ballo [:botond] 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.

	Julien Cristau [:jcristau]
	Comment 15 • 6 years ago

Bug 1495055 should be in today's fennec nightly, can you re-test?

	Levente Sacal
	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.

	Julien Wajsberg [:julienw] Reporter
	Comment 19 • 6 years ago

I still see the issues in latest nightly.

	Botond Ballo [:botond] 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.

	Pascal Chevrel:pascalc (PTO until August 21)
	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

	Julien Wajsberg [:julienw] 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/.

	Julien Wajsberg [:julienw] Reporter
	Comment 23 • 6 years ago

This is still quite easy to reach such a situation.

	Julien Wajsberg [:julienw] 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.

	Pascal Chevrel:pascalc (PTO until August 21)
	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!

	Botond Ballo [:botond] 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.

	Botond Ballo [:botond] Assignee
	Comment 27 • 6 years ago

I believe I've tracked down the remaining issue here.

	Botond Ballo [:botond] Assignee
	Comment 28 • 6 years ago

APZ wants the *size* of the layout viewport from the main thread, but it
knows the position better.

	Botond Ballo [:botond] 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

	Julien Wajsberg [:julienw] 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...

	Pulsebot
	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

	Raul Gurzau (:RaulG)
	Comment 32 • 6 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/1d3872dd20e1

	Botond Ballo [:botond] 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?

	Julien Wajsberg [:julienw] 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?

	Botond Ballo [: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.

	Botond Ballo [: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!)

	Julien Wajsberg [:julienw] Reporter
	Comment 37 • 6 years ago

I filed bug 1502515, not sure what the right component is.

	Julien Wajsberg [:julienw] 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.

	Botond Ballo [:botond] 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.

	Botond Ballo [:botond] 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:

	Julien Wajsberg [:julienw] 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.

	Bogdan Surd, QA [:bsurd, NI]
	Comment 42 • 6 years ago

Device:
 - Nexus 5 (Android 6.0.1)

Verified as fixed in the latest Nightly (2018-10-29).

	Ryan VanderMeulen [:RyanVM]
	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.

	Ryan VanderMeulen [:RyanVM]
	Comment 44 • 6 years ago
bugherder uplift

https://hg.mozilla.org/releases/mozilla-beta/rev/805ba967bcb5

	Botond Ballo [:botond] 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".)

	Kartikaya Gupta (email:kats@mozilla.staktrace.com)
	Comment 46 • 6 years ago

Sure, I don't see much harm in requesting release approval. The patch seems safe enough.

	Botond Ballo [:botond] 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:

	Botond Ballo [:botond] 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.

	Pascal Chevrel:pascalc (PTO until August 21)
	Comment 49 • 6 years ago

Changing 63 status as we will evaluate it for a dot release.

	Andrei Bodea[:andrei]
	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).

	Pascal Chevrel:pascalc (PTO until August 21)
	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.

	Ryan VanderMeulen [:RyanVM]
	Comment 52 • 6 years ago
bugherder uplift

https://hg.mozilla.org/releases/mozilla-release/rev/032ed494717a

	Bogdan Surd, QA [:bsurd, NI]
	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.