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.

Sign up for GitHub

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
Merged

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

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
6d288f1
Revert "Ensure HEIC files selectable from “Upload” button (#66292)"
cbravobernal Nov 19, 2024
97b802f
Update changelog
cbravobernal Nov 19, 2024
13d6ec4
Make it Safari conditional
cbravobernal Nov 19, 2024
544b2ec
Remove extra whitespaces
cbravobernal Nov 19, 2024
2b71be4
Update changelog
cbravobernal Nov 19, 2024
21a1cb0
Use globalthis
cbravobernal Nov 20, 2024
aa2c911
Forgot a #
cbravobernal Nov 20, 2024
460ece4
Make it safer
cbravobernal Nov 20, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions packages/components/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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


### Deprecations

- `Radio`: Deprecate 36px default size ([#66572](https://github.com/WordPress/gutenberg/pull/66572)).
Expand Down
15 changes: 11 additions & 4 deletions packages/components/src/form-file-upload/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
/**
* WordPress dependencies
*/
import { useRef } from '@wordpress/element';
import { useRef, useState, useEffect } from '@wordpress/element';

/**
* Internal dependencies
Expand Down Expand Up @@ -50,9 +50,16 @@ export function FormFileUpload( {
// @todo: Temporary fix a bug that prevents Chromium browsers from selecting ".heic" files
// from the file upload. See https://core.trac.wordpress.org/ticket/62268#comment:4.
// This can be removed once the Chromium fix is in the stable channel.
const compatAccept = !! accept?.includes( 'image/*' )
? `${ accept }, image/heic, image/heif`
: accept;
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 •
edited
Loading

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?

const compatAccept =
! isSafari && !! accept?.includes( 'image/*' )
? `${ accept }, image/heic, image/heif`
: accept;

return (
<div className="components-form-file-upload">
Expand Down
Loading