core(font-display): more accurately follow CSS spec #7191
Conversation
| const rawFontDisplay = declaration.match(/font-display:(.*?);/); | ||
| // Find the font-display value by matching a single token, optionally surrounded by whitespace, | ||
| // followed either by a semicolon or the end of a block. | ||
| const rawFontDisplay = declaration.match(/font-display:(\s*\S+\s*)(;|\s*\})/); |
There was a problem hiding this comment.
is the last \s* not already captured by the first group?
There was a problem hiding this comment.
after just getting burned by the old regex, I feel like we need some extra whitespace tests for the cases we're going to be handling now :)
There was a problem hiding this comment.
+1 to @brendankenny first comment, should be:
font-display:(\s*\S+\s*)(;|\})
iirc it is valid to have white space after the property before the colon like:
font-display : 'optional';in which case we would need:
font-display\s*:(\s*\S+\s*)(;|\})
|
@patrickhulce do you mind adding a test where you check all possibilities from the regex? |
|
great points everyone! since at this point we're just taking a step back on the whole audit, I thought I'd tighten it up and make it more correct rather than the loosely goosey forgiving audit (aside from the semicolon) that it was before :) |
patrickhulce
changed the title
patrickhulce
changed the title
| // If they didn't have a font-display property, it's the default, and it's failing; bail | ||
| if (!rawFontDisplay) continue; | ||
| // If they don't have one of the passing font-display values, it's failing; bail | ||
| const hasPassingFontDisplay = PASSING_FONT_DISPLAY_REGEX.test(rawFontDisplay[0]); | ||
| const hasPassingFontDisplay = PASSING_FONT_DISPLAY_REGEX.test(rawFontDisplay[1].trim()); |
There was a problem hiding this comment.
nit: worth keeping the whitespace in the capture group if only going to trim it?
| }); | ||
|
|
||
| it('resolves URLs relative to stylesheet URL when available', async () => { | ||
| stylesheet.header.sourceURL = 'https://cdn.example.com/foo/bar/file.css'; | ||
| stylesheet.content = ` | ||
| @font-face { | ||
| font-display: 'block'; | ||
| font-display: block; |
|
In which lighthouse version is this published? Report: https://lighthouse-dot-webdotdevsite.appspot.com/lh/html?url=https://rw.schoen.world/ Minified styles in the head using emotion.js |
|
This fix is shipping in v4.2.0+ |
Summary
One of the reasons people seem to be getting false failures is our semicolon. Thanks to @Diklla for pointing this out!
Related Issues/PRs
#6628