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 extraction of background image URLs. #576

Merged
merged 1 commit into from
Oct 12, 2022

Conversation

pmeenan
Copy link
Contributor

@pmeenan pmeenan commented Oct 11, 2022

Also:

  • added support for other types of non-src images
  • fixed edge-side processing of the LCP events
  • Moved the edge-processing to before the debug log gets collected (so logs can be included)

For catchpoint/WebPageTest#2455

Also:
* added support for other types of non-src images
* fixed edge-side processing of the LCP events
* Moved the edge-processing to before the debug log gets collected (so logs can be included)

For catchpoint/WebPageTest#2455
Copy link
Contributor

@stoyan stoyan left a comment

Choose a reason for hiding this comment

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

looks good

if (style.backgroundImage && style.backgroundImage != 'none') {
event.element['background-image'] = style.backgroundImage;
}
if (style.content && style.content != 'none') {
Copy link
Contributor

Choose a reason for hiding this comment

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

Curious how do we use this? Because it can be a whole bunch of stuff other than none, I wonder why we filter out none

Copy link
Contributor Author

Choose a reason for hiding this comment

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

'none' is the default value that gets populated when it's not explicitly set. https://developer.mozilla.org/en-US/docs/Web/CSS/background-image#syntax

@tkadlec tkadlec merged commit 99d7cd6 into catchpoint:master Oct 12, 2022
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