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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 8 commits into from
Jul 15, 2019
Merged

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

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
cd16077
audits(font-size): calculate accurate line/column for inline scripts
connorjclark Jul 11, 2019
a3a8b8a
rename
connorjclark Jul 12, 2019
3d9c211
smaller expectations
connorjclark Jul 13, 2019
4ee49d1
add test cases for sourceURL
connorjclark Jul 13, 2019
8acb3f8
pr
connorjclark Jul 15, 2019
8f7c17e
Update lighthouse-core/audits/seo/font-size.js
connorjclark Jul 15, 2019
efd6108
thank heavens for tests
connorjclark Jul 15, 2019
3784af5
Merge branch 'font-size-improve-line-col' of github.com:GoogleChrome/…
connorjclark Jul 15, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions lighthouse-cli/test/fixtures/seo/seo-tester.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
font-size: 11px;
}
</style>
<style> /* some filler to offset things */ .small-2 { font-size: 11px; }</style>
</head>
<body>
<!-- PASS(hreflang): should ignore links in the body -->
Expand All @@ -43,6 +44,7 @@ <h2>Anchor text</h2>
<h2>Small text</h2>
<!-- PASS(font-size): amount of illegible text is below the 60% threshold -->
<p class='small'> 1 </p>
<p class='small-2'> 2 </p>
<h6>2</h6>
<font size="1">3<b>4</b></font>
<p style='font-size:10px'>5 </p>
Expand Down
59 changes: 56 additions & 3 deletions lighthouse-cli/test/smokehouse/seo/expectations.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -117,9 +117,62 @@ module.exports = [
'font-size': {
score: 1,
details: {
items: {
length: 6,
},
items: [
{
source: 'http://localhost:10200/seo/seo-tester.html?extra_header=link%3D%253Chttp%253A%252F%252Flocalhost%253A10200%252Fseo%252F%253E%253B%2Brel%253D%2522canonical%2522:24:12',
connorjclark marked this conversation as resolved.
Show resolved Hide resolved
selector: '.small',
coverage: '1.32%',
connorjclark marked this conversation as resolved.
Show resolved Hide resolved
fontSize: '11px',
},
{
source: 'http://localhost:10200/seo/seo-tester.html?extra_header=link%3D%253Chttp%253A%252F%252Flocalhost%253A10200%252Fseo%252F%253E%253B%2Brel%253D%2522canonical%2522:28:55',
selector: '.small-2',
coverage: '1.32%',
fontSize: '11px',
},
{
source: 'User Agent Stylesheet',
selector: 'h6',
coverage: '1.32%',
fontSize: '10px',
},
{
source: 'http://localhost:10200/seo/seo-tester.html?extra_header=link%3D%253Chttp%253A%252F%252Flocalhost%253A10200%252Fseo%252F%253E%253B%2Brel%253D%2522canonical%2522',
selector: {
type: 'node',
selector: 'body',
snippet: '<font size="1">',
},
coverage: '1.32%',
fontSize: '10px',
},
{
source: 'http://localhost:10200/seo/seo-tester.html?extra_header=link%3D%253Chttp%253A%252F%252Flocalhost%253A10200%252Fseo%252F%253E%253B%2Brel%253D%2522canonical%2522',
selector: {
type: 'node',
selector: 'font',
snippet: '<b>',
},
coverage: '1.32%',
fontSize: '10px',
},
{
source: 'http://localhost:10200/seo/seo-tester.html?extra_header=link%3D%253Chttp%253A%252F%252Flocalhost%253A10200%252Fseo%252F%253E%253B%2Brel%253D%2522canonical%2522',
selector: {
type: 'node',
selector: 'body',
snippet: '<p style="font-size:10px">',
},
coverage: '1.32%',
fontSize: '10px',
},
{
source: 'Legible text',
selector: '',
coverage: '92.11%',
fontSize: '≥ 12px',
},
],
},
},
'link-text': {
Expand Down
19 changes: 13 additions & 6 deletions lighthouse-core/audits/seo/font-size.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -157,12 +157,19 @@ function findStyleRuleSource(baseURL, styleDeclaration, node) {
source = `${url.href}`;

if (range) {
connorjclark marked this conversation as resolved.
Show resolved Hide resolved
// `stylesheet` can be either an external file (stylesheet.startLine will always be 0),
connorjclark marked this conversation as resolved.
Show resolved Hide resolved
// or a <style> block (stylesheet.startLine will vary)
const absoluteStartLine = range.startLine + stylesheet.startLine + 1;
const absoluteStartColumn = range.startColumn + stylesheet.startColumn + 1;

source += `:${absoluteStartLine}:${absoluteStartColumn}`;
// Add the startLine/startColumn of the <style> element to the range, if stylesheet is inline.
// 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.

range.startLine + stylesheet.startLine + 1 :
range.startLine + 1;
// 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


source += `:${line}:${column}`;
}
} else {
// dynamically injected to page
Expand Down