Proposal: remove valid color checking from PWA requirements #9623
Comments
|
is removing an audit a "breaking change"? |
To clarify, it's not removing an existing audit. It's just changing the behavior of our two existing audits (themed-omnibox and splash-screen) to not fail just because a color was parsed as invalid. |
|
For reference, it was from the Chromium source code for manifest parsing: where I found that https://developers.google.com/web/tools/lighthouse/audits/manifest-contains-background_color |
|
FYI |
|
I'm +1 on removing valid color checking. let's do it. |
isn't that generally true of most of our PWA audits, though? How many sites have a manifest at all? My main issue with dropping it is that invalid CSS colors are treated as unset, so they really won't have a theme color set if the color is invalid. It does look like we were missing a bunch of other functionality in cssstyle specifically for checking colors by trying to just What other new bugs came with up updating to 2.0.0? |
|
(less important, but I also don't think "whack-a-mole problem" is really applicable. AFAICR we added color parsing from devtools-frontend way back at the dawn of time, then switched to this library when we pulled out the last bit of WebInspector :) |
|
@patrickhulce I mean if I guess there's a whitelist of some sorts on Lighthouse and it's missing |
Correct, the version of the library we use does not contain
~6% of sites have an installable manifest. So the upper bound on invalid color applicability to PWA developers is ~.8% (
They don't implement hex color validation correctly so for example our invalid test
I'm probably conflating with our validation of other PWA values that have popped up in iOS, but between the two we've had at least 4 updates to these sorts of validators and still have lots of missing invalid values.
This is fine but my bigger issue is that when we knowingly have incomplete coverage of a problem, whatever the reason may be, falsely failing for correct code is worse than warning it might have been a mistake especially when our accuracy is nearly as bad as a coin flip. |
|
So I ran the spreadsheet of values through an actual setting of a css color to see which ones were valid, then compared against what the library can do. If we
we end up with 8 out of 4910 sites in that HTTPArchive data incorrectly flagged as invalid (8 out of 1265 unique color strings in that sheet). A breakdown:
These could all be fixed pretty easily upstream (as well as the need for Color strings incorrectly flagged as valid is a slightly bigger set, but we're also less worried about bugging users with these. We end up with 29 out of 4910 sites in that HTTPArchive data we consider valid when they're actually invalid (16 out of 1265 unique color strings in that sheet).
|
|
I recently submitted a PR to fix the regex used in clean-css: https://github.com/jakubpawlowicz/clean-css/pull/1083/files I reworked it a bit since then, but here's a Regex that would follow the CSS spec:
It can look really confusing, but if follow the code it's pretty logical. You can see the gist for it here: https://gist.github.com/clshortfuse/acffca0a5f1412edcf9e132f33febe1c As for the point of the EOF parsing, ECMAScript doesn't support There are some things to note:
I'm not sure about too sure about the performance though. It might faster to trim it first, and convert to lowercase and checking against the named colors first. Edit: Forgot about comments, so I had to add it in. I added in |
|
@clshortfuse if we still go this route the changes would be made upstream in |
|
To be clear, I was never attempting to claim it's impossible to fix all the issues in cssstyle :) I'm arguing that the large gap is evidence of the fact that we'd essentially have to fill this gap ourselves and maintain this library for very little LH user value. To quote a great mind,
^^ this. On top of the tiny percentage of users this affects makes this just not seem worth it. |
|
I wrote the crazy regex, and I kinda agree. It's going to be a cat and mouse game with standards. Validating the content of the color properties is really more of a courtesy than necessity. Also, the Manifest spec itself is still in a working draft state. And the draft even lists that color processing steps are incorrect with a reference to this GitHub issue: w3c/manifest#760 So realistically speaking, the process we implement now may change again later. There's also no direction as to how to handle EOF or non-terminated functions On another note, ensuring a string is parse-able (regex), parsing, and validating are all different things. Having to validate or parse the color is a bit more work than just ensuring it can be parsed. I also have no idea what to do with And if we're referencing an external library that's purpose is to validate (and not just parse), Edit: My terminology was mixed up with parsing/validating. Ensuring "parse-able" would regex check on the string. Reading the values into Objects would be "parsing". Converting those Objects into other formats would be related to validation. |
|
We could close the gap between implementation and our color validation by not writing our own color validation (or rather, relying on when the color is invalid, blink pushes an error to the parser: https://cs.chromium.org/chromium/src/third_party/blink/renderer/modules/manifest/manifest_parser.cc?l=164&rcl=65b3e11ce92ca41af626cb68ec84b795c90453ad These end up as console errors: https://cs.chromium.org/chromium/src/third_party/blink/renderer/modules/manifest/manifest_manager.cc?l=194&rcl=ab4ecdb6a2dacf0a35344c2baa9509a9659eec86 |
|
I think that's a great approach @connorjclark, do we know if we need to listen on a specific target for those? I'm not seeing those messages in our existing |
|
that doesn't check meta |
But conceptually we could extend it to the exact same thing if that became the priority of the PWA guidelines, no? As in, Chrome would then have a vested interest in validating the meta tag not just the manifest value. |
|
The only two places we use We decided to pick up discussion next time though :) |
I'm more agnostic today about how we validate colors, but I do think some of the PWA audits suffer from a recency bias in these type of conversations. In a world where we're happy to pull in |
|
While I cannot deny the applicability argument for your |
|
We've decided to move forward with dropping the color checking here. |
patrickhulce
added
P2
and removed
needs-discussion
needs-investigation
needs-priority
labels
paulirish
added
bundlesize
needs-discussion
needs-investigation
needs-priority
and removed
P2
labels
patrickhulce
added
P2
and removed
needs-discussion
needs-investigation
needs-priority
labels
|
Assuming we're still good with dropping valid color checking (in |
Summary
So after #3891 (comment) I went to update our
cssstyledependency to fix thergba(0,0,0,0)bug, but then it turns out the new version that fixes that introduces several other bugs. The valid color checking has been a whack-a-mole problem for years and it feels not very useful so I was curious what it looks like on real sites.According to July HTTPArchive data...
Only .08% of sites ever trigger our invalid color logic and of that set ~42% are actually valid colors that we incorrectly flag as invalid (see trix).
Proposal A: Change invalid color check to a warning on PWA themed-omnibox and splash-screen. It feels like this is a warning-level thing anyway and we're almost wrong as often as we're right.
Proposal B: Remove invalid color check entirely. 20KB for something useful to .05% of sites feels like a bit much, false warnings aren't great either.
The text was updated successfully, but these errors were encountered: