Alow setting the crossOrigin attribute for the image transform's image using a filter #28255
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Size Change: +25 B (0%) Total Size: 1.37 MB
ℹ️ View Unchanged
|
gziolo
added
[Block] Image
sirreal
reviewed
Jan 18, 2021
packages/block-library/src/image/image-editing/use-transform-image.js
Outdated
Show resolved
Hide resolved
packages/block-library/src/image/image-editing/use-transform-image.js
Outdated
Show resolved
Hide resolved
packages/block-library/src/image/image-editing/use-transform-image.js
Outdated
Show resolved
Hide resolved
packages/editor/src/components/provider/use-block-editor-settings.js
Outdated
Show resolved
Hide resolved
fullofcaffeine
force-pushed
the
allow/setting-cross-origin-for-image-editor-img-el
branch
from
41e6196 to
01583b6
Compare
noahtallen
reviewed
Jan 19, 2021
I think that behavior is expected. As @ellatrix pointed out to me in a video call, image files are served straight by the server, with no involvement from WordPress. So in order to change their headers, we'd need to edit the server configuration (which is of cource outside the scope of Gutenberg and even WordPress). |
fullofcaffeine
force-pushed
the
allow/setting-cross-origin-for-image-editor-img-el
branch
from
6683d5d to
518341c
Compare
8 tasks
ellatrix
reviewed
Jan 21, 2021
packages/block-library/src/image/image-editing/use-transform-image.js
Outdated
Show resolved
Hide resolved
ellatrix
reviewed
Jan 21, 2021
ellatrix
reviewed
Jan 21, 2021
packages/block-library/src/image/image-editing/use-transform-image.js
Outdated
Show resolved
Hide resolved
sirreal
reviewed
Jan 21, 2021
packages/block-library/src/image/image-editing/use-transform-image.js
Outdated
Show resolved
Hide resolved
fullofcaffeine
changed the title
sirreal
reviewed
Jan 22, 2021
packages/block-library/src/image/image-editing/use-transform-image.js
Outdated
Show resolved
Hide resolved
fullofcaffeine
changed the title
fullofcaffeine
commented
Jan 26, 2021
| undefined, | ||
| url | ||
| ); | ||
| if ( typeof imgCrossOrigin === 'string' ) { |
There was a problem hiding this comment.
I think we could avoid this condition by setting the return value of the filter directly to crossOrigin 🤔
ellatrix
reviewed
Jan 26, 2021
| @@ -90,6 +91,15 @@ function useTransformState( { url, naturalWidth, naturalHeight } ) { | |||
| const el = new window.Image(); | |||
| el.src = url; | |||
| el.onload = editImage; | |||
|
|
|||
| const imgCrossOrigin = applyFilters( | |||
| 'gutenberg.imageTransform.crossOrigin', | |||
There was a problem hiding this comment.
Can this be "media.crossOrigin" or something like that? How will we make sure the "gutenberg" part is updated or removed when this lands in core? Is there a standard way to name filters?
There was a problem hiding this comment.
Is there a place for documentation of filters?
There was a problem hiding this comment.
Let's use media.crossOrigin and document it here https://github.com/WordPress/gutenberg/tree/004a32ec09d5b789be694d85725fe49cca385f08/docs/designers-developers/developers/filters 👍
|
I have no objection for a filter. The only thing maybe missing here is some documentation. |
fullofcaffeine
requested review from
ajitbohra,
chrisvanpatten and
mkaz
as code owners
ellatrix
approved these changes
Jan 26, 2021
sirreal
approved these changes
Jan 26, 2021
Co-authored-by: Jon Surrell <[email protected]>
…nd not the object
…he image editor in the future
…or it in the jsdoc Renamed because it might be used for other kind of assets in the future (i.e videos).
In this case, the logic to to decide the crossOrigin is in the filter function itself.
Co-authored-by: Jon Surrell <[email protected]>
Co-authored-by: Jon Surrell <[email protected]>
Co-authored-by: Jon Surrell <[email protected]>
sirreal
force-pushed
the
allow/setting-cross-origin-for-image-editor-img-el
branch
from
f4e1c75 to
97eb2f1
Compare
|
This is ready to land, the failing tests seem to be failing on the main branch as well. I've rebased and will review test results to look for differences from the main branch failures then merge. |
|
There are three tests failing right now due to a Core regression in https://core.trac.wordpress.org/ticket/50025 inline-tokens, image size and classic block. If these are the only failures right now, we can force merge. |
sirreal
deleted the
allow/setting-cross-origin-for-image-editor-img-el
branch
46 tasks
Closed
6 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Introduce a new (optional) block editor property that can be used to specify the value of a. Allow setting thecrossOriginattribute for the image element created by the image editorcrossOriginattribute for the image element used by the image transform feature using a WP JS filter. This allows it to be set when the app runs in a context where CORS is needed (i.e when the resulting image from the editing process ends up in a server with a domain that differs from the main blog domain) and decouples the logic that decides about the attribute / set it to the right value from Gutenberg.How has this been tested?
This should be useful in any WP environment that serves image assets from a different subdomain.
Reproducing the issue
Here's how I reproduced the issue in a core WP instance (
5.7-alpha-49963) using the Gutenberg build from this branch (I've usedwp-env):First of all, create two local domain mappings in your hosts file:
Go to
http://localhost:8888/wp-admin/options.phpand change the following settings to the following values:upload_pathto/var/www/html/wp-content/uploads;upload_url_pathtohttp://uploads.wp.local:8888/wp-content/uploads;Edit your
wp-config.phpand change the following defines:Now, if you try to add an image and edit to crop it, the browser will complain that it's not allowed because the CORS policy doesn't allow it.
Trying the fix
Activate the hello test plugin and add the following filter to the end of
hello.php:I'm sure there's a better place to add actions and filters that don't require a plugin, if so, let me know!
Test (currently failing, see why)
This should allow the image editing to work if the image is served from a different domain/subdomain. However, it still fails. The problem it seems is that the request that fetches the image** (see below) doesn't include the CORS headers set above. I don't know how to set the headers in WordPress so that it applies to the request that original fetches the image:
As you can see, the actual post.php request does include the headers we've set above:
Types of changes
New feature (non-breaking change which adds functionality)
Checklist: