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 inline link elements bug #995

Merged
merged 3 commits into from
Sep 30, 2022
Merged

Fix inline link elements bug #995

merged 3 commits into from
Sep 30, 2022

Conversation

YunFeng0817
Copy link
Member

In PR #909, an async mechanism for inlining link elements is introduced but it still has a bug and some negative impacts on the replayer side.

The Bug

In the stylesheet-manager.ts, when the link element loaded the styles, the manager emits an incremental mutation event to inline styles. But the mutation's parentId is wrong.

parentId: mirror.getId(linkEl),

As a result, the event looks like this:
image master...fix-inline-link-elements#diff-988864050cbe6d85b4cb140a06ffce65819d8967e071b175f1d27b8a864b5383L176-L189
The parentNode of the link element is itself and it can't be appended to the dom actually.

Negative Impacts

This mechanism makes elements absent in the normal snapshot stage and causes performance issues and many warnings in the console.
Here's a diagram to explain this problem.

explanation

image

Changes

I built this PR based on #989 so that it contains all changes of that PR. This PR has to get merged after #989.
I made some changes to the current mechanism. In the snapshot stage, the unloaded Link elements will still get serialized without inline styles. After they are loaded. an incremental mutation of attributes is appended which includes an attribute _cssText. Its value is the inlined CSS styles.

{
  source: IncrementalSource.Mutation,
  adds: [],
  removes: [],
  texts: [],
  attributes: [
    {
      id: 9,
      attributes: {
        _cssText: "body { color: pink; }",
      },
    },
  ],
}

This way, the unloaded Link elements can be inlined without causing the above-mentioned side effects.
On the replaying side, when the replayer encounters the '_cssText' attribute change and its target is the Link element, the replayer can replace the old un-inlined one with the inlined loaded Link element.
This method can make it easy to implement the TODO feature as well.

// TODO: take snapshot on stylesheet reload by applying event listener
private trackStylesheet(linkEl: HTMLLinkElement) {
// linkEl.addEventListener('load', () => {
// // re-loaded, maybe take another snapshot?
// });
}

This PR can also fix #981.

@YunFeng0817 YunFeng0817 force-pushed the fix-inline-link-elements branch 2 times, most recently from 7e7f5a6 to 23addd0 Compare September 15, 2022 11:03
texts: [],
attributes: [],
});
if ('_cssText' in (childSn as elementNode).attributes)
Copy link
Contributor

Choose a reason for hiding this comment

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

Is _cssText always going to be set, also if a link element hasn't finished loading yet?

Copy link
Member Author

Choose a reason for hiding this comment

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

According the code below

if (stylesheet) {
cssText = getCssRulesString(stylesheet);
}
if (cssText) {
delete attributes.rel;
delete attributes.href;
attributes._cssText = absoluteToStylesheet(cssText, stylesheet!.href!);

I think it will set _cssText when the link has been loaded. Otherwise, recorder will only keep its original attributes.

Copy link
Contributor

@Juice10 Juice10 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 finding and fixing this bug! I found a one bug and maybe another potential bug. Could you fix those and I'd be happy to re-review & approve!

@YunFeng0817 YunFeng0817 force-pushed the fix-inline-link-elements branch 2 times, most recently from 8e74045 to 6bde25a Compare September 23, 2022 00:47
@YunFeng0817
Copy link
Member Author

Today I encountered an issue with the replayer also caused by this.

This is the link element on the recorder side.
image

While this screenshot is the link element on the replayer side.
image
The element is self-nested in 4 depths.

@YunFeng0817 YunFeng0817 force-pushed the fix-inline-link-elements branch from 6bde25a to d311687 Compare September 23, 2022 04:36
1. make Mirror's replace function act the same with the original one when there's no existed node to get replaced.
2. when replacing with the link/style elements, keep their existing attributes to prevent potential bugs
@YunFeng0817 YunFeng0817 force-pushed the fix-inline-link-elements branch from d311687 to 8aefe2b Compare September 29, 2022 07:33
@YunFeng0817
Copy link
Member Author

I rebased the latest master branch and this pull request is ready to get reviewed and merged.

@YunFeng0817 YunFeng0817 requested a review from Juice10 September 29, 2022 07:37
@Yuyz0112 Yuyz0112 merged commit 55ebce7 into master Sep 30, 2022
@YunFeng0817 YunFeng0817 deleted the fix-inline-link-elements branch October 6, 2022 00:06
@Juice10 Juice10 added the 2.0 label Dec 1, 2022
eoghanmurray added a commit to eoghanmurray/rrweb that referenced this pull request Apr 22, 2024
eoghanmurray added a commit to eoghanmurray/rrweb that referenced this pull request Apr 23, 2024
eoghanmurray added a commit to eoghanmurray/rrweb that referenced this pull request Apr 24, 2024
eoghanmurray added a commit to eoghanmurray/rrweb that referenced this pull request May 14, 2024
eoghanmurray added a commit to eoghanmurray/rrweb that referenced this pull request May 22, 2024
eoghanmurray added a commit to eoghanmurray/rrweb that referenced this pull request May 22, 2024
eoghanmurray added a commit to eoghanmurray/rrweb that referenced this pull request May 22, 2024
eoghanmurray added a commit to eoghanmurray/rrweb that referenced this pull request May 23, 2024
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