-
Notifications
You must be signed in to change notification settings - Fork 9.4k
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
core(emulation): refactor emulation settings & CLI flags #11779
Conversation
8895bd5
to
ce9b4f8
Compare
…creenEmulation is dropped from devtools
Co-authored-by: Brendan Kenny <bckenny@gmail.com>
types/externs.d.ts
Outdated
export type ScreenEmulationSettings = Partial<DeviceMetricsOverrideParams & {disabled: boolean}>; | ||
export type ScreenEmulationSettings = { | ||
/** Overriding width value in pixels (minimum 0, maximum 10000000). 0 disables the override. */ | ||
width: number, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we use ;
up in here
Co-authored-by: Patrick Hulce <phulce@google.com> Co-authored-by: Brendan Kenny <bckenny@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, a bit more stuff :)
coerce: coerceScreenEmulation, | ||
}, | ||
'emulatedUserAgent': { | ||
type: 'string', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
note that doing it this way means --no-emulated-user-agent
works (it gives false
), but
--emulated-user-agent
gives''
--emulated-user-agent false
gives (the string)'false'
.
better to use coerce: coerceOptionalStringBoolean,
like audit-mode
and gather-mode
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ya good call.
lighthouse-cli/cli-flags.js
Outdated
@@ -32,8 +32,8 @@ function getFlags(manualArgv) { | |||
'lighthouse <url> --output=json --output-path=./report.json --save-assets', | |||
'Save trace, screenshots, and named JSON report.') | |||
.example( | |||
'lighthouse <url> --emulated-form-factor=none --throttling-method=provided', | |||
'Disable device emulation and all throttling') | |||
'lighthouse <url> --screenEmulation.disabled --throttling-method=provided --no-emulatedUserAgent', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This definitely looks weird interspersing camelCase and kebab-case. Agreed that --screen-emulation.disabled
also looks weird, but for non-dot ones --no-emulatedUserAgent
is kind of confusing...
@@ -42,7 +42,7 @@ class FullPageScreenshot extends Gatherer { | |||
const height = Math.min(metrics.contentSize.height, MAX_SCREENSHOT_HEIGHT); | |||
|
|||
await driver.sendCommand('Emulation.setDeviceMetricsOverride', { | |||
mobile: passContext.baseArtifacts.TestedAsMobileDevice, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe worth a comment here why it's not set to the emulationSettings.mobile
? (I'm not totally sure I know, actually)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a good point. This should be screenEmulation.mobile
. In practice it doesnt matter because we already exited if there's a mismatch but I like keeping this correct!
@@ -116,8 +116,7 @@ class FullPageScreenshot extends Gatherer { | |||
|
|||
// In case some other program is controlling emulation, try to remember what the device looks | |||
// like now and reset after gatherer is done. | |||
const lighthouseControlsEmulation = passContext.settings.emulatedFormFactor !== 'none' && | |||
!passContext.settings.internalDisableDeviceScreenEmulation; | |||
const lighthouseControlsEmulation = passContext.settings.screenEmulation.disabled !== true; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const lighthouseControlsEmulation = passContext.settings.screenEmulation.disabled !== true; | |
const lighthouseControlsEmulation = passContext.settings.screenEmulation.disabled; |
?
for me, at least, !== true
twists the brain more than just setting it directly :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
agree with simplifying it, but gotta add a bang to your suggestion.
lighthouse-core/lib/emulation.js
Outdated
await driver.sendCommand('Emulation.setDeviceMetricsOverride', params.metrics); | ||
await driver.sendCommand('Emulation.setTouchEmulationEnabled', {enabled: params.touchEnabled}); | ||
// As a result, we don't double-apply viewport emulation (devtools sets `screenEmulation` to `false`). | ||
// UA emulation, however, is lost in the protocol handover from devtools frontend to the lighthouse_worker. So it's always applied. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
these comments might be worth moving to devtools-entry
since this is now pretty generalized in here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done!
"emulatedFormFactor": "mobile", | ||
"internalDisableDeviceScreenEmulation": false, | ||
"formFactor": "mobile", | ||
"emulatedUserAgent": "Mozilla/5.0 (Linux; Android 7.0; Moto G (4)) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/84.0.4143.7 Mobile Safari/537.36 Chrome-Lighthouse", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess the UA string count in the LHR is getting a bit ridiculous but not really a clean way to cut back :)
readme.md
Outdated
@@ -83,14 +87,16 @@ Configuration: | |||
--print-config Print the normalized config for the given config and options, then exit. [boolean] [default: false] | |||
--additional-trace-categories Additional categories to capture with the trace (comma-delimited). [string] | |||
--config-path The path to the config JSON. | |||
An example config file: lighthouse-core/config/lr-desktop-config.js [string] | |||
An example config file: lighthouse-core/config/lr-desktop-config.js [string] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ha, I did manually edit these ones. I guess yargs
does something fancy with the whitespace on these too?
describe: 'Determines how performance metrics are scored and if mobile-only audits are skipped. For desktop, --preset=desktop instead.', | ||
}, | ||
'screenEmulation': { | ||
describe: 'Sets screen emulation parameters. See also --preset. Use --screenEmulation.disabled to disable. Otherwise set these 4 parameters individually: --screenEmulation.mobile --screenEmulation.width=360 --screenEmulation.height=640 --screenEmulation.deviceScaleFactor=2', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
purposefully don't want to break these up like throttling
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
correct. I don't wanna grow --help even more.
// formFactor doesn't control emulation. So we don't want a mismatch: | ||
// Bad mismatch A: user wants mobile emulation but scoring is configured for desktop | ||
// Bad mismtach B: user wants everything desktop and set formFactor, but accidentally not screenEmulation | ||
if (settings.screenEmulation.mobile !== (settings.formFactor === 'mobile')) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
does this mean if you're testing on a real phone and you want to be graded like that, you need to do
formFactor: 'mobile',
screenEmulation: {
mobile: true,
disabled: true,
// ...
}
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had the same thought but it's nested within a check on disabled
so it should be fine just with disabled
, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had the same thought but it's nested within a check on
disabled
so it should be fine just withdisabled
, right?
Oops, yes, good catch. Maybe also worth calling out in the comment then?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
does this mean if you're testing on a real phone and you want to be graded like that, you need to do
aside from the defaults you get for free, yes.
Line 19 in 31a7b55
If you're using Lighthouse on a mobile device, you want to set `--screenEmulation.disabled` and `--throttling.cpuSlowdownMultiplier=1`. (`--formFactor=mobile` is the default already). |
@@ -484,6 +529,10 @@ class Config { | |||
// Locale is special and comes only from flags/settings/lookupLocale. | |||
settingsWithFlags.locale = locale; | |||
|
|||
if (settingsWithFlags.emulatedUserAgent === true) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you know, locale
was special, now everyone wants their setting to have a bespoke initialization :P
I don't know if there's a better way to do this, but leave a comment?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great to me! Way to land a huge improvement in a timeline that didn't seem possible :D
An extra check I wanted to add on top of #11779 It was described back in #10910 > the TestedAsMobileDevice logic correctly handles real-mobile-device, but doesn't handle external UA emulation. We could detect there's external UA emulation being applied and throw if we don't see configuration that matches. Or with the proposal implemented, we may just log a warning to let them know we see i We can only check this sort of mismatch after we've gathered the host UA, so it can't be done earlier.
Fixes #10910
overview
You must always set
formFactor
. It doesn't control emulation, but it determines how Lighthouse should interpret the run in regards to scoring performance metrics and skipping mobile-only tests in desktop.You can choose how
screenEmulation
is applied. It can accept an object of{width: number, height: number, deviceScaleRatio: number, mobile: boolean}
to apply that screen emulation orfalse
if Lighthouse should avoid applying screen emulation. It's typically set tofalse
if either emulation is applied outside of Lighthouse, or it's being run on a mobile device. Themobile
boolean applies overlay scrollbars and a few other mobile-specific screen emulation characteristics.You can choose how to handle userAgent emulation. The
emulatedUserAgent
property accepts either astring
to apply the provided userAgent orfalse
if no UA spoofing should be applied. Typicallyfalse
is used if UA spoofing is apply outside of Lighthouse or on a mobile device. You can also redundantly apply userAgent emulation with no risk.changes
emulatedFormFactor
property (which determined how emulation is applied).TestedAsMobileDevice
artifact is removed. Instead of being inferred, theformFactor
property is used.internalDisableDeviceScreenEmulation
property is removed. It's equivalent to the newscreenEmulation: false
.formFactor
property.screenEmulation
property.emulatedUserAgent
property.throttling
andthrottlingMethod
remain unchanged)To reviewers: i'm not sure there's a good way to break this up. Most of the test changes are rote and boring, except for
emulation-test
.TODO
providedDeviceString
,providedNetworkThrottlingString
,providedCPUThrottlingString
HTML Report renderer incorrect labels for custom throttling & device #7053