Fix accidental settings differences between CanvasRenderingContext2D and OffscreenCanvasRenderingContext2D #10904
Conversation
| below, have an <dfn data-x="concept-canvas-origin-clean">origin-clean</dfn> flag, which can be | ||
| the <code>CanvasRenderingContext2D</code>, <code>OffscreenCanvasRenderingContext2D</code>, and | ||
| <code>ImageBitmapRenderingContext</code> objects below, | ||
| have an <dfn data-x="concept-canvas-origin-clean">origin-clean</dfn> flag, which can be | ||
| set to true or false. Initially, when the <code>canvas</code> element or <code>ImageBitmap</code> | ||
| object is created, its bitmap's <span data-x="concept-canvas-origin-clean">origin-clean</span> | ||
| flag must be set to true.</p> |
There was a problem hiding this comment.
Okay to file as a separate issue: should this also initialize it for the rendering contexts?
There was a problem hiding this comment.
I agree, this should be looked at holistically across the specs. My first inclination was that this paragraph should be hardened -- instead of reading "as well as some of the bitmaps of rendering contexts, such as ..." it could read "as well as some of the bitmaps of CanvasRenderingContext2D...", thereby being explicit about excluding WebGL and WebGPU. But a more general way could be to define it here for all contexts (and just add a note saying that WebGL and WebGPU will always have origin-clean be true). Do you want to file an issue on this, or should I (I'm still not 100% sure which way I'd prefer for it to go ... for now I just wanted to avoid the inappropriate exclusion of OffscreenCanvasRenderingContext2D).
source
Outdated
| <p> | ||
| The <dfn>canvas settings output bitmap initialization algorithm</dfn>, which is passed | ||
| <var>canvas</var> (a <code>CanvasSettings</code> object) and | ||
| <var>settings</var> (a <code>CanvasRenderingContext2DSettings</code> object), consists | ||
| of running these steps: | ||
| </p> |
There was a problem hiding this comment.
Formatting of start and end tags should match the other paragraphs.
Also, for new algorithms we should follow https://infra.spec.whatwg.org/#algorithm-declaration which means that the types precede the parameter names and don't have parenthesis around them.
There was a problem hiding this comment.
Thanks, that makes things much more clear. Updated.
source
Outdated
| @@ -71338,7 +71357,7 @@ interface <dfn interface>OffscreenCanvas</dfn> : <span>EventTarget</span> { | |||
| data-x="offscreencanvas-bitmap">bitmap</span> to reference a newly created bitmap of the same | |||
| dimensions and color space as the previous bitmap, and with its pixels initialized to | |||
| <span>transparent black</span>, or <span>opaque black</span> if the rendering context's <span | |||
| data-x="offscreencontext2d-alpha">alpha</span> flag is set to false.</p> | |||
| data-x="concept-canvas-alpha">alpha</span> flag is set to false.</p> | |||
There was a problem hiding this comment.
| data-x="concept-canvas-alpha">alpha</span> flag is set to false.</p> | |
| data-x="concept-canvas-alpha">alpha</span> is false.</p> |
(it's not defined as a flag (and shouldn't be as we have moved away from calling things flags))
There was a problem hiding this comment.
Incorporated this suggestion.
|
cc @whatwg/canvas |
|
Let me know if there's anything I should do to move this along. The PRs for canvas-float support and HDR tone mapping both build on top of this, since they live in |
|
If @whatwg/canvas has no further comments and OP gets filled out I can merge this Monday. |
Sorry for the potentially clueless question, but is what is OP and is it something I should be doing? |
|
I think it stands for "original post" or something like that. For this PR I'm expecting you to file an MDN issue and a WebKit bug and link them from #10904 (comment). Other people can do this as well, but generally we leave it to the person who created the PR to organize that. |
There was a problem hiding this comment.
This looks good, thanks @ccameron-chromium! I've also pushed up some wrapping nits found with specfmt, and I also found another bug with that tool.
|
Just to note, this isn't only an editorial change and I don't think any other implementer has responded yet. The checkbox in the OP shouldn't be checked yet. |
|
Good call. I think in this case we can assume Chrome given OP and I will say yes for WebKit. Unless Gecko or anyone else objects I think we're good here. |
There is a lot of duplicated spec text for
CanvasRenderingContext2DandOffscreenCanvasRenderingContext2D, especially related to howCanvasRenderingContext2DSettingsis handled.As with all things that are duplicated, there are accidental divergences. In particular, several members of
CanvasRenderingContext2DSettingsare accidentally ignored in theOffscreenCanvasRenderingContext2Dspec text. Additionally, thegetContextAttributesmethod was accidentally not added toOffscreenCanvasRenderingContext2Dwhen it was added toCanvasRenderingContext2D.This creates a new mixin interface CanvasSettings. This will unify the accidentally diverged paths, and also prevent this from happening in the future. See details here.
An archeology of where the accidental divergence came from is in this issue. To the extent that original authors are still contact-able, they have confirmed that the divergences were indeed accidental oversights.
/canvas.html ( diff )
/index.html ( diff )