Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Panic when a stacking context has an invalid transform. #418

Closed
glennw opened this issue Sep 23, 2016 · 5 comments
Closed

Panic when a stacking context has an invalid transform. #418

glennw opened this issue Sep 23, 2016 · 5 comments
Assignees

Comments

@glennw
Copy link
Member

@glennw glennw commented Sep 23, 2016

If a transform matrix cannot be inverted, the webrender code will currently panic. Instead, it should detect this and not try to draw the stacking context.

@j3parker
Copy link

@j3parker j3parker commented Nov 3, 2016

I can grab this. According to https://drafts.csswg.org/css-transforms/#transform-function-lists

If a transform function causes the current transformation matrix (CTM) of an object to be non-invertible, the object and its content do not get displayed.

Would a reasonable implementation in the context of servo mean that the object doesn't show up in the display lists (similar to if it had display: none)? Would that be done in layout?

@glennw
Copy link
Member Author

@glennw glennw commented Nov 4, 2016

It probably can be done in layout, however I think it's probably simpler to handle in WR. If we check in WR, it will also handle this case for any clients that aren't Servo.

This piece of code (https://github.com/servo/webrender/blob/master/webrender/src/tiling.rs#L1822) concatenates the full stacking context transform, and also marks stacking contexts as visible or not. I suspect all you need to do is check if the transform is invertible here, and if not, bail out of the function and mark the stacking context as not visible.

@j3parker
Copy link

@j3parker j3parker commented Nov 4, 2016

Okay! I agree that putting the change in WR would be useful for other clients.

I originally asked because I had made the change you described and noticed that Servo still crashed because of hit-box stuff (i.e. that part of the DOM isn't rendered, but if you mouse in to the window then it panics.) I now think the correct solution is to do the check in multiple places. I'll PR the WR fix, open a related Servo issue this weekend.

@canova canova added the C-assigned label Jan 31, 2017
@canova canova self-assigned this Jan 31, 2017
@jdm
Copy link
Member

@jdm jdm commented Jan 31, 2017

This sounds like servo/servo#13266 and servo/servo#14806 too.

@notriddle
Copy link

@notriddle notriddle commented Jan 31, 2017

It probably can be done in layout, however I think it's probably simpler to handle in WR. If we check in WR, it will also handle this case for any clients that aren't Servo.

Do non-Servo clients want that behavior?

bors-servo added a commit that referenced this issue Feb 2, 2017
Fix the panic when transform is non-invertible

Fix the panic when transform is non-invertible. It also marks the `combined_local_viewport_rect` as not visible.
Fixes #418.

r? @glennw

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/webrender/823)
<!-- Reviewable:end -->
@glennw glennw closed this in #823 Feb 2, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
5 participants
You can’t perform that action at this time.