1063494 - Fixed position header in Gaia messaging app does not stay fixed with async scrolling

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect)

Description

[Chris Lord [:cwiiis]]

STR:

1- Open up a messaging thread long enough to scroll (may need to span more than a day, not sure)
2- Scroll up and down

Expected:

The view scrolls normally.

Actual:

The fixed-position date bar at the top jitters about during scrolling

This is almost certainly fallout from multi-layer apzc, so I'm going to block that, but adding regressionwindow-wanted just in case.

Comment 1

[Kartikaya Gupta (email:[e-mail])]

Attachment: Layers dump

I'm able to reproduce this on a flame running latest master. I used |make reference workload-medium| and opened up one of the SMS threads that gets populated. It looks like the fixed-position layer isn't getting aligned in AsyncCompositionManager and so it only moves on repaints. Layers dump attached.

Comment 2

[Kartikaya Gupta (email:[e-mail])]

This is probably happening because the layer with isStickyPosition set on it is the same layer that has the scrollable metrics. So yeah, almost certainly a regression from multi-layer-apz, but we should be able to fix it in AsyncCompositionManager, I think.

Comment 3

[Kartikaya Gupta (email:[e-mail])]

Attachment: Patch

This fixes it. If I don't take out the clip rect bit then the date row partially disappears while scrolling. I don't understand why the clip rect is shifted at all, it seems to me like if we're not adjusting the clip rect for async scroll then we shouldn't be adjusting it for fixed layers either.

Comment 4

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8485775 [details] [diff] [review]
Patch

Actually after thinking about it some more I think the patch might be wrong. I'll see if I can write some tests to exercise this.

Comment 5

[Kartikaya Gupta (email:[e-mail])]

Attachment: Patch v2

Ok, this patch I think is correct.

Try push with just the sticky-pos test showing that it fails: https://tbpl.mozilla.org/?tree=Try&rev=5461ecde83f8
Try push with the full patch showing that it passes:
https://tbpl.mozilla.org/?tree=Try&rev=35895c90f1e0

The fixed-pos test would have failed on the previous version (I verified that locally, but didn't do a try run) of the patch but passes with this version.

Comment 6

[Chris Lord [:cwiiis]]

Comment on attachment 8486363 [details] [diff] [review]
Patch v2

Not that my opinion matters too much on this now, but this looks good to me :)

Comment 7

[Kartikaya Gupta (email:[e-mail])]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ec8a228486a8

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/ec8a228486a8

Comment 9

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8486363 [details] [diff] [review]
Patch v2

Approval Request Comment
[Feature/regressing bug #]: bug 967844
[User impact if declined]: some pieces of content that are position:fixed or position:sticky jitter while scrolling
[Describe test coverage new/current, TBPL]: added reftests, manual testing
[Risks and why]: low risk, code is pretty well understood. only affects platforms with async scrolling (b2g and fennec)
[String/UUID change made/needed]: none

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/7eb878988c9f

Comment 11

[Coler]

This issue has been verified successfully on Flame2.1&2.2.
Reproducing rate: 0/5
See attachment: Verify_Flame_FTE.png

Flame2.1 build version:
Gaia-Rev        dbaf3e31c9ba9c3436e074381744f2971e15c7bf
Gecko-Rev       https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/ebce587d2194
Build-ID        20141203001205
Version         34.0

Flame2.2 build version:
Gaia-Rev        725685831f5336cf007e36d9a812aad689604695
Gecko-Rev       https://hg.mozilla.org/mozilla-central/rev/2c9781c3e9b5
Build-ID        20141203040207
Version         37.0a1

Comment 12

[Coler]

Attachment: Verify_Flame_SMS.MP4