Closed Bug 1280013 Opened 5 years ago Closed 5 years ago

Subframe scrolling gets perma-checkerboarding

Categories

(Firefox for Android Graveyard :: Toolbar, defect, P2)

48 Branch
All
Android
defect

Tracking

(firefox48+ wontfix, firefox49+ fixed, firefox50+ fixed)

RESOLVED FIXED
Firefox 50
Tracking Status
firefox48 + wontfix
firefox49 + fixed
firefox50 + fixed

People

(Reporter: kats, Assigned: kats)

References

()

Details

(Keywords: regression)

Attachments

(2 files, 1 obsolete file)

Load something like https://treeherder.mozilla.org/#/jobs?repo=try&revision=f615a14b2c7b&group_state=expanded&selectedJob=22404671 in desktop mode in Fennec. Make sure the error pane down below is scrollable, and scroll it. It can easily get stuck in perma-checkerboarding or perma-blurry. Turning on the minimap makes it more obvious. Affects Nightly, Aurora, Beta.
Priority: -- → P2
Version: 50 Branch → 48 Branch
I chased this down a bit. It looks like the displayport base rect for the subframe is empty, so the area of the subframe that gets painted is smaller than what we want. The empty base rect seems to be coming from a bad rootCompositionBounds at [1]. Still digging..

[1] http://searchfox.org/mozilla-central/rev/970569ad57ac4436ff31aa2ac63f38ed3ee2932d/layout/generic/nsGfxScrollFrame.cpp#3539
This appears to be same problem as bug 1259593.
Depends on: 1259593
Actually it's not exactly the same. This manifests even in the steady state and as far as I can tell, doesn't have anything to do with intersecting temporally-different things.
No longer depends on: 1259593
See Also: → 1259593
So the problem here is that the main-thread scroll position on this page never leaves 0,0. Instead the "scrolling" effect of the RCD after zooming in is handled by APZ and the callback transform.

The problem is that the TransformRect call at [1] doesn't know about the scroll position of the RCD, and so translates the root composition bounds as though the top-level page were scrolled at 0,0. If I add the callback transform to the position of the root composition bounds that fixes the problem.

[1] http://searchfox.org/mozilla-central/rev/970569ad57ac4436ff31aa2ac63f38ed3ee2932d/layout/generic/nsGfxScrollFrame.cpp#3537
Attached patch WIP (obsolete) — Splinter Review
This is my fix, although for correctness we might need to walk the entire ancestor chain from the current scrollframe to the root scrollframe and apply all the callback transforms we find, much like we do in APZCCallbackHelper::ApplyCallbackTransform.
Reduced test case (you need a device with a tall-ish screen to see it). I'll see if I can turn it into an automated test.
whoops, that try push had a bad refactoring in it, I cancelled it and started a new one with better patches:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=fa57c723dc84
Attachment #8763675 - Attachment is obsolete: true
[Tracking Requested - why for this release]:

Pretty sure we need this for 48, getting stuck with the blurry or blank screen won't do.
Comment on attachment 8764211 [details]
Bug 1280013 - Incorporate the APZ callback transforms when using the root composition bounds to clip the displayport base.

https://reviewboard.mozilla.org/r/60218/#review57502

::: gfx/layers/apz/util/APZCCallbackHelper.cpp:413
(Diff revision 1)
>      float nonRootResolution = 1.0f;
>      if (nsIPresShell* shell = GetRootContentDocumentPresShellForContent(content)) {
>        nonRootResolution = shell->GetCumulativeNonRootScaleResolution();
>      }
> -    // Now apply the callback-transform.
> -    // XXX: Walk up the frame tree from the frame of this content element
> +    // Now apply the callback-transform. This is only approximately correct,
> +    // see the comment on GetCumulativeApzCallbackTransform for details.

Could you also revise the comment above to not mention "deltas" in the plural?

::: layout/base/nsLayoutUtils.cpp:9351
(Diff revision 1)
> +      void* property = content->GetProperty(nsGkAtoms::apzCallbackTransform);
> +      if (property) {
> +        delta += *static_cast<CSSPoint*>(property);
> +      }
> +    }
> +    frame = GetCrossDocParentFrame(frame);

Note that in the original code in ApplyCallbackTransform(), we weren't going cross-doc. I'll leave it up to Timothy to say whether this aspect of the change is correct.
Attachment #8764211 - Flags: review?(botond) → review+
https://reviewboard.mozilla.org/r/60220/#review57504

::: gfx/layers/apz/test/mochitest/helper_bug1280013.html:3
(Diff revision 1)
> +<!DOCTYPE HTML>
> +<html>
> +<head style="overflow:hidden">

What does it mean for <head> to have style "overflow:hidden"?

::: gfx/layers/apz/test/mochitest/helper_bug1280013.html:33
(Diff revision 1)
> +  // scroll position (it is stored in the callback transform instead). We check
> +  // this by checking the scroll offset.
> +  yield flushApzRepaints(testDriver);
> +  SimpleTest.is(window.scrollY, 0, "Main-thread scroll position is still at 0");
> +
> +  // Scroll the iframe by 100px. Note that since the main-thread scroll position

Aren't we scrolling it by 300px?

::: gfx/layers/apz/test/mochitest/helper_bug1280013.html:68
(Diff revision 1)
> +.then(runContinuation(test))
> +.then(subtestDone);
> +
> +  </script>
> +</head>
> +<body style="overflow:hidden">

