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

Border-radius doesn't clip within a border-radius-clipped element. #11857

Closed
Gozala opened this issue Jun 24, 2016 · 7 comments
Closed

Border-radius doesn't clip within a border-radius-clipped element. #11857

Gozala opened this issue Jun 24, 2016 · 7 comments

Comments

@Gozala
Copy link
Contributor

@Gozala Gozala commented Jun 24, 2016

In browser.html if you hover the title it gets a grey background behind it, which supposed to have rounded corners, but they aren't rounder in servo. I have created following test case to reproduce this issue easily:

https://jsbin.com/zuqete/edit?html,css,output

I see following result when using webrender, it seems that borders are in fact rounded but background isn't clipped properly.

screen shot 2016-06-24 at 11 14 41 am

I also traced issue to specific css rule causing this bug, which is:

* {
  border-width: 0;
}
@paulrouget
Copy link
Contributor

@paulrouget paulrouget commented Jun 25, 2016

Border-radius doesn't clip within a border-radius-clipped element. Minimal test case:

<style>

.a {
  width: 200px;
  height: 200px;

  overflow: hidden;
  border-radius: 1px;
}

.b {
  border: 2px solid blue;
  background: red;
  width: 50px;
  height: 50px;

  border-radius: 25px;
  overflow: hidden;
}

</style>

<div class="a">
  <div class="b"></div>
</div>
@paulrouget paulrouget changed the title Background clipping issue Border-radius doesn't clip within a border-radius-clipped element. Jun 25, 2016
@splav
Copy link
Contributor

@splav splav commented Jun 25, 2016

May I try this?

@jdm
Copy link
Member

@jdm jdm commented Jun 25, 2016

@splav Please do! Ask questions about anything that's unclear!

@splav
Copy link
Contributor

@splav splav commented Jun 25, 2016

It looks like a TODO by gw ( @glennw ?) , not a general bug (https://github.com/servo/webrender/blob/master/src/batch.rs#L225) . May be there are any thoughts on how to implement it?

    // TODO(gw): Support nested complex clip regions!
    pub complex_clip: Option<ComplexClipRegion>,

For now, I'll continue trying to implement it.

@splav
Copy link
Contributor

@splav splav commented Jun 26, 2016

I don't have an ultimate solution yet, but for this case (and I think most of the cases) a fast path can be implemented - one clipping region is completely inside the other. So we can test if all the clipping regions are essentially equivalent to the most inner one and proceed with the internal one. I've a WIP implementation of the logic.

How to implement general case efficiently still remains unclear to me. Will be glad to hear any thoughts. Especially if this fast path is worth implementing and possible ways of general fallback implementation.

@kilobtye
Copy link
Contributor

@kilobtye kilobtye commented Jun 27, 2016

I think #11562 has the same cause.

bors-servo added a commit to servo/webrender that referenced this issue Aug 8, 2016
Omit nested clip regions if internal one is their subregion

In case of nested clipping regions it is common that external ones do contain the most internal clipping region and essentially only that internal one should be used.
I'm not sure that this is the correct path, but it's rather common ([servo/issues/11562](servo/servo#11562), [servo/issues/11857](servo/servo#11857)) and at least can be used as a fast path if the supposed full of nested clipping regions is rather heavy.

This PR implements this fast path. It relies on the fact that int nested regions list the last is the innermost.

While not sure that it's a good idea at all, looks like a hack, but still...

If there are any thought hot to implement full nested regions support I'll try to implement those (Or may be it will be done in WR2 and there is no need to do this right now).
@paulrouget
Copy link
Contributor

@paulrouget paulrouget commented Aug 18, 2016

Works now.

@paulrouget paulrouget closed this Aug 18, 2016
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
6 participants
You can’t perform that action at this time.