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

Status: RESOLVED FIXED

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

Description

[Asif Youssuff]

Attachment: recording_20181022_085251.mp4

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.

Comment 1

[Asif Youssuff]

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!

Comment 2

[Chris Peterson [: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?

Comment 3

[Asif Youssuff]

The issue exists in stable Fennec along with Nightly. I originally saw the issue in Nightly.

Comment 4

[Asif Youssuff]

Also unsure if it is a regression prior to 63 and haven't tested 64.

Comment 5

[Chris Peterson [:cpeterson]]

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.

Comment 6

[David Bolter [:davidb] (NeedInfo me for attention)]

NI'ing some experts re comment 5.

Comment 7

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

Comment 8

[Botond Ballo [:botond]]

I can reproduce the issue in RDM if I set layout.scroll.root-frame-containers=true.

Comment 9

[Botond Ballo [:botond]]

Loading the page in a debug build triggers the assertion from bug 1427792.

Comment 10

[Botond Ballo [:botond]]

(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

Comment 11

[Botond Ballo [:botond]]

Attachment: Reduced testcase

Attached is a reduced testcase.

Notably, the position:sticky element is inside a position:fixed element.

Comment 12

[Botond Ballo [:botond]]

Attachment: Reduced testcase

Now with less tabs.

Comment 13

[Botond Ballo [:botond]]

Attachment: 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.

Comment 14

[Botond Ballo [:botond]]

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.

Comment 15

[Botond Ballo [:botond]]

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

Comment 16

[Botond Ballo [:botond]]

(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

Comment 17

[Botond Ballo [:botond]]

Attachment: Bug 1501342 - In ProcessDisplayItems(), handle the case where scrollMetadataASR and mContainerScrollMetadataASR are in different branches of the ASR tree. r=mstange

Comment 18

[Botond Ballo [:botond]]

Attachment: Bug 1501342 - Add a reftest. r=mstange

Depends on D13346

Comment 19

[Pulsebot]

Pushed by [e-mail]:
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

Comment 20

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/a96e2090fc20
https://hg.mozilla.org/mozilla-central/rev/68db1bda769f

Comment 22

[Botond Ballo [:botond]]

I think we're too close to the 64 release to be uplifting this.

Comment 23

[Botond Ballo [:botond]]

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