941050 - Async scrolling of fixed position content is broken

Status: VERIFIED FIXED

(Core :: Layout, defect, P2)

Description

[Chris Lord [:cwiiis]]

It would appear that async scrolling of fixed-position content has broken in the last few days. I would suspect bug 919144, but it wouldn't hurt to get a regression range.

Let's not let this slip into a release. I'm happy to take a look at this, but I have a couple of other things that I want to finish first so I'm leaving it unassigned for now.

Though I've only tested this on Android, I suspect it affects b2g and Metro (with APZC) too.

Comment 1

[Preeti Raghunath(:Preeti)]

adding koi? to make sure we triage this for 1.2

Comment 2

[Matthew Vaughan]

I tested this issue and could not find much of a difference between the 11/20/13 1.2 build versus the 6/22/13 1.2 build in regards to scrolling in the browser on a Buri. The scrolling wasn't smooth about half the time and appeared to have some graphical glitches when scrolling quickly. At this time I am going to say that this issue has been present since the 6/22/13 1.2 build (6/21 build had issues loading websites), and is also present in the 11/20/13 1.1 build. Additionally, I am seeing this same issue occur in the 11/20/13 1.3 build.

- 1.1 BUILD -
Environmental Variables:
Device: Leo v1.1 COM RIL
BuildID: 20131120041201
Gaia: b585b32441fafa67f2b4582db23be5f3a2afab21
Gecko: a9fa9a04390d
Version: 18.0
RIL Version: 01.01.00.019.281

- 1.3 BUILD -
Environmental Variables:
Device: Buri v1.3 COM RIL
BuildID: 20131120040202
Gaia: c26480b22ce28c812c347290dd4bad090d83db6f
Gecko: 4f993fa378eb
Version: 28.0a1
Firmware Version: 20131115
RIL Version: 01.02.00.019.102

Comment 3

[Matthew Vaughan]

Attachment: Video of graphical glitches while quick scrolling on the 11/20/13 1.2 build.

Comment 4

[Jason Smith [:jsmith]]

This issue doesn't apply to 1.2 - it's unaffected for Fx27, only affects Fx28. Moving to 1.3?

Also - can we get better STR here? Looks like we're having trouble getting a regression range here.

Comment 5

[Kevin Brosnan [Ex-Mozilla]]

Likely bug 941091 comment 3 for a regression window.

Comment 6

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

