-
Notifications
You must be signed in to change notification settings - Fork 255
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
BUG: Fails to apply HSL colors with a decimal point in the hue. #602
Comments
It seems rendered correct result by your (Reproduction) |
Hi, thanks for taking the time to view this issue! The problem is that, while it does work in the Vercel OG playground, it does not work in Next.js! Is there any difference between |
Just checked again to confirm and the linked repository still has the issue. |
My guess is that next.js has it own bundle of satori, which might not same with latest satori |
Testing on the latest Next canary (15.0.4-canary.32), this issue is still reproducible (see https://github.com/erxclau/satori-decimal-hue-reproduction). The Next bundle of Satori was recently bumped to 0.12.0 in vercel/next.js#72954, as shown by the ability to use
I'm not really sure how Relatedly, although the playground link sent above does work in the default tab, the PNG tag shows a black square: |
This can be reproduced in the resvg-js playground: <svg width="600" height="300" xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink">
<rect x="10" y="10" width="20" height="20" fill="hsl(20 90.2% 48.2%)"></rect>
<rect x="40" y="10" width="20" height="20" fill="hsl(20.5 90.2% 48.2%)"></rect>
</svg> |
Some more digging here. I think this happens in When parsing HSL colors, the hue parsed as an integer. I can't confirm, but I imagine having a decimal hue with this would mangle parsing. I believe the CSS color spec (v3 and 4) both suggest that the hue can be a decimal number.
|
Browsers support a color like `hsl(120.152, 100%, 75%)` but this library would incorrectly parse the hue and fallback to black. Changes parsing the hue as an integer to parsing as a number. [CSS Color Module Level 3 4.2.4](https://www.w3.org/TR/css-color-3/#hsl-color) suggests that the hue should be a number (and not strictly an integer): > Hue is represented as an angle of the color circle (i.e. the rainbow represented in a circle). This angle is so typically measured in degrees that the unit is implicit in CSS; syntactically, only a \<number\> is given. CSS Color Module Level 4 is more explicit that hue [can be a number](https://www.w3.org/TR/css-color/#typedef-hue). This PR also comments out the `parse_integer` and `parse_list_integer` functions (and relevant tests) from `src/stream.rs` because leaving them in the code produced these warnings during build: ``` warning: methods `parse_integer` and `parse_list_integer` are never used --> src/stream.rs:417:12 | 109 | impl<'a> Stream<'a> { | ------------------- methods in this implementation ... 417 | pub fn parse_integer(&mut self) -> Result<i32, Error> { | ^^^^^^^^^^^^^ ... 447 | pub fn parse_list_integer(&mut self) -> Result<i32, Error> { | ^^^^^^^^^^^^^^^^^^ | = note: `#[warn(dead_code)]` on by default ``` Happy to uncomment or remove them completely if that's preferred. Related to vercel/satori#602
Bug report
Description / Observed Behavior
When a decimal point is provided
Expected Behavior
How did you expect Satori to behave here?
Reproduction
Note: Satori functions correctly in the OG-Image Playground (Reproduction)
However, in any Next.js app, the bug occurs. I've tried the lastest next-js and satori releases as well as a few versions back. To reproduce this,
git clone https://github.com/ashwin-mahadevan/satori-decimal-hue-reproduction
, start the Next server, and navigate to/
or/?decimal=true
.or use the following code in an app-router
route.tsx
file:The text was updated successfully, but these errors were encountered: