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

Implement correct corner clipping for background color #19675

Merged
merged 1 commit into from Jan 3, 2018

Conversation

@pyfisch
Copy link
Contributor

pyfisch commented Jan 2, 2018

Add one regression ref test.

See also #19649


  • ./mach build -d does not report any errors

  • ./mach test-tidy does not report any errors

  • There are tests for these changes


This change is Reviewable

@highfive

This comment has been minimized.

Copy link

highfive commented Jan 2, 2018

Heads up! This PR modifies the following files:

  • @emilio: components/layout/display_list_builder.rs
@pyfisch

This comment has been minimized.

Copy link
Contributor Author

pyfisch commented Jan 2, 2018

r? @emilio

@bors-servo try

I need help to also support background images. They already have a clip from CSS as a local clip and the corner clipping should attach another clip. How is this best done?

@highfive highfive assigned emilio and unassigned mbrubeck Jan 2, 2018
@bors-servo

This comment has been minimized.

Copy link
Contributor

bors-servo commented Jan 2, 2018

⌛️ Trying commit 528214c with merge 3276785...

bors-servo added a commit that referenced this pull request Jan 2, 2018
Implement correct corner clipping for background color

Add one regression ref test.

See also #19649

---
- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors

- [x] There are tests for these changes

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/19675)
<!-- Reviewable:end -->
@bors-servo

This comment has been minimized.

Copy link
Contributor

bors-servo commented Jan 2, 2018

@emilio
emilio approved these changes Jan 3, 2018
Copy link
Member

emilio left a comment

LGTM with those comments addressed. Thanks a lot!

@@ -1041,6 +1030,25 @@ fn calculate_inner_bounds(mut bounds: Rect<Au>, offsets: SideOffsets2D<Au>) -> R
bounds
}

fn calculate_inner_border_radii(
mut radii: BorderRadii<Au>,
offsets: SideOffsets2D<Au>)

This comment has been minimized.

Copy link
@emilio

emilio Jan 3, 2018

Member

nit:

) -> BorderRadii<Au> {
height: 80px;
border: 20px blue solid;
border-top-right-radius: 20px;
background-color: red;

This comment has been minimized.

Copy link
@emilio

emilio Jan 3, 2018

Member

Consider using a color other than red for the test? red is usually used to signal failure.

</style>
<body>
<div id="a"></div>
<div id="shield"></div>

This comment has been minimized.

Copy link
@emilio

emilio Jan 3, 2018

Member

Maybe the shield comment is useful here too?

This comment has been minimized.

Copy link
@emilio

emilio Jan 3, 2018

Member

Well, I guess there's no corner to hide here, so nbd, please ignore this comment.

@emilio

This comment has been minimized.

Copy link
Member

emilio commented Jan 3, 2018

I need help to also support background images. They already have a clip from CSS as a local clip and the corner clipping should attach another clip. How is this best done?

I think @mrobinson can help you faster than I can, but I can try to look into it if he is busy :)

@mrobinson

This comment has been minimized.

Copy link
Member

mrobinson commented Jan 3, 2018

@pyfisch The right way to do this in WebRender world is to make the corner clip a child of the CSS clip and to ensure that display items for the border use that new clip in the clipping part of their ClipAndScrollInfo. I'm not sure how easy that is to do with our current setup, but creating clips during display list building will be important for the future in order to allow removing the rounded corner API for local clips.

@pyfisch pyfisch force-pushed the pyfisch:issue19649 branch from 528214c to eb9d0dd Jan 3, 2018
@pyfisch

This comment has been minimized.

Copy link
Contributor Author

pyfisch commented Jan 3, 2018

Comments addressed.

The shield basically hides the rounded outer border. This is needed because the background usually covers the whole border-box and ends at the same line as the border. Due to anti aliasing the background shines through at this line and causes slightly different output for both the test and the reference.

In current servo some white is visible between border and background. After this patch there is no more white.

test
ref

fn calculate_inner_border_radii(
mut radii: BorderRadii<Au>,
offsets: SideOffsets2D<Au>
) -> BorderRadii<Au> {

This comment has been minimized.

Copy link
@CYBAI

CYBAI Jan 3, 2018

Collaborator

Minor nit:

I think the indentation @emilio means is

fn calculate_inner_border_radii(
    mut radii: BorderRadii<Au>,
    offsets: SideOffsets2D<Au>
) -> BorderRadii<Au> {

This comment has been minimized.

Copy link
@emilio

emilio Jan 3, 2018

Member

Yeah, that's right, that's the indentation rustfmt would use for multi-line functions.

Add one regression ref test.

See also #19649
@pyfisch pyfisch force-pushed the pyfisch:issue19649 branch from eb9d0dd to b3b49e3 Jan 3, 2018
@pyfisch

This comment has been minimized.

Copy link
Contributor Author

pyfisch commented Jan 3, 2018

Changed indentation.

@emilio
emilio approved these changes Jan 3, 2018
@emilio

This comment has been minimized.

Copy link
Member

emilio commented Jan 3, 2018

@bors-servo

This comment has been minimized.

Copy link
Contributor

bors-servo commented Jan 3, 2018

📌 Commit b3b49e3 has been approved by emilio

@bors-servo

This comment has been minimized.

Copy link
Contributor

bors-servo commented Jan 3, 2018

⌛️ Testing commit b3b49e3 with merge ef42465...

bors-servo added a commit that referenced this pull request Jan 3, 2018
Implement correct corner clipping for background color

Add one regression ref test.

See also #19649

---
- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors

- [x] There are tests for these changes

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/19675)
<!-- Reviewable:end -->
@bors-servo

This comment has been minimized.

Copy link
Contributor

bors-servo commented Jan 3, 2018

@bors-servo bors-servo merged commit b3b49e3 into servo:master Jan 3, 2018
3 checks passed
3 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
@pyfisch pyfisch deleted the pyfisch:issue19649 branch Jan 3, 2018
nox added a commit to web-platform-tests/wpt that referenced this pull request Jan 10, 2018
Add one regression ref test.

See also #19649

Upstreamed from servo/servo#19675 [ci skip]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.