Add conic-gradient #6661

yiyix · 2021-05-07T05:58:02Z

At least two implementers are interested (and none opposed):
- Chrome
- Firefox: in meetings, implemented behind flags now.
- WebKit: Proposal: add createConicGradient to CanvasRenderingContext2D #5431 (comment)
Tests are written and can be reviewed and commented upon at:
- Implement createConicGradient for the new canvas 2d API web-platform-tests/wpt#24299
- Improve test coverage for CreateConicGradient web-platform-tests/wpt#29002
Implementation bugs are filed:
- Chrome: https://bugs.chromium.org/p/chromium/issues/detail?id=1097034
- Firefox: https://bugzilla.mozilla.org/show_bug.cgi?id=1627014
- Safari: https://bugs.webkit.org/show_bug.cgi?id=225539

(See WHATWG Working Mode: Changes for more details.)

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

Kaiido

Spotted a few typos while reading.

source

domenic · 2021-05-07T22:31:55Z

source

+
+  It follows the same rendering rule as CSS <span>conic-gradient</span>. The <var>position</var> is
+  the center defined by <var>x</var> and <var>y</var> in the coordinate space. This function is
+  equivalent to CSS conic-gradient(from <var>startAngle</var> deg at <var>x</var> px <var>y</var> px, <var>angular-color-stop-list</var>),


