Closed Bug 1373802 Opened 7 years ago Closed 7 years ago

Make layout/reftests/async-scrolling/fixed-pos-scrolled-clip-1.html pass on linux64-qr with APZ enabled

Categories

(Core :: Graphics: WebRender, enhancement, P1)

Product:
Core
Component:
Graphics: WebRender
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla58
Tracking Flags:
Tracking Status
firefox56 --- unaffected
firefox57 --- unaffected
firefox58 --- fixed

People

(Reporter: kats, Assigned: kats)

References

Details

(Whiteboard: [wr-mvp] [gfx-noted])

Attachments

(1 file, 3 obsolete files)

Bug 1373802 - Handle more clipping cases.
59 bytes, text/x-review-board-request
mstange
: review+
Details 
With WR+APZ enabled, the layout/reftests/async-scrolling/fixed-pos-scrolled-clip-1.html reftest fails. This bug is for the patches that make it pass, without regressing any "earlier" tests.
I investigated this, and the problem is that at some point we have a (scroll_id, clip_id) of (0, 9) pushed on the stack. From there we want to define a new clip "10" which is a child of "9" and then end up at (scroll_id, clip_id) = (0, 10). Right now the code just goes ahead and defines "10" and pushes it. But that puts us at (10, _) where "10" a child of "0" because of the way the API works [1]. That's totally not what we want.

So instead what we need to do is re-push ClipAndScrollInfo::simple(9) temporarily, then define "10" so that it has "9" as a parent. Then we can pop the 9 that we temporarily pushed, and do a PushClipAndScrollInfo(0, 10). I started doing this but am having trouble with some of the offsets not being correct. I'll finish it up on Monday.

@mrobinson: If you're tweaking the API, this case would be nice to make easier (assuming it makes sense). In order to implement this I need to add a bunch of state-tracking on the caller side of the API which is not really great.

[1] http://searchfox.org/mozilla-central/rev/20d16dadd336e0c6b97e3e19dc4ff907744b5734/gfx/webrender_traits/src/display_list.rs#874
Ok, so I got this working locally. My fix unfortunately doesn't fix any of the other fixed-pos-scrolled-clip-* tests. I started looking at the next one (fixed-pos-scrolled-clip-2.html) and to be honest I'm not really sure how to represent this in WR. Martin, if you get a chance, could you take a look at the test at layout/reftests/async-scrolling/fixed-pos-scrolled-clip-2.html, observe how it behaves in gecko, and let me know if it's possible to represent using the WR API? Even better would be a standalone example app that demonstrates the behaviour in this page. I can start on that tomorrow if you don't get a chance before then.
Flags: needinfo?(mrobinson)
Actually it'll be easier for me to create the example app that demonstrates what the gecko code is currently doing in this case and we can go from there. I'll do that tomorrow.
Flags: needinfo?(mrobinson)
I talked to Markus today and it sounds like the fixed-pos-scrolled-clip-2.html case cannot currently be handled by WebRender. So I might as well land these patches and punt the rest into a follow-up bug for now. The case that's fixed here might help rendering in the wild.
Urgh, looks like it causes a couple of failures now (and an unexpected-pass!). I'll investigate.
I rebased the fix here onto latest m-c and did a try push:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=1086aed99ec650eff0f2afbc104f88a5de164291

The fix makes fixed-pos-scrolled-clip-1.html and fixed-pos-scrolled-clip-4.html pass. It also fixes the reduced test case in bug 1411249, but doesn't seem to fix the original page in that bug. It also doesn't fix bug 1408792.
Attachment #8880874 - Attachment is obsolete: true
Attachment #8880875 - Attachment is obsolete: true
Attachment #8880876 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Priority: P3 → P1
Whiteboard: [gfx-noted] → [wr-mvp] [gfx-noted]
Comment on attachment 8880873 [details]
Bug 1373802 - Handle more clipping cases.

https://reviewboard.mozilla.org/r/152234/#review199098

I'm glad we have so many tests for this stuff. This code is getting out of hand.
Attachment #8880873 - Flags: review?(mstange) → review+
(In reply to Markus Stange [:mstange] from comment #15)
>
> I'm glad we have so many tests for this stuff. This code is getting out of
> hand.

Yeah no kidding. If you have any suggestions on a better approach for dealing with this I'm all ears.
Maybe we should try to make Gecko's structures closer to what we need for webrender. E.g. have explicit connections between clip chains through container display items instead of implicit ones, create clip chain items for masks, create ASRs for sticky items...
If we can pull that off it would certainly help a lot. Would it make things more complicated in the non-WR codepaths (e.g. FrameLayerBuilder)?
Probably, a bit. It would need some serious thought.
Pushed by kgupta@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/d76b2163ba1a
Handle more clipping cases. r=mstange
https://hg.mozilla.org/mozilla-central/rev/d76b2163ba1a
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox58: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: