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

Add isMobile boolean to audit context / create Environment artifact #7043

Closed
patrickhulce opened this issue Jan 17, 2019 · 9 comments
Closed
Assignees

Comments

@patrickhulce
Copy link
Collaborator

Summary
In content width, we branch our logic based on if the audit was on a mobile page or not. I could see plugins wanting to do something similar. Moving this to an artifact or context boolean could be helpful.

Note: Created from a TODO in code so it can be more visible and properly prioritized

@mattzeunert
Copy link
Contributor

I want to do this so we can use it in #6687.

Should we use the Environment artifact approach? Right now the isMobile logic uses artifacts.HostUserAgent, so it we don't know the isMobile value upfront.

@patrickhulce
Copy link
Collaborator Author

I like the idea of an Environment artifact 👍It could even be a base artifact so other gatherers could rely on it if needed.

@mattzeunert
Copy link
Contributor

mattzeunert commented Feb 17, 2019

Cool, I didn't know about base artifacts 🙂. Seems like they're already very similar to an Environment artifact, so we could just add isMobile there? Or refactor it and move stuff like HostUserAgent and BenchmarkIndex into Environment.

Right now we use the HostUserAgent rather than the NetworkUserAgent to detect isMobile. Is that correct when Lighthouse is run on a real mobile device?

Do we even need HostUserAgent? I guess we want it in the LHR so we know what Chrome version was used to generate the report, but should it be an artifact?


Found this relevant PR: #5871

@patrickhulce
Copy link
Collaborator Author

Seems like they're already very similar to an Environment artifact, so we could just add isMobile there?

I think we want something similar to what content-width does now. We want to know both isMobileHost and isMobile/isMobileFromSitesPerspective where isMobileFromSitesPerspective === (isMobileHost && !isEmulatingNonMobile) || isEmulatingMobile.

Right now we use the HostUserAgent rather than the NetworkUserAgent to detect isMobile. Is that correct when Lighthouse is run on a real mobile device?

We use the host user agent combined with our emulation settings to compute the above. When NetworkUserAgent is the same as JS-side user agent isMobileFromSitesPerspective === (isMobileHost && !isEmulatingNonMobile) || isEmulatingMobile is probably the same as just using the NetworkUserAgent yes 👍

Do we even need HostUserAgent? I guess we want it in the LHR so we know what Chrome version was used to generate the report, but should it be an artifact?

Its primary purposes is to know what Chrome version Lighthouse ran under. I guess to flip this question. It's a fact gathered from the browser/page, and we want to surface it in the report. Where should we put it if we didn't put it in artifacts?

@mattzeunert
Copy link
Contributor

I think we want something similar to what content-width does now. We want to know both isMobileHost and isMobile/isMobileFromSitesPerspective where isMobileFromSitesPerspective === (isMobileHost && !isEmulatingNonMobile) || isEmulatingMobile.

Do you mean we want an Environment base artifact with isMobileHost and isMobile properties? I don't think we have have a need to look at isMobileHost right now.

We use the host user agent combined with our emulation settings to compute the above. When NetworkUserAgent is the same as JS-side user agent isMobileFromSitesPerspective === (isMobileHost && !isEmulatingNonMobile) || isEmulatingMobile is probably the same as just using the NetworkUserAgent yes 👍

Ok, I'll change it to NetworkUserAgent then. I can't test this easily, but right now we would not detect a real mobile device without any emulation as mobile? Because host user agent will always be desktop Chrome, right?

Its primary purposes is to know what Chrome version Lighthouse ran under. I guess to flip this question. It's a fact gathered from the browser/page, and we want to surface it in the report. Where should we put it if we didn't put it in artifacts?

Could we pass the driver into _gatherArtifactsFromBrowser? And then we have the driver in Runner.run and can call getBrowserVersion there.

I think we don't want people to use HostUserAgent in their audits, and right now it's tempting to just require it. In my mind HostUserAgent is more like lighthouseVersion than an artifact.

@patrickhulce
Copy link
Collaborator Author

I don't think we have have a need to look at isMobileHost right now.

First good use I can think of would be for automatically disabling our CPU throttling if you've attached to a real device.

I can't test this easily, but right now we would not detect a real mobile device without any emulation as mobile? Because host user agent will always be desktop Chrome, right?

Hmmm, I'm confused. Why not? Host user agent would be a mobile user agent if you're running on a real mobile device. That's why we check isMobileHost && !isEmulatingNonMobile

I think we don't want people to use HostUserAgent in their audits, and right now it's tempting to just require it. In my mind HostUserAgent is more like lighthouseVersion than an artifact.

I don't really see it this way. It's useful to know exactly what device/user agent Lighthouse was running on. Not just what we were emulating. The OS, device type, and other goodies (not just Chrome version) are usually in there.

@mattzeunert
Copy link
Contributor

Host user agent would be a mobile user agent if you're running on a real mobile device.

Oh, I didn't get that, it all makes sense then. I thought desktop Chrome was somehow involved when running on a mobile device.

Will create an Environment base artifact with isMobile and isMobileHost then.

@Joelsz
Copy link

Joelsz commented Aug 12, 2019

On Chrome 76 - where Lighthouse was updated from 4.1.0 to 5.2.0 (including this issue) - we are not able to get the Mobile site audited, only the Desktop site is loading during the Mobile audit.

Anything I can do to fix this?

@Joelsz
Copy link

Joelsz commented Aug 12, 2019

OK, "It's a Chrome bug, we're working on a fix." #9476

(Thanks to @patrickhulce on twitter)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants