-
Notifications
You must be signed in to change notification settings - Fork 9.5k
core: return result for xhtml, but with warning #12351
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
Conversation
This allows LH to audit files that have an application/xhtml+xml mime type with a warning, instead of a fatal run.
/** | ||
* @description Warning that the page is using the xhtml mime type | ||
*/ | ||
warningXhtml: 'The page is using an XHTML mime type.', |
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.
warningXhtml: 'The page is using an XHTML mime type.', | |
warningXhtml: 'The page MIME type is XHTML: Lighthouse does not explicitly support this document type.', |
@@ -48,6 +53,10 @@ const UIStrings = { | |||
*/ | |||
const SLOW_CPU_BENCHMARK_INDEX_THRESHOLD = 1000; | |||
|
|||
// MIME types are case-insenstive but Chrome normalizes MIME types to be lowercase. |
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.
// MIME types are case-insenstive but Chrome normalizes MIME types to be lowercase. | |
// MIME types are case-insensitive but Chrome normalizes MIME types to be lowercase. |
// Error if page is not HTML. | ||
if (nonHtmlError) return nonHtmlError; | ||
|
||
if (finalRecord?.mimeType === XHTML_MIME_TYPE) { |
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.
@brendankenny I _think we can use ?.
but want to double check. Just shouldn't in the report, 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 think we can use
?.
but want to double check. Just shouldn't in the report, right?
not yet, we're still waiting for the switch to node 14 minimum
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 can now use this syntax)
fyi we've have many CI failures lately, you'll need to do a |
Any chance this PR is going to be picked up anytime soon? Would really help me. Thank you. |
What issues are there? Simply changing the content-type http header for my test sites make them all pass. Am I literally going to have to set the content-type to text/html for robots/lightspeed, so I don't get nerfed on google or attempts to get listed in Play Store with a TWA'd PWA? |
@jckr can you give us a signal if you'd like to continue work / address feedback? Otherwise, would someone else like to open a new PR doing the same?
As you can see from reading this PR...this PR is an external contribution and there is some unaddressed feedback. In the future, please refrain from commenting on every related issue with the same message-it's just spam and is not helpful. |
Yeah; I hadn't intended on doing that but kept finding different related/similar ones and wasn't sure where to comment. Regardless: I really appreciate you championing this and pushing it through so quickly, thanks!! |
Summary
This PR is in response to #11482. Currently, we can't run audits on webpages which have the mime type "application/xhtml+xml" - we get nothing. With this PR, we'll get a warning but the audit will run.
Related Issues/PRs