(In reply to Matthew Vaughan from comment #3)
> Created attachment 8335643 [details]
> Video of graphical glitches while quick scrolling on the 11/20/13 1.2 build.

Can you be more specific about what the regression is in this video? I don't see anything wrong in the video other than content sometimes being painted late when it scrolls into view.

Comment 7

[Jason Smith [:jsmith]]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #6)
> (In reply to Matthew Vaughan from comment #3)
> > Created attachment 8335643 [details]
> > Video of graphical glitches while quick scrolling on the 11/20/13 1.2 build.
> 
> Can you be more specific about what the regression is in this video? I don't
> see anything wrong in the video other than content sometimes being painted
> late when it scrolls into view.

I think the video here actually isn't relevant to the bug - the person who tested this didn't reproduce the bug that's involved here based on seeing bug 941091's analysis.

Comment 8

[Matthew Vaughan]

(In reply to Jason Smith [:jsmith] from comment #7)
> (In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #6)
> > (In reply to Matthew Vaughan from comment #3)
> > > Created attachment 8335643 [details]
> > > Video of graphical glitches while quick scrolling on the 11/20/13 1.2 build.
> > 
> > Can you be more specific about what the regression is in this video? I don't
> > see anything wrong in the video other than content sometimes being painted
> > late when it scrolls into view.
> 
> I think the video here actually isn't relevant to the bug - the person who
> tested this didn't reproduce the bug that's involved here based on seeing
> bug 941091's analysis.

That is correct, Jason. I do not believe I reproduced the bug but with the information given in comment 0, I could not determine exactly what issue was being seen by the reporter or how bad it is. What I saw was some issues with the content taking some time to be painted to the screen, which I believe matched bug 941091. I saw that bug 941091 could've possibly been made a dupe of this bug which is why I attached the video to this bug. I can move the video over to 941091 if needed.

Comment 9

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: fixed_pos.mp4

Roc, I'm seeing this on current nightlies and it seems to effect all fixed position content I've encountered. To reproduce, simply scroll twitter.com. Also, see the screen capture.

Comment 10

[Matt Brubeck (:mbrubeck)]

This is also affecting Metro Firefox (bug 941972).

Comment 12

[Jim Mathies [:jimm]]

Attachment: test case

Comment 13

[Brad Lassey [:blassey] (use needinfo?)]

Roc, are you close to a fix? or should we be backing out bug 919144?

Comment 14

[Matt Woodrow (:mattwoodrow)]

We're not setting the fixed position data on the layers we create for the fixed position content because of this check:

http://mxr.mozilla.org/mozilla-central/source/layout/base/FrameLayerBuilder.cpp#1620

It assumes that the fixed position display items are children on the nsDisplayOwnLayer created by the the nsSubDocumentFrame, and that mContainerFrame is the ViewportFrame when we might have fixed position layers.

However, the display items appears to be underneath the nsDisplayScrollLayer, created by the CanvasFrame. In this case mContainerFrame has a parent (the HTMLScroll frame), and we fail this check.

Commenting out this if statement, and the one following it fixes the bug.

Tim, does it make sense for the fixed display items to be children of the nsDisplayScrollLayer? That seems weird to me, but if it does, then these checks are bogus.

Comment 15

[Timothy Nikkel (:tnikkel)]

(In reply to Matt Woodrow (:mattwoodrow) from comment #14)
> Tim, does it make sense for the fixed display items to be children of the
> nsDisplayScrollLayer? That seems weird to me, but if it does, then these
> checks are bogus.

Yeah, it seems very weird to me too. That in of itself could be the cause of this bug. Did bug 919144 have the effect move the fixed position item layers?

Comment 16

[Wesley Johnston (:wesj)]

Attachment: Test for fixed position

This is a super dumb robocop test for this, marked 'todo' for now. I tried adding a test for pinch zoom as well, but it seems to trigger a redraw by Gecko, which fixes things. I left it in, commented out for now (even if this is fixed, it will still fail when waitWithNoPaint sees a paint happen).

Comment 17

[Matt Woodrow (:mattwoodrow)]

Actually it does make sense for the fixed position display items to be children of the scroll frame, since we build these items based on their placeholder frame's position in the frame tree.

So I think these checks are just incorrect, and are assuming we're building display items based on the frame's real position.

Comment 18

[Timothy Nikkel (:tnikkel)]

Yeah, the code is definitely written assuming the fixed items to be children of the root layer and not the scroll layer. But the two checks you mention in comment 14 aren't the only place. Lower in that same function we have
   displayPort += mContainerFrame->GetOffsetToCrossDoc(mContainerReferenceFrame);
which is only correct if displayPort is relative to mContainerFrame, and displayPort is relative to the viewport. So this will be wrong for any fixed items that are contained in container layers other than the root container layer for the document.

Comment 19

[Chris Lord [:cwiiis]]

Comment on attachment 8339484 [details] [diff] [review]
Test for fixed position

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

I think there's just about enough that I don't want to just hand out an r+, so giving an f+ instead.

::: build/mobile/robocop/Assert.java
@@ +22,5 @@
>      // robocop-specific asserts
>      void ispixel(int actual, int r, int g, int b, String name);
>      void isnotpixel(int actual, int r, int g, int b, String name);
> +    void todo_ispixel(int actual, int r, int g, int b, String name);
> +    void todo_isnotpixel(int actual, int r, int g, int b, String name);

I guess there's precedence to not do so, but it'd be great if these methods were documented.

::: mobile/android/base/tests/robocop_fixed_position.html
@@ +5,5 @@
> +    <meta charset="utf-8">
> +  </head>
> +
> +  <body style="margin: 0; padding: 0; min-height: 200%; background-image: linear-gradient(lightgray, lightgray 50%, white 50%, white); background-size: 100px 50px;">
> +    <div style="background-color: rgb(0,255,0); position: fixed; top: 0; left: 0; width: 100%; height: 3em;"></div>

This is good enough, but I think I'd prefer a test where the background of the page is red, the background-colour of the fixed-position div is green and the fixed-position div covered the entire screen. Feels like it's less likely to break due to unrelated changes/oddness, and it's obvious when the test is failing/passing on visual inspection.

Bonus points, add the text 'PASS' in the fixed-pos div and 'FAIL' in the body (obscured by the fixed-pos div, of course).

::: mobile/android/base/tests/testFixedPosition.java
@@ +11,5 @@
> +/**
> + * A basic panning correctness test.
> + * - Loads a page and verifies it draws
> + * - drags page upwards by 100 pixels and verifies it draws
> + * - drags page leftwards by 100 pixels and verifies it draws

This comment needs updating.

@@ +31,5 @@
> +        // set a viewport as if we're mid pan, but don't notify Gecko about the change...
> +        PanZoomTarget target = ((BrowserApp)mActivity).getLayerView().getLayerClient();
> +        ImmutableViewportMetrics metrics = target.getViewportMetrics();
> +        metrics = metrics.setViewportOrigin(metrics.viewportRectLeft, metrics.viewportRectTop + 150);
> +        target.setViewportMetrics(metrics);

Does setting the viewport metrics force a recomposite? I can't remember off-hand, I assume you've checked that it does, but if it doesn't, please check.

@@ +33,5 @@
> +        ImmutableViewportMetrics metrics = target.getViewportMetrics();
> +        metrics = metrics.setViewportOrigin(metrics.viewportRectLeft, metrics.viewportRectTop + 150);
> +        target.setViewportMetrics(metrics);
> +        painted = waitWithNoPaint(mActions.expectPaint());
> +        mAsserter.todo_ispixel(painted.getPixelAt(0, 0), 0, 255, 0, "Scrolled Pixel at 0, 0");

I guess the todo variants warn rather than fail? I guess this is fine, let's just make sure we change this over once the bug is fixed :)

@@ +35,5 @@
> +        target.setViewportMetrics(metrics);
> +        painted = waitWithNoPaint(mActions.expectPaint());
> +        mAsserter.todo_ispixel(painted.getPixelAt(0, 0), 0, 255, 0, "Scrolled Pixel at 0, 0");
> +
> +        /* TODO: This should test pinch zoom, but seems to always force a redraw

Rather than leave this TODO here, let's have two separate tests for this - test_async_fixedpos_panning and test_async_fixedpos_zooming. That way we can test the anchoring code in zooming separately, which is probably more liable to break than the panning code.

Comment 20

[Matt Woodrow (:mattwoodrow)]

Attachment: Fix

Good catch with the display port coordinate space.

Comment 21

[Timothy Nikkel (:tnikkel)]

Comment on attachment 8340152 [details] [diff] [review]
Fix

This looks fine, but I'd like to know if bug 919144 moved our fixed position layers from where we expected them to where we don't expect them (children of scroll layers). If they did then we may be sidestepping the issue here.

Comment 22

[Matt Woodrow (:mattwoodrow)]

It didn't from what I could see.

Comment 23

[Timothy Nikkel (:tnikkel)]

Comment on attachment 8340152 [details] [diff] [review]
Fix

Ok.

This changeset http://hg.mozilla.org/mozilla-central/rev/910b46ec2248 from bug 919144 then also seems unnecessary.

I think it might be better to just get the root frame from the presshell/context instead of walking up the frametree.

Comment 24

[Matt Woodrow (:mattwoodrow)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/040f7055cab9

I think that's true, but I'll wait for roc to get back and read this bug before backing it out.

Comment 25

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

Thanks guys.

(In reply to Timothy Nikkel (:tn) from comment #23)
> I think it might be better to just get the root frame from the
> presshell/context instead of walking up the frametree.

I agree.

(In reply to Timothy Nikkel (:tn) from comment #23)
> This changeset http://hg.mozilla.org/mozilla-central/rev/910b46ec2248 from
> bug 919144 then also seems unnecessary.

Yes, I guess that's true.

Comment 26

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/040f7055cab9

Comment 28

[Jason Smith [:jsmith]]

Already part of 1.3 & a regression - blocking+

Comment 29

[sarsenyev]

The issue reproduces on 1.4
The fixed position content is broken when scrolling

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

Comment 30

[Jason Smith [:jsmith]]

(In reply to sarsenyev from comment #29)
> The issue reproduces on 1.4
> The fixed position content is broken when scrolling
> 
> Device: Buri 1.4 MOZ
> BuildID: 20140219040204
> Gaia: ac06cfbd2baf6494ffbb668cc599e3892cd5e17b
> Gecko: bf0e76f2a7d4
> Version: 30.0a1
> Firmware Version: v1.2-device

Can you file a followup bug?

Comment 31

[sarsenyev]

The follow up bug is created bug 974643

Comment 32

[Roland Kunkel [:RolandK]]

bug 974643 has been resolved as fixed. I am also unable to reproduce this issue on the latest Master Buri build:

Environmental Variables:
4/3 v1.5 Device: Buri 1.5 MOZ RIL
BuildID: 20140403040201
Gaia: 0e974ff33ba47f3d1e59df1e0ad534f1bbe3ef8a
Gecko: 91be2828f17e
Version: 31.0a1
Firmware Version: V1.2-device.cfg

-

1) Verified as fixed against Master, removing verifyme and changing bug status to verified.