Closed Bug 1664101 Opened 5 months ago Closed 4 months ago

Flickering top header on newsweek

Categories

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

defect

Tracking

()

VERIFIED FIXED
83 Branch
Tracking Status
firefox-esr78 --- unaffected
firefox81 --- disabled
firefox82 --- disabled
firefox83 --- verified
firefox84 --- verified

People

(Reporter: yoasif, Assigned: botond)

References

(Blocks 1 open bug, Regression, )

Details

(Keywords: nightly-community, regression)

Attachments

(6 files, 2 obsolete files)

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

Has Regression Range: --- → yes
Has STR: --- → yes
Keywords: regression
Regressed by: desktop-zoom-nightly
Attached file about:support

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.

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

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).

I suspect Bug 1459260.

Regressed by: 1459260

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).

(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.

Regressed by: 1529892
Severity: -- → S3
Priority: -- → P3

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

Assignee: nobody → 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.

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).

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.

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

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.

(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.)
Duplicate of this bug: 1478690
See Also: → 1614067
Duplicate of this bug: 1667442
Depends on: 1667475
Depends on: 1667594
Duplicate of this bug: 1664743
Attachment #9176720 - Attachment is obsolete: true

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

Depends on D90784

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

Similarly for SimpleTest.is().

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

Depends on D92508

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.

Attachment #9179735 - Attachment is obsolete: true

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?

Flags: needinfo?(yoasif)

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

Flags: needinfo?(yoasif)
Blocks: 1669861
Pushed by bballo@mozilla.com:
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
Flags: qe-verify+

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

Status: RESOLVED → VERIFIED
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.