974643 - [Twitter] The fixed position of content is broken (header occasionally disappears, or partly disappears)

Status: RESOLVED FIXED

(Core :: Layout, defect, P2)

Description

[sarsenyev]

Attachment: video_attach2192014.mp4

Description:
When scrolling a twitter page, the fixed content appears broken

Repro Steps:
1) Updated Buri to BuildID: 20140219040204
2) From home screen, open "browser" and type in URL http://twitter.com
3) Log in into "Twitter"
4) Open any page and scroll down and up

Actual:
The fixed content disappears and appears when scrolling

Expected:
The fixed content stays fixed on the twitter page

Environmental Variables:
Device: Buri 1.4 MOZ
BuildID: 20140219040204
Gaia: ac06cfbd2baf6494ffbb668cc599e3892cd5e17b
Gecko: bf0e76f2a7d4
Version: 30.0a1
Firmware Version: v1.2-device

Notes:
This bug is follow up of bug 941050 

Repro frequency: 100%
See attached: video clip

Comment 1

[sarsenyev]

The issue reproduces on the latest 1.3

Device: Buri 1.3 MOZ
BuildID: 20140219004003
Gaia: a43904d9646109b48836d62f7aa51e499fbf4b2e
Gecko: 97922b6daad1
Version: 28.0
Firmware Version: v1.2-device.cfg

Comment 2

[Jason Smith [:jsmith]]

Followup bug to a regressing blocker. Blocks a partner app.

Comment 3

[Chris Lord [:cwiiis]]

Renaming the bug to reflect what's actually happening.

It looks like the fixed position content is probably falling outside of the display port and not getting rendered/only getting partially rendered.

This probably needs to be fixed in nsDisplayList. n? tn for comment (/fix? :))

Comment 4

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

Is this on the app or the website? Comment 0 says website but comment 2 says app.

Comment 5

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

