Skip to content
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

Closed
AmanitaVerna opened this issue May 5, 2021 · 0 comments
Labels
unverified A bug that has been reported but not verified

Comments

@AmanitaVerna
Copy link
Contributor

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:

  • dialog/base.go's themedBackgroundRenderer's Refresh()
  • test/markup_renderer.go's markupRenderer's setColorAttrWithDefault
  • test/markup_renderer.go's rgbaColor
  • widget/button.go's buttonColor() (which appears to have another issue as well, which I'll have to make a separate issue about)

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:

  1. Colorize a resource with NRGBA64{R: 0xff00, G: 0xffff, B: 0x00ff, A: 0xffff}.
  2. Draw it.
  3. It looks cyan when it should look yellow.
  4. Do it again but give it an alpha value of 0x7fff.
  5. It looks like a translucent light green color when it should look like a translucent yellow color.

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.

		"NRGBA64": {
			svgFile: "circles.svg",
			color:     color.NRGBA64{R: 0xff00, G: 0xffff, B: 0x00ff, A: 0xffff},
			wantImage: "colorized/circles_yellow.png",
		},
		"translucent NRGBA64": {
			svgFile:   "circles.svg",
			color:     color.NRGBA64{R: 0xff00, G: 0xffff, B: 0x00ff, A: 0x7fff},
			wantImage: "colorized/circles_yellow_translucent.png",
		},

Device (please complete the following information):

  • OS: Windows 10 Pro
  • Version: 20H2
  • Go version: 1.16.3 windows/amd64
  • Fyne version: 083f034 (which is the last commit on the develop branch at time of writing)
@AmanitaVerna AmanitaVerna added the unverified A bug that has been reported but not verified label 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>
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().
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
Labels
unverified A bug that has been reported but not verified
Projects
None yet
Development

No branches or pull requests

1 participant