If the body is oveflow:hidden, and the viewport has initial-scale:1 (so we're not zoomed in), how can touch-drags scroll the root scroll frame?
(In reply to Botond Ballo [:botond] (C++ standards meeting Jun 20-25) from comment #12)
> Could you also revise the comment above to not mention "deltas" in the
> plural?

Sure

> ::: layout/base/nsLayoutUtils.cpp:9351
> Note that in the original code in ApplyCallbackTransform(), we weren't going
> cross-doc. I'll leave it up to Timothy to say whether this aspect of the
> change is correct.

Yeah, I think the original code was wrong - we should have been going cross-doc. The intent of the code is to walk up the chain of elements corresponding to the layer tree, and apply all those callback transforms. The layer tree is cross-doc, and so this code should be as well.

(In reply to Botond Ballo [:botond] (C++ standards meeting Jun 20-25) from comment #13)
> > +<html>
> > +<head style="overflow:hidden">
> 
> What does it mean for <head> to have style "overflow:hidden"?

Er, whoops. I meant to put that on the <html>

> ::: gfx/layers/apz/test/mochitest/helper_bug1280013.html:33
> > +
> > +  // Scroll the iframe by 100px. Note that since the main-thread scroll position
> 
> Aren't we scrolling it by 300px?

Yeah, sorry. My initial version of the test only scrolled by 100px

> ::: gfx/layers/apz/test/mochitest/helper_bug1280013.html:68
> > +<body style="overflow:hidden">
> 
> If the body is oveflow:hidden, and the viewport has initial-scale:1 (so
> we're not zoomed in), how can touch-drags scroll the root scroll frame?

The viewport width is 980px, so initial-scale:1 actually is "zoomed in", because the default scale is something smaller (whatever zoom it takes to fit 980px). I can change that initial-scale to 1.1 if that makes it more clear; I just used 1 because then I didn't have to worry about any other pixel conversion scales later on.
Comment on attachment 8764211 [details]
Bug 1280013 - Incorporate the APZ callback transforms when using the root composition bounds to clip the displayport base.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/60218/diff/1-2/
^ Patches updated with review comments addressed. I re-verified that the test fails without the patch and passes with the patch (i.e. the misplaced overflow:hidden didn't have any effect).
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #14)
> > ::: gfx/layers/apz/test/mochitest/helper_bug1280013.html:68
> > > +<body style="overflow:hidden">
> > 
> > If the body is oveflow:hidden, and the viewport has initial-scale:1 (so
> > we're not zoomed in), how can touch-drags scroll the root scroll frame?
> 
> The viewport width is 980px, so initial-scale:1 actually is "zoomed in",
> because the default scale is something smaller (whatever zoom it takes to
> fit 980px). I can change that initial-scale to 1.1 if that makes it more
> clear; I just used 1 because then I didn't have to worry about any other
> pixel conversion scales later on.

Maybe just mention in a comment near the meta viewport tag that this will result in us being zoomed in assuming the phone's screen width is < 980 px?
Comment on attachment 8764211 [details]
Bug 1280013 - Incorporate the APZ callback transforms when using the root composition bounds to clip the displayport base.

https://reviewboard.mozilla.org/r/60218/#review57974
Attachment #8764211 - Flags: review?(tnikkel) → review+
Pushed by kgupta@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/09e4d7a5bcbb
Incorporate the APZ callback transforms when using the root composition bounds to clip the displayport base. r=botond,tnikkel
https://hg.mozilla.org/integration/mozilla-inbound/rev/eb477f283276
Add a test. r=botond
https://hg.mozilla.org/mozilla-central/rev/09e4d7a5bcbb
https://hg.mozilla.org/mozilla-central/rev/eb477f283276
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 50
Comment on attachment 8764211 [details]
Bug 1280013 - Incorporate the APZ callback transforms when using the root composition bounds to clip the displayport base.

Approval Request Comment
[Feature/regressing bug #]: APZ
[User impact if declined]: On a small set of pages (desktop-mode pages with overflow:hidden) it's possible to get into a state where content is checkerboarded or low-res and doesn't repaint
[Describe test coverage new/current, TreeHerder]: added an automated test for it, also verified locally
[Risks and why]: medium risk - although the patch is small and intended for Fennec, it touches a codepath shared with desktop and might have unintended consequences. I would like to uplift to 48 beta as well but I think it needs more bake time before that can happen. Would like to uplift to 49 aurora to get more testing coverage in the meantime.
[String/UUID change made/needed]: none
Attachment #8764211 - Flags: approval-mozilla-aurora?
Attachment #8764212 - Flags: approval-mozilla-aurora?
Comment on attachment 8764211 [details]
Bug 1280013 - Incorporate the APZ callback transforms when using the root composition bounds to clip the displayport base.

Checkerboarding issue on android, let's uplift to aurora at least.
Attachment #8764211 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment on attachment 8764212 [details]
Bug 1280013 - Add a test.

New tests, please uplift.
Attachment #8764212 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
has problems to apply to aurora
Flags: needinfo?(bugmail)
Track this since this also affects 48 beta.
Hi  Kartikaya, 
If we let this ride the train on 49/50 only and won't fix in 48, will this be a big impact for user for 48 because the patch is medium risk?
Flags: needinfo?(bugmail)
(In reply to Gerry Chang [:gchang] from comment #29)
> Hi  Kartikaya, 
> If we let this ride the train on 49/50 only and won't fix in 48, will this
> be a big impact for user for 48 because the patch is medium risk?

It's hard to say. I'm not sure how frequently users will run into this bug, because you need to be zoomed in on a page that is overflow:hidden in order to really run into it.

I think for now maybe we can just let it ride the trains. If we encounter a high-visibility site that is affected by this then we can reconsider the uplift to beta. I am still concerned that it might introduce regressions either on desktop or on Fennec, and would really like the extra bake time.
Flags: needinfo?(bugmail)
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.