Skip to content

audits(font-size): calculate accurate line/column for inline styles#9356

Merged
connorjclark merged 8 commits into
masterfrom
font-size-improve-line-col
Jul 15, 2019
Merged

audits(font-size): calculate accurate line/column for inline styles#9356
connorjclark merged 8 commits into
masterfrom
font-size-improve-line-col

Conversation

@connorjclark

@connorjclark connorjclark commented Jul 12, 2019

Copy link
Copy Markdown
Collaborator

split off of #9354

  1. For inline styles, the stylesheet's startColumn was always being added to the rule's column. However, this is only valid if the stylesheet and the rule begin on the same line in the HTML.
  2. For inline styles with a sourceURL magic comment, the line/column was relative to the HTML file. I've changed it to be relative to the beginning of the style tag, as if it were its own file.
  3. Lines are 1-indexed, but columns are 0-indexed. All columns were off by one.

before:

image

after:

image

node lighthouse-cli/ http://misc-hoten.surge.sh/lh-ui-location-font-size/ --only-audits=font-size --view

@connorjclark

Copy link
Copy Markdown
Collaborator Author

I verified that creating elements before a style element is declared does not affect the line/column of the parsed style declaration.

image

image

@patrickhulce patrickhulce left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

thanks for splitting out!

Comment thread lighthouse-cli/test/smokehouse/seo/expectations.js Outdated
Comment thread lighthouse-cli/test/smokehouse/seo/expectations.js Outdated
Comment thread lighthouse-core/audits/seo/font-size.js
<!-- 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>

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

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.

@patrickhulce patrickhulce left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

LGTM!

@brendankenny brendankenny left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

style suggestion, but otherwise LGTM!

Comment thread lighthouse-core/audits/seo/font-size.js Outdated
// 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;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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}`;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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 :)

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

done and done

@paulirish paulirish left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

lg. a few suggestions but nothing huge

.small-3 {
font-size: 6px;
}
/*# sourceURL=seo-tester-inline-magic.css */

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

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 ...)

Comment thread lighthouse-core/audits/seo/font-size.js
Comment thread lighthouse-core/audits/seo/font-size.js Outdated
// 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 ?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

ah nvm brendan's is nicer.

Co-Authored-By: Paul Irish <paulirish@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants