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

core(font-display): do not use invalid sourceURLs #8535

Merged
merged 2 commits into from
Apr 26, 2019

Conversation

patrickhulce
Copy link
Collaborator

@patrickhulce patrickhulce commented Apr 23, 2019

Summary
Apparently /*# sourceURL=custom-url.css */ sets the sourceURL in DevTools to whatever the attribute was, so we should handle that case.

Related Issues/PRs
fixes #8534

@westonruter
Copy link

westonruter commented Apr 23, 2019

Apparently <style custom-name> sets the sourceURL in DevTools to whatever the attribute was, so we should handle that case.

Actually, the custom URL was set manually. See #8534 (comment):

The source URL is coming from the last line of style[amp-custom]:

/*# sourceURL=amp-custom.css */

This was put in place based on a tip from @paulirish, as without it the CSS in the style element fails to appear in Coverage.

@patrickhulce
Copy link
Collaborator Author

Thanks @westonruter! Updated accordingly :)

Copy link
Member

@brendankenny brendankenny left a comment

Choose a reason for hiding this comment

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

LGTM with possible test request

@@ -231,6 +231,8 @@ describe('Performance: Font Display audit', () => {
});

it('handles varied font-display declarations', async () => {
// Make sure we don't use sourceURL when it's not a valid URL, see https://github.com/GoogleChrome/lighthouse/issues/8534
stylesheet.header.sourceURL = 'custom-attribute';
Copy link
Member

Choose a reason for hiding this comment

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

any chance you could split this off into its own test? I could see us revising font-display at some point and having to reconfigure the tests and losing this very important line :)

Copy link
Collaborator Author

@patrickhulce patrickhulce Apr 24, 2019

Choose a reason for hiding this comment

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

sure :)

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.

Font display doesn't handle styles with relative/invalid sourceURL comments
3 participants