Viewport test: accept initial-scale and allow spaces #1267

dracos · 2016-12-23T11:10:05Z

Fixes #641. and comments in it; allow a viewport to pass if it has width or initial-scale, and tweak the displayed error message to say this. It also allows spaces in the viewport elements, they are explicitly given in an example on e.g. https://developer.apple.com/library/content/documentation/AppleApplications/Reference/SafariHTMLRef/Articles/MetaTags.html#//apple_ref/doc/uid/TP40008193

wardpeet · 2016-12-26T15:54:25Z

thanks for this one :) can you add a test case for this as well.

Not sure we should only test for width or initial-scale though.

dracos · 2016-12-26T16:26:13Z

Done! Whilst I agree with your comment elsewhere it probably wants to check for more (though for accessibility, I don’t think it can just be ‘any valid value’ e.g. if it was just maximum-scale!), I think this would hopefully count as a good incremental change prior to that happening, rather than waiting for it to be fully decided :)

ebidel · 2016-12-27T00:38:24Z

lighthouse-core/test/audits/viewport-test.js

@@ -38,4 +38,16 @@ describe('Mobile-friendly: viewport audit', () => {
      Viewport: 'width=device-width, initial-scale=1, minimum-scale=1, maximum-scale=1'
    }).rawValue, true);
  });
+
+  it('passes when a viewport with spaces is provided', () => {


instead of separate tests, what if we expand the "'passes when a viewport is provided" test with a few permutations. Could just rename it to 'passes when a valid viewport is provided'. WDYT?

I’m happy either way, whichever would better fit your existing tests; I assume a failure would be as verbose either way. Pushed change.

paulirish · 2017-01-04T03:26:14Z

http://andreasbovens.github.io/understanding-viewport/ is a good resource for understanding various viewports and how phones handle them.

it looks like just initial-scale is satisfactory for this test, yup. Nice!

googlebot · 2017-01-04T03:46:00Z

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored by someone other than the pull request submitter. We need to confirm that they're okay with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of the commit author(s) and merge this pull request when appropriate.

paulirish

the regex approach is a bit funky.

Should we just graduate to a parser? I discovered https://www.npmjs.com/package/metaviewport-parser which seems good since the spec's parsing language is a bit wild: https://drafts.csswg.org/css-device-adapt/#parsing-algorithm

@dracos I put up a PR to adopt this package into your PR.. let me know what you think!
dracos#1

ebidel · 2017-01-04T04:00:49Z

@dracos There's also a line length issue after my rebase fix. If you can take care of it, that would be splendid :)

Use of metaviewport-parser package added by Paul Irish.

googlebot · 2017-01-04T14:15:31Z

CLAs look good, thanks!

dracos · 2017-01-04T14:24:06Z

@paulirish Thanks for that! Merged and squashed.
@ebidel Fixed :)

paulirish · 2017-01-04T16:27:51Z

@ebidel can you take another look?

ebidel · 2017-01-04T17:05:18Z

Nice changes. A few more things after the change.

ebidel · 2017-01-04T17:01:38Z

lighthouse-core/audits/viewport.js

-        artifacts.Viewport.includes('width=');
+    if (typeof artifacts.Viewport !== 'string') {
+      return Viewport.generateAuditResult({
+        rawValue: false


This should be an error case:

debugString: 'Error in determining viewport', rawValue: -1

see https://github.com/GoogleChrome/lighthouse/blob/master/lighthouse-core/audits/dobetterweb/no-datenow.js#L57-L59

ebidel · 2017-01-04T17:03:51Z

lighthouse-core/audits/viewport.js

+    }
+
+    const viewportProps = Parser.parseMetaViewPortContent(artifacts.Viewport).validProperties;
+    const hasMobileViewport = viewportProps['width'] || viewportProps['initial-scale'];


dot notation where you can: viewportProps.width

ebidel · 2017-01-04T17:04:46Z

lighthouse-core/test/audits/viewport-test.js

+    const viewports = [
+      'width=device-width, initial-scale=1, minimum-scale=1, maximum-scale=1',
+      'width = device-width, initial-scale = 1',
+      'initial-scale=1',


let's include one with just width as well.

ebidel · 2017-01-04T17:13:05Z

lighthouse-core/audits/viewport.js

+      });
+    }
+
+    const viewportProps = Parser.parseMetaViewPortContent(artifacts.Viewport).validProperties;


now that we have a parser, it might be nice to also warn if there are unknownProperties or invalidValues values. If that's the case, add a debugString to the artifact.

let debugString = ''; const parsedProps = Parser.parseMetaViewPortContent(artifacts.Viewport); if (Object.keys(parsedProps.unknownProperties).length) { debugString += `Invalid properties found: ${parsedProps.unknownProperties}`; } if (Object.keys(parsedProps.invalidValues).length) { debugString += `Invalid values found: ${parsedProps.invalidValues}`; } const viewportProps = parsedProps.validProperties; const hasMobileViewport = viewportProps.width || viewportProps['initial-scale']; Viewport.generateAuditResult({ rawValue: !!hasMobileViewport debugString });

Fixes from Eric Bidelman's review, plus his code to add invalid entries to debugString.

dracos · 2017-01-04T18:00:38Z

All done!

ebidel · 2017-01-04T18:07:04Z

lighthouse-core/audits/viewport.js

+      debugString
+    });
+
+    if (typeof artifacts.Viewport !== 'string') {


whoops. this is duplicated from the top.

because pippi longstocking

ebidel · 2017-01-04T18:28:13Z

lighthouse-core/test/audits/viewport-test.js

@@ -24,7 +24,7 @@ describe('Mobile-friendly: viewport audit', () => {
  it('fails when no input present', () => {
    return assert.equal(Audit.audit({
      Viewport: -1
-    }).rawValue, false);
+    }).rawValue, -1);


Sorry! One more thing. Can you add a test that checks that there's a debugString present for this case?

brendankenny

two last things

brendankenny · 2017-01-04T18:51:55Z

lighthouse-core/audits/viewport.js

+      debugString += `Invalid properties found: ${parsedProps.unknownProperties}`;
+    }
+    if (Object.keys(parsedProps.invalidValues).length) {
+      debugString += `Invalid values found: ${parsedProps.invalidValues}`;


this debugString probably needs some separator between the two parts in case there are invalid properties and invalid values

brendankenny · 2017-01-04T18:55:48Z

lighthouse-core/test/audits/viewport-test.js

@@ -33,9 +35,17 @@ describe('Mobile-friendly: viewport audit', () => {
    }).rawValue, false);
  });



