Add roundRect to CanvasRenderingContext2D path #6765

mysteryDate · 2021-06-13T21:05:04Z

Original proposal: https://github.com/fserb/canvas2D/blob/master/spec/roundrect.md
Closes #5619

At least two implementers are interested (and none opposed):
- Chromium
- Webkit
Tests are written and can be reviewed and commented upon at:
- https://github.com/web-platform-tests/wpt/tree/master/html/canvas/element/path-objects
- https://github.com/web-platform-tests/wpt/tree/master/html/canvas/offscreen/path-objects
Implementation bugs are filed:
- Chrome: https://bugs.chromium.org/p/chromium/issues/detail?id=1123971&q=roundrect&can=1
- Firefox: …
- Safari: …

/canvas.html ( diff )
/infrastructure.html ( diff )

Kaiido

I think this is a great addition to the API and I'm looking forward to a wide support.
There is just one point about how the DOMPoint input integrates in there that doesn't seem clear to me, and a small note about where the new subpath should starts.

(Also a bit sad that negative values aren't allowed and thus we'll still need to make concave corners ourselves , but since this is more about replicating CSS border-radius which doesn't support it either, I understand this choice).

Ps: cc @whatwg/canvas

source

kdashg · 2021-06-15T22:55:31Z

I think I'm opposed to this but not Opposed.

I would like to see a JS-only reference polyfill for this.

Kaiido · 2021-06-16T04:27:51Z

I think I'm opposed to this but not Opposed.

Any particular reason to be against this? Given the number of various implementations out there in the wild, I think we can claim it's a feature that web-authors actually want.

I would like to see a JS-only reference polyfill for this.

I made a quick implementation here: https://github.com/Kaiido/roundRect
If you want to fiddle with it: https://jsfiddle.net/dou351aj/

source

kdashg · 2021-06-16T19:45:15Z

I think I'm opposed to this but not Opposed.

Any particular reason to be against this? Given the number of various implementations out there in the wild, I think we can claim it's a feature that web-authors actually want.

I would like to see a JS-only reference polyfill for this.

I made a quick implementation here: https://gist.github.com/Kaiido/1375a1c4c627d4e871191bf68eec1fb3
If you want to fiddle with it: https://jsfiddle.net/n4jLakds/

I care a lot about enabling previously-impossible or -difficult use cases, but not so much about ones that are easy to polyfill. I think proof that implementations exist in the wild indicates that while it may be differentially more ergonomic to have as baseline in browsers, we have to ask: Are people going to stop using their own battle-tested implementations and use ours? Can they really simplify their util libraries usefully? They will still need to support browsers that don't have roundRect for quite some time, and I'm not convinced that it's onerous for them to use their own implementations.

I fear adding this is more likely to cause portability problems between UAs than not having it at all. Without moving this to core, everyone always polyfills, and it works everywhere that supports canvas today. With this though, apps written and tested in UAs that do implement this will fail to work in UAs that have no support. Yes, ideally people look up the support matrix, but that doesn't always happen.

As such, I think the picture for the web is better without this than with it.

Kaiido · 2021-06-17T02:04:23Z

Are people going to stop using their own battle-tested implementations and use ours?

I obviously don't have the answer to that, but I guess that yes, if there is a "standard" way to do this action, why would they keep spending hours / days of search and trial to find the best implementation in the wild?

Can they really simplify their util libraries usefully?

I think so yes, rather than having each library implement there own version of a roundRect primitive, they can implement only once a real polyfill, that actually follows a "standard" behavior. Currently the "various implementations out there in the wild" I talked about are NOT polyfills, they all come with different signatures and are sometimes actually wrong because like the explainer says, it's hard to get it right. They may all do something that someone could call a "rounded rectangle", but they will all do it differently, which means that if you ask three different web-authors to do three different components in the same page where such a shape should be rendered, they may come up with three different versions of the code.

They will still need to support browsers that don't have roundRect for quite some time, and I'm not convinced that it's onerous for them to use their own implementations.

Having one polyfill is not onerous, but once again, without a "standard" there is no polyfill, and having several implementations is onerous.

I fear adding this is more likely to cause portability problems between UAs than not having it at all.
Without moving this to core, everyone always polyfills, and it works everywhere that supports canvas today.

Same, they don't "polyfill", they reimplement, again and again.

With this though, apps written and tested in UAs that do implement this will fail to work in UAs that have no support.

But here they'll be able to really polyfill, with a single polyfill that everyone will agree on, and with standard tests to make it reliable, because there will finally be a "standard".
Also, this argument could be made for every addition to the specs, while backward-compatibility is a must, it should not block the APIs to continue to evolve.

I get that polluting the APIs with easy to come functions is a bad thing, but here I strongly believe this is beneficial for the eco-system.

source

domenic

I pushed a few fixup commits since most of the things I found were really small typographical things. Feel free to look over the diff, but for future reference to refine your (already-great) HTML spec contributions, here are some themes:

We're currently not wrapping the Dependencies section in the same way as the rest of the spec. (Related: Consider not outputting the Dependencies section #5919 to make this less of a nightmare for contributors.)
We use Oxford comma
RangeError is not a DOMException
We try to use spaces around mathematical operators. (We are not consistent about this, as you can see from rect() right above.)
− is nicer than -
Don't forget to cross-link all the instances of dictionary x and y members
Don't forget to <var> all the variables
Don't put <var> around non-variable names, e.g. it's <var>normalizedRadii</var>[0], not <var>normalizedRadii[0]</var>.
Don't forget <p> inside <li>s
Don't forget periods at the end of sentences.
Check out the style for nested list indents.

I left a few inline comments that are somewhat substantial, but besides those, I think we are ready to go.

source

domenic · 2021-06-28T17:28:55Z

source

+    negative number, or is an <code data-x="">{ x, y }</code> object whose <code data-x="">x</code>
+    or <code data-x="">y</code> properties are negative numbers.</p>
+
+    <!-- Consider adding this useful information to all the methods which behave this way: -->


I commented this out for now and added a comment above explaining that we should consider adding it everywhere. I think it is helpful information that would be good to have but I worry that only one method explaining when it silently does nothing would be confusing for developers.

If you're up for fixing the other methods in this PR or a followup PR, that'd be lovely :).

