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)

47 Branch
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla49
Tracking Status
firefox47 --- wontfix
firefox48 --- fixed
firefox49 --- fixed

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).
Attached file Testcase as HTML file (deleted) —
Just for completeness, here's the testcase I used as an HTML file.
Attached image Screenshot of Bugzilla attachment form (deleted) —
And for further illustration of how annoying this is, here's what I just saw in the Bugzilla for to create an attachment.
FWIW, those invisible elements are fully functional, I just can't see where they are.
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
No longer blocks: gtk3
Depends on: 1234158
For your information, I can see this display bug on Archlinux with GTK 3.20 and Mate Desktop GTK3 enabled version.
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
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?
(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.
(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.
Attached patch patch (obsolete) (deleted) — Splinter Review
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)
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
Attachment #8751022 - Flags: review?(karlt)
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.
Attachment #8750356 - Attachment is obsolete: true
Attachment #8750356 - Flags: review?(karlt)
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
Attached patch patch v.2 (obsolete) (deleted) — Splinter Review
Thanks, there's the updated one.
Attachment #8751270 - Flags: review?(karlt)
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)
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 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 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/
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.
Attachment #8751270 - Attachment is obsolete: true
Attachment #8751270 - Flags: review?(karlt)
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
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?
Whiteboard: [uplift needs bug 1234158 landing first]
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?
Whiteboard: [uplift needs bug 1234158 landing first]
Assignee: nobody → stransky
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+
What are the chances of 47.0.2 backport? Month+ is a massively long time to wait and do work with this every day.
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.
Version: unspecified → 47 Branch
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)) { ? :)
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!
(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.

Attachment

General

Creator:
Created:
Updated:
Size: