Closed Bug 941050 Opened 6 years ago Closed 6 years ago

Async scrolling of fixed position content is broken

Categories

(Core :: Layout, defect, P2)

defect

Tracking

()

VERIFIED FIXED
mozilla28
blocking-b2g 1.3+
Tracking Status
firefox27 --- unaffected
firefox28 + fixed
fennec 28+ ---

People

(Reporter: cwiiis, Assigned: roc)

References

Details

(Keywords: regression, reproducible, Whiteboard: [block28])

Attachments

(4 files, 1 obsolete file)

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.
adding koi? to make sure we triage this for 1.2
blocking-b2g: --- → koi?
QA Contact: mvaughan
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
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.
blocking-b2g: koi? → 1.3?
Likely bug 941091 comment 3 for a regression window.
Keywords: regression
Priority: -- → P2
Assignee: nobody → roc
tracking-fennec: ? → 28+
(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.
(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.
(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.
Attached video 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.
Attachment #8335643 - Attachment is obsolete: true
Blocks: 941972
This is also affecting Metro Firefox (bug 941972).
Whiteboard: [block28]
Duplicate of this bug: 941980
Blocks: 943244
Keywords: reproducible
Attached file test case
No longer blocks: 943244
Roc, are you close to a fix? or should we be backing out bug 919144?
Blocks: 919144
Flags: needinfo?(roc)
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.
Flags: needinfo?(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?
Flags: needinfo?(tnikkel)
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).
Attachment #8339484 - Flags: review?(chrislord.net)
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.
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 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.
Attachment #8339484 - Flags: review?(chrislord.net) → feedback+
Attached patch FixSplinter Review
Good catch with the display port coordinate space.
Attachment #8340152 - Flags: review?(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.
It didn't from what I could see.
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.
Attachment #8340152 - Flags: review?(tnikkel) → review+
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.
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.
Flags: needinfo?(roc)
https://hg.mozilla.org/mozilla-central/rev/040f7055cab9
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla28
Duplicate of this bug: 941972
Depends on: 945634
Flags: in-testsuite?
Flags: in-qa-testsuite?
Already part of 1.3 & a regression - blocking+
blocking-b2g: 1.3? → 1.3+
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
(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?
The follow up bug is created bug 974643
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.
Status: RESOLVED → VERIFIED
Keywords: verifyme
You need to log in before you can comment on or make changes to this bug.