Closed Bug 1271524 Opened 4 years ago Closed 3 years ago

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

Categories

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

49 Branch
defect

Tracking

()

RESOLVED FIXED
mozilla51
Tracking Status
firefox51 --- fixed

People

(Reporter: ht990332, Assigned: stransky)

References

(Blocks 2 open bugs)

Details

(Whiteboard: tpi:+)

Attachments

(2 files, 3 obsolete files)

Attached image 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
Component: Untriaged → Widget: Gtk
Product: Firefox → Core
Which GTK theme?
Depends on: 1234158
Adwaita
Thanks.
Blocks: gtk-3.20
I see that on Fedora 24/default theme.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Attached patch patch - move tabs to WidgetCache (obsolete) — Splinter Review
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.
Attached patch tab widget cache patch (obsolete) — Splinter Review
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
Attachment #8778915 - Attachment is obsolete: true
Attachment #8779338 - Flags: review?(andrew)
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.
Attachment #8779338 - Flags: review?(andrew) → review+
Attached patch tabs patch for check-in (obsolete) — Splinter Review
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.
Keywords: checkin-needed
Assignee: nobody → stransky
Attachment #8779338 - Attachment is obsolete: true
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8fe922a0b012
Move tab widget to WidgetCache. r=acomminos
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/8fe922a0b012
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
(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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
See Also: → 1190163
(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.
(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.
(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).
Priority: -- → P4
Whiteboard: tpi:+
Blocks: 1306425
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.
Status: REOPENED → RESOLVED
Closed: 3 years ago3 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.