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

FormFileUpload: Prevent HEIC and HEIF files from always being uploaded on Safari #67139

Merged
merged 8 commits into from
Nov 20, 2024

Conversation

cbravobernal
Copy link
Contributor

@cbravobernal cbravobernal commented Nov 19, 2024

What?

One fix could be reverting this commit c5921d7.

Adding this compatibility is causing the bug reported in https://core.trac.wordpress.org/ticket/62447

The bug reports that uploading a transparent PNG by using the upload button on a Safari browser is renaming the filename and removing the transparent background.

We can make it conditional for non Safari browsers only.

Testing Instructions

Image: charmander

In Chrome or Firefox, create a group, set a background color, upload a block image and use the upload button to upload you're transparent PNG. Check on the frontend that the image uploaded is a transparent PNG.

Do the same in Safari. Check on the frontend that the image uploaded is a transparent PNG.

Screenshots or screencast

Screenshot 2024-11-19 at 21 53 50 Screenshot 2024-11-19 at 21 54 21

@cbravobernal cbravobernal 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 labels Nov 19, 2024
@cbravobernal cbravobernal changed the title Revert "Ensure HEIC files selectable from “Upload” button (#66292)" Revert "Ensure HEIC files selectable from “Upload” button" Nov 19, 2024
@cbravobernal cbravobernal changed the title Revert "Ensure HEIC files selectable from “Upload” button" Image Block: Fix transparent pngs not being uploaded correctly on Safari browser. Nov 19, 2024
@cbravobernal cbravobernal added the Backport to WP Minor Release Pull request that needs to be backported to a WordPress minor release label Nov 19, 2024
@cbravobernal cbravobernal self-assigned this Nov 19, 2024
@cbravobernal cbravobernal marked this pull request as ready for review November 19, 2024 21:31
Copy link

github-actions bot commented Nov 19, 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: cbravobernal <cbravobernal@git.wordpress.org>
Co-authored-by: mirka <0mirka00@git.wordpress.org>
Co-authored-by: azaozz <azaozz@git.wordpress.org>
Co-authored-by: desrosj <desrosj@git.wordpress.org>
Co-authored-by: Mamaduka <mamaduka@git.wordpress.org>
Co-authored-by: jsnajdr <jsnajdr@git.wordpress.org>

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

Comment on lines 53 to 58
const [ isSafari, setIsSafari ] = useState( false );
useEffect( () => {
setIsSafari(
/^((?!chrome|android).)*safari/i.test( navigator.userAgent )
);
}, [] );
Copy link
Contributor Author

Choose a reason for hiding this comment

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

If I do a similar thing to get Safari like in duotone.js.

const isSafari =
	window?.navigator.userAgent &&
	window.navigator.userAgent.includes( 'Safari' ) &&
	! window.navigator.userAgent.includes( 'Chrome' ) &&
	! window.navigator.userAgent.includes( 'Chromium' );

eslint alerts: Use of DOM global 'window' is forbidden in module scope.

Also, checking the userAgent with a variable will trigger:
Use of DOM global 'navigator' is forbidden in the render-cycle of a React FC, consider moving this inside useEffect().

I'm not really convinced this is a good fix.
Pinging @WordPress/gutenberg-components for a quality check 😅

Copy link
Member

Choose a reason for hiding this comment

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

This seems to me like a reasonable and unavoidable bandaid fix in this case 👍

Copy link
Contributor

@azaozz azaozz Nov 20, 2024

Choose a reason for hiding this comment

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

May be wrong but moving the const isSafari = ... to the top of the file (after the imports) won't trigger the error? If not, there seems to be a function isSafari() in the build, imported from @ariakit? Not sure if/how that can be used though.

Copy link
Member

Choose a reason for hiding this comment

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

The point of executing that in a useEffect is so we don't break SSR use cases.

there seems to be a function isSafari() in the build, imported from https://github.com/ariakit?

Good find! But that isn't really a public API and the underlying detection mechanisms seem similar anyway. We can always encapsulate it ourselves for reuse if we need this kind of browser detection more.

Copy link
Contributor

Choose a reason for hiding this comment

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

Right, browser detection from userAgent is generally not recommended. This seems to be the second place it's needed though, after duotone.js. This may be pretty short-lived considering one of the causes for it is already fixed in Chromium and is in Chrome Canary. Seems WP 6.7.1 may end up with reverting this fix completely.

Copy link
Member

Choose a reason for hiding this comment

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

The point of executing that in a useEffect is so we don't break SSR use cases.

