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

RESOLVED FIXED in Firefox 51

Status

()

Core
Widget: Gtk
P4
normal
RESOLVED FIXED
2 years ago
a year ago

People

(Reporter: Hussam Al-Tayeb, Assigned: stransky)

Tracking

(Blocks: 2 bugs)

49 Branch
mozilla51
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox51 fixed)

Details

(Whiteboard: tpi:+)

Attachments

(2 attachments, 3 obsolete attachments)

(Reporter)

Description

2 years ago
Created attachment 8750605 [details]
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
(Reporter)

Updated

2 years ago
Component: Untriaged → Widget: Gtk
Product: Firefox → Core
Which GTK theme?
Depends on: 1234158
(Reporter)

Comment 2

2 years ago
Adwaita
Thanks.
Blocks: 1264079
(Assignee)

Comment 4

2 years ago
I see that on Fedora 24/default theme.
Status: UNCONFIRMED → NEW
Ever confirmed: true
(Assignee)

Comment 5

a year ago
Created attachment 8778915 [details] [diff] [review]
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.
(Assignee)

Comment 6

a year ago
Created attachment 8779338 [details] [diff] [review]
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
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+
(Assignee)

Comment 8

a year ago
Created attachment 8782877 [details] [diff] [review]
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.
(Assignee)

Comment 9

a year ago
Created attachment 8783404 [details] [diff] [review]
fixed tabs patch for check-in

Fixed one. 
Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=9c0df252afe8
Attachment #8782877 - Attachment is obsolete: true
(Assignee)

Updated

a year ago
Keywords: checkin-needed
Assignee: nobody → stransky
Attachment #8779338 - Attachment is obsolete: true

Comment 10

a year ago
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
Last Resolved: a year ago
status-firefox51: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
(Reporter)

Comment 12

a year ago
(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.
(Reporter)

Updated

a year ago
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
See Also: → bug 1190163
(Assignee)

Comment 13

a year ago
(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.
(Reporter)

Comment 14

a year ago
(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.
(Reporter)

Comment 15

a year ago
(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
Last Resolved: a year agoa year ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.