Closed Bug 1147038 Opened 7 years ago Closed 7 years ago

layout/reftests/bugs/593243-1.html fails on desktop with APZ enabled

Categories

(Core :: Panning and Zooming, defect)

All
Gonk (Firefox OS)
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla40
Tracking Status
firefox40 --- fixed

People

(Reporter: kats, Assigned: kats)

References

Details

Attachments

(2 files, 3 obsolete files)

See latest try run with APZ enabled on windows: https://treeherder.mozilla.org/#/jobs?repo=try&revision=90511ba86561

There is a reftest failure for layout/reftests/bugs/593243-1.html, which appears to be related to the reftest-viewport properties.
Removing the reftest-viewport-* properties from the test does in fact make it pass (tested locally on OS X with --setpref layers.async-pan-zoom.enabled=true). However I think removing that would defeat the purpose of the test (which is still valid). We'll need to figure out why the viewport is being used to clip the content and position the scrollbars in this case on desktop. I don't think that should be happening.
It's definitely the clip in ScrollFrameHelper::ComputeFrameMetrics that is causing the extra content not to be drawn. If I comment it out I get the extra content.
The (classic) scrollport is not right thing to be clipping to when the viewport has been overridden. We should either not clip in that case (like we do for containerful scrolling) or (better yet) use the correct clip which should be the composition bounds (in the proper units).
(In reply to Timothy Nikkel (:tn) from comment #2)
> It's definitely the clip in ScrollFrameHelper::ComputeFrameMetrics that is
> causing the extra content not to be drawn. If I comment it out I get the
> extra content.

I'm not able to reproduce this - I commented out the line at http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsGfxScrollFrame.cpp?rev=c309c399dfad#3219 and ran using "mach reftest --setpref layers.async-pan-zoom.enabled=true layout/reftests/bugs/593243-1.html". It fails and the screenshot for the test image just shows a green box in the top-left and white/gray for the rest of it. Are you doing something different?
I'm doing
EXTRA_TEST_ARGS='--setpref=layers.async-pan-zoom.enabled=true' ./mach reftest layout/reftests/bugs/ --filter=593243

My mozilla-central tip is a little bit old though, http://hg.mozilla.org/mozilla-central/rev/5e198fd4017f from ten days ago.
Same result with latest m-c tip.

Note that the reftest still fails even after fixing the clipping issue because the test shows scrollbars and the reference does not.
(In reply to Timothy Nikkel (:tn) from comment #5)
> I'm doing
> EXTRA_TEST_ARGS='--setpref=layers.async-pan-zoom.enabled=true' ./mach
> reftest layout/reftests/bugs/ --filter=593243

Even with this I get the same result as I was getting before :( I'll update to tip and try again, although my tree isn't that out of date.

(In reply to Timothy Nikkel (:tn) from comment #6)
> Note that the reftest still fails even after fixing the clipping issue
> because the test shows scrollbars and the reference does not.

I'm able to make the scrollbars go away by adding a call to windowUtils().setScrollPositionClampingScrollPortSize(800, 1000) at [1]. Conceptually I think that's the right thing to be doing, and we probably want to be using that as the clip instead of the CSS viewport. We'll probably need to add support for reftest-spcsps-w and reftest-spcsps-h properties similar to how reftests can set the css viewport and displayport, and use that feature for these tests.

[1] http://mxr.mozilla.org/mozilla-central/source/layout/tools/reftest/reftest-content.js?rev=77f34f458d02#202
tn told me on IRC that he has a local patch to s/1000/700/ in a couple of places in reftest.js; when I do that I get what he was seeing (stuff drawn outside the scrollbars). Yay progress!
Attached patch Patch (obsolete) — Splinter Review
This makes the test pass locally (with and without APZ) for me. Thoughts?
Attachment #8586195 - Flags: feedback?(tnikkel)
Attachment #8586195 - Flags: feedback?(mstange)
Comment on attachment 8586195 [details] [diff] [review]
Patch

Although we will probably need this reftest feature I'm not sure we need it here. We should be able to compute the proper clip instead of set it. The proper clip should just be the composition bounds, which we can compute. I meant to write a patch for that and test it but I haven't gotten to it yet.
Attached patch fix the clipSplinter Review
Attachment #8590091 - Flags: review?(mstange)
Comment on attachment 8590091 [details] [diff] [review]
fix the clip

I don't yet completely understand in what cases we need to override the viewport and how it all works, so this review is mostly a "looks like it makes sense, you seem to know what you're doing".
Attachment #8590091 - Flags: review?(mstange) → review+
Nice, thanks! That works to fix the clip but I think we still need to set the scrollPositionClampingScrollPortSize explicitly to get rid of the scrollbars.
Attachment #8586195 - Attachment is obsolete: true
Attachment #8586195 - Flags: feedback?(tnikkel)
Attachment #8586195 - Flags: feedback?(mstange)
Attachment #8590362 - Flags: review?(tnikkel)
... although maybe in light of bug 1133905 it's true that the overlay scrollbars are just not being positioned correctly, and if we fix that we won't need to set the scrollPositionClampingScrollPortSize.
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #17)
> ... although maybe in light of bug 1133905 it's true that the overlay
> scrollbars are just not being positioned correctly, and if we fix that we
> won't need to set the scrollPositionClampingScrollPortSize.

Yeah, I came here to post a comment saying about as much. But I think bug 1133905 is a different issue.

Setting the scrollPositionClampingScrollPortSize effectively hides the scrollbars (by making layout think there is no longer anything to scroll as we use the clamping size to determine if scrollbars are visible and their size, but not their position within the page). But if we add content to these reftests to make them scrollable the scrollbars show up in the wrong place again.

My understanding was layout was to put the scrollbars at the layout viewport (set by nsIDOMWindowUtils::SetCSSViewport) and then AZPC moved them to the correct location. So then for desktop platforms with APZC enabled we presumably need to add the code to move the scrollbars that we already have on b2g?
(In reply to Timothy Nikkel (:tn) from comment #18)
> Setting the scrollPositionClampingScrollPortSize effectively hides the
> scrollbars (by making layout think there is no longer anything to scroll as
> we use the clamping size to determine if scrollbars are visible and their
> size, but not their position within the page). But if we add content to
> these reftests to make them scrollable the scrollbars show up in the wrong
> place again.

Ah good point.

> 
> My understanding was layout was to put the scrollbars at the layout viewport
> (set by nsIDOMWindowUtils::SetCSSViewport) and then AZPC moved them to the
> correct location. So then for desktop platforms with APZC enabled we
> presumably need to add the code to move the scrollbars that we already have
> on b2g?

(For those following along, see bug 1133905 comment 15). I think layout does need to change where the scrollbars are positioned. After that fix is made, we may or may not need to set the scrollPositionClampingScrollPortSize here.
(In reply to Markus Stange [:mstange] from comment #14)
> Comment on attachment 8590091 [details] [diff] [review]
> fix the clip
> 
> I don't yet completely understand in what cases we need to override the
> viewport and how it all works, so this review is mostly a "looks like it
> makes sense, you seem to know what you're doing".

Overriding the viewport is something the APZC code does. Normally a document would size itself to the size of it's "container". If the document is a subdocument it will use the size of the iframe. If it's a root document it will use the size of the widget it is in. Overriding the viewport breaks this link, the document is now sized to whatever size the last call to nsIDOMWindowUtils::SetCSSViewport had. Of course this would produce a weird display on screen so we use this in conjunction with setting a resolution on the document. This means that the layout viewport (the size of the root frame and also approximately mScrollPort) is different from the visual viewport (what's visible on screen). The visual viewport is the composition bounds.
This fixed a reftest apparantly, so I marked it as passing.
https://hg.mozilla.org/integration/mozilla-inbound/rev/4c57b7e4a21f
Updated commit message to be a bit more useful.
Attachment #8590362 - Attachment is obsolete: true
Attachment #8590362 - Flags: review?(tnikkel)
Attachment #8591803 - Flags: review?(tnikkel)
No longer depends on: 1133905
Whoops, forgot I meant to leave the tests disabled on fennec, because there things are just different and will be clobbering the CSS viewport/scrollPositionClampingScrollPortSize values set in the tests.
Attachment #8591808 - Flags: review?(tnikkel)
Attachment #8591803 - Attachment is obsolete: true
Attachment #8591803 - Flags: review?(tnikkel)
So far all my attempts to make this test pass on android have been failures. Here's the latest one:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=eb541b5f0015
Comment on attachment 8591808 [details] [diff] [review]
Part 2 - Update tests to set the scrollPositionClampingScrollPortSize

Dropping flag on this for now (should have done this yesterday) per discussion with tn - we should figure out what's happening on android here. I've requested a loaner to get to the bottom of it.
Attachment #8591808 - Flags: review?(tnikkel)
I got a loaner and after some fiddling finally got these tests running. Unfortunately I can't actually *see* the pages load because they load in the background and all I can see is about:home, which is kinda lame. It might also mean that the reftests are running in an environment that is never exercised during normal use, and has all kinds of weird behaviour.
Further investigation and staring at code seems to indicate that the bootstrap script loads the reftest.jsm into the same nsWindow holding Fennec's browser.xul, [1]. The reftest.jsm then rips out everything inside browser.xul's window [2][3] and injects its own xul:browser element which is used to load the reftest pages. It doesn't use Fennec's normal tab-loading code, and depending on how the listeners and triggers in Fennec's browser.js are registered, they may or may not run on the reftest pages. I see from the logs some stuff in browser.js is running but because the environment that they're running in (browser.xul gutted, no deck or tabs) breaks a lot of assumptions they basically do random things. I think this has gotten to the point where I'm just going to file a follow-up bug for it but right now I'm thinking if we run into reftest failures on Android for things related to APZ/viewport/displayport stuff we should just skip them because they are not really representative of what happens in any sane environment.

[1] http://mxr.mozilla.org/mozilla-central/source/layout/tools/reftest/bootstrap.js?rev=e51a54f1817d#28
[2] http://mxr.mozilla.org/mozilla-central/source/layout/tools/reftest/reftest.js?rev=e51a54f1817d#279
[3] http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/browser.xul?rev=cddde1da19ae#8
Comment on attachment 8591808 [details] [diff] [review]
Part 2 - Update tests to set the scrollPositionClampingScrollPortSize

So in light of comment 30 (for which I filed bug 1156817) I think this patch is probably the closest to representative behaviour on mobile devices with APZ enabled, and does get the test passing. I also have a rewrite of the test based on our discussion last week (https://hg.mozilla.org/try/rev/b3967b7e4d15 has a version of it) but I think that the test as-is is probably a better test of what it was meant to test.
Attachment #8591808 - Flags: review?(tnikkel)
Attachment #8591808 - Flags: review?(tnikkel) → review+
https://hg.mozilla.org/mozilla-central/rev/c646f3bfcbd9
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
No longer depends on: 1156817
You need to log in before you can comment on or make changes to this bug.