246667 2022-10-17 18:18:30 -0700 [WPE] checkbox and radio button aren't painted because ThemeAdwaita::m_accentColor is the invalid color 2022-10-18 01:37:39 -0700 1 1 1 Unclassified WebKit WPE WebKit WebKit Nightly Build Unspecified Unspecified RESOLVED FIXED https://bugs.webkit.org/show_bug.cgi?id=238398 P2 Normal --- 1 fujii fujii alicem bugs-noreply cgarcia oldest_to_newest 1906337 0 fujii 2022-10-17 18:18:30 -0700 [WPE] checkbox and radio button aren't painted because ThemeAdwaita::m_accentColor is the invalid color 1906338 1 463046 fujii 2022-10-17 18:24:10 -0700 Created attachment 463046 Patch 1906339 2 463046 alicem 2022-10-17 18:29:07 -0700 Comment on attachment 463046 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. 1906340 3 463046 fujii 2022-10-17 18:40:35 -0700 Comment on attachment 463046 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. 1906343 4 alicem 2022-10-17 18:43:46 -0700 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? 1906345 5 fujii 2022-10-17 18:53:52 -0700 (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. 1906346 6 fujii 2022-10-17 18:56:33 -0700 I misunderstood your comment. paintCheckbox doesn't check the validity. https://github.com/WebKit/WebKit/blob/d9124122cd00f55a24eb658fe1e12b4a47ff9513/Source/WebCore/platform/adwaita/ThemeAdwaita.cpp#L262 1906349 7 alicem 2022-10-17 19:28:13 -0700 Oh whoops, I shouldn't review patches at 6 AM. LGTM 😅️ 1906351 8 alicem 2022-10-17 19:29:21 -0700 > I misunderstood your comment. No, you didn't. I indeed misread the patch as adding that check instead of removing it. 1906412 9 ews-feeder 2022-10-18 01:37:34 -0700 Committed 255668@main (d03dce63e768): <https://commits.webkit.org/255668@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 463046. 463046 2022-10-17 18:24:10 -0700 2022-10-18 01:37:36 -0700 Patch bug-246667-20221018102409.patch text/plain 1844 fujii RnJvbSAxNWRlYjU0MjkzNmM3NzZlMDEyNmI1MzY0NmRiNWNkZTQxZDU0MjY1IE1vbiBTZXAgMTcg MDA6MDA6MDAgMjAwMQpGcm9tOiBGdWppaSBIaXJvbm9yaSA8SGlyb25vcmkuRnVqaWlAc29ueS5j b20+CkRhdGU6IFR1ZSwgMTggT2N0IDIwMjIgMTA6MTk6MDIgKzA5MDAKU3ViamVjdDogW1BBVENI XSBbV1BFXSBjaGVja2JveCBhbmQgcmFkaW8gYnV0dG9uIGFyZW4ndCBwYWludGVkIGJlY2F1c2UK IFRoZW1lQWR3YWl0YTo6bV9hY2NlbnRDb2xvciBpcyB0aGUgaW52YWxpZCBjb2xvcgogaHR0cHM6 Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dpP2lkPTI0NjY2NwoKUmV2aWV3ZWQgYnkgTk9C T0RZIChPT1BTISkuCgpUaGVtZUFkd2FpdGEgaXMgcGFpbnRpbmcgY2hlY2tib3ggYW5kIHJhZGlv IGJ1dHRvbiB3aXRoIHRoZSBhY2NlbnQKY29sb3IuIEdUSyBwb3J0IHN1cHBvcnRzIGFjY2VudCBj b2xvciwgYnV0IFdQRSBkb2Vzbid0LgpUaGVtZUFkd2FpdGE6Om1fYWNjZW50Q29sb3Igd2FzIGlu aXRpYWxpemVkIGFzIHRoZSBpbnZhbGlkIGNvbG9yIHRoYXQKd2FzIHRyYW5zcGFyZW50LgoKKiBT b3VyY2UvV2ViQ29yZS9wbGF0Zm9ybS9hZHdhaXRhL1RoZW1lQWR3YWl0YS5jcHA6CihXZWJDb3Jl OjpUaGVtZUFkd2FpdGE6OmFjY2VudENvbG9yKToKKiBTb3VyY2UvV2ViQ29yZS9wbGF0Zm9ybS9h ZHdhaXRhL1RoZW1lQWR3YWl0YS5oOgotLS0KIFNvdXJjZS9XZWJDb3JlL3BsYXRmb3JtL2Fkd2Fp dGEvVGhlbWVBZHdhaXRhLmNwcCB8IDIgKy0KIFNvdXJjZS9XZWJDb3JlL3BsYXRmb3JtL2Fkd2Fp dGEvVGhlbWVBZHdhaXRhLmggICB8IDIgKy0KIDIgZmlsZXMgY2hhbmdlZCwgMiBpbnNlcnRpb25z KCspLCAyIGRlbGV0aW9ucygtKQoKZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJDb3JlL3BsYXRmb3Jt L2Fkd2FpdGEvVGhlbWVBZHdhaXRhLmNwcCBiL1NvdXJjZS9XZWJDb3JlL3BsYXRmb3JtL2Fkd2Fp dGEvVGhlbWVBZHdhaXRhLmNwcAppbmRleCBlNDQxZTk1NmRiYjIuLjY5YzRjY2M4OWZmZiAxMDA2 NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvcGxhdGZvcm0vYWR3YWl0YS9UaGVtZUFkd2FpdGEuY3Bw CisrKyBiL1NvdXJjZS9XZWJDb3JlL3BsYXRmb3JtL2Fkd2FpdGEvVGhlbWVBZHdhaXRhLmNwcApA QCAtNTM3LDcgKzUzNyw3IEBAIHZvaWQgVGhlbWVBZHdhaXRhOjpzZXRBY2NlbnRDb2xvcihjb25z dCBDb2xvciYgY29sb3IpCiAKIENvbG9yIFRoZW1lQWR3YWl0YTo6YWNjZW50Q29sb3IoKQogewot ICAgIHJldHVybiBtX2FjY2VudENvbG9yLmlzVmFsaWQoKSA/IG1fYWNjZW50Q29sb3IgOiBTUkdC QTx1aW50OF90PiB7IDUyLCAxMzIsIDIyOCB9OworICAgIHJldHVybiBtX2FjY2VudENvbG9yOwog fQogCiB9IC8vIG5hbWVzcGFjZSBXZWJDb3JlCmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9w bGF0Zm9ybS9hZHdhaXRhL1RoZW1lQWR3YWl0YS5oIGIvU291cmNlL1dlYkNvcmUvcGxhdGZvcm0v YWR3YWl0YS9UaGVtZUFkd2FpdGEuaAppbmRleCA3ZTI4MTJlODQyOWYuLmUzOTk0YTBhNTU4ZCAx MDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvcGxhdGZvcm0vYWR3YWl0YS9UaGVtZUFkd2FpdGEu aAorKysgYi9Tb3VyY2UvV2ViQ29yZS9wbGF0Zm9ybS9hZHdhaXRhL1RoZW1lQWR3YWl0YS5oCkBA IC02MCw3ICs2MCw3IEBAIHByaXZhdGU6CiAKICAgICBzdGF0aWMgQ29sb3IgZm9jdXNDb2xvcihj b25zdCBDb2xvciYpOwogCi0gICAgQ29sb3IgbV9hY2NlbnRDb2xvcjsKKyAgICBDb2xvciBtX2Fj Y2VudENvbG9yIHsgU1JHQkE8dWludDhfdD4geyA1MiwgMTMyLCAyMjggfSB9OwogfTsKIAogfSAv LyBuYW1lc3BhY2UgV2ViQ29yZQo=