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: The URL involved in the dynamic modification element inline style is not converted to an absolute path #768

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

champkeh
Copy link
Contributor

No description provided.

@Yuyz0112
Copy link
Member

Maybe we need transform all the style attribute here?

@champkeh
Copy link
Contributor Author

Yes, indeed. I'll update it later.

@eoghanmurray
Copy link
Contributor

Is this to fix that there are URLs in the background-image attribute?

  • does transformAttribute handle css shorthand e.g. background: black url(/xyz.jpg) no-repeat;
  • should (can) we do this on the replayer side?
  • is it possible to write a test for this?

@champkeh champkeh changed the title fix: 修复属性中的背景图片路径没有转为绝对路径的bug fix: The URL involved in the dynamic modification element inline style is not converted to an absolute path Jan 13, 2022
@champkeh
Copy link
Contributor Author

champkeh commented Jan 13, 2022

@eoghanmurray I wrote some test case for this.

@eoghanmurray
Copy link
Contributor

Thanks for the test case!

Is this going down the if (name === 'style' && value) code path in transformAttribute ?
I think in that case, calling absoluteToStylesheet directly is preferable as we are not actually dealing with an entire attribute here.

Is it necessary to update your test case with a shorthand one; background: black url(/xyz.jpg) no-repeat; or can we trust that absoluteToStylesheet will do the right thing?

const newPriority = target.style.getPropertyPriority(pname);
if (
newValue !== old.style.getPropertyValue(pname) ||
newPriority !== old.style.getPropertyPriority(pname)
) {
newValue = transformAttribute(
Copy link
Contributor

Choose a reason for hiding this comment

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

could call absoluteToStylesheet directly here on newValue

Copy link
Contributor

Choose a reason for hiding this comment

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

like newValue = attributeToStylesheet(newValue);

Copy link
Contributor

Choose a reason for hiding this comment

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

(would need to import attributeToStylesheet and also maybe change it's href attribute to default to getHref() rather than importing getHref also)

const newPriority = target.style.getPropertyPriority(pname);
if (
newValue !== old.style.getPropertyValue(pname) ||
newPriority !== old.style.getPropertyPriority(pname)
) {
newValue = transformAttribute(
Copy link
Contributor

Choose a reason for hiding this comment

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

(would need to import attributeToStylesheet and also maybe change it's href attribute to default to getHref() rather than importing getHref also)

@@ -490,12 +490,18 @@ export default class MutationBuffer {
}
const styleObj = item.attributes.style as styleAttributeValue;
for (const pname of Array.from(target.style)) {
const newValue = target.style.getPropertyValue(pname);
let newValue = target.style.getPropertyValue(pname);
Copy link
Contributor

Choose a reason for hiding this comment

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

Thought we might need to do the transformation here, but you are right to wait til after the comparison.

<button onclick='addDynamicBackgroundImage()'>add dynamic background image</button>
<script>
function addDynamicBackgroundImage() {
document.querySelector('#div2').style.setProperty('background-image', 'url(dynamic.png)')
Copy link
Contributor

Choose a reason for hiding this comment

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

The images are cute! However they do need to be downloaded by everyone who downloads rrweb ... so they can be removed without affecting the test case (it shouldn't matter for the purposes of the test whether they actually exist)

@eoghanmurray eoghanmurray self-assigned this Apr 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants