Closed Bug 1045532 Opened 5 years ago Closed 5 years ago

picture element: selection by type does not work

Categories

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

33 Branch
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla36

People

(Reporter: koch, Assigned: johns)

References

Details

Attachments

(3 files, 1 obsolete file)

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:30.0) Gecko/20100101 Firefox/30.0 (Beta/Release)
Build ID: 20140605174243

Steps to reproduce:

1. Use Aurora 33.0a2 (2014-07-26)
2. switch on dom.image.picture.enabled
3. Load http://waldbaer.leute.server.de/picture/
4. See test 'picture: type selection' (this is the third test on the page)
5. read currentSrc


Actual results:

currentSrc is bee_480.webp
No image shown


Expected results:

currentSrc should be bee_480.jpg
Image shown

As Firefox does not support WEBP (AFAIK), the selection algo should select the JPEG image.
Assignee: nobody → jschoenick
Status: UNCONFIRMED → ASSIGNED
Component: General → DOM
Ever confirmed: true
OS: Windows 7 → All
Hardware: x86_64 → All
Per the "update the image data" steps, we can just drop this alongside media
Attachment #8502051 - Flags: review?(bzbarsky)
This should trigger a mutation identical to media changing -- and since we re-evaluate all <sources> when either happen, we don't actually need to be passing what they changed to around here
Attachment #8502053 - Flags: review?(bzbarsky)
Just drops the todo from the bug 1037643 tests

Bug 1023519 <picture> tests will cover the non-mutation tests of this
Attachment #8502055 - Flags: review?(bzbarsky)
Comment on attachment 8502051 [details] [diff] [review]
Part 1 - Drop <picture> sources with unsupported types.

>+!imgLoader::SupportImageWithMimeType(NS_LossyConvertUTF16toASCII(type).get())) {

This is wrong.  Say "type" were "image/giŦ".  That last character is U+0166.  After the lossy conversion, you'd get "image/gif", because it would just drop the high byte, ending up with U+0066, which is "f".

What you want here is a conversion to UTF-8, not ASCII.  Please add a testcase.

Also add some tests that check that "image/GIF" works but "İmage/gif" does not (that first char in the last string is U+0130 LATIN CAPITAL LETTER I WITH DOT ABOVE).

r=me with the lossiness fixed.
Attachment #8502051 - Flags: review?(bzbarsky) → review+
Comment on attachment 8502053 [details] [diff] [review]
Part 2 - Trigger <picture> source selection when a previous source type is updated

r=me
Attachment #8502053 - Flags: review?(bzbarsky) → review+
Comment on attachment 8502055 [details] [diff] [review]
Update picture mutations test

r=me
Attachment #8502055 - Flags: review?(bzbarsky) → review+
(In reply to Boris Zbarsky [:bz] from comment #5)
> >+!imgLoader::SupportImageWithMimeType(NS_LossyConvertUTF16toASCII(type).get())) {
>
> This is wrong.  Say "type" were "image/giŦ".  That last character is U+0166.
> After the lossy conversion, you'd get "image/gif", because it would just
> drop the high byte, ending up with U+0066, which is "f".
> What you want here is a conversion to UTF-8, not ASCII.  Please add a
> testcase.
>
> Also add some tests that check that "image/GIF" works but "İmage/gif" does
> not (that first char in the last string is U+0130 LATIN CAPITAL LETTER I
> WITH DOT ABOVE).

You're right, I was looking at if the mime type service expected UTF8 data and saw this pattern being used in other places -- and was under the impression NS_LossyConver* lossy convert was "İmage/gif -> ?mage/gif" or similar. Fixed here, and I'll add unicode variables to the other type tests in bug 1023519
Attachment #8502051 - Attachment is obsolete: true
Attachment #8505105 - Flags: review+
> and was under the impression NS_LossyConver* lossy convert was "İmage/gif -> ?mage/gif" 

For that case, the 'İ' would become U+30, which is '0'.  So even with the lossy conversion you would have been OK; the danger with 'İ' is the opposite: doing full-on Unicode case-insensitive comparisons instead of ASCII-only ones.
While this might be a little bit offtopic, I'm curious, wether and which mime type you do provide to distinguish between apng and normal png support.
Works correctly in my test case with Firefox Developer Exidition. Thanks for fixing.
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.