Kartikaya Gupta (email:kats@mozilla.staktrace.com) Assignee
	Description • 9 years ago

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.

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Assignee
	Comment 1 • 9 years ago

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.

	Timothy Nikkel (:tnikkel)
	Comment 2 • 9 years ago

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.

	Timothy Nikkel (:tnikkel)
	Comment 3 • 9 years ago

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).

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Assignee
	Comment 4 • 9 years ago

(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?

	Timothy Nikkel (:tnikkel)
	Comment 5 • 9 years ago

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.

	Timothy Nikkel (:tnikkel)
	Comment 6 • 9 years 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.

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Assignee
	Comment 7 • 9 years ago

(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

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Assignee
	Comment 8 • 9 years ago

Still same thing with tip. This is my output: http://people.mozilla.org/~kgupta/bug/1147038-log.txt

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Assignee
	Comment 9 • 9 years ago

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!

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Assignee
	Comment 10 • 9 years ago

This makes the test pass locally (with and without APZ) for me. Thoughts?

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Assignee
	Comment 11 • 9 years ago

Try run at https://treeherder.mozilla.org/#/jobs?repo=try&revision=1c8fc90cb2ea includes this patch and is promisingly green.

	Timothy Nikkel (:tnikkel)
	Comment 12 • 9 years ago

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.

	Markus Stange [:mstange]
	Comment 14 • 9 years ago

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".

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Assignee
	Comment 15 • 9 years ago

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.

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Assignee
	Comment 16 • 9 years ago
android-failure try

Try push with the two patches: https://treeherder.mozilla.org/#/jobs?repo=try&revision=3c9a6ded3231

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Assignee
	Comment 17 • 9 years ago

... 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.

	Timothy Nikkel (:tnikkel)
	Comment 18 • 9 years ago

(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?

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Assignee
	Comment 19 • 9 years ago

(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.

	Timothy Nikkel (:tnikkel)
	Comment 20 • 9 years ago

(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.

	Timothy Nikkel (:tnikkel)
	Comment 21 • 9 years ago

Pushed my patch
https://hg.mozilla.org/integration/mozilla-inbound/rev/c937debbffc4


green try server run on win32
https://treeherder.mozilla.org/#/jobs?repo=try&revision=7e5b85259327

	Timothy Nikkel (:tnikkel)
	Comment 22 • 9 years ago

This fixed a reftest apparantly, so I marked it as passing.
https://hg.mozilla.org/integration/mozilla-inbound/rev/4c57b7e4a21f

	Carsten Book [:Tomcat]
	Comment 23 • 9 years ago

https://hg.mozilla.org/mozilla-central/rev/c937debbffc4
https://hg.mozilla.org/mozilla-central/rev/4c57b7e4a21f

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Assignee
	Comment 24 • 9 years ago

Updated commit message to be a bit more useful.

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Assignee
	Comment 25 • 9 years ago

Updated try push with the above patch:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=72fdf07882d9

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Assignee
	Comment 26 • 9 years ago

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.

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Assignee
	Comment 27 • 9 years ago

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

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Assignee
	Comment 28 • 9 years ago

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.

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Assignee
	Comment 29 • 9 years ago

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.

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Assignee
	Comment 30 • 9 years ago

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

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Assignee
	Comment 31 • 9 years ago

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.

	Pulsebot
	Comment 32 • 9 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/c646f3bfcbd9

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 33 • 9 years ago

https://hg.mozilla.org/mozilla-central/rev/c646f3bfcbd9