-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
fix: wujie shadow root #1763
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: wujie shadow root #1763
Conversation
🦋 Changeset detectedLatest commit: 4ec8273 The changes in this PR will be included in the next version bump. This PR includes changesets to release 19 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 |
|
Wujie framework proxies
This explained the log output below: const iframe = window.document.querySelector('IFRAME');
const head = iframe.contentDocument.querySelector('head');
// updated code simulate output
iframe.contentDocument.contains(head); // true
// Previous code simulate output
window.Node.prototype.contains.call(iframe.contentDocument, head); // false
iframe.contentWindow.Node.prototype.contains.bind(iframe.contentDocument)(head); // false
|
|
Is there anyone follow up this pr? |
packages/rrweb/src/utils.ts
Outdated
| if (!doc) return false; | ||
| return dom.contains(doc, n) || shadowHostInDom(n); | ||
| // fix: https://github.com/rrweb-io/rrweb/issues/1682 | ||
| return doc.contains(n) || shadowHostInDom(n); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
dom.contains was trying to get the original unproxied .contains.
Changing it to use the regular doc.contains could fix it for wujie but would break it for other frameworks that monkeypatch .contains and make it do non-standard behavior.
To fix this correctly check out how dom.contains is implemented, it sounds like wujie is getting around our proxy bypass
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Happy New Year Justin! Thanks for your great suggestion, in fact, wujie modified the this reference in the iframe, binding it to the this reference in shadowRoot. To be honest, I didn't think it was a very elegant fix from the start and want to discuss with you.
packages/rrweb/src/utils.ts
Outdated
| } | ||
|
|
||
| export function inDom(n: Node): boolean { | ||
| const doc = n.ownerDocument; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It turns out the real problem is here, wujie monkey patches n.ownerDocument, we should make sure access to this method goes through getUntaintedAccessor like we do with many other accessors, then this should record ok:
rrweb/packages/utils/src/index.ts
Lines 172 to 186 in c059dbf
| export function childNodes(n: Node): NodeListOf<Node> { | |
| return getUntaintedAccessor('Node', n, 'childNodes'); | |
| } | |
| export function parentNode(n: Node): ParentNode | null { | |
| return getUntaintedAccessor('Node', n, 'parentNode'); | |
| } | |
| export function parentElement(n: Node): HTMLElement | null { | |
| return getUntaintedAccessor('Node', n, 'parentElement'); | |
| } | |
| export function textContent(n: Node): string | null { | |
| return getUntaintedAccessor('Node', n, 'textContent'); | |
| } |
|
Looks like this is the correct fix: wfk007/rrweb-wujie#1 |
This pr is working for me! 👍 |
@Juice10 This is how I understand:
so, we need to keep |
3938584 to
67d8900
Compare


fix:#1682
reproduce step: https://github.com/wfk007/rrweb-wujie