Bug 212489 – [macOS] Allow selecting HEIF images if the 'accept' attribute includes an image MIME type that the platform can transcode the HEIF to

• Status: RESOLVED
• Resolution: FIXED
• Component: Images

[Said Abou-Hallawa]

If a page has an <input> file element which accepts an image like one of these:

<input type="file" accept="image/*">
<input type="file" accept="image/png, image/jpeg">

And the user clicks it, the File Picker will be launched. This File Picker should allow selecting all the image from the formats as specified in the "accept" attribute. In addition to that, it should also allow selecting the other formats which can be converted to any of the "accept" attribute formats. When selecting an image from unaccepted format, we will convert it to an accepted format. The converted image will be stored in a temporary directory.

[Radar WebKit Bug Importer]

<rdar://problem/63731679>

[Said Abou-Hallawa]

Created attachment 401459 [details]
Patch

[Said Abou-Hallawa]

Created attachment 401462 [details]
Patch for review

[Said Abou-Hallawa]

Created attachment 401498 [details]
Patch

[Said Abou-Hallawa]

Created attachment 401507 [details]
Patch

[Said Abou-Hallawa]

Created attachment 401509 [details]
Patchtch for review

[Said Abou-Hallawa]

Created attachment 401518 [details]
Patch

[Said Abou-Hallawa]

Created attachment 401519 [details]
Patchtch for review

[Said Abou-Hallawa]

Created attachment 401629 [details]
Patch

[Tim Horton]

Comment on attachment 401629 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=401629&action=review

> Source/WebKit/UIProcess/API/Cocoa/WKOpenPanelParameters.mm:159
> +- (NSArray<NSString *> *)_allowedFileExtensionsTitles

Confused. How does localization work at all for this set of strings?

> Source/WebKit/UIProcess/API/Cocoa/WKOpenPanelParameters.mm:172
> +                [titles addObject:[NSString stringWithFormat:@"%@ %@", [subType uppercaseString], type]];

Assuming these should be capitalized (and that appending their top-level type reads correctly) seems pretty bold.

> Source/WebKit/UIProcess/API/Cocoa/WKOpenPanelParametersPrivate.h:35
> +@property (nonatomic, readonly, copy) NSArray<NSString *> *_allImageFileExtensions WK_API_AVAILABLE(macos(10.13.4));

The availability here is incorrect.

[Darin Adler]

Comment on attachment 401629 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=401629&action=review

>> Source/WebKit/UIProcess/API/Cocoa/WKOpenPanelParameters.mm:159
>> +- (NSArray<NSString *> *)_allowedFileExtensionsTitles
> 
> Confused. How does localization work at all for this set of strings?

Said told me when I reviewed a previous version of this patch that this is temporarily non-localized and will be localized after a future check-in. Of course, our traditional way to indicate that would be a FIXME comment.

[Said Abou-Hallawa]

Created attachment 402072 [details]
Patch

[Said Abou-Hallawa]

Comment on attachment 401629 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=401629&action=review

>>> Source/WebKit/UIProcess/API/Cocoa/WKOpenPanelParameters.mm:159
>>> +- (NSArray<NSString *> *)_allowedFileExtensionsTitles
>> 
>> Confused. How does localization work at all for this set of strings?
> 
> Said told me when I reviewed a previous version of this patch that this is temporarily non-localized and will be localized after a future check-in. Of course, our traditional way to indicate that would be a FIXME comment.

I made all strings localizable. Only image, video and audio mime types will be have specific titles in the OpenPanel filter. Other mime types will be shown as "Custom Files".

>> Source/WebKit/UIProcess/API/Cocoa/WKOpenPanelParameters.mm:172
>> +                [titles addObject:[NSString stringWithFormat:@"%@ %@", [subType uppercaseString], type]];
> 
> Assuming these should be capitalized (and that appending their top-level type reads correctly) seems pretty bold.

The top level type will be localized and the sub-types will be capitalized if the length is four characters or less.

>> Source/WebKit/UIProcess/API/Cocoa/WKOpenPanelParametersPrivate.h:35
>> +@property (nonatomic, readonly, copy) NSArray<NSString *> *_allImageFileExtensions WK_API_AVAILABLE(macos(10.13.4));
> 
> The availability here is incorrect.

I removed all the availability macros from all the properties in this file. I think none of them is related to the macOS version.

[Tim Horton]

(In reply to Said Abou-Hallawa from comment #13)
> Comment on attachment 401629 [details]
> Patch
> 
> >> Source/WebKit/UIProcess/API/Cocoa/WKOpenPanelParametersPrivate.h:35
> >> +@property (nonatomic, readonly, copy) NSArray<NSString *> *_allImageFileExtensions WK_API_AVAILABLE(macos(10.13.4));
> > 
> > The availability here is incorrect.
> 
> I removed all the availability macros from all the properties in this file.
> I think none of them is related to the macOS version.

You went the wrong way with this :) It should indicate the first time they were available in the SDK. Put the old ones back, and your new one should have WK_MAC_TBA availability.

