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

RESOLVED FIXED in Firefox 34, Firefox OS v2.1

Status

()

Core
Graphics: Layers
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: cwiiis, Assigned: kats)

Tracking

({regression})

Trunk
mozilla35
All
Gonk (Firefox OS)
regression
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

3 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.
Created attachment 8485748 [details]
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.
Created attachment 8485775 [details] [diff] [review]
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.
Assignee: nobody → bugmail.mozilla
Attachment #8485775 - Flags: review?(botond)
Attachment #8485775 - Flags: review?(bgirard)
Keywords: regressionwindow-wanted
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)
Created attachment 8486363 [details] [diff] [review]
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.
Attachment #8485775 - Attachment is obsolete: true
Attachment #8486363 - Flags: review?(botond)
Attachment #8486363 - Flags: review?(bgirard)
(Reporter)

Comment 6

3 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+

Updated

3 years ago
Attachment #8486363 - Flags: review?(botond) → review+

Updated

3 years ago
Attachment #8486363 - Flags: review?(bgirard) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/ec8a228486a8
https://hg.mozilla.org/mozilla-central/rev/ec8a228486a8
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
status-b2g-v2.0: --- → unaffected
status-b2g-v2.1: --- → affected
status-b2g-v2.2: --- → fixed
status-firefox33: --- → unaffected
status-firefox34: --- → affected
status-firefox35: --- → fixed
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?

Updated

3 years ago
Attachment #8486363 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
https://hg.mozilla.org/releases/mozilla-aurora/rev/7eb878988c9f
status-b2g-v2.1: affected → fixed
status-firefox34: affected → fixed
Depends on: 1100140

Comment 11

3 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
status-b2g-v2.1: fixed → verified
status-b2g-v2.2: fixed → verified

Comment 12

3 years ago
Created attachment 8531879 [details]
Verify_Flame_SMS.MP4
You need to log in before you can comment on or make changes to this bug.