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

core(tap-targets): disable font size and tap targets audit on desktop #7393

Merged
merged 2 commits into from
Mar 6, 2019

Conversation

mattzeunert
Copy link
Contributor

Summary

These two audits aren't important to desktop SEO, so we should disable them there.

Related Issues/PRs

Closes #6687

@mattzeunert mattzeunert changed the title Disable font size and tap targets audit on desktop core(tap-targets): disable font size and tap targets audit on desktop Mar 6, 2019
Copy link
Collaborator

@patrickhulce patrickhulce left a comment

Choose a reason for hiding this comment

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

LGTM

@@ -274,6 +274,15 @@ class TapTargets extends Audit {
* @return {Promise<LH.Audit.Product>}
*/
static async audit(artifacts, context) {
if (!artifacts.TestedAsMobileDevice) {
// Tap target sizes aren't important for desktop SEO, so disable the audit there.
// On desktop people also tend to have more precise pointing devices than fingers.
Copy link
Collaborator

Choose a reason for hiding this comment

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

It might be an interesting exercise to try to find tap targets completely occluded by other elements at some point in the future 🤔

Copy link
Member

@exterkamp exterkamp left a comment

Choose a reason for hiding this comment

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

LGTM. Is the alternative to this to have a separate desktop-config that skipAudits contains font-size and tap-targets? Like lr-config ignoring uses-http2?

@paulirish paulirish merged commit d93bdf3 into master Mar 6, 2019
@paulirish paulirish deleted the disable-font-size-tap-targets-on-desktop branch March 6, 2019 17:30
@paulirish
Copy link
Member

Is the alternative to this to have a separate desktop-config

Doing a desktop run is already something the config allows users to do (disableDeviceEmulation). In this PR's approach we take on the complexity and dynamically change the runtime based on the passed in config.

The alternative is we pass the complexity to the user and have them choose which audits are appropriate given their selected configuration. I think we'd agree that is less desirable.

@jabcreations
Copy link

This kind of screw up is why I won't show Lighthouse to clients inquiring. If I have to explain your bugs for you and then show not one though two developers who think it's perfectly acceptable to run a mobile test for desktop then the entire test is invalid. That applies to all my clients for my web platform business.

@paulirish
Copy link
Member

@jabcreations hmmmm.. you don't?

The change here (from 5 years ago) made it so you don't need to worry about this.

font-size and tap-target audits are only evaluated if it's a mobile run. if it's a desktop run then these two audits will be passing.

From an SEO POV these checks are moot in desktop.

@jabcreations
Copy link

A desktop device test with Lighthouse in Chrome 122.0:
seo

So yeah, either the change didn't work or someone has since then broken it.

@connorjclark
Copy link
Collaborator

connorjclark commented Mar 25, 2024

That image shows an internal error in Lighthouse when running that audit. If you hover over it, it'll tell us which artifact error'd. The audit only marks itself inapplicable for desktop runs if it makes it past the artifact verification stage (unfortunately).

This is not related to this old PR, but if you open a new issue with reproduction steps we can look into it. Please keep in mind we have a code of conduct and your initial engagement with us is not adhering to it.

@paulirish
Copy link
Member

for eng team:

reproable on https://www.jabcreations.com/features/

image

and here's it in CDP: CSS.createStyleSheet

we're gonna drop this audit in exchange for axe, so this will get resolved via that. ✅

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

Successfully merging this pull request may close these issues.

None yet

6 participants