Closed Bug 1549976 Opened 5 years ago Closed 5 years ago

document splitting: "minecraft.net has control of your mouse pointer" slips behind website content

Categories

(Core :: Graphics: WebRender, defect, P3)

x86_64
Linux
defect

Tracking

()

RESOLVED FIXED
mozilla69
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)

Attached video 2019-05-08_14-45-16.mp4

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/

Priority: -- → P3
Assignee: nobody → dothayer

Having a separate document for this drop down seems like the best solution. Unfortunately it may not be the easiest to implement.

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)

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?

Flags: needinfo?(matt.woodrow)

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.

Flags: needinfo?(matt.woodrow)

Right now I'm having a few difficulties:

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

(In reply to Doug Thayer [:dthayer] from comment #5)

Right now I'm having a few difficulties:

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

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

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

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.

Flags: needinfo?(matt.woodrow)

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?

Flags: needinfo?(matt.woodrow)

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

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

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?

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

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.

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?

Flags: needinfo?(matt.woodrow)
Flags: needinfo?(dothayer)

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.

Flags: needinfo?(dothayer)

Yes, that matches my understanding.

Flags: needinfo?(matt.woodrow)

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.

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.

Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla69
Regressions: 1563178
Flags: qe-verify+

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.

Flags: qe-verify+ → needinfo?(jan)

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.

Flags: needinfo?(jan)

Bugbug thinks this bug is a regression, but please revert this change in case of error.

Keywords: regression
Regressions: 1590057
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: