-
Notifications
You must be signed in to change notification settings - Fork 144
[BUGFIX] Handle incorrect RGB colors better #485
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
[BUGFIX] Handle incorrect RGB colors better #485
Conversation
Handle incorrect inputs color which have less than 6 chars
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.
Hi @ali-khalili,
Thanks for this. Good catch.
I think when an invalid property value is encountered, an UnexpectedTokenException
should be thrown. This will be caught upstream so that the invalid property is dropped (what most browsers do), rather than resulting in a property with a now-valid value (causing a change of behaviour if the rendered CSS is used in place of the input CSS).
Also, could you add an entry to CHANGELOG.md
(newest first within the 'Fixed' section)?
Thanks <3
@oliverklee, @sabberworm, do you spot any other issue?
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.
Yes, please let's discard invalid rules instead of falling back to some default.
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.
Looks good to me now. Thanks for contributing 👍
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.
Thanks! ❤️
It didn't go unnoticed that you had to wait for the CI checks to be triggered by a maintainer. Thanks for persevering. I hope we can imporove the process for future contributions/contributors (#486). |
@JakeQZ @sabberworm Is this something that would make sense to backport to 8.x? |
I think so:
|
Handle incorrect inputs color which have less than 6 chars
Done: #490 |
There is an issue when handling incorrect RGB colors which have lesser character numbers than 6. In this situation, we should probably either throw an exception OR define a fallback value for the color.
In this PR, I have updated the code to use RGB(0,0,0) as the fallback color when this situation happens.
Thank you