Bug 212489 - [macOS] Allow selecting HEIF images if the 'accept' attribute includes an image MIME type that the platform can transcode the HEIF to
Summary: [macOS] Allow selecting HEIF images if the 'accept' attribute includes an ima...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Images (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Said Abou-Hallawa
URL:
Keywords: InRadar
Depends on: 212485
Blocks: 212482 213347
  Show dependency treegraph
 
Reported: 2020-05-28 13:55 PDT by Said Abou-Hallawa
Modified: 2022-10-06 11:44 PDT (History)
16 users (show)

See Also:


Attachments
Patch (21.10 KB, patch)
2020-06-09 12:00 PDT, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Patch for review (12.55 KB, patch)
2020-06-09 12:33 PDT, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Patch (22.05 KB, patch)
2020-06-09 17:35 PDT, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Patch (23.27 KB, patch)
2020-06-09 20:07 PDT, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Patchtch for review (17.04 KB, patch)
2020-06-09 20:14 PDT, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Patch (23.25 KB, patch)
2020-06-10 01:41 PDT, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Patchtch for review (17.55 KB, patch)
2020-06-10 01:48 PDT, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Patch (14.51 KB, patch)
2020-06-11 02:06 PDT, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Patch (19.64 KB, patch)
2020-06-16 19:44 PDT, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Patch (19.54 KB, patch)
2020-06-16 21:04 PDT, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Patch (34.66 KB, patch)
2020-07-01 23:27 PDT, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Patch (35.14 KB, patch)
2020-07-02 00:17 PDT, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Patch (36.09 KB, patch)
2020-07-02 02:21 PDT, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Patch (33.80 KB, patch)
2020-07-03 22:03 PDT, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Patch (33.73 KB, patch)
2020-07-03 22:31 PDT, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Patch (33.80 KB, patch)
2020-07-03 22:37 PDT, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Patch (39.49 KB, patch)
2020-07-04 01:28 PDT, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Patch (23.12 KB, patch)
2020-07-04 02:25 PDT, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Patch (24.34 KB, patch)
2020-07-04 17:00 PDT, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Patch (25.50 KB, patch)
2020-07-05 02:32 PDT, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Said Abou-Hallawa 2020-05-28 13:55:39 PDT
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.
Comment 1 Radar WebKit Bug Importer 2020-05-28 13:56:16 PDT
<rdar://problem/63731679>
Comment 2 Said Abou-Hallawa 2020-06-09 12:00:23 PDT
Created attachment 401459 [details]
Patch
Comment 3 Said Abou-Hallawa 2020-06-09 12:33:16 PDT
Created attachment 401462 [details]
Patch for review
Comment 4 Said Abou-Hallawa 2020-06-09 17:35:56 PDT
Created attachment 401498 [details]
Patch
Comment 5 Said Abou-Hallawa 2020-06-09 20:07:32 PDT
Created attachment 401507 [details]
Patch
Comment 6 Said Abou-Hallawa 2020-06-09 20:14:16 PDT
Created attachment 401509 [details]
Patchtch for review
Comment 7 Said Abou-Hallawa 2020-06-10 01:41:57 PDT
Created attachment 401518 [details]
Patch
Comment 8 Said Abou-Hallawa 2020-06-10 01:48:41 PDT
Created attachment 401519 [details]
Patchtch for review
Comment 9 Said Abou-Hallawa 2020-06-11 02:06:17 PDT
Created attachment 401629 [details]
Patch
Comment 10 Tim Horton 2020-06-11 12:45:36 PDT
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.
Comment 11 Darin Adler 2020-06-12 16:30:41 PDT
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.
Comment 12 Said Abou-Hallawa 2020-06-16 19:44:01 PDT
Created attachment 402072 [details]
Patch
Comment 13 Said Abou-Hallawa 2020-06-16 19:52:31 PDT
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.
Comment 14 Tim Horton 2020-06-16 20:24:59 PDT
(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.
Comment 15 Said Abou-Hallawa 2020-06-16 21:04:25 PDT
Created attachment 402077 [details]
Patch
Comment 16 Said Abou-Hallawa 2020-06-16 21:07:06 PDT
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));
Comment 17 Tim Horton 2020-06-16 21:20:26 PDT
Sure
Comment 18 Said Abou-Hallawa 2020-07-01 22:43:00 PDT
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.
Comment 19 Said Abou-Hallawa 2020-07-01 23:27:10 PDT
Created attachment 403345 [details]
Patch
Comment 20 Said Abou-Hallawa 2020-07-02 00:17:22 PDT
Created attachment 403348 [details]
Patch
Comment 21 Said Abou-Hallawa 2020-07-02 02:21:26 PDT
Created attachment 403356 [details]
Patch
Comment 22 Said Abou-Hallawa 2020-07-03 21:53:33 PDT
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.
Comment 23 Said Abou-Hallawa 2020-07-03 22:03:57 PDT
Created attachment 403508 [details]
Patch
Comment 24 Darin Adler 2020-07-03 22:15:00 PDT
Retitled to what I *think* you mean, Said. Let me know if it’s wrong.
Comment 25 Said Abou-Hallawa 2020-07-03 22:31:42 PDT
Created attachment 403511 [details]
Patch
Comment 26 Said Abou-Hallawa 2020-07-03 22:37:05 PDT
Created attachment 403512 [details]
Patch
Comment 27 Darin Adler 2020-07-03 22:38:33 PDT
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?
Comment 28 Darin Adler 2020-07-03 22:42:29 PDT
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.
Comment 29 Darin Adler 2020-07-03 22:54:07 PDT
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.
Comment 30 Said Abou-Hallawa 2020-07-04 01:28:24 PDT
Created attachment 403516 [details]
Patch
Comment 31 Said Abou-Hallawa 2020-07-04 02:25:02 PDT
Created attachment 403518 [details]
Patch
Comment 32 Darin Adler 2020-07-04 10:36:43 PDT
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.
Comment 33 Said Abou-Hallawa 2020-07-04 17:00:10 PDT
Created attachment 403538 [details]
Patch
Comment 34 Said Abou-Hallawa 2020-07-04 17:05:36 PDT
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.
Comment 35 Said Abou-Hallawa 2020-07-04 17:08:29 PDT
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.
Comment 36 Darin Adler 2020-07-04 19:18:41 PDT
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.
Comment 37 EWS 2020-07-05 01:52:02 PDT
/Volumes/Data/worker/Commit-Queue/build/Source/WebKit/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive).
Comment 38 Said Abou-Hallawa 2020-07-05 02:32:41 PDT
Created attachment 403549 [details]
Patch
Comment 39 EWS 2020-07-05 03:02:06 PDT
Committed r263949: <https://trac.webkit.org/changeset/263949>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 403549 [details].
Comment 40 Micah 2022-10-06 10:18:03 PDT
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.