1271524 - Tabs in certificate viewer are not themed at all under gtk3.20 (looks like windows95)

Status: RESOLVED FIXED

(Core :: Widget: Gtk, defect, P4)

Description

[Hussam Al-Tayeb]

Attachment: Notice the tabs

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:49.0) Gecko/20100101 Firefox/49.0
Build ID: 20160509214821

Steps to reproduce:

Tabs in certificate viewer dialog are not themed at all under gtk3.20 (looks like windows95).
I compiled against gtk+ 3.20.3

Comment 1

[Karl Tomlinson (:karlt back Feb 10)]

Which GTK theme?

Comment 2

[Hussam Al-Tayeb]

Adwaita

Comment 3

[Karl Tomlinson (:karlt back Feb 10)]

Thanks.

Comment 4

[Martin Stránský [:stransky] (ni? me) (PTO, back on Feb 17)]

I see that on Fedora 24/default theme.

Comment 5

[Martin Stránský [:stransky] (ni? me) (PTO, back on Feb 17)]

Attachment: patch - move tabs to WidgetCache

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

This patch does not fix the 3.20 tabs rendering, just moves the tab widget/styles to WidgetCache and needs to be tested on pre-3.20 gtk.

The rendering fix needs to be addressed in another patch.

Comment 6

[Martin Stránský [:stransky] (ni? me) (PTO, back on Feb 17)]

Attachment: tab widget cache patch

Updated widget cache patch. Also fixes Gtk2 part, tested on gtk2,3 and pre/post 3.20. The 3.20 is still broken and needs extra patch.
Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=702ffd306f07

Comment 7

[Andrew Comminos [:acomminos]]

Comment on attachment 8779338 [details] [diff] [review]
tab widget cache patch

Review of attachment 8779338 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, thanks!

::: widget/gtk/gtk2drawing.c
@@ +3021,5 @@
>  
>  gint
>  moz_gtk_get_tab_border(gint* left, gint* top, gint* right, gint* bottom, 
> +                       GtkTextDirection direction, GtkTabFlags flags,
> +                      WidgetNodeType widget)

Minor alignment issue with WidgetNodeType here.

::: widget/gtk/gtk3drawing.cpp
@@ +2378,5 @@
>      moz_gtk_add_style_padding(style, left, top, right, bottom);
>  
> +    // Gtk >= 3.20 does not use those styles
> +    bool pre_3_20 = (gtk_check_version(3, 20, 0) != nullptr);
> +    if (pre_3_20) {

Don't need to declare a new variable for this, just throw the call to gtk_check_version into the conditional.

::: widget/gtk/nsNativeThemeGTK.cpp
@@ +638,5 @@
>      break;
>    case NS_THEME_TAB:
>      {
> +      if (IsBottomTab(aFrame)) {
> +          aGtkWidgetType = MOZ_GTK_TAB_BOTTOM;

2 space indent here.

Comment 8

[Martin Stránský [:stransky] (ni? me) (PTO, back on Feb 17)]

Attachment: tabs patch for check-in

Thanks, there's a patch for check-in.
Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=9ce0b8b100d3
A patch from Bug 1278108 needs to be checked-in first.

Comment 9

[Martin Stránský [:stransky] (ni? me) (PTO, back on Feb 17)]

Attachment: fixed tabs patch for check-in

Fixed one. 
Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=9c0df252afe8

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8fe922a0b012
Move tab widget to WidgetCache. r=acomminos

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/8fe922a0b012

Comment 12

[Hussam Al-Tayeb]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #11)
> https://hg.mozilla.org/mozilla-central/rev/8fe922a0b012

Are you sure this fixes the actual bug? Comment 5 and comment 6 suggest the actual rendering will be fixed in an upcoming patch.

Comment 13

[Martin Stránský [:stransky] (ni? me) (PTO, back on Feb 17)]

(In reply to Hussam Al-Tayeb from comment #12)
> Are you sure this fixes the actual bug? Comment 5 and comment 6 suggest the
> actual rendering will be fixed in an upcoming patch.

To be fair here I have no idea how to fix tab rendering on gtk3.20+ with the nss nodes and I doubt it's worth to invest much to this bug. 

The options UI is already running as html so it makes more sense to me to also move the certificate viewer there.

Comment 14

[Hussam Al-Tayeb]

(In reply to Martin Stránský from comment #13)
> The options UI is already running as html so it makes more sense to me to
> also move the certificate viewer there.

That will be a good solution.

Comment 15

[Hussam Al-Tayeb]

(In reply to Hussam Al-Tayeb from comment #14)
> (In reply to Martin Stránský from comment #13)
> > The options UI is already running as html so it makes more sense to me to
> > also move the certificate viewer there.
> 
> That will be a good solution.

On second thought, that may not work well unless "Page info" becomes html themed as well.
A third option is to use some new fancy css styling for those tabs (similar to what the "Developer" theme does for browser tabs).

Comment 16

[Karl Tomlinson (:karlt back Feb 10)]

I'm resolving this "fixed" just to mean that a change was made.
That helps us to track what changed in which revision.
Bug 1306425 can track 3.20-specific workarounds.