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

core(base-artifacts): add FormFactorIsMobile base artifact #7280

Merged
merged 10 commits into from
Mar 5, 2019
Merged

core(base-artifacts): add FormFactorIsMobile base artifact #7280

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
a701cd0
Create IsMobile and IsMobileHost base artifacts
mattzeunert Feb 20, 2019
73432a5
Use IsMobile in content-width audit
mattzeunert Feb 20, 2019
44f2e89
Test cases for IsMobile and IsMobileHost
mattzeunert Feb 20, 2019
9f4439e
Period
brendankenny Feb 21, 2019
9c618c0
Update types/artifacts.d.ts
brendankenny Feb 21, 2019
ee475bb
Remove duplicate test
mattzeunert Feb 21, 2019
0d2e829
PR feedback
mattzeunert Feb 21, 2019
78b82e1
Rename IsMobile to IsMobileFormFactor
mattzeunert Mar 1, 2019
2947f2c
Rename IsMobileFormFactor to FormFactorIsMobile
mattzeunert Mar 1, 2019
54b5d94
FormFactorIsMobile => TestedAsMobileDevice
mattzeunert Mar 2, 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
13 changes: 4 additions & 9 deletions lighthouse-core/audits/content-width.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,26 +19,21 @@ class ContentWidth extends Audit {
description: 'If the width of your app\'s content doesn\'t match the width ' +
'of the viewport, your app might not be optimized for mobile screens. ' +
'[Learn more](https://developers.google.com/web/tools/lighthouse/audits/content-sized-correctly-for-viewport).',
requiredArtifacts: ['ViewportDimensions', 'HostUserAgent'],
requiredArtifacts: ['ViewportDimensions', 'IsMobile'],
};
}

