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: regression of issue: ShadowHost can't be a string (issue 941) #1092

Merged
merged 2 commits into from
Feb 9, 2023

Conversation

YunFeng0817
Copy link
Member

Bug

image

This PR is to fix a bug which is a regression of #941. The original one has been fixed but imported again in #1049

Reason

Given the text node of a detached HTMLAnchorElement, getRootNode() will return the anchor element itself and its host property is a string.
image

Fix

The fix is like #1020. I added unit tests to cover all key codes and I hope this bug won't show up anymore.

@YunFeng0817 YunFeng0817 added bug Something isn't working 2.0 regression labels Jan 13, 2023
@YunFeng0817 YunFeng0817 requested a review from Juice10 January 13, 2023 06:01
@Juice10
Copy link
Contributor

Juice10 commented Jan 13, 2023

The problem here is that ShadowRoot and HTMLAnchorElement have colliding host properties. In the jsdom unit tests created this passes. But re-reading the code a couple times I can’t see how the host collision issue is bypassed.

@YunFeng0817
Copy link
Member Author

YunFeng0817 commented Jan 13, 2023

The problem here is that ShadowRoot and HTMLAnchorElement have colliding host properties. In the jsdom unit tests created this passes. But re-reading the code a couple times I can’t see how the host collision issue is bypassed.

I think the problem in the left side is this line
image
https://github.com/rrweb-io/rrweb/pull/1092/files#diff-048da15f20b717b1ea5aa5346398dfed2fe4e24a7d38445b94cbe707640c9fc5L523
It retrieves the host property without checking the element type.

Here is the process how the error happens:
The input of inDom() is a text node in <a>. This <a> is special because it is not mounted on the Dom.
image
Then we try to get its rootShadowHost.
image
Finally the string 'github.com' is returned as the "rootShadowHost".

@bhavitsharma
Copy link

@Mark-Fenng I am a bit lost here, we do check if the getRootNode().nodeType is a Document Fragment so why did the check fail?

@YunFeng0817
Copy link
Member Author

YunFeng0817 commented Jan 20, 2023

@Mark-Fenng I am a bit lost here, we do check if the getRootNode().nodeType is a Document Fragment so why did the check fail?

https://github.com/rrweb-io/rrweb/pull/1092/files#diff-048da15f20b717b1ea5aa5346398dfed2fe4e24a7d38445b94cbe707640c9fc5L523
This line doesn't check the node type.

@bhavitsharma
Copy link

Ah, I understand it better now. Thanks. Can we merge this PR then?

@YunFeng0817
Copy link
Member Author

Ah, I understand it better now. Thanks. Can we merge this PR then?

We need at least one reviewer to approve this before merging it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2.0 bug Something isn't working regression
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants