Closed
Bug 1147038
Opened 10 years ago
Closed 10 years ago
layout/reftests/bugs/593243-1.html fails on desktop with APZ enabled
Categories
(Core :: Panning and Zooming, defect)
Tracking
()
RESOLVED
FIXED
mozilla40
Tracking | Status | |
---|---|---|
firefox40 | --- | fixed |
People
(Reporter: kats, Assigned: kats)
References
Details
Attachments
(2 files, 3 obsolete files)
2.44 KB,
patch
|
mstange
:
review+
kats
:
checkin+
|
Details | Diff | Splinter Review |
5.71 KB,
patch
|
tnikkel
:
review+
kats
:
checkin+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Updated•10 years ago
|
Blocks: apz-desktop
Depends on: 1146626
Assignee | ||
Comment 1•10 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.
Comment 2•10 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.
Comment 3•10 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).
Assignee | ||
Comment 4•10 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?
Comment 5•10 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.
Comment 6•10 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.
Assignee | ||
Comment 7•10 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
Assignee | ||
Comment 8•10 years ago
|
||
Still same thing with tip. This is my output: http://people.mozilla.org/~kgupta/bug/1147038-log.txt
Assignee | ||
Comment 9•10 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!
Assignee | ||
Comment 10•10 years ago
|
||
This makes the test pass locally (with and without APZ) for me. Thoughts?
Attachment #8586195 -
Flags: feedback?(tnikkel)
Attachment #8586195 -
Flags: feedback?(mstange)
Assignee | ||
Comment 11•10 years ago
|
||
Try run at https://treeherder.mozilla.org/#/jobs?repo=try&revision=1c8fc90cb2ea includes this patch and is promisingly green.
Comment 12•10 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.
Comment 13•10 years ago
|
||
Attachment #8590091 -
Flags: review?(mstange)
Comment 14•10 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".
Attachment #8590091 -
Flags: review?(mstange) → review+
Assignee | ||
Comment 15•10 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.
Attachment #8586195 -
Attachment is obsolete: true
Attachment #8586195 -
Flags: feedback?(tnikkel)
Attachment #8586195 -
Flags: feedback?(mstange)
Attachment #8590362 -
Flags: review?(tnikkel)
Assignee | ||
Comment 16•10 years ago
|
||
android-failure try |
Try push with the two patches: https://treeherder.mozilla.org/#/jobs?repo=try&revision=3c9a6ded3231
Assignee | ||
Comment 17•10 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.
Comment 18•10 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?
Assignee | ||
Comment 19•10 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.
Comment 20•10 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.
Comment 21•10 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
Assignee | ||
Updated•10 years ago
|
Keywords: leave-open
Comment 22•10 years ago
|
||
This fixed a reftest apparantly, so I marked it as passing.
https://hg.mozilla.org/integration/mozilla-inbound/rev/4c57b7e4a21f
Comment 23•10 years ago
|
||
Assignee | ||
Comment 24•10 years ago
|
||
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)
Assignee | ||
Comment 25•10 years ago
|
||
Updated try push with the above patch:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=72fdf07882d9
Assignee | ||
Comment 26•10 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.
Attachment #8591808 -
Flags: review?(tnikkel)
Assignee | ||
Updated•10 years ago
|
Attachment #8591803 -
Attachment is obsolete: true
Attachment #8591803 -
Flags: review?(tnikkel)
Assignee | ||
Comment 27•10 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
Assignee | ||
Comment 28•10 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.
Attachment #8591808 -
Flags: review?(tnikkel)
Assignee | ||
Comment 29•10 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.
Assignee | ||
Comment 30•10 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
Assignee | ||
Updated•10 years ago
|
Attachment #8590091 -
Flags: checkin+
Assignee | ||
Comment 31•10 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.
Attachment #8591808 -
Flags: review?(tnikkel)
Updated•10 years ago
|
Attachment #8591808 -
Flags: review?(tnikkel) → review+
Comment 32•10 years ago
|
||
Assignee | ||
Updated•10 years ago
|
Attachment #8591808 -
Flags: checkin+
Assignee | ||
Updated•10 years ago
|
Keywords: leave-open
Status: NEW → RESOLVED
Closed: 10 years ago
status-firefox40:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
You need to log in
before you can comment on or make changes to this bug.
Description
•