Clamp border corner radius #2187

nc4rrillo · 2017-12-07T04:22:27Z

This change is

mstange · 2017-12-07T04:36:34Z

This is a bit too restrictive - corner radii of more than half the width or height are permitted in CSS if the neighboring corner leaves enough space. See the testcase in #2174 for an example.

Here's the code in Firefox that limits corner radii: https://searchfox.org/mozilla-central/rev/f5f1c3f294f89cfd242c3af9eb2c40d19d5e04e7/layout/generic/nsFrame.cpp#1699-1720
The radii of neighboring corners are scaled down proportionally. The spec text is at https://drafts.csswg.org/css-backgrounds/#corner-overlap .

nc4rrillo · 2017-12-07T05:18:06Z

@mstange I've taken another stab at it, I'm not experienced at reading the specs but I think this matches the Gecko functionality.

I get

with the following YAML which incorporates your case in #2174:

---
root: 
  items: 
    - 
      bounds: [0, 0, 1024, 768]
      "clip-rect": [0, 0, 1024, 768]
      "clip-and-scroll": 0
      "backface-visible": true
      type: "stacking-context"
      "scroll-policy": scrollable
      "transform-style": flat
      filters: []
      items: 
        - 
          bounds: [0, 0, 200, 200]
          "clip-rect": [0, 0, 200, 200]
          "clip-and-scroll": 0
          "backface-visible": true
          type: border
          width: 10
          "border-type": normal
          color: red
          style: dashed
          radius: [180, 180]
        - 
          bounds: [200, 0, 380, 380]
          "clip-rect": [200, 0, 380, 380]
          "clip-and-scroll": 0
          "backface-visible": true
          type: border
          width: 10
          "border-type": normal
          color: green
          style: dashed
          radius: [180, 180]
        - 
          bounds: [580, 0, 200, 200]
          "clip-rect": [580, 0, 200, 200]
          "clip-and-scroll": 0
          "backface-visible": true
          type: border
          width: 10
          "border-type": normal
          color: red  
          style: solid
          radius: [180, 180]
        - 
          bounds: [780, 0, 380, 380]
          "clip-rect": [780, 0, 380, 380]
          "clip-and-scroll": 0
          "backface-visible": true
          type: border
          width: 10
          "border-type": normal
          color: green
          style: solid
          radius: [180, 180]
        - 
          bounds: [0, 380, 200, 200]
          "clip-rect": [0, 380, 200, 200]
          "clip-and-scroll": 0
          "backface-visible": true
          type: border
          width: 10
          "border-type": normal
          color: red
          style: dotted
          radius: [180, 180]
        - 
          bounds: [200, 380, 200, 200]
          "clip-rect": [200, 380, 200, 200]
          "clip-and-scroll": 0
          "backface-visible": true
          type: border
          width: 10
          "border-type": normal
          color: green
          style: dotted
          radius:
                top-left: [180, 180]
                top-right: [0, 0]
                bottom-left: [0, 0]
                bottom-right: [180, 180]
  id: [0, 0]
pipelines: []

nc4rrillo · 2017-12-07T05:41:46Z

Looks like one failing reftest:

REFTEST TEST-UNEXPECTED-FAIL | reftests/border/border-no-bogus-line.yaml == 
reftests/border/border-no-bogus-line-ref.png | 
image comparison, max difference: 77, number of differing pixels: 476

glennw · 2017-12-07T05:47:50Z

Depending on what the differences are, it may just be a case of re-generating the reference image. If you open the reftest-analyzer.html file you should be able to see the differences.

nc4rrillo · 2017-12-07T05:58:03Z

Compared to the reference the circle looks like it gets a tad bit tighter which shifts everything over a few pixels. May just be the result of how the CSS distribution algorithm works. I will regenerate the test if everything else here looks good.

mstange · 2017-12-07T17:44:38Z

reftests/border/border-no-bogus-line.yaml has a border-radius of 40.5 which is slightly larger than half the element's height (80), so it makes sense that it would be affected by these changes.

@subsevenx2001, you added this test in #1975; can you check whether this test still tests what it's supposed to test if this PR lands?

mstange · 2017-12-07T17:46:54Z

The patch looks good to me; it's definitely much easier to understand than the Gecko code.

I would move the calls to clamp_corner_radius into the calculate_ratio function so that you don't have to pull the radii into local variables twice, and then rename calculate_ratio to ensure_no_corner_overlap or something like that.

subsevenx2001 · 2017-12-07T17:56:59Z