So you mean all path functions that fail silently and return if their inputs are non-finite?

I'd be happy to tackle that in a separate PR once the Canvas2D new api is launched 😃

mysteryDate · 2021-07-02T17:22:40Z

Thanks for the feedback @domenic! I'll save this list for future reference.

domenic

LGTM!

Can you confirm the test coverage is complete? In particular all the details we hashed out about early-returns vs. exceptions, and what type of exceptions? I didn't find any in a quick skim of the test directories you linked; I only saw success cases.

mysteryDate · 2021-07-02T21:17:14Z

Empty array for radii throws: https://github.com/web-platform-tests/wpt/blob/master/html/canvas/element/path-objects/2d.path.roundrect.radius.none.html

5 elements for radii array throws: https://github.com/web-platform-tests/wpt/blob/master/html/canvas/element/path-objects/2d.path.roundrect.radius.toomany.html

Nonfinite fails silently: https://github.com/web-platform-tests/wpt/blob/master/html/canvas/element/path-objects/2d.path.roundrect.nonfinite.html

Looks like we're just missing an exception from negative radii. I'll add that now.

mysteryDate · 2021-07-06T13:33:32Z

Negative radius tests are added:

domenic · 2021-07-07T19:20:26Z

All the exception tests should be updated to RangeError, not "IndexSizeError" DOMException.

Nonfinite fails silently:

Can you test the { x, y } variant too? Just a few would be fine; no need to go combinatoric on all possibilities.

Negative radius tests are added:

Can you test the { x, y } variant too?

mysteryDate · 2021-07-08T18:56:24Z

All the exception tests should be updated to RangeError, not "IndexSizeError" DOMException.
Can you test the { x, y } variant too?

Done and done

domenic · 2021-07-08T19:03:06Z

Oh, it looks like your implementation has a bug: it's using DOMPoint, not DOMPointInit. https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/modules/canvas/canvas2d/canvas_path.idl;l=16?q=canvas_path.idl&ss=chromium

Let's get that updated and add some test cases (again, just a sprinkling, no need to be exhaustive) that use pure { x, y } objects, instead of DOMPoints!

(Note that since DOMPoints also satisfy the DOMPointInit contract, you can use either.)

mysteryDate · 2021-07-08T19:10:15Z

Yeah, and I ALSO broke backwards compatibility. It's just roundRect that should move to a RangeError, right?

https://chromium-review.googlesource.com/c/chromium/src/+/3015806

