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

Jump to bottom

Very large box shadows cause rendering bugs #2061

Closed
mstange opened this issue Nov 19, 2017 · 9 comments
Closed

Very large box shadows cause rendering bugs #2061

mstange opened this issue Nov 19, 2017 · 9 comments
Labels
difficulty: easy help wanted

Comments

@mstange
Copy link
Contributor

mstange commented Nov 19, 2017

Testcase: data:text/html,<div style="height:20px;box-shadow:0 0 2000px 2000px rgba(0,0,0,0.4)">

Loading this page in a WebRender Firefox renders a blank tab and makes text in the tab bar disappear. I think unreasonably large box-shadow blurs should be clamped, or the shadow should be rendered without a blur. In this case, the blur achieves nothing, but I ran into a website that did this anyway.
@glennw
Copy link
Member

glennw commented Nov 20, 2017

Wow! Yes, clamping or ignoring the blur sounds reasonable. Do you have a suggested threshold value for blur radius we could use?

@mstange
Copy link
Contributor Author

mstange commented Nov 20, 2017

Gecko seems to allow a maximum stddev of 300: https://searchfox.org/mozilla-central/rev/c633ffa4c4611f202ca11270dcddb7b29edddff8/layout/painting/nsCSSRendering.cpp#4412-4428

@mstange
Copy link
Contributor Author

mstange commented Nov 20, 2017

Oh, I think that's the maximum radius (not stddev), and the variable blurStdDev is misnamed.

@glennw
Copy link
Member

glennw commented Nov 20, 2017

Added help wanted and easy tags - this is an easy task for a new contributor. We would need to clamp the blur radius, and add some wrench reftests for this case.

@glennw
Copy link
Member

glennw commented Nov 20, 2017

We might as well also apply the max spread radius that Gecko uses in the above code.

@mstange
Copy link
Contributor Author

mstange commented Nov 20, 2017

I think the max spread radius is only used when spreading has to be applied per-pixel. For most box-shadows, where the spread radius only affects the dimensions of the shape that's used for the shadow, the spread radius is not clamped.

Gecko uses per-pixel spreading only if a box-shadow is applied on an elements with -moz-appearance (where the shape is unknown and up to the platform). With WebRender, such elements currently fall back to Gecko rendering.

@glennw
Copy link
Member

glennw commented Nov 20, 2017

OK, that makes sense. Thanks!

@ncarrillo
Copy link
Contributor

Would this be more involved than blur_radius = f32::min(blur_radius, MAX_BLUR_RADIUS) inside of add_box_shadow?

@glennw
Copy link
Member

glennw commented Nov 21, 2017

I think that's probably all that's needed, yep!

@ncarrillo ncarrillo mentioned this issue Nov 21, 2017
Merged
bors-servo pushed a commit that referenced this issue Nov 22, 2017
Auto merge of #2076 - nc4rrillo:clamp-blur-radius, r=glennw
fb25304 
Clamp the blur radius

Fixes #2061
Still needs a reftest

<!-- 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/2076)
<!-- Reviewable:end -->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
difficulty: easy help wanted
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@mstange @glennw @ncarrillo