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.
<rdar://problem/63731679>
Created attachment 401459 [details] Patch
Created attachment 401462 [details] Patch for review
Created attachment 401498 [details] Patch
Created attachment 401507 [details] Patch
Created attachment 401509 [details] Patchtch for review
Created attachment 401518 [details] Patch
Created attachment 401519 [details] Patchtch for review
Created attachment 401629 [details] Patch
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 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.
Created attachment 402072 [details] Patch
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.
(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.
Created attachment 402077 [details] Patch
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));
Sure
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.
Created attachment 403345 [details] Patch
Created attachment 403348 [details] Patch
Created attachment 403356 [details] Patch
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.
Created attachment 403508 [details] Patch
Retitled to what I *think* you mean, Said. Let me know if it’s wrong.
Created attachment 403511 [details] Patch
Created attachment 403512 [details] Patch
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 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 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.
Created attachment 403516 [details] Patch
Created attachment 403518 [details] Patch
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.
Created attachment 403538 [details] Patch
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 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 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.
/Volumes/Data/worker/Commit-Queue/build/Source/WebKit/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive).
Created attachment 403549 [details] Patch
Committed r263949: <https://trac.webkit.org/changeset/263949> All reviewed patches have been landed. Closing bug and clearing flags on attachment 403549 [details].
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.