Robert Kaiser Reporter
	Description • 8 years ago

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).

	Jan Alexander Steffens [:heftig]
	Comment 1 • 8 years ago

This needs bug 1234158

	Robert Kaiser Reporter
	Comment 2 • 8 years ago

Just for completeness, here's the testcase I used as an HTML file.

	Robert Kaiser Reporter
	Comment 3 • 8 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.

	Robert Kaiser Reporter
	Comment 4 • 8 years ago

FWIW, those invisible elements are fully functional, I just can't see where they are.

	Robert Kaiser Reporter
	Comment 5 • 8 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.

	Frederic Bezies
	Comment 6 • 8 years ago

For your information, I can see this display bug on Archlinux with GTK 3.20 and Mate Desktop GTK3 enabled version.

	Stephen Ostermiller
	Comment 15 • 8 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?

	Ronan Jouchet
	Comment 16 • 8 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.

	Jason Papakostas
	Comment 17 • 8 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.

	Martin Stránský [:stransky] (ni? me) Assignee
	Comment 18 • 8 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.

	Karl Tomlinson (:karlt)
	Comment 20 • 8 years ago

Review commit: https://reviewboard.mozilla.org/r/51761/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/51761/

	Karl Tomlinson (:karlt)
	Comment 21 • 8 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.

	Martin Stránský [:stransky] (ni? me) Assignee
	Comment 23 • 8 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

	Martin Stránský [:stransky] (ni? me) Assignee
	Comment 24 • 8 years ago

Thanks, there's the updated one.

	Martin Stránský [:stransky] (ni? me) Assignee
	Comment 25 • 8 years ago

Comment on attachment 8751270 [details] [diff] [review]
patch v.2

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=2780b6f3077c

	Karl Tomlinson (:karlt)
	Comment 26 • 8 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/

	Karl Tomlinson (:karlt)
	Comment 27 • 8 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.

	Karl Tomlinson (:karlt)
	Comment 28 • 8 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.

	Karl Tomlinson (:karlt)
	Comment 29 • 8 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/

	Karl Tomlinson (:karlt)
	Comment 30 • 8 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.

	Pulsebot
	Comment 31 • 8 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/62fd545b97fc

	Carsten Book [:Tomcat]
	Comment 32 • 8 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/62fd545b97fc

	Karl Tomlinson (:karlt)
	Comment 33 • 8 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.

	Bill Gianopoulos [:WG9s]
	Comment 35 • 8 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?

	Sylvestre Ledru [:Sylvestre]
	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

	Carsten Book [:Tomcat]
	Comment 38 • 8 years ago
bugherder uplift

https://hg.mozilla.org/releases/mozilla-aurora/rev/17ae44b60a7c

	Leho Kraav (:macmaN @lkraav)
	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.

	Karl Tomlinson (:karlt)
	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.

	Oleg Mikheev
	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)) {

? :)

	Kevin Folz
	Comment 42 • 7 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!

	Karl Tomlinson (:karlt)
	Comment 43 • 7 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.