Seems like a weird hack for SSR :( Btw, using lazy initialization for state silences the warnings.

const [ isSafari ] = useState( () =>
	/^((?!chrome|android).)*safari/i.test( navigator?.userAgent )
);

Copy link
Member

Choose a reason for hiding this comment

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

The goal here is to reference window or navigator in a safe way, so that it doesn't crash when run in Node.js which doesn't have these globals.

The ssr-friendly ESLint plugin is not perfect and it both accepts code that is bad and rejects code that is good.

This is OK but ssr-friendly will complain:

const isSafari = typeof window !== 'undefined' ? window.navigator.userAgent.includes(...) : false;

And this @Mamaduka's suggestion will crash because the useState initializer is called during SSR. ssr-friendly just doesn't detect it:

const [ isSafari ] = useState( () => window.navigator.userAgent.includes(...) );

Be careful that this will also crash in Node.js despite the optional chaining:

window?.navigator

Because the window global doesn't exist, the code will crash even before getting to apply the ?. operator. That's why the typeof window check is done, because typeof can deal with undefined variables.

One of good solutions is also using globalThis:

globalThis.window?.navigator.userAgent
globalThis.navigator?.userAgent

The globalThis variable is always present, and in browsers it has the window and navigator fields, in Node.js it doesn't have them. The ?. operator works here because we're now dealing with fields of an object, not with global variables.

I think we shouldn't need useEffect here. The UA check can be done in one pass, the component doesn't need to be rendered twice.

So, all you need is to find a solution that:

  1. Actually works and doesn't crash, even if ssr-friendly doesn't warn you
  2. Satisfies the ssr-friendly rules. You might need to reshuffle code that is already good, e.g., by wrapping it in a function etc.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @jsnajdr !

I updated with the globalThis approach in 21a1cb0

Can I get a green check?

@cbravobernal cbravobernal requested a review from a team November 19, 2024 21:42
@@ -2,6 +2,10 @@

## Unreleased

### Bug Fixes

- `FormFileUpload`: Image Block: Fix transparent pngs not being uploaded correctly on Safari browser. ([67139](https://github.com/WordPress/gutenberg/pull/67139)).
Copy link
Member

Choose a reason for hiding this comment

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

This is the changelog for the components package, so we need to describe the change in terms of the FormFileUpload component, not for whatever Gutenberg feature it's fixing. Something like this maybe?

Suggested change
- `FormFileUpload`: Image Block: Fix transparent pngs not being uploaded correctly on Safari browser. ([67139](https://github.com/WordPress/gutenberg/pull/67139)).
- `FormFileUpload`: Prevent HEIC and HEIF files from being uploaded on Safari ([67139](https://github.com/WordPress/gutenberg/pull/67139)).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated in 21a1cb0

@mirka mirka added the [Package] Components /packages/components label Nov 20, 2024
Copy link
Contributor

@desrosj desrosj left a comment

Choose a reason for hiding this comment

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

I'm not familiar enough with these parts to review the code actual code changes, but this is testing well for me.

In both Safari and Chrome, I'm able to upload JPGs and HEIC images by both dragging and dropping and using the Upload option to select the files. The accept attribute is correctly reflecting the expected value for each browser.

@cbravobernal cbravobernal changed the title Image Block: Fix transparent pngs not being uploaded correctly on Safari browser. FormFileUpload: Prevent HEIC and HEIF files from being uploaded on Safari Nov 20, 2024
Copy link
Member

@mirka mirka left a comment

Choose a reason for hiding this comment

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

Thank you for the fix!

Just as a note for similar checks in the future though — although this works in this particular case, we need to be careful that we don't assume that the existence of globalThis.navigator implies that we are in a browser context, or that the APIs are the same as a browser. Node v21 adds a partial implementation of window.navigator to globalThis.

: accept;
// Prevent Safari from adding "image/heic" and "image/heif" to the accept attribute.
const isSafari =
globalThis.navigator?.userAgent.includes( 'Safari' ) &&
Copy link
Member

@mirka mirka Nov 20, 2024

Choose a reason for hiding this comment

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

You know what, if I look closely at the Node docs, there is a small range of versions where navigator exists (>= v21.0.0) but navigator.userAgent doesn't (< v21.1.0) 😂 So maybe optional chain that, too. This kind of edge case may already be a reason to prefer approaches that are safer in the general case (e.g. useEffect or globalThis.window?.navigator).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated in 460ece4

@cbravobernal cbravobernal merged commit ebf1149 into trunk Nov 20, 2024
64 checks passed
@cbravobernal cbravobernal deleted the revert/ensure-heic-files-selectable branch November 20, 2024 13:11
@github-actions github-actions bot added this to the Gutenberg 19.8 milestone Nov 20, 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 <cbravobernal@git.wordpress.org>
Co-authored-by: mirka <0mirka00@git.wordpress.org>
Co-authored-by: azaozz <azaozz@git.wordpress.org>
Co-authored-by: desrosj <desrosj@git.wordpress.org>
Co-authored-by: Mamaduka <mamaduka@git.wordpress.org>
Co-authored-by: jsnajdr <jsnajdr@git.wordpress.org>
@cbravobernal
Copy link
Contributor Author

Cherry-picked to wp/6.7 for a minor release.

@desrosj desrosj changed the title FormFileUpload: Prevent HEIC and HEIF files from being uploaded on Safari FormFileUpload: Prevent HEIC and HEIF files from almways being uploaded on Safari Nov 20, 2024
@desrosj desrosj changed the title FormFileUpload: Prevent HEIC and HEIF files from almways being uploaded on Safari FormFileUpload: Prevent HEIC and HEIF files from always being uploaded on Safari Nov 20, 2024
pento pushed a commit to WordPress/wordpress-develop that referenced this pull request Nov 20, 2024
Syncs Editor packages for WordPress 6.7.1 RC1. Includes the following PRs:
- WordPress/gutenberg#66945
- WordPress/gutenberg#66889
- WordPress/gutenberg#67139

Props mmaattiiaass, ramonopoly, mamaduka, get_dave, poena, ntsekouras, mcsf, jsnajdr, 0mirka00, desrosj, joemcgill, cbravobernal, azaozz, room34, mayanktripathi32, im3dabasia1, jonsurrell.

Fixes #62478, #62447. 



git-svn-id: https://develop.svn.wordpress.org/branches/6.7@59437 602fd350-edb4-49c9-b593-d223f7449a82
markjaquith pushed a commit to markjaquith/WordPress that referenced this pull request Nov 20, 2024
Syncs Editor packages for WordPress 6.7.1 RC1. Includes the following PRs:
- WordPress/gutenberg#66945
- WordPress/gutenberg#66889
- WordPress/gutenberg#67139

Props mmaattiiaass, ramonopoly, mamaduka, get_dave, poena, ntsekouras, mcsf, jsnajdr, 0mirka00, desrosj, joemcgill, cbravobernal, azaozz, room34, mayanktripathi32, im3dabasia1, jonsurrell.

Fixes #62478, #62447. 


Built from https://develop.svn.wordpress.org/branches/6.7@59437


git-svn-id: http://core.svn.wordpress.org/branches/6.7@58823 1a063a9b-81f0-0310-95a4-ce76da25c4cd
pento pushed a commit to WordPress/wordpress-develop that referenced this pull request Nov 20, 2024
Syncs Editor packages for WordPress 6.7.1 RC1. Includes the following PRs:

- WordPress/gutenberg#66945
- WordPress/gutenberg#66889
- WordPress/gutenberg#67139 

Reviewed by desrosj.
Merges [59437] to trunk.

Props mmaattiiaass, ramonopoly, mamaduka, get_dave, poena, ntsekouras, mcsf, jsnajdr, 0mirka00, desrosj, joemcgill, cbravobernal, azaozz, room34, mayanktripathi32, im3dabasia1, jonsurrell.
Fixes #62478, #62447. 

git-svn-id: https://develop.svn.wordpress.org/trunk@59438 602fd350-edb4-49c9-b593-d223f7449a82
github-actions bot pushed a commit to platformsh/wordpress-performance that referenced this pull request Nov 20, 2024
Syncs Editor packages for WordPress 6.7.1 RC1. Includes the following PRs:

- WordPress/gutenberg#66945
- WordPress/gutenberg#66889
- WordPress/gutenberg#67139 

Reviewed by desrosj.
Merges [59437] to trunk.

Props mmaattiiaass, ramonopoly, mamaduka, get_dave, poena, ntsekouras, mcsf, jsnajdr, 0mirka00, desrosj, joemcgill, cbravobernal, azaozz, room34, mayanktripathi32, im3dabasia1, jonsurrell.
Fixes #62478, #62447. 
Built from https://develop.svn.wordpress.org/trunk@59438


git-svn-id: https://core.svn.wordpress.org/trunk@58824 1a063a9b-81f0-0310-95a4-ce76da25c4cd
markjaquith pushed a commit to markjaquith/WordPress that referenced this pull request Nov 20, 2024
Syncs Editor packages for WordPress 6.7.1 RC1. Includes the following PRs:

- WordPress/gutenberg#66945
- WordPress/gutenberg#66889
- WordPress/gutenberg#67139 

Reviewed by desrosj.
Merges [59437] to trunk.

Props mmaattiiaass, ramonopoly, mamaduka, get_dave, poena, ntsekouras, mcsf, jsnajdr, 0mirka00, desrosj, joemcgill, cbravobernal, azaozz, room34, mayanktripathi32, im3dabasia1, jonsurrell.
Fixes #62478, #62447. 
Built from https://develop.svn.wordpress.org/trunk@59438


git-svn-id: http://core.svn.wordpress.org/trunk@58824 1a063a9b-81f0-0310-95a4-ce76da25c4cd
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Backport to WP Minor Release Pull request that needs to be backported to a WordPress minor release [Block] Image Affects the Image Block [Feature] Media Anything that impacts the experience of managing media [Package] Components /packages/components [Type] Bug An existing feature does not function as intended
Projects
Development

Successfully merging this pull request may close these issues.

6 participants