1664101 - Flickering top header on newsweek

Status: VERIFIED FIXED

(Core :: Panning and Zooming, defect, P3)

Description

[Asif Youssuff]

Steps to reproduce:

1. Navigate to https://www.newsweek.com/how-defeat-trumps-plan-overturn-election-opinion-1526242
2. Scroll down to middle of page
3. scroll up and down using touchpad quickly

What happens:

The red navbar disappears and reappears. Other page elements like bottom ads may also disappear and reappear.

Expected result:

Static navbar

81:54.50 INFO: Narrowed integration regression window from [a518405a, a76193de] (3 builds) to [a518405a, a09e4ce9] (2 builds) (~1 steps left)
81:54.50 INFO: No more integration revisions, bisection finished.
81:54.50 INFO: Last good revision: a518405abe4c93e6d8df622f59d5f01c4766e1d5
81:54.50 INFO: First bad revision: a09e4ce9ebba8fa4a29a4833560c8ee807018c03
81:54.50 INFO: Pushlog:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=a518405abe4c93e6d8df622f59d5f01c4766e1d5&tochange=a09e4ce9ebba8fa4a29a4833560c8ee807018c03

Comment 1

[Asif Youssuff]

Attachment: about:support

Comment 2

[Timothy Nikkel (:tnikkel)]

Hmm, I couldn't reproduce on mac. It would be interesting to see how long this has been broken when the apz.allow_zooming pref is forced to true.

Comment 3

[Alice0775 White]

I can reproduce the issue on Windows10 when dragging scrollbar thumb. And I got same regression window of comment#0.
screen capture:https://youtu.be/n0AO6jEiHiQ

Comment 4

[Asif Youssuff]

With apz.allow_zooming pref set to true:

36:39.75 INFO: Got as far as we can go bisecting nightlies...
36:39.75 INFO: Last good revision: a0403c2bfae40a8b3cebd532ce730fa824181fee (2019-05-13)
36:39.75 INFO: First bad revision: 6f732caaed60783f57944a66f7ea494f5fd78d6c (2019-05-14)
36:39.75 INFO: Pushlog:
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=a0403c2bfae40a8b3cebd532ce730fa824181fee&tochange=6f732caaed60783f57944a66f7ea494f5fd78d6c

...

36:49.62 INFO: There are no build artifacts for these changesets (they are probably too old).

Comment 5

[Alice0775 White]

I suspect Bug 1459260.

Comment 6

[Asif Youssuff]

Ran another mozregression with "apz.allow_zooming:true" "dom.meta-viewport.enabled:true".

Note that I marked some builds "good" even though there was movement inside of the viewport, resulting in jiggling of the navbar, but not outright flickering (disappearance and reappearance).

23:49.11 INFO: Got as far as we can go bisecting nightlies...
23:49.11 INFO: Last good revision: 7e40e33da3da2640e965a153254594a234231f76 (2019-04-25)
23:49.11 INFO: First bad revision: b13f2b24ae625d16fdeeb61cdec10978c3c75638 (2019-04-26)
23:49.11 INFO: Pushlog:
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=7e40e33da3da2640e965a153254594a234231f76&tochange=b13f2b24ae625d16fdeeb61cdec10978c3c75638

...

23:59.33 INFO: There are no build artifacts for these changesets (they are probably too old).

Comment 7

[Timothy Nikkel (:tnikkel)]

(In reply to Asif Youssuff from comment #6)

Ran another mozregression with "apz.allow_zooming:true" "dom.meta-viewport.enabled:true".

Note that I marked some builds "good" even though there was movement inside of the viewport, resulting in jiggling of the navbar, but not outright flickering (disappearance and reappearance).

23:49.11 INFO: Got as far as we can go bisecting nightlies...
23:49.11 INFO: Last good revision: 7e40e33da3da2640e965a153254594a234231f76 (2019-04-25)
23:49.11 INFO: First bad revision: b13f2b24ae625d16fdeeb61cdec10978c3c75638 (2019-04-26)
23:49.11 INFO: Pushlog:
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=7e40e33da3da2640e965a153254594a234231f76&tochange=b13f2b24ae625d16fdeeb61cdec10978c3c75638

...

23:59.33 INFO: There are no build artifacts for these changesets (they are probably too old).

Thanks. I suspect bug 1529892.

Comment 8

[Botond Ballo [:botond]]

This definitely looks like a regression from bug 1529892. I'll investigate.

Comment 9

[Botond Ballo [:botond]]

One thing I'm noticing is that, while the intent of the change in bug 1529892 was to shrink the dirty rect for fixed elements, by shrinking to the visual viewport and then expanding to the displayport, it actually had the effect of expanding the dirty rect in cases where we not zoomed in much, since in those cases the displayport can be larger than the layout viewport (which was the original size of the dirty rect).

This in and of itself wouldn't cause this bug, but it's something we should probably fix while touching this code.

Comment 10

[Botond Ballo [:botond]]

Here is what seems to be happening during frames where the fixed elements checkerboard:

• Due to fast scrolling, there is a large (transient) async translation between the visual and layout scroll offset. For example, the layout scroll offset (where we last painted) may be at y=2000, and the visual scroll offset (where we are currently compositing) may be at y=5000.
• A paint happens, at a time when APZ has already sent a repaint request reflecting the y=5000 visual offset, but the main thread has not yet processed this repaint request. (The repaint request is sitting behind the paint in the event queue, as described in bug 1242609 comment 2.)
• The main thread peeks at the pending repaint request before painting, so it knows to paint a display port that reflects the fact we're compositing at y=5000. However, the layout scroll offset is still at y=2000 (it won't be updated until the repaint request is actually processed).
• As a result, the display port rect computed at painting time, which is interpreted as being relative to the layout scroll offset, is something like y=3000, minus any display port margin -- so let's say we had a top=500 margin, it would be y=2500. This is then added to the layout scroll offset of y=2000 to produce y=4500, which is the document-relative offset where we actually start painting (appropriate for compositing around y=5000).
• The problem occurs when we apply this display port rect to fixed content. Fixed content is fixed to the layout viewport, so naively, applying that same y=2500 display port rect to the fixed content should work. Indeed, it works in cases where you're zoomed in, and there's a non-transient y=3000 offset between the visual and layout viewports; in such a case, we really do want to start painting the fixed content at something like y=2500 to reflect that we're only compositing a part of it that starts at y=3000. (This is the sort of case bug 1529892 intended to address: painting at y=0 in such a case ended up painting a very large texture and OOMing at high zoom levels.)
• However, in a case like this where there is a transient y=3000 offset, this logic results in the display port rect clipping out parts of the fixed content that are actually visible (in this case, all of it).

Comment 11

[Botond Ballo [:botond]]

To explain the problem a different way:

• For scrolled content, it does not matter whether a discrepancy between the last-painted layout scroll offset and the latest visual scroll offset is in visual component of the async transform, or the layout component: at composition time, we apply both components to scrolled content, so the outcome is all the same (e.g. layout will paint at y=3000, and the compositor will apply a y=-3000 transform to the layers, and the two cancel out).
• For fixed content, however, we only apply the visual component of the async transform to them at composition time. If a y=3000 discrepancy is actually part of the layout component of the transform, and we paint the fixed content at y=3000, there is no corresponding compositor transform to cancel that out, and the fixed content remains offscreen.

Comment 12

[Botond Ballo [:botond]]

Attachment: Bug 1664101 - Avoid expanding the visible rect for fixed content to be larger than the layout viewport. r=kats

Comment 13

[Botond Ballo [:botond]]

Attachment: Bug 1664101 - Fix an incorrect usage of the visual viewport offset in ComputeVisibleRectForFrame(). r=kats

It's used as the offset of the visible rect, which is intended to be
relative to the layout viewport.

This was previous hidden by the fact that we'd basically always
overwrite this value with the displayport rect offset.

As this patch makes it such that we only sometimes use the displayport
rect offset, it becomes more important that the fallback value is
correct.

Depends on D90783

Comment 14

[Botond Ballo [:botond]]

The patches I posted are more of a workaround, so that we can un-regress the nightly behaviour relatively quickly. The underlying problem described in comment 10 is still there, and can still cause incorrect checkerboarding of fixed elements, but now only at zoom levels that are large enough such that the displayport is smaller than the layout viewport in area.

I do have a proper fix for underlying problem in mind, but that will be a bit more involved, as it requires refactoring the way we store and compute displayports on the main thread a bit.

Comment 15

[Botond Ballo [:botond]]

(In reply to Botond Ballo [:botond] from comment #14)

I do have a proper fix for underlying problem in mind, but that will be a bit more involved, as it requires refactoring the way we store and compute displayports on the main thread a bit.

Here's an outline of the fix I have in mind:

• Instead of storing just a set of margins in DisplayPortMarginsPropertyData, which are possibly the result of adjustment to reflect async scrolling, store the inputs to the adjustment (namely, the unadjusted margins, the APZ scroll position, and the layout scroll position) instead.
• When querying the display port margins for the purpose of applying them to scrolled content, apply the adjustment using the saved inputs at query time.
• When querying the display port margins for the purpose of applying them to fixed content, apply a different adjustment appropriate for fixed content instead. (This basically entails computing an "async layout viewport" the way APZ would, using that to compute the visual component of the async transform, and applying an adjustment using the visual component only.)

Comment 19

[Botond Ballo [:botond]]

Attachment: Bug 1664101 - Move DisplayPort*PropertyData below DisplayPortOptions. r=kats

In a later patch, DisplayPortMarginsPropertyData will have a
dependency on one of the options.

Depends on D90784

Comment 20

[Botond Ballo [:botond]]

Attachment: Bug 1664101 - Store displayport margins in unadjusted form. r=kats

Where an adjustment (to reflect a delta between the APZ and layout
scroll offsets) is necessary, the inputs needed to compute the
adjustment are stored with the margins, and the adjustment is
applied at query time.

A couple of notes on this patch:

• Storing DisplayPortMargins::mLayoutOffset is probably unnecessary,
  we should be able to just query the scroll frame's layout offset
  when applying the margins.

• Some callers of DisplayPortMargins::WithNoAdjustment() may be
  incorrect, in that they pass in margins that are relative to the
  visual viewport but do not make a corresponding adjustment.
  This is a pre-existing issue that this patch just makes clearer.

As this is a regression-prone area, this patch is careful to avoid
making any functional changes, leaving the above issues to be
addressed in future bugs.

Depends on D92505

Comment 21

[Botond Ballo [:botond]]

Attachment: Bug 1664101 - Propagate DisplayPortOptions up to GetHighResolutionDisplayPort and down to GetDisplayPortFromMarginsData. r=kats

Depends on D92506

Comment 22

[Botond Ballo [:botond]]

Attachment: Bug 1664101 - When querying the displayport for the purpose of applying it to fixed content, apply an async scroll adjustment suitable for fixed content. r=kats

Depends on D92507

Comment 23

[Botond Ballo [:botond]]

Attachment: Bug 1664101 - Use plain ok() rather than SimpleTest.ok() in apz_test_utils.js. r=kats

Similarly for SimpleTest.is().

This ensures that in subtests, the wrapper function that also displays
the subtest name is used.

Depends on D92508

Comment 24

[Phabricator Automation]

Comment on attachment 9179735 [details]
Bug 1664101 - Use plain ok() rather than SimpleTest.ok() in apz_test_utils.js. r=kats

Revision D92509 was moved to bug 1669362. Setting attachment 9179735 [details] to obsolete.

Comment 25

[Botond Ballo [:botond]]

Asif, could I ask you to try a build from this Try push (such as, for Windows, this build) and check that the issue is resolved in that build?

Comment 26

[Asif Youssuff]

Botond, I tried the build on Linux and the issue is fixed for me on this page. Thanks!

Comment 27

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5cfe0d20b639
Fix an incorrect usage of the visual viewport offset in ComputeVisibleRectForFrame(). r=kats
https://hg.mozilla.org/integration/autoland/rev/d2ecf9f43535
Move DisplayPort*PropertyData below DisplayPortOptions. r=kats
https://hg.mozilla.org/integration/autoland/rev/428d3b604dac
Store displayport margins in unadjusted form. r=kats
https://hg.mozilla.org/integration/autoland/rev/8c919de3454f
Propagate DisplayPortOptions up to GetHighResolutionDisplayPort and down to GetDisplayPortFromMarginsData. r=kats
https://hg.mozilla.org/integration/autoland/rev/80c240859254
When querying the displayport for the purpose of applying it to fixed content, apply an async scroll adjustment suitable for fixed content. r=kats

Comment 28

[Atila Butkovits]

https://hg.mozilla.org/mozilla-central/rev/5cfe0d20b639
https://hg.mozilla.org/mozilla-central/rev/d2ecf9f43535
https://hg.mozilla.org/mozilla-central/rev/428d3b604dac
https://hg.mozilla.org/mozilla-central/rev/8c919de3454f
https://hg.mozilla.org/mozilla-central/rev/80c240859254

Comment 29

[Hani Yacoub, Desktop QA]

Verified as fixed on Firefox 84.0a1 (2020-11-10) and on Firefox 83.0b10 on Windows 10 x64 and Ubuntu 20.04 x64.