inlineImages: Setting of image.crossOrigin is not always necessary
#1468
Conversation
🦋 Changeset detectedLatest commit: ba7ca2d The changes in this PR will be included in the next version bump. This PR includes changesets to release 8 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
eoghanmurray
changed the title
image.crossOrigin is not always necessaryimage.crossOrigin is not always necessary
|
Hi, thanks for the PR! I didn't use this project anymore, so I won't test the code on my own. |
Thank you so much for suggesting this; in fact the new code had a bug not present in the prior version which I've fixed here: 236cd54 |
| <source type="image/webp" srcset="assets/img/characters/robot.webp" /> | ||
| <img src="assets/img/characters/robot.png" /> | ||
| </picture> | ||
| <img src="/images/robot.png" alt="This is a robot" /> | ||
| <img src="https://avatars.githubusercontent.com/u/43396833?s=20&v=4" alt="CORS restricted but has access-control-allow-origin: *" /> |
There was a problem hiding this comment.
I was also thinking of using https://www.rrweb.io/favicon.png here but that isn't served with the requisite access-control-allow-origin: * — @Yuyz0112 if you wish to add that header I can update the test to point to our own resource
There was a problem hiding this comment.
You could also check out how we do cross origin tests and do it in a similar fashion:
This uses {SERVER_URL} to which gets replaced with a locally hosted server, and since about:blank is a different origin than that server you get your cross origin image
There was a problem hiding this comment.
ah that's good
There was a problem hiding this comment.
I'll check if I can get our test server to set the access-control headers.
There was a problem hiding this comment.
That's done in d2df9bb
I can't believe it but it worked first time.
There was a problem hiding this comment.
I didn't explicitly set or check that the access-control-origin header is actually being set; but I verified that failing to set the crossorigin="anonymous" on the <img> causes the test to fail
…s, and causes an immediate image reload necessitating the load event listener (albeit from cache)
… by the snapshotting process
… load, as calling `recordInlineImage` immediately will result in an empty data url
…ffline' is also separately checked against a __snapshot__
eoghanmurray
force-pushed
the
inlineImages-crossOrigin
branch
from
61f3115
to
f8cbf9c
Compare
…1468) Setting of the `crossorigin` attribute is not necessary for same-origin images, and causes an immediate image reload (albeit from cache) necessitating the use of a load event listener which subsequently mutates the snapshot. This change allows us to avoid the mutation of the snapshot for the same-origin case. * Modify inlineImages test to remove delay and show that we can inline images without mutation * Add an explicit test for when the `image.crossOrigin = 'anonymous';` method is necessary. Uses a combination of about:blank and our test server to simulate a cross-origin context * Other test changes: there were some spurious rrweb mutations being generated by the addition of the crossorigin attribute that are now elimnated from the rrweb/__snapshots__/integration.test.ts.snap after this PR - this is good
…1468) Setting of the `crossorigin` attribute is not necessary for same-origin images, and causes an immediate image reload (albeit from cache) necessitating the use of a load event listener which subsequently mutates the snapshot. This change allows us to avoid the mutation of the snapshot for the same-origin case. * Modify inlineImages test to remove delay and show that we can inline images without mutation * Add an explicit test for when the `image.crossOrigin = 'anonymous';` method is necessary. Uses a combination of about:blank and our test server to simulate a cross-origin context * Other test changes: there were some spurious rrweb mutations being generated by the addition of the crossorigin attribute that are now elimnated from the rrweb/__snapshots__/integration.test.ts.snap after this PR - this is good
…1468) Setting of the `crossorigin` attribute is not necessary for same-origin images, and causes an immediate image reload (albeit from cache) necessitating the use of a load event listener which subsequently mutates the snapshot. This change allows us to avoid the mutation of the snapshot for the same-origin case. * Modify inlineImages test to remove delay and show that we can inline images without mutation * Add an explicit test for when the `image.crossOrigin = 'anonymous';` method is necessary. Uses a combination of about:blank and our test server to simulate a cross-origin context * Other test changes: there were some spurious rrweb mutations being generated by the addition of the crossorigin attribute that are now elimnated from the rrweb/__snapshots__/integration.test.ts.snap after this PR - this is good
…cessary (rrweb-io#1468)" This reverts commit 4014305.
Setting of
image.crossOriginis not necessary for same origin images, and causes an immediate image reload necessitating the load event listener (albeit from cache)Changeset:
"inlineImages: during snapshot avoid adding an event listener for inlining of same-origin images (async listener mutates the snapshot which can be problematic)"