Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove SVG Blob support from createImageBitmap #10172

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Kaiido
Copy link
Member

@Kaiido Kaiido commented Mar 2, 2024

This got added in #972 but never has been implemented since then.
It seems that supporting SVG in Workers is a no-go for most implementers as this would mean at least some DOM would need to be available in these contexts.

  • At least two implementers are interested (and none opposed):
    • They all already do fire an InvalidStateError in such a case, so I guess there is agreement for not changing their behavior. Though we should probably wait for confirmation.
  • Tests are written and can be reviewed and commented upon at:
    • Tests for the inverse behavior have been removed in https://chromium.googlesource.com/chromium/src/+/c09f91b2ff9b32da19ac09f02b721688c8c33fa8 but the reasoning isn't available to me, and no replacement test has been provided then. I'm not entirely sure where the tests for throwing on SVG Blobs should live... Canvas tests are still messy in WPT (and the fact I can't run the builder on my machine doesn't help at all). If someone is ready to take over on that one, I'd be grateful.
  • Implementation bugs are filed:
    • Chromium: No change needed
    • Gecko: No change needed
    • WebKit: No change needed
  • MDN issue is filed: They currently don't say nothing about Blob with no intrinsic size throwing, but maybe it can be a good idea to note there that SVG Blobs do throw. I'll wait for feedback here before doing so.
  • The top of this comment includes a clear commit message to use.

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


/imagebitmap-and-animations.html ( diff )

This got added in whatwg#972 but never has been implemented since then.
Supporting SVG in Workers is a no-go for most implementers as this would mean at least some DOM would need to be available in these contexts.
@Kaiido
Copy link
Member Author

Kaiido commented Mar 2, 2024

cc @whatwg/canvas and particularly @yiyix since you did remove the previous tests.

@annevk
Copy link
Member

annevk commented Mar 3, 2024

This seems fine to me, although it seems surprising it wouldn't be supported anywhere. This is not too different from using an SVG data: URL in an img element for instance.

I think it would be okay to add parallel tests for non-support if the canvas harness is too frustrating, although I suspect @fserb probably knows how to get it working.

@annevk annevk added the removal/deprecation Removing or deprecating a feature label Mar 3, 2024
@Kaiido
Copy link
Member Author

Kaiido commented Mar 4, 2024

although it seems surprising it wouldn't be supported anywhere. This is not too different from using an SVG data: URL in an img element for instance.

Yes, I suppose it could be implemented in a Window context. The issue is with Worker contexts, where you don't have access to the DOM at all and thus can't really load an SVG image since these rely on a DOM.

So indeed, another possibility would be to add SVG Blob's support only for Window contexts, but maybe that makes the API more complex for something users can anyway workaround by passing an <img>. Still, I personally wouldn't mind if implementers were willing to add support only there. I just did lean to the status quo.

@annevk
Copy link
Member

annevk commented Mar 4, 2024

Here's an interesting test:

type is set to image/svg+xml but the blob decodes as a PNG. I strongly suspect this will not be rejected today. But that seems worth fixing at a minimum.

@Kaiido
Copy link
Member Author

Kaiido commented Mar 4, 2024

Oh, very good point...

There is actually already a test for a similar case where a Blob with a "text/html" MIME type but actually containing a PNG image works fine. The same test with an SVG MIME also passes everywhere today.

But with the change it should throw because the MIME sniffing algo would return "image/svg+xml" without actually sniffing since it's an XML type.
This means throwing on the type as I did here doesn't work...

@annevk
Copy link
Member

annevk commented Mar 4, 2024

I think throwing on the SVG type is probably what we want though. That's a small change that would allow us to introduce this in the future if we wanted to and makes it more consistent with how the img element works.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
removal/deprecation Removing or deprecating a feature topic: canvas
Development

Successfully merging this pull request may close these issues.

None yet

2 participants