Skip to content

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

Merged
merged 4 commits into from
Jun 29, 2022
Merged

Conversation

jckr
Copy link
Contributor

@jckr jckr commented Apr 9, 2021

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

jckr added 2 commits April 5, 2021 15:28
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.',
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
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.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
// 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) {
Copy link
Collaborator

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?

Copy link
Contributor

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

Copy link
Collaborator

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)

@connorjclark
Copy link
Collaborator

fyi we've have many CI failures lately, you'll need to do a git pull && git merge origin/master.

@simonbesters
Copy link

Any chance this PR is going to be picked up anytime soon? Would really help me. Thank you.

@ForbiddenEra
Copy link

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?

@connorjclark
Copy link
Collaborator

connorjclark commented Jun 21, 2022

@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?

What issues are there?

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.

@connorjclark connorjclark changed the title Work with "application/xhtml+xml" documents core: return result for xhtml, but with warning Jun 29, 2022
@connorjclark connorjclark merged commit 4c46d4e into master Jun 29, 2022
@connorjclark connorjclark deleted the issue-11482 branch June 29, 2022 22:11
@ForbiddenEra
Copy link

@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?

What issues are there?

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!!

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

Successfully merging this pull request may close these issues.

6 participants