[Said Abou-Hallawa]

Created attachment 402077 [details]
Patch

[Said Abou-Hallawa]

Comment on attachment 401629 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=401629&action=review

>>>> Source/WebKit/UIProcess/API/Cocoa/WKOpenPanelParametersPrivate.h:35
>>>> +@property (nonatomic, readonly, copy) NSArray<NSString *> *_allImageFileExtensions WK_API_AVAILABLE(macos(10.13.4));
>>> 
>>> The availability here is incorrect.
>> 
>> I removed all the availability macros from all the properties in this file. I think none of them is related to the macOS version.
> 
> You went the wrong way with this :) It should indicate the first time they were available in the SDK. Put the old ones back, and your new one should have WK_MAC_TBA availability.

I added the following properties in r262895 so they have to have WK_MAC_TBA also:

@property (nonatomic, readonly, copy) NSArray<NSString *> *_allowedFileExtensions WK_API_AVAILABLE(macos(10.13.4));
@property (nonatomic, readonly, copy) NSArray<NSString *> *_allowedFileExtensionsTitles WK_API_AVAILABLE(macos(10.13.4));

[Tim Horton]

Sure

[Said Abou-Hallawa]

The plan for the UI will change:

The file picker will include a checkbox with the text "Select All Files". The default state of this checkbox is off. Here is the rule for selecting and deselecting it:

1. Selected: All files are allowed to be selected. Upon selecting a file, no image transcoding will be performed. The File object will be created with the original image.

2. Not selected: Only files with extensions which are specified in the 'accept' attribute will be allowed to be selected. If the 'accept' attribute includes one of the formats which is allowed for encoding, files with any image extension will be allowed to be selected. Upon selecting an image, we check if mime type of the image is not one of the mime types of 'accept' attribute, the image will be transcoded to the first mime type in the 'accept' attribute and which is allowed for encoding.

[Said Abou-Hallawa]

Created attachment 403345 [details]
Patch

[Said Abou-Hallawa]

Created attachment 403348 [details]
Patch

[Said Abou-Hallawa]

Created attachment 403356 [details]
Patch

[Said Abou-Hallawa]

Then scope of the change will be limited. No UI change will be done. Only allow HEIF/HEIC images to be selected the 'accept' attribute include a mime type which is supported for encoding.

[Said Abou-Hallawa]

Created attachment 403508 [details]
Patch

[Darin Adler]

Retitled to what I *think* you mean, Said. Let me know if it’s wrong.

[Said Abou-Hallawa]

Created attachment 403511 [details]
Patch

[Said Abou-Hallawa]

Created attachment 403512 [details]
Patch

[Darin Adler]

Comment on attachment 403511 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=403511&action=review

Is there a regression test here? I can’t find it.

> Source/WebCore/platform/FileChooserSettings.h:42
> +enum MediaCaptureType {
> +    MediaCaptureTypeNone,
> +    MediaCaptureTypeUser,
> +    MediaCaptureTypeEnvironment
> +};

Seems like this should be an enum class. Obviously we don’t need to make this change right now.

> Source/WebCore/platform/FileChooserSettings.h:50
> +    WEBCORE_EXPORT Vector<String> allowedMIMETypes() const;
> +    WEBCORE_EXPORT Vector<String> allowedFileExtensions() const;

Not sure these should be member functions. Could they just be free functions that take a const FileChooserSettings&? Or maybe static member functions of some class where this policy belongs?

[Darin Adler]

Comment on attachment 403512 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=403512&action=review

> Source/WebCore/platform/FileChooserSettings.h:50
> +    WEBCORE_EXPORT Vector<String> allowedMIMETypes() const;
> +    WEBCORE_EXPORT Vector<String> allowedFileExtensions() const;

I think maybe these belong as MIMETypeRegistry functions.

[Darin Adler]

Comment on attachment 403512 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=403512&action=review

> Source/WebCore/platform/FileChooserSettings.cpp:71
> +        acceptMIMETypes.appendVector(Vector<String>({ "image/heif", "image/heic" }));

Typically we’d want to use _s so we get the ASCIILiteral optimization. Also not sure if appendVector is more efficient than two calls to append.

[Said Abou-Hallawa]

Created attachment 403516 [details]
Patch

[Said Abou-Hallawa]

Created attachment 403518 [details]
Patch

[Darin Adler]

Comment on attachment 403518 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=403518&action=review

> Source/WebCore/platform/MIMETypeRegistry.cpp:796
> +static inline bool shouldAppendNotAcceptedExtensions(const Vector<String>& mimeTypes)

The name here isn’t great. I would not call these "not-accepted extensions".

I think the question this function answers is containsImageMIMETypeForEncoding.

A name for this could be shouldIncludeImageTypesForTranscoding?

