1106522 - `<picture>` mime type selection doesn't pick SVG

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Yoav Weiss]

`<img>`'s source selection with a `<picture>` parent seems to be able to do MIME type based selection (using the `type` attribute), as we can see in http://jsbin.com/jivavejifu/1

OTOH, it doesn't seem to pick an SVG MIME type (image/xml+svg) using the same logic. See http://ericportis.com/etc/type-svg-test/

In Chrome/Blink there was a similar bug where SVG wasn't considered a supported *image* MIME type, since it's XML. It is now fixed in Chrome 39.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Yeah, in HTMLImageElement::TryCreateResponsiveSelector we call imgLoader::SupportImageWithMimeType which calls Image::GetDecoderType which only knows about jpeg, gif, png, bmp, ico, icon.

That's what other consumers of SupportImageWithMimeType want, because they're switching between image and document (<iframe> and <object>).  But that's not what we want here.

Inside imagelib itself, GetDecoderType is only called from RasterImage, and there is an explicit check for the SVG type in ImageFactory::CreateImage for "image/svg+xml", picking VectorImage if so and RasterImage otherwise.

Seth, do you want to add some sort of argument to SupportImageWithMimeType to indicate whether SVG should be considered an image type, or just add an explicit check for "image/svg+xml" in the HTMLImageElement code?

Comment 2

[Seth Fowler [:seth] [:s2h]]

(In reply to Boris Zbarsky [:bz] from comment #1)
> Seth, do you want to add some sort of argument to SupportImageWithMimeType
> to indicate whether SVG should be considered an image type, or just add an
> explicit check for "image/svg+xml" in the HTMLImageElement code?

I suppose I think it's marginally cleaner to support it in SupportImageWithMimeType. I'll work up a patch.

Comment 3

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 1) - Make imgLoader::SupportImageWithMimeType optionally support image/svg+xml

This needs a test, but I'll go ahead and upload the patch now.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8532161 [details] [diff] [review]
(Part 1) - Make imgLoader::SupportImageWithMimeType optionally support image/svg+xml

An enum would probably be more self-documenting than a boolean argument....

r=me

Comment 5

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 2) - Add a test for SVG-as-image in picture elements

Here's the test. I added two variants of the test, one with a |type| attribute and one without, because it looked to me like the code paths are somewhat different.

Comment 6

[Seth Fowler [:seth] [:s2h]]

Thanks for the review!

(In reply to Boris Zbarsky [:bz] from comment #4)
> Comment on attachment 8532161 [details] [diff] [review]
> (Part 1) - Make imgLoader::SupportImageWithMimeType optionally support
> image/svg+xml
> 
> An enum would probably be more self-documenting than a boolean argument....
> 
> r=me

I was on the fence about it myself; I'll switch it out.

Comment 7

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 1) - Make imgLoader::SupportImageWithMimeType optionally support image/svg+xml

Addressed review comments.

Comment 8

[Seth Fowler [:seth] [:s2h]]

Try job here:

https://tbpl.mozilla.org/?tree=Try&rev=e8f8258737c1

Comment 9

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8532197 [details] [diff] [review]
(Part 2) - Add a test for SVG-as-image in picture elements

r=me.  Thank you!

Comment 10

[Seth Fowler [:seth] [:s2h]]

Thanks for the review! Pushed:

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/019d55dc6783
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/cbba93f5833b

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/019d55dc6783
https://hg.mozilla.org/mozilla-central/rev/cbba93f5833b