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

Fix: shadow dom bugs #1049

Merged
merged 13 commits into from
Jan 10, 2023
Merged

Fix: shadow dom bugs #1049

merged 13 commits into from
Jan 10, 2023

Conversation

Juice10
Copy link
Contributor

@Juice10 Juice10 commented Nov 9, 2022

Fixes a number of shadow dom bugs included bugs related to moving shadow dom parents, and attaching elements to a shadow dom that hasn't been added to a page yet.

  • create test cases
  • fix issues

@Juice10 Juice10 marked this pull request as ready for review November 11, 2022 12:40
Copy link
Member

@YunFeng0817 YunFeng0817 left a comment

Choose a reason for hiding this comment

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

I apologise for my late review. It took me a long time to understand the logic of ProcessedNodeManager as well as the new appended shadow dom tests. They are actually essential and delicate. There are two points which I think can be improved.

packages/rrweb-snapshot/test/rebuild.test.ts Outdated Show resolved Hide resolved
packages/rrweb/src/record/processed-node-manager.ts Outdated Show resolved Hide resolved
@YunFeng0817 YunFeng0817 merged commit 07aa1b2 into master Jan 10, 2023
@YunFeng0817 YunFeng0817 deleted the shadow-dom-bugs branch January 10, 2023 10:51
Vadman97 added a commit to highlight/highlight that referenced this pull request Jan 11, 2023
## Summary

Bring in rrweb-io/rrweb#1049 to fix shadow dom
recording bugs.

Related PRs:
* highlight/docs#32
* highlight/highlight-javascript#9
* highlight/rrweb#100

## How did you test this change?

Local deploy on /1/buttons shadow dom example.

## Are there any deployment considerations?

New version 5.2.2.
@bhavitsharma
Copy link

@Mark-Fenng @Juice10
I think the implementation of processed node manager is causing some regression in the rrweb related to the order of scroll and click events. I am pretty sure requestAnimationFrame is causing some kind of delay but need to confirm again.

@Juice10
Copy link
Contributor Author

Juice10 commented Jan 18, 2023

@bhavitsharma could you create a separate issue for this including a test case and/or example recording where this is going wrong? Looking at the requestAnimationFrame code, it shouldn’t impact any scroll or click events.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants