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

[Bug]: rrweb is not reverting "crossorigin" attribute changes made to "img" elements #904

Closed
1 task done
dkozlovskyi opened this issue May 27, 2022 · 3 comments · Fixed by #906
Closed
1 task done
Labels
bug Something isn't working

Comments

@dkozlovskyi
Copy link
Contributor

Preflight Checklist

  • I have searched the issue tracker for a bug report that matches the one I want to file, without success.

What package is this bug report for?

rrweb

Expected Behavior

rrweb sets the original "crossorigin" attribute value or removes it in case it was empty before the recording

Actual Behavior

rrweb is referencing to the attribute in CamelCase ("crossOrigin"), as the result it can not preserve and revert the original value.

Testcase Gist URL

No response

Additional Information

Steps to reproduce:

  1. Go to https://drive.google.com
  2. Start recording
  3. Check the broken thumbnails.

Fix suggestion
reference to the case-sensitive "crossorigin" attribute instead of "crossOrigin".

@dkozlovskyi dkozlovskyi added the bug Something isn't working label May 27, 2022
@Juice10
Copy link
Contributor

Juice10 commented May 30, 2022

Hi @dkozlovskyi, thanks for submitting the bug report!
I looked through drive and I could only find this element with crossorigin on it, is this the one you are having issues with?
image

I looked through the rrweb source code and noticed rrweb isn't changing any of the cases. As far as I know every attribute is automatically lowercased by the browser before it is returned. Example below:

const el = document.createElement('div');
el.setAttribute('MYATTR', 'upcase?')
console.log(el.attributes[0].name) //=> 'myattr'

Could you share your events.json file so we can see what is going on?

@dkozlovskyi
Copy link
Contributor Author

Thanks for your response.

rrweb is changing value of attribute in this line and reverting back original value here.

Setting crossorigin attribute is case-insensitive because it always puts the value into the lower case. However, reading the attribute is case-sensitive. Below you can see code example that you can run in the browser:

const img = document.createElement("img")
img.src = "https://raw.githubusercontent.com/github/explore/78df643247d429f6cc873026c0622819ad797942/topics/github/github.png"
document.body.append(img)
console.log(img); // <img src=​"https:​/​/​raw.githubusercontent.com/​github/​explore/​78df643247d429f6cc873026c0622819ad797942/​topics/​github/​github.png">​

img.crossOrigin = "anonymous";
console.log(img); // <img src=​"https:​/​/​raw.githubusercontent.com/​github/​explore/​78df643247d429f6cc873026c0622819ad797942/​topics/​github/​github.png" crossorigin=​"anonymous">​

delete img.crossOrigin // true
console.log(img) // <img src=​"https:​/​/​raw.githubusercontent.com/​github/​explore/​78df643247d429f6cc873026c0622819ad797942/​topics/​github/​github.png" crossorigin=​"anonymous">​

Video Recording
On the attached video recording, you can see that the crossorigin="anonymous" attribute value is not being reverted to the original value as it should be due to the case sensitivity problem.

rrweb-crossorigin-issue-recording.mov

Please correct me if I'm wrong.

Juice10 added a commit that referenced this issue May 30, 2022
Properly remove crossorigin attribute
@Juice10 Juice10 mentioned this issue May 30, 2022
@Juice10
Copy link
Contributor

Juice10 commented May 30, 2022

Thanks for clarifying that, adding a video and pointing to exactly the lines in the rrweb source code that are causing this issue @dkozlovskyi! You made the fix quite easy, here it is: #906

Yuyz0112 pushed a commit that referenced this issue May 31, 2022
Properly remove crossorigin attribute
Juice10 added a commit that referenced this issue Jun 6, 2022
Properly remove crossorigin attribute
Yuyz0112 added a commit that referenced this issue Jun 6, 2022
* Speed up snapshotting of many new dom nodes

By avoiding reflow we shave about 15-25% off our snapshotting time

* Improve newlyAddedElement docs

* Optimize needMaskingText by using el.closest and less recursion

* Serve all rrweb dist files

* Split serializeNode into smaller functions

Makes it easier to profile

* Slow down cpu enhance tracing on fast machines

* Increase timeout

* Perf: only loop through ancestors when they have something to compare to

* Perf: `hasNode` is cheaper than `getMeta`

* Perf: If parents where already checked, no need to do it again

* Perf: reverse for loops are faster

Because they only do the .lenght check once. In this case I don't think we'll see much performance gains if any

* Clean up code

* Perf: check ancestors once with isBlocked

* guessing this might fixes canvas test

* Update packages/rrweb/src/record/observers/canvas/webgl.ts

Co-authored-by: yz-yu <[email protected]>

* Fix #904 (#906)

Properly remove crossorigin attribute

* Bump minimist from 1.2.5 to 1.2.6 (#902)

Bumps [minimist](https://github.com/substack/minimist) from 1.2.5 to 1.2.6.
- [Release notes](https://github.com/substack/minimist/releases)
- [Commits](https://github.com/substack/minimist/compare/1.2.5...1.2.6)

---
updated-dependencies:
- dependency-name: minimist
  dependency-type: indirect
...

Signed-off-by: dependabot[bot] <[email protected]>

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

Co-authored-by: yz-yu <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
2 participants