@mstange
If the reftest result does not show bogus line like the attachment, it is just OK to regenerate the png ref image.

mstange · 2017-12-07T18:02:21Z

Thanks! I'm just confused why the radius of 40.5 was necessary in the test, instead of just 40. Because Gecko should have limited the radius to 40 on the Gecko side before sending it to WebRender.

subsevenx2001 · 2017-12-07T18:06:25Z

@mstange
To choose this strange radius is intended to reproduce #1805 in resolution 1920x1080.
It is choosen by try-and-error.

mstange · 2017-12-07T18:08:40Z

Ok, but with the change in this PR we will treat it as 40.

nc4rrillo · 2017-12-07T19:48:22Z

This change causes a regression with dashed borders:

---
root: 
  items: 
    - 
      bounds: [0, 0, 1024, 768]
      "clip-rect": [0, 0, 1024, 768]
      "clip-and-scroll": 0
      "backface-visible": true
      type: "stacking-context"
      "scroll-policy": scrollable
      "transform-style": flat
      filters: []
      items: 
        - 
          bounds: [0, 0, 200, 200]
          "clip-rect": [0, 0, 200, 200]
          "clip-and-scroll": 0
          "backface-visible": true
          type: border
          width: 10
          "border-type": normal
          color: red
          style: dashed
          radius:
                top-left: [180, 180]
                top-right: [0, 0]
                bottom-left: [0, 0]
                bottom-right: [180, 180]

I am investigating

nc4rrillo · 2017-12-07T22:33:45Z

Upon further investigation this case also looks glitched on Firefox Nightly with gfx.webrender.enabled set to true

Here's a fiddler

glennw · 2017-12-07T23:07:32Z

OK, is this ready for review / merge then?

nc4rrillo · 2017-12-07T23:10:06Z

@glennw it is, I just am not sure what was decided w.r.t regenerating the border-no-bogus-line test.
Code is good to go for review though.

mstange · 2017-12-07T23:15:07Z

I think you can just regenerate the reference. The scenario that I was concerned about is rather theoretical.

nc4rrillo · 2017-12-07T23:25:38Z

Done.

glennw

Looks good, thanks! I'll set up a gecko try run with this shortly, and if that's all green, this should be good to merge.

glennw · 2017-12-08T20:22:17Z

Oh, wait - was there a reftest to go with this PR?

nc4rrillo · 2017-12-08T20:36:28Z

I can quick add one to this PR


        Clamp border corner radius

nc4rrillo · 2017-12-08T21:02:44Z

@glennw done :)

glennw · 2017-12-08T22:19:57Z

Pending Gecko try here:

https://hg.mozilla.org/try/rev/7fbf230e21b8ef4310dfb76fa6db5cf57bb81c56
https://treeherder.mozilla.org/#/jobs?repo=try&revision=7fbf230e21b8ef4310dfb76fa6db5cf57bb81c56

If this is all green when complete, this is ready to merge.

glennw · 2017-12-09T00:08:06Z

Try looks green.

@bors-servo r+ 🚀

bors-servo · 2017-12-09T00:08:07Z

📌 Commit a2967ee has been approved by glennw

bors-servo · 2017-12-09T00:29:40Z

⌛ Testing commit a2967ee with merge 562b9b9...


        Auto merge of #2187 - nc4rrillo:clamp-corner-radius, r=glennw

Clamp border corner radius Fixes #2183  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/webrender/2187)

bors-servo · 2017-12-09T01:39:54Z

☀️ Test successful - status-appveyor, status-travis
Approved by: glennw
Pushing 562b9b9 to master...

mstange · 2017-12-10T19:38:54Z

webrender/src/border.rs

+    let bottom_right_radius = &mut radius.bottom_right;
+    let bottom_left_radius = &mut radius.bottom_left;
+
+    let sum = top_left_radius.width + bottom_left_radius.width;


I think this needs to be top_right instead of bottom_left

mstange · 2017-12-10T19:38:54Z

webrender/src/border.rs

+        ratio = f32::min(ratio, info.rect.size.width / sum);
+    }
+
+    let sum = top_right_radius.width + bottom_right_radius.width;


and this bottom_left instead of top_right


        Auto merge of #2286 - mstange:correct-corner-overlap, r=emilio

Fix a bug in the corner radius overlap calculation. This fixes a bug in #2187 which I only noticed after the pull request was merged. I'm also adding a test.  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/webrender/2286)

nc4rrillo force-pushed the nc4rrillo:clamp-corner-radius branch 2 times, most recently from 9b64ffc to 182b886 Dec 7, 2017