maybe add a better failure case than this old test of just not having a viewport meta tag? e.g. a non-mobile friendly but valid viewport

Also is it worth adding a test for an invalid property and/or value and test to make sure it's in the debugString?

Added some more tests for this.
Used JSON.stringify to print out the invalid property/values, sorry if there's a better way of doing this that I missed!

brendankenny · 2017-01-12T18:31:01Z

lighthouse-core/audits/viewport.js

+    if (Object.keys(parsedProps.invalidValues).length) {
+      debugString += `Invalid values found: ${JSON.stringify(parsedProps.invalidValues)}. `;
+    }
+    debugString = debugString.trim();


nice solution :)

brendankenny

Sorry I was so slow on this. Looks great!

) Introduce `metaviewport-parser` module for better parsing.

dracos mentioned this pull request Dec 23, 2016

Viewport description isn't clear why it's failing #641

Closed

ebidel suggested changes Dec 27, 2016

View reviewed changes

ebidel changed the title ~~Allow initial-scale and spaces in viewport test.~~ Viewport test: accept initial-scale and allow spaces Dec 30, 2016

ebidel changed the title ~~Viewport test: accept initial-scale and allow spaces~~ Viewport test: accept initial-scale and allow spaces values Dec 30, 2016

ebidel changed the title ~~Viewport test: accept initial-scale and allow spaces values~~ Viewport test: accept initial-scale and allow spaces Dec 30, 2016

ebidel approved these changes Jan 4, 2017

View reviewed changes

paulirish mentioned this pull request Jan 4, 2017

use metaviewport-parser package. dracos/lighthouse#1

Closed

paulirish previously requested changes Jan 4, 2017

View reviewed changes

Allow initial-scale and spaces in viewport test.

bd2353c

Use of metaviewport-parser package added by Paul Irish.

dracos force-pushed the point-of-viewport branch from 606b1e9 to bd2353c Compare January 4, 2017 14:15

ebidel suggested changes Jan 4, 2017

View reviewed changes

squash! Allow initial-scale and spaces in viewport test.

a8b2818

Fixes from Eric Bidelman's review, plus his code to add invalid entries to debugString.

ebidel suggested changes Jan 4, 2017

View reviewed changes

fixup! squash! Allow initial-scale and spaces in viewport test.

71a7537

ebidel reviewed Jan 4, 2017

View reviewed changes

fixup! squash! Allow initial-scale and spaces in viewport test.

6d060bc

ebidel approved these changes Jan 4, 2017

View reviewed changes

brendankenny requested changes Jan 4, 2017

View reviewed changes

dracos added 2 commits January 9, 2017 13:01

fixup! squash! Allow initial-scale and spaces in viewport test.

f86315a

fixup More tests for viewports. JSON stringify invalid.

2a29890

ebidel assigned brendankenny Jan 9, 2017

brendankenny reviewed Jan 12, 2017

View reviewed changes

brendankenny approved these changes Jan 12, 2017

View reviewed changes

brendankenny merged commit 4514a73 into GoogleChrome:master Jan 12, 2017

andrewrota pushed a commit to andrewrota/lighthouse that referenced this pull request Jan 13, 2017

Viewport audit: accept initial-scale and allow spaces (GoogleChrome#1267

4401e75

) Introduce `metaviewport-parser` module for better parsing.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Viewport test: accept initial-scale and allow spaces #1267

Viewport test: accept initial-scale and allow spaces #1267

dracos commented Dec 23, 2016 •

edited by ebidel

Loading

wardpeet commented Dec 26, 2016 •

edited

Loading

dracos commented Dec 26, 2016

ebidel Dec 27, 2016

dracos Dec 27, 2016

paulirish commented Jan 4, 2017

googlebot commented Jan 4, 2017

paulirish left a comment

ebidel commented Jan 4, 2017

googlebot commented Jan 4, 2017

dracos commented Jan 4, 2017

paulirish commented Jan 4, 2017

ebidel commented Jan 4, 2017

ebidel Jan 4, 2017

ebidel Jan 4, 2017

ebidel Jan 4, 2017

ebidel Jan 4, 2017

dracos commented Jan 4, 2017

ebidel Jan 4, 2017

dracos Jan 4, 2017

ebidel Jan 4, 2017 •

edited

Loading

dracos Jan 4, 2017

ebidel Jan 4, 2017

brendankenny left a comment

brendankenny Jan 4, 2017

brendankenny Jan 4, 2017 •

edited

Loading

dracos Jan 9, 2017

brendankenny Jan 12, 2017

brendankenny left a comment

		@@ -33,9 +35,17 @@ describe('Mobile-friendly: viewport audit', () => {
		}).rawValue, false);
		});

Viewport test: accept initial-scale and allow spaces #1267

Viewport test: accept initial-scale and allow spaces #1267

Conversation

dracos commented Dec 23, 2016 • edited by ebidel Loading

wardpeet commented Dec 26, 2016 • edited Loading

dracos commented Dec 26, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paulirish commented Jan 4, 2017

googlebot commented Jan 4, 2017

paulirish left a comment

Choose a reason for hiding this comment

ebidel commented Jan 4, 2017

googlebot commented Jan 4, 2017

dracos commented Jan 4, 2017

paulirish commented Jan 4, 2017

ebidel commented Jan 4, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dracos commented Jan 4, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ebidel Jan 4, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brendankenny left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brendankenny Jan 4, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brendankenny left a comment

Choose a reason for hiding this comment

dracos commented Dec 23, 2016 •

edited by ebidel

Loading

wardpeet commented Dec 26, 2016 •

edited

Loading

ebidel Jan 4, 2017 •

edited

Loading

brendankenny Jan 4, 2017 •

edited

Loading