/**
* @param {LH.Artifacts} artifacts
* @param {LH.Audit.Context} context
* @return {LH.Audit.Product}
*/
static audit(artifacts, context) {
const userAgent = artifacts.HostUserAgent;
static audit(artifacts) {
const IsMobile = artifacts.IsMobile;
const viewportWidth = artifacts.ViewportDimensions.innerWidth;
const windowWidth = artifacts.ViewportDimensions.outerWidth;
const widthsMatch = viewportWidth === windowWidth;

const isMobileHost = userAgent.includes('Android') || userAgent.includes('Mobile');
const isMobile = context.settings.emulatedFormFactor === 'mobile' ||
(context.settings.emulatedFormFactor !== 'desktop' && isMobileHost);

if (isMobile) {
if (IsMobile) {
return {
rawValue: widthsMatch,
explanation: this.createExplanation(widthsMatch, artifacts.ViewportDimensions),
Expand Down
11 changes: 10 additions & 1 deletion lighthouse-core/gather/gather-runner.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -395,10 +395,19 @@ class GatherRunner {
* @return {Promise<LH.BaseArtifacts>}
*/
static async getBaseArtifacts(options) {
const hostUserAgent = (await options.driver.getBrowserVersion()).userAgent;

const {emulatedFormFactor} = options.settings;
const IsMobileHost = hostUserAgent.includes('Android') || hostUserAgent.includes('Mobile');
const IsMobile = emulatedFormFactor === 'mobile' ||
(emulatedFormFactor !== 'desktop' && IsMobileHost);

return {
fetchTime: (new Date()).toJSON(),
LighthouseRunWarnings: [],
HostUserAgent: (await options.driver.getBrowserVersion()).userAgent,
IsMobile,
IsMobileHost,
HostUserAgent: hostUserAgent,
NetworkUserAgent: '', // updated later
BenchmarkIndex: 0, // updated later
WebAppManifest: null, // updated later
Expand Down
14 changes: 7 additions & 7 deletions lighthouse-core/test/audits/content-width-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,25 +13,25 @@ const assert = require('assert');
describe('Mobile-friendly: content-width audit', () => {
it('fails when scroll width differs from viewport width', () => {
const result = Audit.audit({
HostUserAgent: 'Desktop',
IsMobile: true,
ViewportDimensions: {
innerWidth: 100,
outerWidth: 300,
},
}, {settings: {emulatedFormFactor: 'mobile'}});
});

assert.equal(result.rawValue, false);
assert.ok(result.explanation);
});

it('fails when host user agent is a phone', () => {
mattzeunert marked this conversation as resolved.
Show resolved Hide resolved
const result = Audit.audit({
HostUserAgent: 'Mobile Android',
IsMobile: true,
ViewportDimensions: {
innerWidth: 100,
outerWidth: 300,
},
}, {settings: {emulatedFormFactor: 'none'}});
});

assert.equal(result.rawValue, false);
assert.ok(result.explanation);
Expand All @@ -47,13 +47,13 @@ describe('Mobile-friendly: content-width audit', () => {
}, {settings: {emulatedFormFactor: 'mobile'}}).rawValue, true);
});

it('not applicable when device emulation is turned off', () => {
it('not applicable when run on desktop', () => {
return assert.equal(Audit.audit({
HostUserAgent: 'Mobile Android Chrome',
IsMobile: false,
ViewportDimensions: {
innerWidth: 300,
outerWidth: 450,
},
}, {settings: {emulatedFormFactor: 'desktop'}}).notApplicable, true);
}).notApplicable, true);
});
});
150 changes: 81 additions & 69 deletions lighthouse-core/test/gather/fake-driver.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,79 @@
*/
'use strict';

function makeFakeDriver({protocolGetVersionResponse}) {
mattzeunert marked this conversation as resolved.
Show resolved Hide resolved
return {
getBrowserVersion() {
return Promise.resolve(Object.assign({}, protocolGetVersionResponse, {milestone: 71}));
},
getBenchmarkIndex() {
return Promise.resolve(125.2);
},
getAppManifest() {
return Promise.resolve(null);
},
connect() {
return Promise.resolve();
},
disconnect() {
return Promise.resolve();
},
gotoURL() {
return Promise.resolve('https://www.reddit.com/r/nba');
},
beginEmulation() {
return Promise.resolve();
},
setThrottling() {
return Promise.resolve();
},
dismissJavaScriptDialogs() {
return Promise.resolve();
},
assertNoSameOriginServiceWorkerClients() {
return Promise.resolve();
},
reloadForCleanStateIfNeeded() {
return Promise.resolve();
},
enableRuntimeEvents() {
return Promise.resolve();
},
evaluateScriptOnLoad() {
return Promise.resolve();
},
cleanBrowserCaches() {},
clearDataForOrigin() {},
cacheNatives() {
return Promise.resolve();
},
evaluateAsync() {
return Promise.resolve({});
},
registerPerformanceObserver() {
return Promise.resolve();
},
beginTrace() {
return Promise.resolve();
},
endTrace() {
return Promise.resolve(
require('../fixtures/traces/progressive-app.json')
);
},
beginDevtoolsLog() {},
endDevtoolsLog() {
return require('../fixtures/artifacts/perflog/defaultPass.devtoolslog.json');
},
blockUrlPatterns() {
return Promise.resolve();
},
setExtraHTTPHeaders() {
return Promise.resolve();
},
};
}

// https://chromedevtools.github.io/devtools-protocol/tot/Browser#method-getVersion
const protocolGetVersionResponse = {
protocolVersion: '1.3',
Expand All @@ -14,77 +87,16 @@ const protocolGetVersionResponse = {
userAgent: 'Mozilla/5.0 (Macintosh; Intel Mac OS X 10_13_6) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/71.0.3577.0 Safari/537.36',
jsVersion: '7.1.314',
};
const fakeDriver = makeFakeDriver({protocolGetVersionResponse});

const fakeDriver = {
getBrowserVersion() {
return Promise.resolve(Object.assign({}, protocolGetVersionResponse, {milestone: 71}));
},
getBenchmarkIndex() {
return Promise.resolve(125.2);
},
getAppManifest() {
return Promise.resolve(null);
},
connect() {
return Promise.resolve();
},
disconnect() {
return Promise.resolve();
},
gotoURL() {
return Promise.resolve('https://www.reddit.com/r/nba');
},
beginEmulation() {
return Promise.resolve();
},
setThrottling() {
return Promise.resolve();
},
dismissJavaScriptDialogs() {
return Promise.resolve();
},
assertNoSameOriginServiceWorkerClients() {
return Promise.resolve();
},
reloadForCleanStateIfNeeded() {
return Promise.resolve();
const fakeDriverUsingRealMobileDevice = makeFakeDriver({
protocolGetVersionResponse: {
...protocolGetVersionResponse,
// eslint-disable-next-line max-len
userAgent: 'Mozilla/5.0 (Linux; Android 6.0.1; Nexus 5 Build/MRA58N) AppleWebKit/537.36(KHTML, like Gecko) Chrome/69.0.3464.0 Mobile Safari/537.36',
},
enableRuntimeEvents() {
return Promise.resolve();
},
evaluateScriptOnLoad() {
return Promise.resolve();
},
cleanBrowserCaches() {},
clearDataForOrigin() {},
cacheNatives() {
return Promise.resolve();
},
evaluateAsync() {
return Promise.resolve({});
},
registerPerformanceObserver() {
return Promise.resolve();
},
beginTrace() {
return Promise.resolve();
},
endTrace() {
return Promise.resolve(
require('../fixtures/traces/progressive-app.json')
);
},
beginDevtoolsLog() {},
endDevtoolsLog() {
return require('../fixtures/artifacts/perflog/defaultPass.devtoolslog.json');
},
blockUrlPatterns() {
return Promise.resolve();
},
setExtraHTTPHeaders() {
return Promise.resolve();
},
};
});

module.exports = fakeDriver;
module.exports.fakeDriverUsingRealMobileDevice = fakeDriverUsingRealMobileDevice;
module.exports.protocolGetVersionResponse = protocolGetVersionResponse;
45 changes: 45 additions & 0 deletions lighthouse-core/test/gather/gather-runner-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ class TestGathererNoArtifact extends Gatherer {
}

const fakeDriver = require('./fake-driver');
const fakeDriverUsingRealMobileDevice = fakeDriver.fakeDriverUsingRealMobileDevice;

function getMockedEmulationDriver(emulationFn, netThrottleFn, cpuThrottleFn,
blockUrlFn, extraHeadersFn) {
Expand Down Expand Up @@ -156,6 +157,50 @@ describe('GatherRunner', function() {
});
});

describe('collects IsMobile and IsMobileHost as artifacts', () => {
const url = 'https://example.com';
it('works when running on desktop device without emulation', async () => {
const driver = fakeDriver;
const config = new Config({passes: [{}]});
const settings = {};
const options = {url, driver, config, settings};

const results = await GatherRunner.run(config.passes, options);
expect(results.IsMobile).toBe(false);
expect(results.IsMobileHost).toBe(false);
});
it('works when running on desktop device with mobile emulation', async () => {
mattzeunert marked this conversation as resolved.
Show resolved Hide resolved
const driver = fakeDriver;
const config = new Config({passes: [{}]});
const settings = {emulatedFormFactor: 'mobile'};
const options = {url, driver, config, settings};

const results = await GatherRunner.run(config.passes, options);
expect(results.IsMobile).toBe(true);
expect(results.IsMobileHost).toBe(false);
});
it('works when running on mobile device without emulation', async () => {
const driver = fakeDriverUsingRealMobileDevice;
const config = new Config({passes: [{}]});
const settings = {};
const options = {url, driver, config, settings};

const results = await GatherRunner.run(config.passes, options);
expect(results.IsMobile).toBe(true);
expect(results.IsMobileHost).toBe(true);
});
it('works when running on mobile device with desktop emulation', async () => {
const driver = fakeDriverUsingRealMobileDevice;
const config = new Config({passes: [{}]});
const settings = {emulatedFormFactor: 'desktop'};
const options = {url, driver, config, settings};

const results = await GatherRunner.run(config.passes, options);
expect(results.IsMobile).toBe(false);
expect(results.IsMobileHost).toBe(true);
});
});

it('sets up the driver to begin emulation when all emulation flags are undefined', () => {
const tests = {
calledDeviceEmulation: false,
Expand Down
2 changes: 2 additions & 0 deletions lighthouse-core/test/results/artifacts/artifacts.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
{
"LighthouseRunWarnings": [],
"BenchmarkIndex": 1000,
"IsMobile": true,
"IsMobileHost": false,
"HostUserAgent": "Mozilla/5.0 (Macintosh; Intel Mac OS X 10_13_3) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/66.0.3358.0 Safari/537.36",
"NetworkUserAgent": "Mozilla/5.0 (Linux; Android 6.0.1; Nexus 5 Build/MRA58N) AppleWebKit/537.36(KHTML, like Gecko) Chrome/66.0.3359.30 Mobile Safari/537.36",
"fetchTime": "2018-03-13T00:55:45.840Z",
Expand Down
4 changes: 4 additions & 0 deletions types/artifacts.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,10 @@ declare global {
fetchTime: string;
/** A set of warnings about unexpected things encountered while loading and testing the page. */
LighthouseRunWarnings: string[];
/** Whether the page was loaded on either a real or emulated mobile device */
mattzeunert marked this conversation as resolved.
Show resolved Hide resolved
IsMobile: boolean;
Copy link
Member

Choose a reason for hiding this comment

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

we might need to bikeshed on these. When I see artifacts.isMobile, my first thought is "is what mobile?" :)

It'll make a little more sense when nested in an environment artifact, but do we have other ideas?

(I also like reusing host for the UA string artifacts, but (super nitpicky) dislike the host prefix vs suffix :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

IsMobileFormFactor?

/** Whether Lighthouse was run on a mobile device (i.e. not on a desktop machine) */
mattzeunert marked this conversation as resolved.
Show resolved Hide resolved
IsMobileHost: boolean;
Copy link
Member

Choose a reason for hiding this comment

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

apparently, this is false if it was real mobile hardware but desktop was emulated.
which isn't what I expected given the name.

Copy link
Member

Choose a reason for hiding this comment

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

how about slightly different booleans?

isMobileHardware
isMobileEmulated

They will combine differently, but I think they communicate all the same things?

Copy link
Member

Choose a reason for hiding this comment

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

how about slightly different booleans?
isMobileHardware
isMobileEmulated

That kind of just punts the question back to the audits again, though. Really we just want to be able to easily answer "were my artifacts gathered in a mobile environment, whether real or emulated?".

The value should be true if run on desktop with mobile emulation, run on mobile with mobile emulation, or run on mobile with no emulation, otherwise false.

Maybe we should just drop IsMobileHost for now (I don't think anything needs it at this point?) and just pick a name we all like for isMobile/IsMobileFormFactor/isMobileEmulated?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I like isMobileHardware to replace isMobileHost 👍

when discussing @paulirish 's comment we just used the phrase isMobileConditions which I kinda like over isMobile/isMobileFormFactor. I'd want to avoid Emulated since that's not really what we are getting at

Copy link
Member

Choose a reason for hiding this comment

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

isMobileHardware
wasMobileConditions
???

the second is past tense because it's sometimes true and sometimes not.
crazy but semantically reasonable?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Renamed to FormFactorIsMobile and removed IsMobileHost from the base artifacts for now.

Copy link
Member

Choose a reason for hiding this comment

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

at first read, I'd probably have to look up whether FormFactorIsMobile meant either tested on actual mobile hardware or the definition we're going for.

What about something around TestedAsMobileDevice?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm starting to circle back around to @mattzeunert 's suggestion that this represents whether the device presented itself as mobile to the site, i.e network user agent was mobile :)

at this point though, I think we've all identified it's hard to name and we should just document well in the comment above it what it really represents

Copy link
Member

Choose a reason for hiding this comment

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

user agent is independent of form factor, though, isn't it? That's why I was suggesting we go with whatever awkward string reasonably encapsulates RanLighthouseWithADeviceThatEitherWasOrPretendedToBeAMobileOne, which would include the UA string and screen characteristics

Copy link
Contributor Author

@mattzeunert mattzeunert Mar 2, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

TestedAsMobileDevice works for me, updated the PR.

/** The user agent string of the version of Chrome used. */
HostUserAgent: string;
/** The user agent string that Lighthouse used to load the page. */
Expand Down