Closed
Bug 1262136
Opened 9 years ago
Closed 9 years ago
With GTK 3.20 default theme, checkbox and radio on web pages are invisible unless checked
Categories
(Core :: Widget: Gtk, defect)
Tracking
()
RESOLVED
FIXED
mozilla49
People
(Reporter: kairo, Assigned: stransky)
References
Details
(Keywords: regression)
Attachments
(4 files, 2 obsolete files)
As seen in the attachment, in my configuration, checkboxes and radio controls on web pages are invisible unless checked. The border of the checkbox or radio is always missing, the check marker is visible though.
This is with openSUSE Tumbleweed, Plasma 5 (KDE) as the main desktop and the GTK3 "Adwaita" theme selected (GTK3 "Default" theme is the same).
Comment 1•9 years ago
|
||
This needs bug 1234158
Reporter | ||
Comment 2•9 years ago
|
||
Just for completeness, here's the testcase I used as an HTML file.
Reporter | ||
Comment 3•9 years ago
|
||
And for further illustration of how annoying this is, here's what I just saw in the Bugzilla for to create an attachment.
Reporter | ||
Comment 4•9 years ago
|
||
FWIW, those invisible elements are fully functional, I just can't see where they are.
Reporter | ||
Comment 5•9 years ago
|
||
Oh, and this is on Nightly 48 and I never saw this back when we still had GTK2, it started with GTK3, but it may be a connection of KDE/Plasma 5 and GTK3. I'm seeing this on two machines that both have openSUSE Tumbleweed, Plasma 5 and GTK3 Nightly builds.
Keywords: regression
Updated•9 years ago
|
Comment 6•9 years ago
|
||
For your information, I can see this display bug on Archlinux with GTK 3.20 and Mate Desktop GTK3 enabled version.
Updated•9 years ago
|
Updated•9 years ago
|
Updated•9 years ago
|
Summary: With GTK3 default theme, checkbox and radio on web pages are invisible unless checked → With GTK3 default theme, checkbox and radio on web pages are invisible unless checked and GTK 3.20
Comment 15•9 years ago
|
||
Are there any workarounds for this bug? Maybe, a theme that doesn't have this problem or a user style sheet that can be installed?
Comment 16•9 years ago
|
||
(In reply to Stephen Ostermiller from comment #15)
> Are there any workarounds for this bug? Maybe, a theme that doesn't have
> this problem or a user style sheet that can be installed?
https://github.com/horst3180/arc-theme doesn't have this problem.
Other workarounds still welcome, I'm using Arc by the time this bug is fixed, but I'd rather stay with vanilla Adwaita.
Comment 17•9 years ago
|
||
(In reply to Ronan Jouchet from comment #16)
> https://github.com/horst3180/arc-theme doesn't have this problem.
You're right, the master branch (but not the latest tagged release) of arc-theme has some temporary workarounds specifically for Firefox.
scrollbar workaround: https://github.com/horst3180/arc-theme/commit/935b23cc0723233861ba9c79a3aa4efb44997b3d
checkbox and radio workaround: https://github.com/horst3180/arc-theme/commit/90f1c140669cea6ce43a3112ffa3af92fe1f3531
There's also mention (https://github.com/horst3180/arc-theme/issues/457#issuecomment-209117510) of an Arch Linux patch being applied to the stable firefox package as a workaround. Probably this one https://git.archlinux.org/svntogit/packages.git/tree/trunk/firefox-gtk3-20.patch?h=packages/firefox but I didn't try it; I'm using firefox-developer.
Assignee | ||
Comment 18•9 years ago
|
||
There's a patch for checkboxes/radiobuttons, tested on gtk 3.16 and 3.20.
It uses the style direction - we should address that in another patch. I also rearranged the GetStyleInternal switch to make it more clear.
Attachment #8750356 -
Flags: review?(karlt)
Blocks: 1270609
Updated•9 years ago
|
Summary: With GTK3 default theme, checkbox and radio on web pages are invisible unless checked and GTK 3.20 → With GTK 3.20 default theme, checkbox and radio on web pages are invisible unless checked
Comment 20•9 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/51761/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/51761/
Attachment #8751022 -
Flags: review?(karlt)
Updated•9 years ago
|
Attachment #8751022 -
Flags: review?(karlt)
Comment 21•9 years ago
|
||
Comment on attachment 8751022 [details]
MozReview Request: Bug 1262136 - implement checkboxes/radiobuttons for Gtk 3.20, r=?karlt
https://reviewboard.mozilla.org/r/51761/#review48591
Thanks! Just some small things.
::: widget/gtk/WidgetStyleCache.cpp:231
(Diff revision 1)
> + GtkWidget* widget = GetWidget(aNodeType);
> + if (widget) {
> + return gtk_widget_get_style_context(widget);
> + }
> +
> + MOZ_FALLTHROUGH_ASSERT("missing style context for node type");
MOZ_FALLTHROUGH_ASSERT is an annotation to suppress compiler warnings about
switch cases that MOZ_ASSERT(false) (or its alias MOZ_ASSERT_UNREACHABLE) in
debug builds, but intentionally fall through in release builds to handle
unexpected values.
Use MOZ_ASSERT_UNREACHABLE() here.
::: widget/gtk/gtk3drawing.cpp:710
(Diff revision 1)
> - ensure_checkbox_widget();
> -
> + GtkStyleContext *style = ClaimStyleContext(MOZ_GTK_CHECKBUTTON_CONTAINER);
> + gtk_style_context_get_style(style,
> - gtk_style_context_get_style(gtk_widget_get_style_context(gCheckboxWidget),
> "indicator_size", indicator_size,
> "indicator_spacing", indicator_spacing,
> NULL);
> -
> + ReleaseStyleContext(style);
Now that we've decided to implement GetWidget() for appropriate widgets, including this one, please use that with gtk_widget_style_get() when the style context is not required.
That avoids the need for ReleaseStyleContext().
gtk_style_context_get_style() will only succeed when the style context is for a widget node anyway.
::: widget/gtk/gtk3drawing.cpp:722
(Diff revision 1)
> - ensure_radiobutton_widget();
> -
> + GtkStyleContext *style = ClaimStyleContext(MOZ_GTK_RADIOBUTTON_CONTAINER);
> + gtk_style_context_get_style(style,
> - gtk_style_context_get_style(gtk_widget_get_style_context(gRadiobuttonWidget),
> "indicator_size", indicator_size,
> "indicator_spacing", indicator_spacing,
> NULL);
> -
> + ReleaseStyleContext(style);
Similarly here.
::: widget/gtk/gtk3drawing.cpp:918
(Diff revision 1)
> - if (isradio) {
> - moz_gtk_radio_get_metrics(&indicator_size, &indicator_spacing);
> - w = gRadiobuttonWidget;
> - } else {
> - moz_gtk_checkbox_get_metrics(&indicator_size, &indicator_spacing);
> - w = gCheckboxWidget;
> - }
> + style = ClaimStyleContext(isradio ? MOZ_GTK_RADIOBUTTON_CONTAINER :
> + MOZ_GTK_CHECKBUTTON_CONTAINER);
> + gtk_style_context_get_style(style,
> + "indicator_size", &indicator_size,
> + "indicator_spacing", &indicator_spacing,
> + NULL);
> + ReleaseStyleContext(style);
GetWiget().
nullptr
::: widget/gtk/gtk3drawing.cpp:942
(Diff revision 1)
> focus_width = width + 2 * indicator_spacing;
> focus_height = height + 2 * indicator_spacing;
> -
> - style = gtk_widget_get_style_context(w);
>
> - gtk_widget_set_sensitive(w, !state->disabled);
> + style = ClaimStyleContext(isradio ? MOZ_GTK_RADIOBUTTON :
I assume GTK_STATE_FLAG_INSENSITIVE needs to be considered.
::: widget/gtk/gtk3drawing.cpp:954
(Diff revision 1)
> + gtk_render_background(style, cr, x, y, width, height);
> + gtk_render_frame(style, cr, x, y, width, height);
This looks right for 3.20, but not pre-3.20 GTK.
gtk_real_check_button_draw_indicator() uses a saved context without CLASS_CHECK/RADIO for gtk_render_background(), but the allocation dimensions correspond more closely with moz_gtk_container_paint, so I don't think that is appropriate here.
gtk_render_frame() is not used pre-3.20 AFAICS.
::: widget/gtk/gtk3drawing.cpp:972
(Diff revision 1)
> focus_x, focus_y, focus_width, focus_height);
> }
> }
> - gtk_style_context_restore(style);
>
> + gtk_style_context_set_state(style, GTK_STATE_FLAG_NORMAL);
I don't think we want to always reset the state. I think most callers will be setting the state if it is relevant (or maybe getting ClaimStyleContext() to do it), and so that will be a simpler model to follow. It may also give us a better chance of reusing the same cached style context resolved style.
::: widget/gtk/gtk3drawing.cpp:1800
(Diff revision 1)
> if (state_flags & GTK_STATE_FLAG_PRELIGHT) {
> gtk_render_background(style, cr,
> rect->x, rect->y, rect->width, rect->height);
> }
> -
> - gtk_style_context_restore(style);
> +
> + gtk_style_context_set_state(style, GTK_STATE_FLAG_NORMAL);
Skip gtk_style_context_set_state(style, GTK_STATE_FLAG_NORMAL);
::: widget/gtk/gtk3drawing.cpp:1814
(Diff revision 1)
> - if (isradio) {
> - ensure_radiobutton_widget();
> - widget = gRadiobuttonWidget;
> + GtkStyleContext *style = ClaimStyleContext(isradio ?
> + MOZ_GTK_RADIOBUTTON_LABEL :
> + MOZ_GTK_CHECKBUTTON_LABEL);
gtk_check_button_paint() draws the focus around the label using the (container) widget's style context.
ClaimStyleContext need not be implemented for _LABEL.
Updated•9 years ago
|
Attachment #8750356 -
Attachment is obsolete: true
Attachment #8750356 -
Flags: review?(karlt)
Assignee | ||
Comment 23•9 years ago
|
||
https://reviewboard.mozilla.org/r/51761/#review48591
> MOZ_FALLTHROUGH_ASSERT is an annotation to suppress compiler warnings about
> switch cases that MOZ_ASSERT(false) (or its alias MOZ_ASSERT_UNREACHABLE) in
> debug builds, but intentionally fall through in release builds to handle
> unexpected values.
>
> Use MOZ_ASSERT_UNREACHABLE() here.
done.
> Now that we've decided to implement GetWidget() for appropriate widgets, including this one, please use that with gtk_widget_style_get() when the style context is not required.
> That avoids the need for ReleaseStyleContext().
> gtk_style_context_get_style() will only succeed when the style context is for a widget node anyway.
ok
> Similarly here.
ok
> GetWiget().
>
> nullptr
ok
> I assume GTK_STATE_FLAG_INSENSITIVE needs to be considered.
That's handled by GetStateFlagsFromGtkWidgetState() which sets GTK_STATE_FLAG_INSENSITIVE to state_flags.
> This looks right for 3.20, but not pre-3.20 GTK.
>
> gtk_real_check_button_draw_indicator() uses a saved context without CLASS_CHECK/RADIO for gtk_render_background(), but the allocation dimensions correspond more closely with moz_gtk_container_paint, so I don't think that is appropriate here.
>
> gtk_render_frame() is not used pre-3.20 AFAICS.
ok
> I don't think we want to always reset the state. I think most callers will be setting the state if it is relevant (or maybe getting ClaimStyleContext() to do it), and so that will be a simpler model to follow. It may also give us a better chance of reusing the same cached style context resolved style.
ok
> Skip gtk_style_context_set_state(style, GTK_STATE_FLAG_NORMAL);
ok
> gtk_check_button_paint() draws the focus around the label using the (container) widget's style context.
>
> ClaimStyleContext need not be implemented for _LABEL.
ok
Assignee | ||
Comment 24•9 years ago
|
||
Thanks, there's the updated one.
Attachment #8751270 -
Flags: review?(karlt)
Assignee | ||
Comment 25•9 years ago
|
||
Comment on attachment 8751270 [details] [diff] [review]
patch v.2
Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=2780b6f3077c
Comment 26•9 years ago
|
||
Comment on attachment 8751022 [details]
MozReview Request: Bug 1262136 - implement checkboxes/radiobuttons for Gtk 3.20, r=?karlt
Review request updated; see interdiff: https://reviewboard.mozilla.org/r/51761/diff/1-2/
Attachment #8751022 -
Attachment description: MozReview Request: bug 1262136 add support for GTK+-3.20 checkboxes and radio buttons r?karlt → MozReview Request: Bug 1262136 - implement checkboxes/radiobuttons for Gtk 3.20, r=?karlt
Attachment #8751022 -
Flags: review?(karlt)
Comment 27•9 years ago
|
||
https://reviewboard.mozilla.org/r/51761/#review48591
> ok
Please use nullptr instead of NULL.
> That's handled by GetStateFlagsFromGtkWidgetState() which sets GTK_STATE_FLAG_INSENSITIVE to state_flags.
Ah, right. Thanks.
Comment 28•9 years ago
|
||
Comment on attachment 8751022 [details]
MozReview Request: Bug 1262136 - implement checkboxes/radiobuttons for Gtk 3.20, r=?karlt
https://reviewboard.mozilla.org/r/51761/#review49283
::: widget/gtk/gtk3drawing.cpp:949
(Diff revisions 1 - 2)
> state_flags = static_cast<GtkStateFlags>(state_flags|GTK_STATE_FLAG_INCONSISTENT);
>
> gtk_style_context_set_state(style, state_flags);
> gtk_style_context_set_direction(style, direction);
>
> + if (!gtk_check_version(3, 20, 0)) {
Please remove the trailing whitespace here.
Attachment #8751022 -
Flags: review?(karlt) → review+
Comment 29•9 years ago
|
||
Comment on attachment 8751022 [details]
MozReview Request: Bug 1262136 - implement checkboxes/radiobuttons for Gtk 3.20, r=?karlt
Review request updated; see interdiff: https://reviewboard.mozilla.org/r/51761/diff/2-3/
Comment 30•9 years ago
|
||
https://reviewboard.mozilla.org/r/51761/#review49283
> Please remove the trailing whitespace here.
I made this gtk_check_version(3, 20, 0) == nullptr to clarify that a boolean return value is not expected.
Comment 31•9 years ago
|
||
Updated•9 years ago
|
Attachment #8751270 -
Attachment is obsolete: true
Attachment #8751270 -
Flags: review?(karlt)
Comment 32•9 years ago
|
||
bugherder |
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox49:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
Comment 33•9 years ago
|
||
Comment on attachment 8751022 [details]
MozReview Request: Bug 1262136 - implement checkboxes/radiobuttons for Gtk 3.20, r=?karlt
Approval Request Comment
[Feature/regressing bug #]: regression in GTK code
https://bugzilla.gnome.org/show_bug.cgi?id=757503
[User impact if declined]:
Invisible checkboxes for people testing latest GTK.
[Describe test coverage new/current, TreeHerder]:
Manual testing only.
[Risks and why]:
There is a risk in older versions due to changes in code for focus indication around xul checkbox labels.
That still works with Adwaita (for Fedora), and Ambiance (for Ubuntu) didn't render focus before this patch anyway.
Otherwise changes should only affect latest GTK where the affected controls are currently invisible.
[String/UUID change made/needed]:
none.
Attachment #8751022 -
Flags: approval-mozilla-aurora?
Updated•9 years ago
|
Whiteboard: [uplift needs bug 1234158 landing first]
Comment 35•9 years ago
|
||
Now that we have scrollbars under gtk3 it would be really nice to have checkboxes too. Can we get this landed on aurora before the next uplift, so it will get into Firefox 48?
Updated•9 years ago
|
Whiteboard: [uplift needs bug 1234158 landing first]
Updated•9 years ago
|
Assignee: nobody → stransky
status-firefox48:
--- → affected
Comment 37•8 years ago
|
||
Comment on attachment 8751022 [details]
MozReview Request: Bug 1262136 - implement checkboxes/radiobuttons for Gtk 3.20, r=?karlt
Improve gtk3 support, taking it
Attachment #8751022 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment 38•8 years ago
|
||
bugherder uplift |
Comment 39•8 years ago
|
||
What are the chances of 47.0.2 backport? Month+ is a massively long time to wait and do work with this every day.
Comment 40•8 years ago
|
||
This won't be uplifted to 47.
I recommend using Firefox Beta if using GTK+ 3.20. They are each at a similar stage in their development processes.
Updated•8 years ago
|
status-firefox47:
--- → wontfix
Updated•8 years ago
|
Version: unspecified → 47 Branch
Comment 41•8 years ago
|
||
I see this issue in FF 50.0.2 and GTK+ 3.22.4
I hope it's not because of
> + if (!gtk_check_version(3, 20, 0)) {
? :)
Comment 42•8 years ago
|
||
Wow this was as tricky bug report to find (until I realized it was GTK related), try searching for something like "firefox radio buttons background missing". This may be fixed for some, it still was a problem for me running 50.0~b11+build1-0ubuntu0.14.04.1 with libgtk-3-0 3.18.9-1ubuntu3.1 tried safe-mode, hardware acceleration, font settings, firefox themes, upgrading gtk libs, etc. to no avail.
I run Xubuntu 14.04, which was originally installed as 13.10 and upgraded along the way. Current theme for me was "Greybird". I found the theme changing tool in XFCE (Settings -> Appearance). Adwaita, High Contrast, and Raleigh do work, a bunch didn't.
I don't recall ever changing the theme in the first place. So suggestion to users is to try different themes that may be installed on your system.
Thank you for your hard work. Glad this is resolved!
Comment 43•8 years ago
|
||
(In reply to kevinf from comment #42)
> Wow this was as tricky bug report to find (until I realized it was GTK
> related), try searching for something like "firefox radio buttons background
> missing". This may be fixed for some, it still was a problem for me running
> 50.0~b11+build1-0ubuntu0.14.04.1 with libgtk-3-0 3.18.9-1ubuntu3.1 tried
> safe-mode, hardware acceleration, font settings, firefox themes, upgrading
> gtk libs, etc. to no avail.
>
> I run Xubuntu 14.04, which was originally installed as 13.10 and upgraded
> along the way. Current theme for me was "Greybird". I found the theme
> changing tool in XFCE (Settings -> Appearance). Adwaita, High Contrast, and
> Raleigh do work, a bunch didn't.
This will not fix your issue as the change here was just for GTK+ 3.20.
libgtk-3-0 3.18.9-1ubuntu3.1 doesn't sound like the version usually on Xubuntu 14.04. Each GTK theme will only work with the GTK version for which it is designed.
If your Greybird theme was backported with the libgtk, then please file a new bug, but it sounds like an incompatibility between Greybird and libgtk.
You need to log in
before you can comment on or make changes to this bug.
Description
•