Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ensure HEIC files selectable from “Upload” button #66292

Merged
merged 5 commits into from
Nov 4, 2024

Conversation

adamsilverstein
Copy link
Member

@adamsilverstein adamsilverstein commented Oct 21, 2024

Fixes #66291.

This may be an upstream Chrome bug, see https://core.trac.wordpress.org/ticket/62268

Copy link

github-actions bot commented Oct 21, 2024

The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the props-bot label.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: adamsilverstein <[email protected]>
Co-authored-by: Mamaduka <[email protected]>
Co-authored-by: swissspidy <[email protected]>
Co-authored-by: ironprogrammer <[email protected]>
Co-authored-by: ndiego <[email protected]>
Co-authored-by: afercia <[email protected]>
Co-authored-by: getdave <[email protected]>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

@ironprogrammer
Copy link
Contributor

Thank you for the PR, @adamsilverstein!

In my testing, this resolves the issue, and allows HEIC files to be selected for upload on the post screen. ✅

@swissspidy
Copy link
Member

For testing, what if the server doesn‘t support HEIC?

@andrewserong andrewserong added [Type] Bug An existing feature does not function as intended [Feature] Media Anything that impacts the experience of managing media [Block] Image Affects the Image Block Backport to WP 6.7 Beta/RC Pull request that needs to be backported to the WordPress major release that's currently in beta labels Oct 22, 2024
@ndiego
Copy link
Member

ndiego commented Oct 22, 2024

For testing, what if the server doesn‘t support HEIC?

Yeah, while this fix addresses the situation where an HEIC image is not selectable, there is still an issue with HEIC images being uploadable when they shouldn't be.

In the video below, I tested in Playground, which does not include ImageMagick. In this situation, I don't think you should be able to upload an HEIC image. Now, it's important to note that this behavior currently exists in 6.6.

So, it's not a regression, but it's still an unfortunate bug that will become more noticeable once 6.7 is out and people start trying to use HEIC images when they are not supported.

heic-bug.mp4

@afercia
Copy link
Contributor

afercia commented Oct 22, 2024

Related: #66293

@Mamaduka
Copy link
Member

Instead of updating all individual uses, maybe we should enhance the FormFileUpload component when the accept prop contains image/*. I believe all accept props from different components are passed down to it at the end.

@adamsilverstein
Copy link
Member Author

Instead of updating all individual uses, maybe we should enhance the FormFileUpload component when the accept prop contains image/*. I believe all accept props from different components are passed down to it at the end.

That makes sense and it would be easier to revert if/when Chrome lands the fix in stable as well. I looked at that originally, but since FormFileUpload could be used elsewhere there would need to be added logic to only add the type when uploading images as you said, adding some complexity.

I can rework with that approach.

@adamsilverstein
Copy link
Member Author

adamsilverstein commented Oct 22, 2024

In the video below, I tested in Playground, which does not include ImageMagick. In this situation, I don't think you should be able to upload an HEIC image. Now, it's important to note that this behavior currently exists in 6.6.

You shouldn't be able to upload HEIC files when the server doesn't support them. This is a completely separate issue from being able to select HEIC images in the file upload button though.

It looks like core media handles this correctly, but not in Gutenberg. This issue should address that: #66293

@Mamaduka
Copy link
Member

@adamsilverstein, a bit of naive logic below, but I think it should do the trick.

// @todo: Explain the need for the `image/heic` type and link the Chrome bug.
const compatAccept = !! accept?.includes( 'image/*' )
	? `${ accept }, image/heic`
	: accept;

@swissspidy
Copy link
Member

This should probably also add image/heif btw

@adamsilverstein
Copy link
Member Author

This should probably also add image/heif btw

We can, although I have yet to locate an image with the mime type image/heif...

@swissspidy
Copy link
Member

There was just one added to https://core.trac.wordpress.org/ticket/62272 yesterday

@adamsilverstein
Copy link
Member Author

Note: this has been fixed upstream in Chromium; I'm not sure how long that fix takes to make it to the Stable channel.

https://chromium-review.googlesource.com/c/chromium/src/+/5967494

@getdave
Copy link
Contributor

getdave commented Nov 4, 2024

Testing this now with a view to bring into the packages release for the final RC3 of WP 6.7.

Copy link
Contributor

@getdave getdave left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tested this and confirmed that this allows selection of HEIC images. The rationale for the fix is good and I think we should bring this into WordPress 6.7.

@getdave getdave merged commit c5921d7 into WordPress:trunk Nov 4, 2024
64 of 65 checks passed
@github-actions github-actions bot added this to the Gutenberg 19.7 milestone Nov 4, 2024
@github-actions github-actions bot removed the Backport to WP 6.7 Beta/RC Pull request that needs to be backported to the WordPress major release that's currently in beta label Nov 4, 2024
gutenbergplugin pushed a commit that referenced this pull request Nov 4, 2024
* Ensure HEIC files selectable from “Upload” button

* Update packages/components/src/form-file-upload/index.tsx

Co-authored-by: George Mamadashvili <[email protected]>

* move “image/heic” addition to FormFileUpload component

* add image/heif

---------

Co-authored-by: adamsilverstein <[email protected]>
Co-authored-by: Mamaduka <[email protected]>
Co-authored-by: swissspidy <[email protected]>
Co-authored-by: ironprogrammer <[email protected]>
Co-authored-by: ndiego <[email protected]>
Co-authored-by: afercia <[email protected]>
Co-authored-by: getdave <[email protected]>
Copy link

github-actions bot commented Nov 4, 2024

I just cherry-picked this PR to the wp/6.7 branch to get it included in the next release: 5d0412a

@github-actions github-actions bot added the Backported to WP Core Pull request that has been successfully merged into WP Core label Nov 4, 2024
karthick-murugan pushed a commit to karthick-murugan/gutenberg that referenced this pull request Nov 13, 2024
* Ensure HEIC files selectable from “Upload” button

* Update packages/components/src/form-file-upload/index.tsx

Co-authored-by: George Mamadashvili <[email protected]>

* move “image/heic” addition to FormFileUpload component

* add image/heif

---------

Co-authored-by: adamsilverstein <[email protected]>
Co-authored-by: Mamaduka <[email protected]>
Co-authored-by: swissspidy <[email protected]>
Co-authored-by: ironprogrammer <[email protected]>
Co-authored-by: ndiego <[email protected]>
Co-authored-by: afercia <[email protected]>
Co-authored-by: getdave <[email protected]>
cbravobernal added a commit that referenced this pull request Nov 19, 2024
cbravobernal added a commit that referenced this pull request Nov 20, 2024
…Safari (#67139)

* Revert "Ensure HEIC files selectable from “Upload” button (#66292)"

This reverts commit c5921d7.

* Update changelog

* Make it Safari conditional

* Remove extra whitespaces

* Update changelog

* Use globalthis

* Forgot a #

* Make it safer

Co-authored-by: cbravobernal <[email protected]>
Co-authored-by: mirka <[email protected]>
Co-authored-by: azaozz <[email protected]>
Co-authored-by: desrosj <[email protected]>
Co-authored-by: Mamaduka <[email protected]>
Co-authored-by: jsnajdr <[email protected]>
cbravobernal added a commit that referenced this pull request Nov 20, 2024
…Safari (#67139)

* Revert "Ensure HEIC files selectable from “Upload” button (#66292)"

This reverts commit c5921d7.

* Update changelog

* Make it Safari conditional

* Remove extra whitespaces

* Update changelog

* Use globalthis

* Forgot a #

* Make it safer

Co-authored-by: cbravobernal <[email protected]>
Co-authored-by: mirka <[email protected]>
Co-authored-by: azaozz <[email protected]>
Co-authored-by: desrosj <[email protected]>
Co-authored-by: Mamaduka <[email protected]>
Co-authored-by: jsnajdr <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Backported to WP Core Pull request that has been successfully merged into WP Core [Block] Image Affects the Image Block [Feature] Media Anything that impacts the experience of managing media [Type] Bug An existing feature does not function as intended
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Chrome: Unable to upload HEIC image through post editor
8 participants