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

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

Merged
merged 8 commits into from
Jul 15, 2019

Conversation

connorjclark
Copy link
Collaborator

@connorjclark connorjclark commented Jul 12, 2019

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

Copy link
Collaborator

@patrickhulce patrickhulce left a comment

Choose a reason for hiding this comment

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

thanks for splitting out!

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

Copy link
Collaborator

@patrickhulce patrickhulce left a comment

Choose a reason for hiding this comment

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

LGTM!

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.

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;
Copy link
Member

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
Member

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

Copy link
Member

@paulirish paulirish left a comment

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

lighthouse-core/audits/seo/font-size.js Show resolved Hide resolved
// 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
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
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