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

Box-shadow has wrong corner radius in this testcase #2581

Closed
mstange opened this issue Mar 27, 2018 · 6 comments
Closed

Box-shadow has wrong corner radius in this testcase #2581

mstange opened this issue Mar 27, 2018 · 6 comments
Assignees
Labels

Comments

@mstange
Copy link
Contributor

@mstange mstange commented Mar 27, 2018

It looks correct without a blur, but as soon as you add 1px of blur, things start to go wrong.

http://tests.themasta.com/box-shadow-playground/#blue_red_outset_0_1_265_265_250x250_0x0_0x0_250x250_350_0_1_-80

This is also broken in non-webrender Firefox, but in different and exciting ways.

@kvark kvark added the type: bug label Mar 27, 2018
@pcwalton pcwalton self-assigned this Aug 2, 2018
@pcwalton
Copy link
Collaborator

@pcwalton pcwalton commented Sep 17, 2018

Still broken.

screen shot 2018-09-17 at 1 38 36 pm

pcwalton added a commit to pcwalton/webrender that referenced this issue Sep 18, 2018
`ClipItem::new_box_shadow()`.

If no blur is present, `DisplayListFlattener::add_box_shadow()` calls
`ClipItem::new_rounded_rect()` in its fast path, which calls
`ensure_no_corner_overlap()`. However, in the slow path, which is used when
blur is present, `ClipItem::new_box_shadow()` is called instead. Prior to this
patch, `ClipItem::new_box_shadow() neglected to call
`ensure_no_corner_overlap()`. This is why the bug manifested itself only when
blur was present.

Closes servo#2581.
pcwalton added a commit to pcwalton/webrender that referenced this issue Sep 18, 2018
`ClipItem::new_box_shadow()`.

If no blur is present, `DisplayListFlattener::add_box_shadow()` calls
`ClipItem::new_rounded_rect()` in its fast path, which calls
`ensure_no_corner_overlap()`. However, in the slow path, which is used when
blur is present, `ClipItem::new_box_shadow()` is called instead. Prior to this
patch, `ClipItem::new_box_shadow()` neglected to call
`ensure_no_corner_overlap()`. This is why the bug manifested itself only when
blur was present.

Closes servo#2581.
bors-servo added a commit that referenced this issue Sep 18, 2018
…r=kvark

Call `ensure_no_corner_overlap()` to legalize border radii in `ClipItem::new_box_shadow()`.

If no blur is present, `DisplayListFlattener::add_box_shadow()` calls
`ClipItem::new_rounded_rect()` in its fast path, which calls
`ensure_no_corner_overlap()`. However, in the slow path, which is used when blur is present, `ClipItem::new_box_shadow()` is called instead. Prior to this patch, `ClipItem::new_box_shadow()` neglected to call
`ensure_no_corner_overlap()`. This is why the bug manifested itself only when blur was present.

Closes #2581.

r? @gw3583

<!-- 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/3080)
<!-- Reviewable:end -->
pcwalton added a commit to pcwalton/webrender that referenced this issue Sep 18, 2018
`ClipItem::new_box_shadow()`.

If no blur is present, `DisplayListFlattener::add_box_shadow()` calls
`ClipItem::new_rounded_rect()` in its fast path, which calls
`ensure_no_corner_overlap()`. However, in the slow path, which is used when
blur is present, `ClipItem::new_box_shadow()` is called instead. Prior to this
patch, `ClipItem::new_box_shadow()` neglected to call
`ensure_no_corner_overlap()`. This is why the bug manifested itself only when
blur was present.

Closes servo#2581.
pcwalton added a commit to pcwalton/webrender that referenced this issue Sep 18, 2018
`ClipItem::new_box_shadow()`.

If no blur is present, `DisplayListFlattener::add_box_shadow()` calls
`ClipItem::new_rounded_rect()` in its fast path, which calls
`ensure_no_corner_overlap()`. However, in the slow path, which is used when
blur is present, `ClipItem::new_box_shadow()` is called instead. Prior to this
patch, `ClipItem::new_box_shadow()` neglected to call
`ensure_no_corner_overlap()`. This is why the bug manifested itself only when
blur was present.

Closes servo#2581.
bors-servo added a commit that referenced this issue Sep 19, 2018
…r=gw3583

Call `ensure_no_corner_overlap()` to legalize border radii in `ClipItem::new_box_shadow()`.

If no blur is present, `DisplayListFlattener::add_box_shadow()` calls
`ClipItem::new_rounded_rect()` in its fast path, which calls
`ensure_no_corner_overlap()`. However, in the slow path, which is used when blur is present, `ClipItem::new_box_shadow()` is called instead. Prior to this patch, `ClipItem::new_box_shadow()` neglected to call
`ensure_no_corner_overlap()`. This is why the bug manifested itself only when blur was present.

Closes #2581.

r? @gw3583

<!-- 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/3080)
<!-- Reviewable:end -->
@mstange
Copy link
Contributor Author

@mstange mstange commented Sep 20, 2018

After the fix, this shadow is still a lot smaller than if you give it zero blur.

@pcwalton
Copy link
Collaborator

@pcwalton pcwalton commented Sep 20, 2018

OK, reopening so I can investigate.

@pcwalton pcwalton reopened this Sep 20, 2018
@mstange
Copy link
Contributor Author

@mstange mstange commented Sep 21, 2018

(In the box-shadow-playground tool, you can manipulate the blur radius by dragging the corners of the red boxes in the left half of the screen.)

@mstange
Copy link
Contributor Author

@mstange mstange commented Sep 21, 2018

Oh, maybe the build I'm running doesn't have this fix yet.

@mstange
Copy link
Contributor Author

@mstange mstange commented Sep 21, 2018

I was running a build without the fix. Now I'm running a build which does have the fix, and this bug is indeed fixed. Thanks!

@mstange mstange closed this Sep 21, 2018
fschutt added a commit to fschutt/webrender that referenced this issue Sep 23, 2018
`ClipItem::new_box_shadow()`.

If no blur is present, `DisplayListFlattener::add_box_shadow()` calls
`ClipItem::new_rounded_rect()` in its fast path, which calls
`ensure_no_corner_overlap()`. However, in the slow path, which is used when
blur is present, `ClipItem::new_box_shadow()` is called instead. Prior to this
patch, `ClipItem::new_box_shadow()` neglected to call
`ensure_no_corner_overlap()`. This is why the bug manifested itself only when
blur was present.

Closes servo#2581.
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
3 participants
You can’t perform that action at this time.