(Also, should it still be 1.3+ if it's on the website?)

Comment 6

[sarsenyev]

it happens on both and web and the app

Comment 7

[bhavana bajaj [:bajaj]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #5)
> (Also, should it still be 1.3+ if it's on the website?)

when blocking this the understanding was it happens everywhere and basically this is a gecko regression and a follow-up from a blocker bug 941050. And given this is a top app/website it made a strong blocking case.

Comment 8

[Timothy Nikkel (:tnikkel)]

So far I've only seen the problem when scrolling very fast so my first guess is that the displayport is completely outside the page and so doesn't intersect the fixed position content.

Comment 9

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

A quick test confirms.. this happens when the displayport doesn't contain the fixed pos content. Setting the displayport heuristic to "center displayport" in the developer options makes this bug go away (although results in more checkerboarding).

Given that this isn't very easy to reproduce and that it's a very transient problem (only a couple of frames) that goes away on its own I don't think this is a very high priority. Fixing it also has a nontrivial risk of regression.

Comment 10

[Jason Smith [:jsmith]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #9)
> A quick test confirms.. this happens when the displayport doesn't contain
> the fixed pos content. Setting the displayport heuristic to "center
> displayport" in the developer options makes this bug go away (although
> results in more checkerboarding).
> 
> Given that this isn't very easy to reproduce and that it's a very transient
> problem (only a couple of frames) that goes away on its own I don't think
> this is a very high priority. Fixing it also has a nontrivial risk of
> regression.

This is a blocker for a content partner in all target markets & affects a top mobile site. We've got no choice here but to fix this.

Comment 11

[Jason Smith [:jsmith]]

So I tried testing this myself for a point of comparison on a 2/13 1.3 build - I couldn't reproduce this. Two things to clarify here:

1. What exactly happened on 1.3? Can we get a video showing 1.3's behavior?
2. Are you able to reproduce this on a 2/13 1.3 build as a point of comparison?

Build I used to test:

Build ID: 20140213175711
Device: Sora
Version: 1.3

Comment 12

[Matthew Vaughan]

Attachment: Behavior on 2/24 1.3 (header completely disappears for a moment)

(In reply to Jason Smith [:jsmith] from comment #11)
> So I tried testing this myself for a point of comparison on a 2/13 1.3 build
> - I couldn't reproduce this. Two things to clarify here:
> 
> 1. What exactly happened on 1.3? Can we get a video showing 1.3's behavior?

On the 02/24/14 1.3, Twitter's header will partially to fully disappear for a moment or so, but the header will reappear after that. 

This issue will reproduce after quickly scrolling for a little bit and the rest of the webpage starts checkerboarding. 

> 2. Are you able to reproduce this on a 2/13 1.3 build as a point of
> comparison?

This issue does reproduce for me on the 02/13/14 1.3 build on a Buri. The issue is similar when comparing the 2/13 and the 2/24 1.3 builds.

Comment 13

[Jason Smith [:jsmith]]

(In reply to Matthew Vaughan from comment #12)
> Created attachment 8380970 [details]
> Behavior on 2/24 1.3 (header completely disappears for a moment)
> 
> (In reply to Jason Smith [:jsmith] from comment #11)
> > So I tried testing this myself for a point of comparison on a 2/13 1.3 build
> > - I couldn't reproduce this. Two things to clarify here:
> > 
> > 1. What exactly happened on 1.3? Can we get a video showing 1.3's behavior?
> 
> On the 02/24/14 1.3, Twitter's header will partially to fully disappear for
> a moment or so, but the header will reappear after that. 
> 
> This issue will reproduce after quickly scrolling for a little bit and the
> rest of the webpage starts checkerboarding. 
> 
> > 2. Are you able to reproduce this on a 2/13 1.3 build as a point of
> > comparison?
> 
> This issue does reproduce for me on the 02/13/14 1.3 build on a Buri. The
> issue is similar when comparing the 2/13 and the 2/24 1.3 builds.

This isn't the same bug. The bug here involves the fact that twitter header is blinking during scrolling. It's not a full window checkerboarding effect. Please check again.

Comment 14

[Timothy Nikkel (:tnikkel)]

Attachment: patch

I don't think the displayport is even relevant to postion: fixed content (it is relevant to background-attachment: fixed, at least how it is currently implemented). The displayport is only relevant to scrolling content. The only part of position fixed content we'll ever show is what's inside the viewport, or the scroll position clamping scroll port rect (which probably needs a better name now that it is doing a lot more than just that).

This patch fixes the issue for me on hamachi.

Comment 15

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I don't understand this bug and patch. Is the APZC making the displayport not contain the whole CSS viewport? That seems wrong.

Comment 16

[Timothy Nikkel (:tnikkel)]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #15)
> I don't understand this bug and patch. Is the APZC making the displayport
> not contain the whole CSS viewport? That seems wrong.

Yes, that is correct. If the velocity is high then the content we want to draw might be offset enough so that it doesn't cover the viewport (even if it's height is larger than the viewport height). The theory is that by the time the paint is done the scroll offset will have changed enough so that it does cover the part of the scrolled content that we want to composite to the screen.

Comment 17

[Chris Lord [:cwiiis]]

Comment on attachment 8381163 [details] [diff] [review]
patch

Review of attachment 8381163 [details] [diff] [review]:
-----------------------------------------------------------------

Fly-by to say that the patch looks sound (though I'd probably not know if it wasn't...), but some commenting would be nice.

Comment 18

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

Does comment 16 answer your question?

Comment 19

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8381163 [details] [diff] [review]
patch

Review of attachment 8381163 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/base/FrameLayerBuilder.cpp
@@ +1707,5 @@
> +    displayPort.MoveTo(0, 0);
> +    if (presShell->IsScrollPositionClampingScrollPortSizeSet()) {
> +      displayPort.SizeTo(presShell->GetScrollPositionClampingScrollPortSize());
> +    } else {
> +      displayPort.SizeTo(viewport->GetSize());

Change the name of "displayPort" to something else since it's not the displayport anymore.

Comment 20

[Timothy Nikkel (:tnikkel)]

(In reply to Chris Lord [:cwiiis] from comment #17)
> Fly-by to say that the patch looks sound (though I'd probably not know if it
> wasn't...), but some commenting would be nice.

Added some comments.

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #19)
> Change the name of "displayPort" to something else since it's not the
> displayport anymore.

Called it fixedVisibleRect.

https://hg.mozilla.org/integration/b2g-inbound/rev/809aaff30752

Comment 21

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/809aaff30752

Comment 22

[Ryan VanderMeulen [:RyanVM]]

Please request b2g28 uplift on this patch when you get a chance :)

Comment 23

[Timothy Nikkel (:tnikkel)]

Comment on attachment 8381163 [details] [diff] [review]
patch

NOTE: This flag is now for security issues only. Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 919144 added the code changed here
User impact if declined: scrolling pages with fixed position content will sometimes not draw the fixed position content when scrolling quickly
Testing completed: been on m-c for a bit
Risk to taking this patch (and alternatives if risky): should be pretty low
String or UUID changes made by this patch: none

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g28_v1_3/rev/bd9ffe8b889f