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: add mutation lost in slimDOMOptions #994

Merged
merged 2 commits into from
Sep 15, 2022

Conversation

wfk007
Copy link
Contributor

@wfk007 wfk007 commented Sep 8, 2022

fix: #993

@wfk007
Copy link
Contributor Author

wfk007 commented Sep 8, 2022

when using slimDOMOptions in rrweb@1.1.3

Comment width a node.__sn.id === -2 which is exactly the same with IGNORED_NODE

image

when you getNextId with a Node which is followed by a comment, you will get -2 which is equal to IGNORED_NODE causing the while loop continue, and getNextId will finally return null

image

but in rrweb@2.0.0-alpha.1, node.__sn.id is dropped by the PR https://github.com/rrweb-io/rrweb/pull/868/files
causing that getNextId will finally return -1 which affect the following judge
image

@wfk007
Copy link
Contributor Author

wfk007 commented Sep 9, 2022

in rrweb@2.0.0-alpha.1, mirror.getId() return -1 may caused by:

  1. mutation add: child node may be pushed before its newly added parent, rrweb uses a DoubleLinkedList to store these nodes
  2. slimDOMOptions: true, and the Node is an IGNORED_NODE, but not in Mirror

considering case 2:
in rrweb@1.1.3, we can use node.__sn to check whether nextSibling is an IGNORED_NODE
BUT in in rrweb@2.0.0-alpha.1, we can not
because the information with IGNORED_NODE neither in node.__sn nor in Mirror

@wfk007
Copy link
Contributor Author

wfk007 commented Sep 9, 2022

before:

before.mov

after fixed:

after.mov

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.

Thanks for your very nice pull request. The description and solution are very clear and clean.

The failure of Format Check CI can be ignored because it may be caused by the old branch base. I've checked the snapshot.ts and there's no formatting issue.

@wfk007
Copy link
Contributor Author

wfk007 commented Sep 15, 2022

@Mark-Fenng @Yuyz0112
Hi, is this pending any other changes?

@YunFeng0817
Copy link
Member

Nope, I think everything is good. Just waiting for @Yuyz0112's review.

@Yuyz0112
Copy link
Member

sorry for the late

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.

[Bug]: element in add mutation LOST when using slimDOMOptions
4 participants