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

RESOLVED FIXED in Firefox 34

Status

()

defect
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: cwiiis, Assigned: kats)

Tracking

({regression})

Trunk
mozilla35
All
Gonk (Firefox OS)
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox33 unaffected, firefox34 fixed, firefox35 fixed, b2g-v2.0 unaffected, b2g-v2.1 verified, b2g-v2.2 verified)

Details

Attachments

(3 attachments, 1 obsolete attachment)

Reporter

Description

5 years ago
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.
Posted file 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.
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.
Posted patch Patch (obsolete) — Splinter Review
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.
Assignee: nobody → bugmail.mozilla
Attachment #8485775 - Flags: review?(botond)
Attachment #8485775 - Flags: review?(bgirard)
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.
Attachment #8485775 - Flags: review?(botond)
Attachment #8485775 - Flags: review?(bgirard)
Posted patch Patch v2Splinter Review
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.
Attachment #8485775 - Attachment is obsolete: true
Attachment #8486363 - Flags: review?(botond)
Attachment #8486363 - Flags: review?(bgirard)
Reporter

Comment 6

5 years ago
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 :)
Attachment #8486363 - Flags: feedback+
Attachment #8486363 - Flags: review?(botond) → review+
Attachment #8486363 - Flags: review?(bgirard) → review+
https://hg.mozilla.org/mozilla-central/rev/ec8a228486a8
Status: NEW → RESOLVED
Closed: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
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
Attachment #8486363 - Flags: approval-mozilla-aurora?
Attachment #8486363 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Depends on: 1100140

Comment 11

5 years ago
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

5 years ago
Posted video Verify_Flame_SMS.MP4
You need to log in before you can comment on or make changes to this bug.