-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Themed resources are drawn with the wrong colors if a theme's Color() method returns certain types of Colors #2220
Labels
unverified
A bug that has been reported but not verified
Comments
This was referenced May 5, 2021
andydotxyz
pushed a commit
that referenced
this issue
May 10, 2021
* Fix svg's colorToHexAndOpacity malfunctioning with most Color types, add tests, lay framework for fixing other stuff with the same issues * Further fixes to code which calls RGBA() * Moved ColorToNRGBA and unmultiplyAlpha into internal, fixed tests that created RGBAs Co-authored-by: Christophe Meessen <christophe@meessen.net>
3 tasks
AmanitaVerna
added a commit
to AmanitaVerna/fyne
that referenced
this issue
May 14, 2021
… feeling - Replaces the math used for blending the hover color in button.go's buttonRenderer's buttonColor() to fix fyne-io#2222 and also to fix fyne-io#2220 for buttonColor(). - Fixes fyne-io#2220 in button.go's newButtonTapAnimation() as well. - Adds a math-based test which uses an alternate way of calculating the same blending operation to test buttonRenderer's hover code against (TestButton_Hover_Math in button_internal_test.go) - Updates the hover test image used by button_test.go's TestButton_Hover() - Updates hover_overflow.xml, hover_all_tabs.xml, hover_create_tabs.xml, hover_second.xml, menu_bar_hovered_content.png so that the tests that use them expect the corrected output from buttonColor() instead of the previous, incorrect, output.
AmanitaVerna
added a commit
to AmanitaVerna/fyne
that referenced
this issue
May 20, 2021
- Replaces the math used for blending the hover color in button.go's buttonRenderer's buttonColor() to fix fyne-io#2222. - Fixes the last remaining cases of fyne-io#2220 that I've found, in buttonColor() and in button.go's newButtonTapAnimation(). - Adds a math-based test which uses an alternate way of calculating the same blending operation to test buttonRenderer's hover code against (TestButton_Hover_Math in button_internal_test.go) - Updates the hover test image used by button_test.go's TestButton_Hover() - Updates a bunch of hover-related tests so that they look for the corrected output from buttonColor().
3 tasks
Jacalz
added a commit
that referenced
this issue
May 27, 2021
- Replaces the math used for blending the hover color in button.go's buttonRenderer's buttonColor() to fix #2222. - Fixes the last remaining cases of #2220 that I've found, in buttonColor() and in button.go's newButtonTapAnimation(). - Adds a math-based test which uses an alternate way of calculating the same blending operation to test buttonRenderer's hover code against (TestButton_Hover_Math in button_internal_test.go). - Updates the hover test image used by button_test.go's TestButton_Hover() - Updates a bunch of hover-related tests so that they look for the corrected output from buttonColor(), by updating the four XML files and one image that they test against. Fixes #2222 Fixes #2220
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Describe the bug:
I've written a fix for this, and a bunch of tests. (I'm also going to make a pull request after posting this)
Fyne displays incorrect colors in PrimaryThemedResources (such as checkboxes) which are SVGs, when a theme's Color method returns an RGBA64, NRGBA64, RGBA with 0<a<0xff, NRGBA with 0<a<0xff, Alpha16, Gray16, or any custom Color implementation where the least significant byte of each component returned by RGBA() is not equal to the most significant byte. The correct colors are only shown with RGBA with a=0 or a=0xff, Alpha, or Gray, and those only appear correct because their RGBA() method simply clones each 8 bit component of the color into both the high and low byte of the 16 bit output components (Go's (color.RGBA) RGBA() returns r<<8|r, g<<8|g, b<<8|b, a<<8|a, where r is uint32(c.R), etc). The reason it gives incorrect colors when 0<a<0xff for RGBA and NRGBA is because of the alpha-premultiplication (RGBA has it built into the components, unless an RGBA is incorrectly constructed without it, and NRGBA's RGBA() multiplies the alpha into the r, g, and b return values), and theme/svg.go's colorToHexAndOpacity's current code is written as if it expects RGBA() to return non-alpha-premultiplied values (it doesn't remove the alpha-premultiplication) whose most significant byte matches their least significant byte (it doesn't >>8 the return values from RGBA() before casting them to bytes).
I also found the same problem in other places in Fyne's code:
I noticed this because I was returning an (opaque) NRGBA64 from a custom Theme's Color method. I have a video of that below.
(When I looked into this, I found out that svgs can't support 64 bit colors anyways, but this still seems like a bug worth fixing, especially since not all resources are SVGs, and there's other code with the same problem that doesn't use SVGs.)
To Reproduce:
Steps to reproduce the behaviour:
Video:
In this video, I change the value of an NRGBA64 color returned by a theme by dragging a hue slider (which goes from 0 to 360). The color created is opaque, and generated from converting HSL to RGB. You can see everything in the window change at the correct rate except the checkbox, which changes much more quickly because colorToHexAndOpacity is calling RGBA() and using the least significant byte of R, G, and B, instead of the most significant byte.
https://user-images.githubusercontent.com/6283593/116925385-20dd2d00-ac27-11eb-92d5-7a2c74528cfd.mp4
Example code:
These are two of the subtests I added to theme/icons_test.go's TestColorizeResource. If colorToHexAndOpacity was working correctly, the first would be #ffff00, and the second would be #ffff00 with 50% alpha. Instead, the first is #00FFFF, and the second is #7fff7f with 50% alpha.
Device (please complete the following information):
The text was updated successfully, but these errors were encountered: