Fixed sticky header on NYTimes comment section not sticky on Fennec/GV

RESOLVED FIXED in Firefox 65

Status

()

defect
P3
normal
RESOLVED FIXED
10 months ago
5 months ago

People

(Reporter: yoasif, Assigned: botond)

Tracking

(Blocks 2 bugs, {regression})

unspecified
mozilla65
Unspecified
Android
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox63 wontfix, firefox64 wontfix, firefox65 fixed)

Details

(Whiteboard: [geckoview:p3][gfx-noted])

Attachments

(5 attachments, 1 obsolete attachment)

Steps to reproduce:

1. Navigate to https://www.nytimes.com/2018/10/22/world/europe/khashoggi-fiance-photos.html?action=click&module=Top+Stories&pgtype=Homepage#commentsContainer on Fennec or Firefox Focus using GV
2. Scroll down

What happens:

The sticky header containing "NYT Picks, Reader Picks, All" disappears and flickers. 

Expected result:

The sticky header containing "NYT Picks, Reader Picks, All" should remain static on screen as a user scrolls. 

See screen recording attached.
Component: General → Layout
Component: Layout → Web Painting
Hey Chris, saw your activity in other GV tickets and was hoping you could help direct this to the right place. Sorry about NIing you!
Flags: needinfo?(cpeterson)
Thanks for reporting this bug, Asif. Your recording makes the header problem easy to see!

Which Fennec version did you test? Is this a new problem in Fennec 63/64/65?
Flags: needinfo?(cpeterson)
Whiteboard: [geckoview]
The issue exists in stable Fennec along with Nightly. I originally saw the issue in Nightly.
Also unsure if it is a regression prior to 63 and haven't tested 64.
David, can a graphics engineer take a look at this bug? GeckoView triage is not sure whether this is a graphics bug or a perf issue with the website trying to use JS to reposition the sticky header.
Flags: needinfo?(dbolter)
OS: Unspecified → Android
NI'ing some experts re comment 5.
Flags: needinfo?(rhunt)
Flags: needinfo?(dbolter)
Flags: needinfo?(botond)
(In reply to Chris Peterson [:cpeterson] from comment #5)
> a perf issue with the website
> trying to use JS to reposition the sticky header.

That was my first thought, but using Inspector in WebIDE I can see that the header actually is position:sticky, and while the page does have scroll event handlers, as far as I can tell they're not trying to adjust the position of the header.

So it seems we may be looking at a platform bug in APZ handling of position:sticky.
Component: Web Painting → Panning and Zooming
Flags: needinfo?(rhunt)
Flags: needinfo?(botond)
I can reproduce the issue in RDM if I set layout.scroll.root-frame-containers=true.
Loading the page in a debug build triggers the assertion from bug 1427792.
Depends on: 1427792
Priority: -- → P3
Whiteboard: [geckoview] → [geckoview][gfx-noted]
Whiteboard: [geckoview][gfx-noted] → [geckoview:p3][gfx-noted]
(In reply to Botond Ballo [:botond] from comment #9)
> Loading the page in a debug build triggers the assertion from bug 1427792.

If you comment out that assertion, you then get assertion in APZ code here [1] and here [2].

[1] https://searchfox.org/mozilla-central/rev/d850d799a0009f851b5535580e0a8b4bb2c591d7/gfx/layers/apz/src/HitTestingTreeNode.cpp#96
[2] https://searchfox.org/mozilla-central/rev/d850d799a0009f851b5535580e0a8b4bb2c591d7/gfx/layers/apz/src/HitTestingTreeNode.cpp#409
Blocks: APZLayout
Posted file Reduced testcase (obsolete) —
Attached is a reduced testcase.

Notably, the position:sticky element is inside a position:fixed element.
Posted file Reduced testcase
Now with less tabs.
Attachment #9025787 - Attachment is obsolete: true
Posted file Frame tree
Attached is the frame tree of the reduced testcase.

The assertion is firing when ProcessDisplayItems() is creating the nsDisplayStickyPosition item for the sticky element. The first ASR (from ContainerState::mContainerScrollMetadataASR) is the ASR for the root scroll frame, and the second ASR (from nsDisplayItem::mActiveScrolledRoot) is the ASR for the subframe. Since the subframe is in an out-of-flow subtree, neither ASR is an ancestor of the other.
I've discussed this with Markus. The underlying issue is not specific to containerful scrolling, but can also be triggered in containerless scenarios with certain uses of scrolled clips. The issue is related to how the display list's model of ASRs and clips is mapped onto the layers API's model by FrameLayerBuilder.

We have discussed a potential path forward for how to revise the relevant code in FrameLayerBuilder to perform this mapping more accurately.
Blocks: 1493250
Assignee: nobody → botond
Attachment #9025789 - Attachment mime type: text/plain → text/html
(In reply to Botond Ballo [:botond] from comment #14)
> We have discussed a potential path forward for how to revise the relevant
> code in FrameLayerBuilder to perform this mapping more accurately.

We've talked through a way to clean up this code a bit, in a way that should also fix this bug.

However, I'd also like to try for a more targeted fix here, rather than blocking on the cleanup.
(In reply to Botond Ballo [:botond] from comment #15)
> However, I'd also like to try for a more targeted fix here, rather than
> blocking on the cleanup.

That being:

https://hg.mozilla.org/try/rev/18d59deab9054ffe687c4df238cc5c3afa1ae0bf
Depends on D13346
Pushed by bballo@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/a96e2090fc20
In ProcessDisplayItems(), handle the case where scrollMetadataASR and mContainerScrollMetadataASR are in different branches of the ASR tree. r=mstange
https://hg.mozilla.org/integration/autoland/rev/68db1bda769f
Add a reftest. r=mstange
https://hg.mozilla.org/mozilla-central/rev/a96e2090fc20
https://hg.mozilla.org/mozilla-central/rev/68db1bda769f
Status: NEW → RESOLVED
Closed: 9 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla65
No longer blocks: 1493250
Duplicate of this bug: 1493250
I think we're too close to the 64 release to be uplifting this.
(In reply to Botond Ballo [:botond] from comment #15)
> We've talked through a way to clean up this code a bit, in a way that should
> also fix this bug.
> 
> However, I'd also like to try for a more targeted fix here, rather than
> blocking on the cleanup.

Filed bug 1511419 to track the cleanup.
Blocks: 1511419
No longer depends on: 1427792
You need to log in before you can comment on or make changes to this bug.