It looks like the convention (e.g. in https://html.spec.whatwg.org/#flow-content-3:is-modal) is to wrap CSS expressions in single quotes '.

source

domenic · 2021-05-07T22:41:45Z

source

+  equivalent to CSS conic-gradient(from <var>startAngle</var> deg at <var>x</var> px <var>y</var> px, <var>angular-color-stop-list</var>),
+  where <var>angular-color-stop-list</var> is to be applied to the <code>CanvasGradient</code> by
+  calling <span data-x="dom-canvasgradient-addColorStop">addColorStop</span>.
+


Does anything need to be said about the coordinate spaces involved? Like, are CSS pixels the same as the canvas pixels?

if it helps the clarification, i can add something. I see CSS pixel defined and used in Canvas, (ex: actualBoundingBoxRight). I can may add it in the previous paragraph where i explain what is x and y

Kaiido · 2021-05-10T01:26:29Z

source

+  <p>The <dfn method for="CanvasFillStrokeStyles"><code
+  data-x="dom-context-2d-createConicGradient">createConicGradient(<var>startAngle</var>, <var>x</var>,
+  <var>y</var>)</code></dfn> method takes three arguments, the first argument, <var>startAngle</var>,
+  represents the angle in degree at which the gradient begins, and the last two arguments,


I don't see where it has been defined that angle should be in degree.
In #5431 (comment) @nt1m was asking if this should be in radians or degrees, but it seems there hasn't been any response in that thread, and given current Nightly still uses radians in the implementation, I guess they haven't been made aware of that decision to use degrees.

For what it's worth, if it's still time to have this discussion, I feel as a web-author that having the angle in degrees is very confusing: If I'm not mistaken, it is the only place in the whole canvas API that we would have an angle expressed in degrees instead of in radians. So if we want to play with that angle and e.g the transformation of the context, or the start/end angles of an ellipse, we'd have to convert between degrees and radians, not to mention the mental gymnastic of remembering where to use what...
If the goal was to have a simpler mapping from CSS, then note that only the <angle> 0 can have the unit omitted, and that CSS has a rad unit.

Note that there is already a discrepancy with arc (and ellipse), where both startAngle and endAngle are relative to the x-axis, while CSS conic gradients asks that "0deg points to the top of the page", but that one sounds a bit less dramatic to me.

(@Kaiido to be clear, now is definitely the time. Thanks for raising this! See also https://whatwg.org/working-mode#changes if you want to get more familiar with the process in general.)

+1 for radians. I'm not an expert, but all the JS APIs I know of use (e.g. the Math functions) use radians, and canvas should definitely be internally consistent.

The inconistency between axes does seem problematic though. Would it make sense to make the mapping (startAngle + Math.PI / 2) radians so that it matches other canvas APIs? We could also even add a counterclockwise boolean like those APIs have.

I implemented this many months ago and cannot remember why I chose degrees. I think I was trying to match some CSS behaviour.
I also would prefer radians, personally. +1 to rotating everything by PI/2 as well to match arc and ellipse.

Strong agree that an angle number without additional type info should be interpreted as radians, coming from a JS API.

Argh at the mismatch of canvas api start angle. CSS copied from SVG, using "bearing angles". I think we should match existing canvas precedent here.

I am so glad we have this discussion here now before i change more Canvas API.

+1 for radians and rotating everything by PI/2. Canvas API should be consistent. Changing spec. I will also change our implementation to map this.

domenic

I pushed some editorial tweaks. It now LGTM!

However I noticed that the tests at https://github.com/web-platform-tests/wpt/pull/24299/files seem rather basic. They all use zero angle, so they would not detect the change we made here to radians or to offsetting by pi/2. And if I am reading them right, they all test only a single color (0,255,0,255), even though they use multiple color stops. Can we improve the test coverage here?

yiyix · 2021-05-13T04:54:49Z

I pushed some editorial tweaks. It now LGTM!

However I noticed that the tests at https://github.com/web-platform-tests/wpt/pull/24299/files seem rather basic. They all use zero angle, so they would not detect the change we made here to radians or to offsetting by pi/2. And if I am reading them right, they all test only a single color (0,255,0,255), even though they use multiple color stops. Can we improve the test coverage here?

Due to this change, I updated the tests accordingly -> https://chromium-review.googlesource.com/c/chromium/src/+/2891049
I modified the tests a bit, so it's testing for positive/negative angle rotation and 1 test for no-rotation with several colorstop.

I will also add another test for argument validation. What are the other tests you would suggest? I am happy to address them in a later cl.

domenic · 2021-05-13T16:51:56Z

I'm still not seeing any tests that actually test the gradient-ness: every pixel test is testing against the same 0,255,0,255 value. So if I am understanding the tests correctly, an implementation would be able to pass these tests if it only ever used the first color stop in the color stop list.

Are there tests that I am missing which test more than one color? Or am I misunderstanding the test suite?

I do see a test under third_party/blink/web_tests/fast/canvas/conic-gradient.html that seems like it'd be good, but it's an internal Blink test, not a web platform test. Maybe it could be converted into a reftest?

yiyix · 2021-05-14T19:12:01Z

I'm still not seeing any tests that actually test the gradient-ness: every pixel test is testing against the same 0,255,0,255 value. So if I am understanding the tests correctly, an implementation would be able to pass these tests if it only ever used the first color stop in the color stop list.

Are there tests that I am missing which test more than one color? Or am I misunderstanding the test suite?

I do see a test under third_party/blink/web_tests/fast/canvas/conic-gradient.html that seems like it'd be good, but it's an internal Blink test, not a web platform test. Maybe it could be converted into a reftest?

Thank you for the feedbacks! I added much more tests in https://chromium-review.googlesource.com/c/chromium/src/+/2896865

domenic · 2021-05-14T19:28:54Z

Excellent, that's perfect! I'm happy to merge this now. I'll give this until Monday in case @annevk or anyone else wants to have a final look, then I'll press the merge button.

yiyix · 2021-05-15T05:15:21Z

Added the new tests pull request in the description. web-platform-tests/wpt#29002

annevk

Modulo nits this seems okay. At some point we also need to modernize these method definitions.

annevk · 2021-05-17T08:55:42Z

source

+   <dd>
+   <p>Returns a <code>CanvasGradient</code> object that represents a
+   conic gradient that paints clockwise along the rotation around the center
+   represented by the arguments.</p>


Nit: indentation and wrapping is off and misses an end tag.

annevk · 2021-05-17T08:56:22Z

source

+  data-x="dom-context-2d-createConicGradient">createConicGradient(<var>startAngle</var>, <var>x</var>,
+  <var>y</var>)</code></dfn> method takes three arguments, the first argument, <var>startAngle</var>,
+  represents the angle in radians at which the gradient begins, and the last two arguments,
+  (<var>x</var>, <var>y</var>), represent the center of the gradient in <span data-x="'px'">CSS pixels</span>.


Nit: exceeds a 100 columns.

yiyix and others added 13 commits March 17, 2021 00:52

merge to masteer branch

c34a139

small fix

2263557

changes

27a4c5c

Add more text modifier properties

c464652

pull

2f236c9

address comments

7429bc0

address comments

44daada

update text preparation algorithm

b44ec37

Merge remote-tracking branch 'upstream/main'

c3238b9

update text preparation algorithm

be3c905

formatting nits

aad64f0

Merge remote-tracking branch 'upstream/main'

7ac879d

add conic gradient

68eb67d

Kaiido reviewed May 7, 2021

View reviewed changes

source Show resolved Hide resolved

source Show resolved Hide resolved

source Show resolved Hide resolved

fix typo

c404de9

domenic reviewed May 7, 2021

View reviewed changes

address comments

15686d4

Kaiido reviewed May 10, 2021

View reviewed changes

yiyix and others added 2 commits May 12, 2021 04:20

update conicgradient to take radians

8bb28c0

Editorial tweaks

5dee3b5

domenic added addition/proposal New features or enhancements topic: canvas labels May 12, 2021

domenic approved these changes May 12, 2021

View reviewed changes

yisibl mentioned this pull request May 14, 2021

New Canvas 2D API Brooooooklyn/canvas#246

Open

4 tasks

annevk reviewed May 17, 2021

View reviewed changes

Fix nits

f3d6c6e

domenic merged commit cf2dab0 into whatwg:main May 18, 2021

Kaiido mentioned this pull request Aug 23, 2022

Makes Canvas' ConicGradient match CSS angle #8213

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add conic-gradient #6661

Add conic-gradient #6661

yiyix commented May 7, 2021 •

edited by pr-preview bot

Loading

Kaiido left a comment

domenic May 7, 2021

domenic May 7, 2021

yiyix May 8, 2021

Kaiido May 10, 2021 •

edited

Loading

annevk May 10, 2021

domenic May 10, 2021

mysteryDate May 10, 2021

tabatkins May 10, 2021

yiyix May 12, 2021

domenic left a comment

yiyix commented May 13, 2021

domenic commented May 13, 2021

yiyix commented May 14, 2021

domenic commented May 14, 2021

yiyix commented May 15, 2021

annevk left a comment

annevk May 17, 2021

annevk May 17, 2021

Add conic-gradient #6661

Add conic-gradient #6661

Conversation

yiyix commented May 7, 2021 • edited by pr-preview bot Loading

Kaiido left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Kaiido May 10, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

domenic left a comment

Choose a reason for hiding this comment

yiyix commented May 13, 2021

domenic commented May 13, 2021

yiyix commented May 14, 2021

domenic commented May 14, 2021

yiyix commented May 15, 2021

annevk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yiyix commented May 7, 2021 •

edited by pr-preview bot

Loading

Kaiido May 10, 2021 •

edited

Loading