nc4rrillo force-pushed the nc4rrillo:clamp-corner-radius branch from 182b886 to fa45261 Dec 7, 2017

nc4rrillo force-pushed the nc4rrillo:clamp-corner-radius branch 2 times, most recently from d31c417 to b9d9ffa Dec 7, 2017

nc4rrillo force-pushed the nc4rrillo:clamp-corner-radius branch from b9d9ffa to 31972f0 Dec 7, 2017

nc4rrillo force-pushed the nc4rrillo:clamp-corner-radius branch from 31972f0 to d8f0a80 Dec 7, 2017

glennw self-requested a review Dec 8, 2017

glennw approved these changes Dec 8, 2017

View changes

Clamp border corner radius

Loading status checks…

a2967ee

nc4rrillo force-pushed the nc4rrillo:clamp-corner-radius branch from d8f0a80 to a2967ee Dec 8, 2017

bors-servo merged commit a2967ee into servo:master Dec 9, 2017
3 checks passed

3 checks passed

Taskcluster (pull_request) TaskGroup: success
Details

continuous-integration/appveyor/pr AppVeyor build succeeded
Details

homu Test successful
Details

mstange reviewed Dec 10, 2017

View changes

mstange mentioned this pull request Jan 11, 2018

Fix a bug in the corner radius overlap calculation. #2286

Merged

servo / webrender

Clamp border corner radius #2187

Clamp border corner radius #2187

nc4rrillo commented Dec 7, 2017 •

edited

mstange commented Dec 7, 2017

nc4rrillo commented Dec 7, 2017 •

edited

nc4rrillo commented Dec 7, 2017

glennw commented Dec 7, 2017

nc4rrillo commented Dec 7, 2017 •

edited

mstange commented Dec 7, 2017

mstange commented Dec 7, 2017

subsevenx2001 commented Dec 7, 2017

mstange commented Dec 7, 2017

subsevenx2001 commented Dec 7, 2017

mstange commented Dec 7, 2017 •

edited

nc4rrillo commented Dec 7, 2017 •

edited

nc4rrillo commented Dec 7, 2017 •

edited

glennw commented Dec 7, 2017

nc4rrillo commented Dec 7, 2017

mstange commented Dec 7, 2017

nc4rrillo commented Dec 7, 2017

glennw left a comment

glennw commented Dec 8, 2017

nc4rrillo commented Dec 8, 2017

nc4rrillo commented Dec 8, 2017

glennw commented Dec 8, 2017

glennw commented Dec 9, 2017

bors-servo commented Dec 9, 2017

bors-servo commented Dec 9, 2017

bors-servo commented Dec 9, 2017

This comment has been minimized.

This comment has been minimized.

servo / webrender

Join GitHub today

Clamp border corner radius #2187

Clamp border corner radius #2187

Conversation

nc4rrillo commented Dec 7, 2017 • edited

mstange commented Dec 7, 2017

nc4rrillo commented Dec 7, 2017 • edited

nc4rrillo commented Dec 7, 2017

glennw commented Dec 7, 2017

nc4rrillo commented Dec 7, 2017 • edited

mstange commented Dec 7, 2017

mstange commented Dec 7, 2017

subsevenx2001 commented Dec 7, 2017

mstange commented Dec 7, 2017

subsevenx2001 commented Dec 7, 2017

mstange commented Dec 7, 2017 • edited

nc4rrillo commented Dec 7, 2017 • edited

nc4rrillo commented Dec 7, 2017 • edited

glennw commented Dec 7, 2017

nc4rrillo commented Dec 7, 2017

mstange commented Dec 7, 2017

nc4rrillo commented Dec 7, 2017

glennw left a comment

glennw commented Dec 8, 2017

nc4rrillo commented Dec 8, 2017

nc4rrillo commented Dec 8, 2017

glennw commented Dec 8, 2017

glennw commented Dec 9, 2017

bors-servo commented Dec 9, 2017

bors-servo commented Dec 9, 2017

bors-servo commented Dec 9, 2017

This comment has been minimized.

mstange Dec 10, 2017

This comment has been minimized.

mstange Dec 10, 2017

nc4rrillo commented Dec 7, 2017 •

edited

nc4rrillo commented Dec 7, 2017 •

edited

nc4rrillo commented Dec 7, 2017 •

edited

mstange commented Dec 7, 2017 •

edited

nc4rrillo commented Dec 7, 2017 •

edited

nc4rrillo commented Dec 7, 2017 •

edited