> Source/WebCore/platform/MIMETypeRegistry.cpp:828
> +    if (shouldAppendNotAcceptedExtensions(allowedMIMETypes(mimeTypes, extensions))) {
> +        effectiveMIMETypes.append("image/heif"_s);
> +        effectiveMIMETypes.append("image/heic"_s);
> +    }

The correctness of this is not obvious without a comment. And, hardwiring this for all platforms seems incorrect; can all platforms with WebKit convert HEIF and HEIC? Also, long term this should add all images that we know how to encode.

Finally, where is the logic that causes the re-encoding?

> Tools/MiniBrowser/mac/WK2BrowserWindowController.m:-696
> -        [openPanel setAccessoryView:filterView];

Change log doesn’t explain why we no longer need to set an accessory view. I trust that it’s a thoughtful, intentional change, but it’s not obvious to me why it was valuable before and why it’s no longer needed.

[Said Abou-Hallawa]

Created attachment 403538 [details]
Patch

[Said Abou-Hallawa]

Comment on attachment 403518 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=403518&action=review

>> Source/WebCore/platform/MIMETypeRegistry.cpp:796
>> +static inline bool shouldAppendNotAcceptedExtensions(const Vector<String>& mimeTypes)
> 
> The name here isn’t great. I would not call these "not-accepted extensions".
> 
> I think the question this function answers is containsImageMIMETypeForEncoding.
> 
> A name for this could be shouldIncludeImageTypesForTranscoding?

I added two functions to MIMETypeRegistry: (1) preferredImageMIMETypeForEncoding() which returns the first MIME type which CG supports encoding HEIF to. (2) containsImageMIMETypeForEncoding() which returns true if there is such MIME type.

>> Source/WebCore/platform/MIMETypeRegistry.cpp:828
>> +    }
> 
> The correctness of this is not obvious without a comment. And, hardwiring this for all platforms seems incorrect; can all platforms with WebKit convert HEIF and HEIC? Also, long term this should add all images that we know how to encode.
> 
> Finally, where is the logic that causes the re-encoding?

I moved adding the HEIF/HEIC MIME types to OpenPanelParameters::allowedFileExtensions() and I surrounded the code by #if PLATFORM(MAC).

>> Tools/MiniBrowser/mac/WK2BrowserWindowController.m:-696
>> -        [openPanel setAccessoryView:filterView];
> 
> Change log doesn’t explain why we no longer need to set an accessory view. I trust that it’s a thoughtful, intentional change, but it’s not obvious to me why it was valuable before and why it’s no longer needed.

Changing the UI still valuable. But we decided to scope down the change to just fix the HEIF/HEIC case.

[Said Abou-Hallawa]

Comment on attachment 403518 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=403518&action=review

>>> Source/WebCore/platform/MIMETypeRegistry.cpp:828
>>> +    }
>> 
>> The correctness of this is not obvious without a comment. And, hardwiring this for all platforms seems incorrect; can all platforms with WebKit convert HEIF and HEIC? Also, long term this should add all images that we know how to encode.
>> 
>> Finally, where is the logic that causes the re-encoding?
> 
> I moved adding the HEIF/HEIC MIME types to OpenPanelParameters::allowedFileExtensions() and I surrounded the code by #if PLATFORM(MAC).

I will upload this code to bug 213347. It depends on the patch of bug and the patch of bug 213828. So I will add the diff on top of these two patches.

[Darin Adler]

Comment on attachment 403538 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=403538&action=review

> Source/WebKit/UIProcess/API/APIOpenPanelParameters.cpp:80
> +#if PLATFORM(MAC)
> +    auto acceptMIMETypes = m_settings.acceptMIMETypes;
> +
> +    // On macOS allow selecting HEIF/HEIC images if acceptMIMETypes or acceptFileExtensions include at least
> +    // one MIME type which CG supports encoding to.
> +    if (MIMETypeRegistry::containsImageMIMETypeForEncoding(acceptMIMETypes, m_settings.acceptFileExtensions)) {
> +        acceptMIMETypes.append("image/heif"_s);
> +        acceptMIMETypes.append("image/heic"_s);
> +    }
> +    
> +    return API::Array::createStringArray(WebCore::MIMETypeRegistry::allowedFileExtensions(acceptMIMETypes, m_settings.acceptFileExtensions));
> +#else

This is OK for now. Longer term it would be nice if any platform could take advantage of transcoding as part of the upload process, not just macOS.

[EWS]

/Volumes/Data/worker/Commit-Queue/build/Source/WebKit/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive).

[Said Abou-Hallawa]

Created attachment 403549 [details]
Patch

[EWS]

Committed r263949: <https://trac.webkit.org/changeset/263949>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 403549 [details].

[Micah]

Would it be possible to also convert these images to JPEG when dragging and dropping? For instance, on the drop event, to have event.dataList.files contain a JPEG instead of an HEIC?

Right now web apps like befunky.com can open an HEIC image via a file input (due to this fix), but if a user drag and drops an HEIC into the page, it isn't converted to JPEG automatically and thus we can't render it to a canvas or load it in an ImageElement.