document splitting: "minecraft.net has control of your mouse pointer" slips behind website content
Categories
(Core :: Graphics: WebRender, defect, P3)
Tracking
()
Tracking | Status | |
---|---|---|
firefox-esr60 | --- | unaffected |
firefox66 | --- | unaffected |
firefox67 | --- | unaffected |
firefox68 | --- | disabled |
firefox69 | --- | fixed |
People
(Reporter: jan, Assigned: alexical)
References
(Regressed 1 open bug)
Details
(Keywords: correctness, nightly-community, regression)
Attachments
(2 files)
Debian Testing, KDE, X11, Macbook Pro
mozregression --launch 2019-05-07 --pref gfx.webrender.all:true gfx.webrender.split-render-roots:true -a https://classic.minecraft.net/
Updated•5 years ago
|
Updated•5 years ago
|
Comment 1•5 years ago
|
||
Having a separate document for this drop down seems like the best solution. Unfortunately it may not be the easiest to implement.
Comment 2•5 years ago
|
||
My guess would be that something similar happens for the devtools inspector highlighter when inspecting the browser window chrome document. (Web Developer -> Browser Toolbox, hover over elements in the inspector)
Assignee | ||
Comment 3•5 years ago
|
||
This is actually turning out simpler than I thought so far. However, I'm a bit stumped on how to account for this transform when computing the render root's rect. I suspect that this is kind of a sticky problem, and we may need to come at it with another approach. Matt, do you have any thoughts?
Comment 4•5 years ago
|
||
As mentioned on IRC, the coordinates will be in the space of their reference frame, which is the nearest transformed ancestor.
nsLayoutUtils::TransformFrameRectToAncestor is probably the easiest way to convert into root frame coordinates.
Assignee | ||
Comment 5•5 years ago
|
||
Right now I'm having a few difficulties:
- The warning has a box shadow, which of course extends past the element's bounds. This means that if we use the element's bounds for our call to SetDocumentView, we end up scissoring out the box shadow.
- There's some complicated bits around what happens if we have no nsDisplayRenderRoot for a given renderroot (in our case the new "Popover" renderroot). If we just don't send any updates, then the most recent frame stays valid, so the popover hangs around. I played around with deleting the document / removing it from Renderer::active_documents, but it's all really finicky.
The simplest thing I tried is just wrapping the popover elements in a div that covers the whole window. Then the popover document is always valid and we just treat it the same as content. I'm trying to work out if this is problematic from a performance perspective though. If anyone has any intuitions on how expensive it might be to keep a visually inert document around indefinitely, feel free to chime in. I don't think it should be in theory, but of course it might have problematic bits due to our document implementation.
Comment 6•5 years ago
|
||
(In reply to Doug Thayer [:dthayer] from comment #5)
Right now I'm having a few difficulties:
- The warning has a box shadow, which of course extends past the element's bounds. This means that if we use the element's bounds for our call to SetDocumentView, we end up scissoring out the box shadow.
Use GetVisualOverflowRectRelativeToSelf instead of GetRectRelativeToSelf to include all pixels drawn by that frame and it's descendants.
- There's some complicated bits around what happens if we have no nsDisplayRenderRoot for a given renderroot (in our case the new "Popover" renderroot). If we just don't send any updates, then the most recent frame stays valid, so the popover hangs around. I played around with deleting the document / removing it from Renderer::active_documents, but it's all really finicky.
Can you keep a list of active render roots, and update it each frame? Noticing when you don't have any updates for one, and sending a notification that it's now gone?
Assignee | ||
Comment 7•5 years ago
|
||
(In reply to Matt Woodrow (:mattwoodrow) from comment #6)
Use GetVisualOverflowRectRelativeToSelf instead of GetRectRelativeToSelf to include all pixels drawn by that frame and it's descendants.
Ah - thank you!
Can you keep a list of active render roots, and update it each frame? Noticing when you don't have any updates for one, and sending a notification that it's now gone?
I mean, that's effectively what we're doing, yes. I'm able to send something up to WebRenderBridgeParent when the nsDisplayRenderRoot for the popover no longer exists. So far the tricky bits have all been in RenderThread.cpp / render_backend.rs / renderer.rs. But this is all very hand-wavey - I just noticed that all of this complexity (potentially just perceived complexity, but it seems not) melts away if I just use a wrapping div that covers the whole window and doesn't go away, but I wasn't sure about the performance implications of doing so.
Assignee | ||
Comment 8•5 years ago
|
||
Sorry to keep bothering you Matt, but since you've had most of the tools I need so far, do you have anything that could help we with the fact that the transform for this thing is animated? I suspect not, since the animation should be happening compositor-side, so I would have to handle transform animations specially and call SetDocumentView on them if I know we're animating the root of the document. Unfortunately I don't see a way of doing that without adding significant complexity.
Comment 9•5 years ago
|
||
Hmm, how do we handle the case where a render root like this has a transform as an ancestor? What renders that transformation? What if the transform has other children that aren't in the RR?
Anyway, the renderroot could have both ancestor and descendant transforms that could basically change arbitrarily at any time, so any rect that we compute on the client side isn't going to be useful.
I think there are two possible approaches.
Rather than looking at the current bounds, we could consider the potential bounds, which is the area that we're clipped to. That's probably going to be the entire top-level window area in most cases.
I'm not sure how big the downsides are of that are. Are we going to expose those renderroot to the OS compositor? Does that mean the overlay renderroot is going to be a giant transparent surface that needs a lot of bandwidth to composite, even though only a small area of opaque pixels have content?
The other alternative would be to get WebRender to figure this out internally. It seems like we should know the resulting bounds when build the actual frame ready for rasterization, can we just use that internally, rather than require c++ code to provide it upfront?
Comment 10•5 years ago
|
||
(In reply to Matt Woodrow (:mattwoodrow) from comment #9)
Hmm, how do we handle the case where a render root like this has a transform as an ancestor? What renders that transformation? What if the transform has other children that aren't in the RR?
We don't. That this scenario would not occur was one of the assumptions we had going into this. I'll forward you an email thread.
Comment 11•5 years ago
|
||
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #10)
(In reply to Matt Woodrow (:mattwoodrow) from comment #9)
Hmm, how do we handle the case where a render root like this has a transform as an ancestor? What renders that transformation? What if the transform has other children that aren't in the RR?
We don't. That this scenario would not occur was one of the assumptions we had going into this. I'll forward you an email thread.
Thanks!
So, the original plan was that we'd just have two render roots, for areas that don't overlap in any way? And now we're extending that to have a 3rd (at least) that overlays the other two?
I guess restructuring the front end content to have a wrapper div with the render root attribute on it, so that the overlay render root is always outside any potential transforms etc works from a rendering perspective.
I still think getting WebRender to compute the bounds of this internally (during frame building, once all async animations within the overlay RR have been resolved for the frame) is probably a good solution for performance.
This all feels very fragile though, since it seems super easy for otherwise valid frontend changes to end up being something WebRender can't draw (without adding an outer wrapping div and renderroot annotation, which feels like arcane knowledge). Are we sure there aren't other types of overlays that are also affected? Seems hard to audit for.
Assignee | ||
Comment 12•5 years ago
|
||
I don't think at present there is a performance cost for having large bounds on this. Documents do not at present get their own surfaces, however the eventual goal I've always had in mind was for them to eventually have their own surfaces separately passed to the OS compositor. So yes, in that case we would certainly want a solution which constrained the bounds either during frame building or not.
It is a bit arcane and fragile - I'm not sure there's a workable solution to that. I'm certainly open to one. We could for every display item determine if it intersects both the chrome and content render roots, and if so throw it into a third renderroot. I'm not sure exactly where that would fit in, and it seems like it might be a tiny bit costly. Other thoughts?
Comment 13•5 years ago
|
||
(In reply to Doug Thayer [:dthayer] from comment #12)
It is a bit arcane and fragile - I'm not sure there's a workable solution to that. I'm certainly open to one. We could for every display item determine if it intersects both the chrome and content render roots, and if so throw it into a third renderroot. I'm not sure exactly where that would fit in, and it seems like it might be a tiny bit costly. Other thoughts?
That seems plausible, but wouldn't we need to include ancestors (and with those, siblings), such that the renderroot never has any sort of effect (transform, opacity etc) outside of it? What if that then includes display items behind the content renderoot (or the content renderroot itself)?
How much power do addons have to affect this? Could an addon add a new overlay, especially one hits the hard case above?
Imposing these restrictions just on our frontend teams seems somewhat feasible since we can add messages directing them to the WR team if they hit the problem case. It's probably a lot harder for addon authors to figure this out though.
Comment 14•5 years ago
|
||
So I agree that we can probably automatically detect and fixup some cases here, but it's not super easy, and it would still be possibly (easy?) to create cases where we just can't handle it.
I think the next step is to make a call about whether we think that is something we can reasonably ship.
If so, then we should look at adding ways to detect and communicate the problem to authors in a useful way.
If not, then we probably need to go back to the drawing board for how document splitting works.
Comment 15•5 years ago
|
||
I think the next step is to make a call about whether we think that is something we can reasonably ship.
Just following up on this. Matt, Doug - what are the next steps for making this decision?
Assignee | ||
Comment 16•5 years ago
|
||
Paraphrasing an IRC conversation between Matt and Jeff (please correct this if it's wrong, Matt) - I believe the decision was, yes this is something we can reasonably ship, but we should continue evaluating (document splitting in general) to not paint ourselves into a corner. And we should certainly have a conversation in Whistler about it.
Assignee | ||
Comment 18•5 years ago
|
||
If we decide to just go with an overlay that sits fully over the
window (which I don't personally see a perf problem with right now,
but correct me if you can think of one), then this should be all
we need.
Comment 19•5 years ago
|
||
I think the patch Doug posted is a good solution to the problem as we have it now. When we get around to the "each document gets its own surface passed to the OS compositor" we may need to revisit the perf aspect of this and try to constrain the bounds of the popover document. Probably worth filing a bug on that and making it block bug 1479786.
Comment 20•5 years ago
|
||
Pushed by gijskruitbosch@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/5a18f5c929ab Implement popover render root r=kats,Gijs
Comment 21•5 years ago
|
||
bugherder |
Updated•5 years ago
|
Comment 22•5 years ago
|
||
Hello,
I tried to reproduce this issue on our hardware but unfortunately I could not reproduce it. Jan Andre Ikenmeyer would you be so kind to see if you can reproduce the issue as we currently don't have a machine with Debian on it that also has the specs to support webrender.
Comment 23•5 years ago
|
||
You shouldn't bother doing any QA on "document splitting" bugs, it's a feature that's off by default, and so unstable that it can't yet even be dogfooded.
Comment 24•5 years ago
|
||
Bugbug thinks this bug is a regression, but please revert this change in case of error.
Description
•