domenic · 2021-07-08T19:14:23Z

Yeah. I mean, I'd kind of like to fix the others at some point, but it carries a likely-small-but-potentially-real compat risk, so I don't think it's a great idea to tie it into this change.

This is what we decided on in the spec debate. whatwg/html#6765 Bug: 1193694 Change-Id: Ic24a8cf9ec61ab584318a40660c0c0e06a0b9848

This is what we decided on in the spec debate. whatwg/html#6765 Bug: 1193694 Change-Id: Ic24a8cf9ec61ab584318a40660c0c0e06a0b9848 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3016815 Commit-Queue: Aaron Krajeski <[email protected]> Reviewed-by: Justin Novosad <[email protected]> Cr-Commit-Position: refs/heads/master@{#899818}

domenic · 2021-07-12T16:37:08Z

Alright, the tests look pretty comprehensive to me now! I'll merge this. @mysteryDate, when you have some time, please file bugs on WebKit and Gecko and update the OP with links to them.

Kaiido · 2021-07-13T08:16:42Z

source

+     data-x="DOMPointInit-x">x</code>"].</p></li>
+
+     <li><p>Let <var>left</var> be <var>lowerLeft</var>["<code data-x="DOMPointInit-y">y</code>"]
+     + <var>lowerRight</var>["<code data-x="DOMPointInit-y">y</code>"]</p></li>


I spot this a bit late, sorry...
I think this should read "Let left be upperLeft["y"] + lowerLeft["y"].

Oh, thanks for the catch. Would you or @mysteryDate be up for a quick followup PR to fix this?

I opened #6871

@mysteryDate

The algorithm to fix corner overlapping has a typo. I missed it during the review of whatwg#6765 so here is a follow-up. If @mysteryDate could have a look that'd be great.

Automatic update from web-platform-tests Use DOMPointInit for roundRect This is what we decided on in the spec debate. whatwg/html#6765 Bug: 1193694 Change-Id: Ic24a8cf9ec61ab584318a40660c0c0e06a0b9848 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3016815 Commit-Queue: Aaron Krajeski <[email protected]> Reviewed-by: Justin Novosad <[email protected]> Cr-Commit-Position: refs/heads/master@{#899818} -- wpt-commits: adcec00147f8dc59daaca5793d8bcd5f8d2d519a wpt-pr: 29615

This is what we decided on in the spec debate. whatwg/html#6765 Bug: 1193694 Change-Id: Ic24a8cf9ec61ab584318a40660c0c0e06a0b9848 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3016815 Commit-Queue: Aaron Krajeski <[email protected]> Reviewed-by: Justin Novosad <[email protected]> Cr-Commit-Position: refs/heads/master@{#899818} NOKEYCHECK=True GitOrigin-RevId: 0011ce02c0881ded430eb0fd7fa03c78d7411dfe

mysteryDate added 2 commits June 12, 2021 10:44

Add roundRect path function

3a89604

Add negative

1f70525

Kaiido reviewed Jun 14, 2021

View reviewed changes

source Outdated Show resolved Hide resolved

source Outdated Show resolved Hide resolved

Kaiido reviewed Jun 14, 2021

View reviewed changes

source Outdated Show resolved Hide resolved

domenic reviewed Jun 14, 2021

View reviewed changes

source Show resolved Hide resolved

source Outdated Show resolved Hide resolved

source Outdated Show resolved Hide resolved

source Outdated Show resolved Hide resolved

source Outdated Show resolved Hide resolved

source Outdated Show resolved Hide resolved

domenic added addition/proposal New features or enhancements topic: canvas labels Jun 14, 2021

mysteryDate added 3 commits June 15, 2021 12:38

Clear up the algo

0400c91

More descriptive names

2074427

Move note outside of list

cfa994d

Kaiido reviewed Jun 16, 2021

View reviewed changes

source Outdated Show resolved Hide resolved

source Outdated Show resolved Hide resolved

Forgot the normalization

b191825

Had the ratios backwards

9b7d823

domenic requested changes Jun 23, 2021

View reviewed changes

mysteryDate and others added 8 commits June 25, 2021 18:54

Domenics feedback on the algorithm

7976094

Define DOMPoint

c64a408

Some extra DOMPoint attributes

d9660c8

Forgot to wrap

f7b8d42

Minor tweaks

77075de

method steps and no whitespace in method name

c0f84b6

Fix nested list indents

31b9e28

Don't need those

e21eed2

domenic reviewed Jun 28, 2021

View reviewed changes

Normalize procedure language

d46ed6c

Fix wrapping

a2e3149

domenic approved these changes Jul 2, 2021

View reviewed changes

chromium-wpt-export-bot mentioned this pull request Jul 8, 2021

Use DOMPointInit for roundRect web-platform-tests/wpt#29615

Merged

chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Jul 8, 2021

Use DOMPointInit for roundRect

c8c757a

This is what we decided on in the spec debate. whatwg/html#6765 Bug: 1193694 Change-Id: Ic24a8cf9ec61ab584318a40660c0c0e06a0b9848

chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Jul 9, 2021

Use DOMPointInit for roundRect

4f3f739

This is what we decided on in the spec debate. whatwg/html#6765 Bug: 1193694 Change-Id: Ic24a8cf9ec61ab584318a40660c0c0e06a0b9848

yiyix mentioned this pull request Jul 9, 2021

Add page for CanvasRenderingContext2D.roundRect GoogleChromeLabs/puppy-content#29

Merged

Kaiido mentioned this pull request Jul 12, 2021

roundRect requiring an array fserb/canvas2D#20

Closed

domenic merged commit 5c8cfae into whatwg:main Jul 12, 2021

Kaiido reviewed Jul 13, 2021

View reviewed changes

Kaiido mentioned this pull request Jul 17, 2021

Correct canvas roundRect's corner overlap #6871

Merged

3 tasks

codingedgar mentioned this pull request Sep 27, 2021

roundRect Fyrd/caniuse#6034

Open

Kaiido mentioned this pull request Nov 19, 2022

Add roundRect method to Path2D mdn/content#22407

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add roundRect to CanvasRenderingContext2D path #6765

Add roundRect to CanvasRenderingContext2D path #6765

mysteryDate commented Jun 13, 2021 •

edited by pr-preview bot

Loading

Kaiido left a comment •

edited

Loading

kdashg commented Jun 15, 2021

Kaiido commented Jun 16, 2021 •

edited

Loading

kdashg commented Jun 16, 2021

Kaiido commented Jun 17, 2021

domenic left a comment

domenic Jun 28, 2021

mysteryDate Jul 2, 2021

mysteryDate commented Jul 2, 2021

domenic left a comment

mysteryDate commented Jul 2, 2021

mysteryDate commented Jul 6, 2021

domenic commented Jul 7, 2021

mysteryDate commented Jul 8, 2021

domenic commented Jul 8, 2021

mysteryDate commented Jul 8, 2021

domenic commented Jul 8, 2021

domenic commented Jul 12, 2021

Kaiido Jul 13, 2021

domenic Jul 13, 2021

Kaiido Jul 17, 2021

Add roundRect to CanvasRenderingContext2D path #6765

Add roundRect to CanvasRenderingContext2D path #6765

Conversation

mysteryDate commented Jun 13, 2021 • edited by pr-preview bot Loading

Kaiido left a comment • edited Loading

Choose a reason for hiding this comment

kdashg commented Jun 15, 2021

Kaiido commented Jun 16, 2021 • edited Loading

kdashg commented Jun 16, 2021

Kaiido commented Jun 17, 2021

domenic left a comment

Choose a reason for hiding this comment

domenic Jun 28, 2021

Choose a reason for hiding this comment

mysteryDate Jul 2, 2021

Choose a reason for hiding this comment

mysteryDate commented Jul 2, 2021

domenic left a comment

Choose a reason for hiding this comment

mysteryDate commented Jul 2, 2021

mysteryDate commented Jul 6, 2021

domenic commented Jul 7, 2021

mysteryDate commented Jul 8, 2021

domenic commented Jul 8, 2021

mysteryDate commented Jul 8, 2021

domenic commented Jul 8, 2021

domenic commented Jul 12, 2021

Kaiido Jul 13, 2021

Choose a reason for hiding this comment

domenic Jul 13, 2021

Choose a reason for hiding this comment

Kaiido Jul 17, 2021

Choose a reason for hiding this comment

mysteryDate commented Jun 13, 2021 •

edited by pr-preview bot

Loading

Kaiido left a comment •

edited

Loading

Kaiido commented Jun 16, 2021 •

edited

Loading