Conversation
Handwavy things that need fleshing out are marked with 👋
Kaiido
left a comment
There was a problem hiding this comment.
Glad to see this being worked on, thanks.
Not quite sure how much discussion should be held at this stage. So to note, this doesn't seem to fully match the latest state of https://github.com/WICG/html-in-canvas. e.g. the rename to drawHTMLElement. The layoutsubtree attribute is also missing along with the implications to the existing fallback contents.
Still, thanks for making this move.
|
@Kaiido thank you for the review! I've fleshed things out more, renaming to There's still some handwaving going on of course, in particular what causes the subtree to be laid out but not painted. |
|
I've fleshed this out some more now, in particular the hit testing. |
There was a problem hiding this comment.
One common complain with the use of dictionaries in the Canvas2D API is that this makes GC kick in very often during animations which has a non-negligible performance cost.
This API shape makes a big use of such dictionaries with one for the wrapper CanvasElementHitTestRegion and then a nested one for the CanvasHitTestRect, and I guess there will be scenarios where multiple of these will need to be updated at every frame. Since the values are copied over from the passed objects to new internal objects, it's unclear if even a careful author, who would try to reuse the same objects, could avoid GC at all here.
On the other hand, I really like how this API shape enables future additions like using a Path2D, or even a bitmap mask, instead of a CanvasHitTestRect. (btw can we bikeshed on rect for that purpose?)
It's not my area of expertise, but would an actual exposed interface allow for non copy from JS, so that authors can just update the regions instead of setting new ones?
|
|
||
| <ol> | ||
| <li> | ||
| <p>For each <span>hit test region</span> <var>region</var> in <var>canvas</var>'s <span>hit test regions</span>:</p> |
There was a problem hiding this comment.
Definitely an edge case, and I suppose it's a bit of a gray area (see whatwg/infra#396) but how is this supposed to work if setHitTestRegions is called during this iteration? E.g.
// Add multiple regions
ctx.setHitTestRegions([
{ element, rect: { x, y } },
{ element: anotherElement, rect: { x: anotherX, y: anotherY } }
]);
element.onclick = e => ctx.setHitTestRegions([]); // that was a 'once' handlerShould the anotherElement still perform the hit-test?
There was a problem hiding this comment.
This kind of problem is sometimes handled in the spec by making a frozen copy of the thing to iterate before starting iteration. Do you think that'd be OK here?
There was a problem hiding this comment.
That would certainly be clearer as to what's supposed to happen yes. Now, whether it's the best behavior or not, I don't know and don't have any strong opinion. Both possibilities might come surprising depending on the case. The fact that the timing of hit-testing w.r.t. events propagation isn't well defined doesn't help...
There was a problem hiding this comment.
@szager made a good point here which is that really when we start dispatching events hit testing is already done. So rather than making a copy of the list here, the spec here needs to make clear how the list is used in hit testing and that it all happens before event dispatch.
|
Drive-by comment about styling/layout of the contents of a Canvas being a replaced element doesn't stop selectors from matching its children; that part already works normally. It does prevent us from laying out the children, absent instructions on how to do so. I think what's expected here is that the elements lay out as normal children of the canvas, with the appropriate containments applied by the UA style sheet, right? Like, the canvas is just an ordinary block container as far as they're concerned. We can just specify that, and further say (already present in the spec) that they don't paint, but they do respond to hit-tests, selections, etc as normal. The element snapshot is then them doing a special paint operation. Or does each child lay out independently, as if it's the sole child of the canvas, so that when you draw we can just snapshot from the canvas's 0,0 point and place that at the draw location? That works too, it just needs some clarifying text if so. |
|
Can we get a canvas method that renders CSS anchors at (X,Y) positions as shadowdom elements? Then, canvas children just anchor position to their respective points by name and we get to use actual HTML on canvas instead of images of HTML in canvas? Or is there another use case here that I don't see? |
Hi @AutoSponge , are you asking why we don't use anchor positioning to move elements on top of the canvas, like https://github.com/shuding/cobe? This is a good question, as this API ultimately does need to keep the DOM synchronized with what is being drawn into the canvas. Our goal with this API is to enable patterns that are just not possible today, such as interleaving canvas drawing commands with html (e.g., jelly slider). The explainer lists the use cases this API is solving. |
|
In the explainer, what you're calling the I want to help devs use |
| data-x="dom-context-2d-drawElementImage">drawElementImage()</code>. An <span | ||
| data-x="concept-element-image-snapshot">element image snapshot</span> has a <dfn | ||
| data-x="concept-element-image-snapshot-width">width</dfn> and <dfn | ||
| data-x="concept-element-image-snapshot-height">height</dfn> (dimensions in <span>output |
There was a problem hiding this comment.
This should probably reference the "parent" canvas so that we can link to its output bitmap.
| <li><p>If <var>dw</var> and <var>dh</var> are given and either are infinite or NaN, then | ||
| return.</p></li> | ||
|
|
||
| <li><p>Let <var>snapshot</var> be the result of <span>get element image snapshot</span> given |
There was a problem hiding this comment.
Other Canvas2D API tend to silently not draw rather than throwing. But I don't know what to think about that legacy API design. Here in particular, since it also does return an object and isn't void like e.g. drawImage, it might be better to have it throw.
However I think other invalid attributes should also throw (maybe RangeErrors?) rather than silently return undefined (which will potentially cause throwing lower down the authors' code).
There was a problem hiding this comment.
+1, to specifying that the error conditions throw. here are the associated tests for this. Can you update "get element image snapshot" to throw a InvalidStateError if called prior to paint?
| <span>ImageBitmap</span> <span data-x="dom-OffscreenCanvas-transferToImageBitmap">transferToImageBitmap</span>(); | ||
| <span data-x="idl-Promise">Promise</span><<span>Blob</span>> <span data-x="dom-OffscreenCanvas-convertToBlob">convertToBlob</span>(optional <span>ImageEncodeOptions</span> options = {}); | ||
|
|
||
| <span>DOMMatrix</span> <span data-x="dom-OffscreenCanvas-getElementTransform">getElementTransform</span>((<span>Element</span> or <span>ElementImage</span>) element, <span>DOMMatrix</span> drawTransform); |
There was a problem hiding this comment.
Element should be removed here.
At first I was wondering if it would work with Web-IDL in workers, but that doesn't matter, the get element image snapshot algorithm would anyway not work with an Element since the context is bound to an OffscreenCanvas, not a <canvas>.
|
|
||
| <li><p><span>Assert</span>: <var>element</var> is an <code>Element</code>.</p></li> | ||
|
|
||
| <li><p>Let <var>canvas</var> be the <code>canvas</code> element to which <var>context</var> is |
There was a problem hiding this comment.
Should there be an assert canvas is a <canvas> and not an OffscreenCanvas?
There was a problem hiding this comment.
Oh and context is actually undefined here, unlike draw an element above, this algorithm doesn't take a context input, and can be called from <canvas>'s or OffscreenCanvas's getElementTransform() methods directly, before there is any context.
| <var>element</var>.</p></li> | ||
|
|
||
| <li><p>Return the result of <span>get element transform</span> with <span>this</span>'s <span | ||
| data-x="offscreencanvas-placeholder">placeholder <code>canvas</code> element</span>, |
There was a problem hiding this comment.
What when there is no placeholder?
And even when there is one, can this work synchronously across realms?
|
|
||
| <dd> | ||
| <p>Returns a <span>frozen array</span> of the elements that have changed, or the | ||
| <code>ElementImage</code> objects representing those elements for |
There was a problem hiding this comment.
Does it still fire on OffscreenCanvas?
|
Is it correct that so far only Chromium has shown intent to ship, or are there responses from Firefox/Webkit/… as well? |
| <dl> | ||
| <dt>All content | ||
| <dd><span>CORS-cross-origin</span> data. | ||
| <dd>Non-default colors, fonts, themes, and preferences. |
There was a problem hiding this comment.
What is a non-default font? Or rather, what is a default font? If it must be a font that's available on all platforms, wouldn't that prevent basically all the fonts?
Moreover, aren't fonts already exposed by fillText() (minus a few settings that are already exposed through <foreignObject>)?
Also regarding this paragraph, are all these testable?
There was a problem hiding this comment.
+1, can you remove "fonts" from this list? This is already readily available via fillText/layout.
Also regarding this paragraph, are all these testable?
The cross-origin parts are testable. The theme colors are not testable (web-platform-tests/wpt#29309).
|
|
||
| attribute <span>EventHandler</span> <span data-x="handler-offscreencanvas-oncontextlost">oncontextlost</span>; | ||
| attribute <span>EventHandler</span> <span data-x="handler-offscreencanvas-oncontextrestored">oncontextrestored</span>; | ||
| attribute <span>EventHandler</span> <span data-x="handler-offscreencanvas-onpaint">onpaint</span>; |
There was a problem hiding this comment.
Can you remove the offscreen canvas paint event (here and line 76636)? We can let developers implement this themselves by listening to the canvas element's paint event, and forwarding this information to the offscreen canvas. For example, in the case of an offscreen canvas in a worker thread, the developer can postMessage the updated ElementImage snapshots to the worker thread.
| <ol> | ||
| <li><p><span>Ensure non-nested canvas</span> given the canvas element to which <var>context</var> | ||
| is bound.</p></li> | ||
|
|
There was a problem hiding this comment.
I think we need to specify that we only allow drawing Elements into their parent canvas, and also that we only allow drawing ElementImages into a context of a canvas that they are associated with. (I have a CL in review to implement the latter in blink). Can this be specified with a "validate the Element/ElementImage" step that works for both Element and ElementImage, and that can be referenced by both drawElementImage and getElementTransform? Would this allow us to remove the "Ensure non-nested canvas" section?
There was a problem hiding this comment.
The CL mentions security reasons, could you expand on it?
To me, the sizing issue seems to reflect more that the sizing rules in general are fragile, and I don't see how this solution would handle e.g. the case of a source canvas that's been resized between the time the ElementImage has been generated and the time it's drawn.
|
|
||
| <dl> | ||
| <dt>All content | ||
| <dd><span>CORS-cross-origin</span> data. |
There was a problem hiding this comment.
Can you expand on this? Suggestion: This includes cross-origin data in embedded content (e.g., <iframe>, <img>), references (e.g., background-image, clip-path), <canvas> elements tainted with cross-origin data, and SVG (e.g., <use>, <pattern>, <feImage>). Note that same-origin iframes would still paint, but cross-origin content in them would not.
| <p>If <var>element</var> is an <code>ElementImage</code>:</p> | ||
|
|
||
| <ol> | ||
| <li><p>If <var>element</var>'s <span>[[Detached]]</span> internal slot's value is true, then |
There was a problem hiding this comment.
We need to fix WICG/html-in-canvas#116 in the implementation. Do we need to update the spec for this too?
(See WHATWG Working Mode: Changes for more details.)
/canvas.html ( diff )
/dom.html ( diff )
/index.html ( diff )
/indices.html ( diff )
/infrastructure.html ( diff )
/references.html ( diff )
/rendering.html ( diff )
/webappapis.html ( diff )