-
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(viewport): include meta viewport string in debugDetails #15727
Conversation
// Set a mobile-friendly viewport only on load. | ||
const metaViewport = document.querySelector('meta[name="viewport"]'); | ||
metaViewport.content = 'width=device-width, initial-scale=1, minimum-scale=1'; |
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 really important that we take the viewport content
from the element (not parsed from the tag in the html) because site can and do change it.
We already do this, but we don't currently have any test coverage for that fact :)
It probably makes sense for the terrible dbw page to have a weird viewport instead of a nice one, but doing so changes test expectations for way too many assertions, so it isn't worth the effort.
})); | ||
}); | ||
|
||
it('recognizes interactive-widget property', async () => { | ||
const viewport = 'width=device-width, interactive-widget=resizes-content'; | ||
const {parserWarnings} = await ViewportMeta.compute_(makeMetaElements(viewport)); | ||
assert.equal(parserWarnings[0], undefined); |
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.
driveby change of assert.equal(parserWarnings[0], undefined)
to assert.equal(parserWarnings.length, 0)
because the first one seems like it's asserting that there is at least one warning, but its value is undefined
. Not sure if this was important at the time, but the length check seems correct now, at least (happened in #14664 @connorjclark, maybe it had to do with the metaviewport-parser
release workaround or something?)
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 probably just copied it from the place you changed below 🤷
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've dipped my toes into #15627 a bit and discovered that reality is even more nuanced than the proposed changes listed there. However, any analysis after the fact is difficult because we don't include the page's actual meta viewport
content
anywhere in the LHR, just whether or not it passed the audit's current mobile viewport checks.This PR just includes the full viewport string in the audit's debugData.