Bug 246667 – [WPE] checkbox and radio button aren't painted because ThemeAdwaita::m_accentColor is the invalid color

• Status: RESOLVED
• Resolution: FIXED
• Component: WPE WebKit

[Fujii Hironori]

[WPE] checkbox and radio button aren't painted because ThemeAdwaita::m_accentColor is the invalid color

[Fujii Hironori]

Created attachment 463046 [details]
Patch

[Alice Mikhaylenko]

Comment on attachment 463046 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=463046&action=review

> Source/WebCore/platform/adwaita/ThemeAdwaita.cpp:-540
> -    return m_accentColor.isValid() ? m_accentColor : SRGBA<uint8_t> { 52, 132, 228 };

Is this part needed given it's about the default color? Though really might be better to set it externally, like GTK port does.

[Fujii Hironori]

Comment on attachment 463046 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=463046&action=review

>> Source/WebCore/platform/adwaita/ThemeAdwaita.cpp:-540
>> -    return m_accentColor.isValid() ? m_accentColor : SRGBA<uint8_t> { 52, 132, 228 };
> 
> Is this part needed given it's about the default color? Though really might be better to set it externally, like GTK port does.

Sorry, but I don't understand your suggestion.
GTK port is using gtk_style_context_lookup_color to the system accent color.
However, WPE can't do that.
I think it's a good idea to have the default accent color here.

[Alice Mikhaylenko]

I know it can't do that, but it can set the hardcoded default color from the same place. E.g. if WPE port adds API to set the accent color at some point, it would make it easier.

Though that's not important, my main question is why do you need to both set the default color and check color validity later - shouldn't just one of those be enough here?

[Fujii Hironori]

(In reply to Alexander Mikhaylenko from comment #4)
> I know it can't do that, but it can set the hardcoded default color from the
> same place. E.g. if WPE port adds API to set the accent color at some point,
> it would make it easier.

Yup, it seems a good idea to add a new API for WPE port.
I'm going to use RenderThemeAdwaita for WinCairo port now (bug#246604).
I think here is the good place to have the default accent color for ports without HAVE_APP_ACCENT_COLORS.

> Though that's not important, my main question is why do you need to both set
> the default color and check color validity later - shouldn't just one of
> those be enough here?

My patch actually removes the validity checking.

[Fujii Hironori]

I misunderstood your comment.
paintCheckbox doesn't check the validity. 
https://github.com/WebKit/WebKit/blob/d9124122cd00f55a24eb658fe1e12b4a47ff9513/Source/WebCore/platform/adwaita/ThemeAdwaita.cpp#L262

[Alice Mikhaylenko]

Oh whoops, I shouldn't review patches at 6 AM.

LGTM 😅️

[Alice Mikhaylenko]

> I misunderstood your comment.

No, you didn't. I indeed misread the patch as adding that check instead of removing it.

[EWS]

Committed 255668@main (d03dce63e768): <https://commits.webkit.org/255668@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 463046 [details].