fix: Add favicon to html report

[brettz9]

Add favicon no-op

Avoid missing favicon requests being reported; also make consistent self-closing tag style and remove redundant type=text/css on <style>

[coreyfarrell]

Thank you for the contribution however from what I can see this favicon is not actually no-op. In currently my own browser (Firefox 69.0.1 on Fedora) shows no icon in the tab. This change results in a blank favicon being shown for the browser tab (a spacer). My understanding is that this may cause other browsers to display the empty icon rather than the browser icon. So I'm leaning towards not wanting to merge this favicon data url. I think the least harm is for us to allow the favicon.ico to be requested and result in a 404.

I have no complaints about making the self-closing consistent, this seems like a good idea.

Although I have no specific issue with removing type='text/css' from the style element I worry about potential issues with very old browsers. Although the type attribute is redundant in modern browsers was it always? My understanding is that this default html report works in very old browsers.

[brettz9]

Re: the favicon, why do you think users will want the browser icon to show up anyways? I can understand wanting one's own favicon, but that would be a request for customizing it (which I'd be ok with as well).

The reason I consider this important enough to submit a PR for it is that particularly during testing, I do not like to see red flags anywhere, and the 404's end up in red on console. It is also not a good practice for testers getting used to ignoring red.

Re: type=text/css, HTML 4 technically required it, but browser implementations regularly diverged from the spec, and as I recall, browsers never imposed this requirement. I do not have a source for that recollection, however. But this defaulting behavior is not only a part of HTML5, but the spec states (and the spec took pains to enforce existing behavior where practical):

Authors should not specify a type attribute on a style element. If the attribute is present, its value must be an ASCII case-insensitive match for "text/css"

[coreyfarrell]

Re: the favicon, why do you think users will want the browser icon to show up anyways? I can understand wanting one's own favicon, but that would be a request for customizing it (which I'd be ok with as well).

I'm not really interested in seeing the browser icon, Firefox currently shows no icon in the tab bar for coverage reports. Your change causes a blank space to shift the title. In any case if we're going to do this then it needs to use the istanbuljs icon:

This icon would need to be created as a static asset and linked by relative URL the same way we link to stylesheet's.

The reason I consider this important enough to submit a PR for it is that particularly during testing, I do not like to see red flags anywhere, and the 404's end up in red on console. It is also not a good practice for testers getting used to ignoring red.

I'm not clear what browser actually shows errors for missing favicon, it's extremely common for favicon to be missing (sometimes intentionally). Reducing the amount of title text visible in the browser tab is an undesirable change so I asked for justification.

Re: type=text/css, HTML 4 technically required it, but browser implementations regularly diverged from the spec, and as I recall, browsers never imposed this requirement. I do not have a source for that recollection, however. But this defaulting behavior is not only a part of HTML5, but the spec states (and the spec took pains to enforce existing behavior where practical):

Authors should not specify a type attribute on a style element. If the attribute is present, its value must be an ASCII case-insensitive match for "text/css"

"should not" is not the same as "must not". If it's technically required by HTML 4 and ignored by HTML 5 then we have no reason to remove it. We have no idea what browsers are being used to view this report but we do know that the presence of this attribute causes no harm anywhere. Please leave it.

FYI another patch had to be merged to eliminate the chronic problem of handlebars vulnerabilities, this conflicted with your PR. packages/istanbul-reports/lib/html/templates/head.txt no longer exists, that template code is now part of packages/istanbul-reports/lib/html/index.js, the htmlHead function.

[brettz9]

Here's a complaint with screenshot on how Chrome adds an error for a favicon 404 in its Developer console: https://stackoverflow.com/questions/30693021/chrome-developer-tools-shows-favicon-404-error-in-brackets-livepreview

Besides seeing this in Chrome with the network info logging on (which is otherwise helpful), I see 404 errors in red on my static web server log (node-static).

I really do seem to remember that browsers never imposed the HTML4 requirement re: the style type, but without a source, I can understand the desire for caution. I've avoided adding this change to the rebase.

Thank you very much for the background and info on how to find the new template location. I've now rebased and also pointed to the istanbuljs icon in case you agree to go forward.

[coreyfarrell]

@brettz9 Would you mind running npx eslint . --fix and committing the change it makes to index.js? Once that's done the tests will pass and I will merge this.

[coreyfarrell]

This caused a test failure on html-spa, I pushed an additional commit to fix that test. Will let it run through CI then merge.

[brettz9]

Was just fixing it, though see you got there first. :) Thanks...

[brettz9]

This is great, thanks very much!