audits(font-size): calculate accurate line/column for inline styles#9356
Conversation
patrickhulce
left a comment
There was a problem hiding this comment.
thanks for splitting out!
| <!-- font-size items are ordered by text length, so force an order with filler for stable expectations. --> | ||
| <p class='small'> 1.... </p> | ||
| <p class='small-2'> 2... </p> | ||
| <p class='small-3'> 3.. </p> |
There was a problem hiding this comment.
adding these two cases made the actual items jump around in font-size, on account of most items being the same text length and the audit using an unstable sort on that property. I added filler text to maintain the order.
brendankenny
left a comment
There was a problem hiding this comment.
style suggestion, but otherwise LGTM!
| // The column the stylesheet begins on is only relevant if the rule is declared on the same line. | ||
| const column = addHtmlLocationOffset && range.startLine === 0 ? | ||
| range.startColumn + stylesheet.startColumn : | ||
| range.startColumn; |
There was a problem hiding this comment.
just a style issue, but interleaving the offset makes this read a little more confusingly than it is. What about something like
let line = range.startLine + 1;
let column = range.startColumn;
// Add the startLine/startColumn of the <style> element to the range, if stylesheet is inline
// and a sourceURL magic comment is not present (`hasSourceURL` is true).
const addHtmlLocationOffset = stylesheet.isInline && !stylesheet.hasSourceURL;
if (addHtmlLocationOffset) {
line += stylesheet.startLine;
column += stylesheet.startColumn;
}
source += `:${line}:${column}`;There was a problem hiding this comment.
also may be worth calling out why sourceURL means you don't want the offset because it isn't obvious from this context, at least (admittedly I've paged all font-size stuff out of my brain :)
There was a problem hiding this comment.
done and done
paulirish
left a comment
There was a problem hiding this comment.
lg. a few suggestions but nothing huge
| .small-3 { | ||
| font-size: 6px; | ||
| } | ||
| /*# sourceURL=seo-tester-inline-magic.css */ |
There was a problem hiding this comment.
sourceURL's appear at the end of the block, by convention.. but they can show up at the top.
i think your code handles this situation, but FYI in case you want the coverage.
There was a problem hiding this comment.
should be good, from what i know of the css parser. would just be testing the css parser (which I'm already doing perhaps too much here w/ the offset stuff ...)
| // Just use the rule's location if a sourceURL magic comment is present (`hasSourceURL` is true). | ||
| const addHtmlLocationOffset = stylesheet.isInline && !stylesheet.hasSourceURL; | ||
|
|
||
| const line = addHtmlLocationOffset ? |
There was a problem hiding this comment.
i'd separate these concerns a little more but its no big deal really. calling out the +1 for lines but not columns is potentially worthwhile.
let line = range.startLine;
line++; // lines are 0-indexed in the protocol, but users expect 1-indexed line values;
if (addHtmlLocationOffset) line += stylesheet.startLine;Co-Authored-By: Paul Irish <paulirish@google.com>
…lighthouse into font-size-improve-line-col


split off of #9354
before:
after: