Theme issues with GTK 3.14

RESOLVED FIXED in Firefox 40

Status

()

Core
Widget: Gtk
RESOLVED FIXED
3 years ago
a year ago

People

(Reporter: heftig, Assigned: stransky)

Tracking

Trunk
mozilla40
x86_64
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox40 fixed)

Details

Attachments

(8 attachments, 14 obsolete attachments)

71.57 KB, image/png
Details
51.86 KB, image/png
Details
4.57 KB, patch
karlt
: review+
RyanVM
: checkin+
Details | Diff | Splinter Review
10.03 KB, patch
RyanVM
: checkin+
Details | Diff | Splinter Review
61.79 KB, image/png
Details
3.11 KB, patch
karlt
: review+
RyanVM
: checkin+
Details | Diff | Splinter Review
23.89 KB, patch
RyanVM
: checkin+
Details | Diff | Splinter Review
18.93 KB, patch
RyanVM
: checkin+
Details | Diff | Splinter Review
(Reporter)

Description

3 years ago
Created attachment 8495433 [details]
Screenshot from 2014-09-25 20:32:46.png

Since upgrading from GTK 3.12 to GTK 3.14, Firefox using GTK3 has several problems drawing widgets:

- Check boxes and radio buttons are missing the check marks or dots.
- Unless another button is focused, the 'default' button of most dialogs is drawn with a double border.
- The selection color is black on very-light-gray, which doesn't stand out.

All of these issues are visible in the profile manager.

Also, tab strips (e.g. in the advanced preferences) have no borders. This already was the case in GTK 3.12.

Firefox 35.0a1 ca1cd09259a5 (also reproduced using 34.0a2)
Adwaita (default) GTK theme
GNOME 3.14
Arch Linux x86_64
(Reporter)

Updated

3 years ago
Blocks: 627699

Comment 1

3 years ago
Have you found a theme that works better with Firefox?  I am wondering if adwaita is just really buggy -- it also causes problems with libreoffice...
(Reporter)

Comment 2

3 years ago
No; I do not use custom themes.

Comment 3

3 years ago
this issue is irritating enough to make me want to use them.  I look forward to hearing about workarounds if you find any.  

ps, I meant GTK themes -- just realized that language was ambivalent.
Blocks: 1091136

Comment 4

3 years ago
Created attachment 8525814 [details]
zukitre workaround

(In reply to Matt Price from comment #3)
> I look forward to hearing about workarounds if you find any.  

Like Zukitre theme? Here's a screenshot from gtk-3.15.0.
(Assignee)

Comment 5

3 years ago
There are more issues there - wrong colors for selected text for instance. I'll look at it.
Assignee: nobody → stransky
Status: UNCONFIRMED → NEW
Ever confirmed: true
(Assignee)

Comment 6

3 years ago
Created attachment 8525997 [details] [diff] [review]
text-color.patch

Fixes color of selected text - view use is more appropriate here, selection color is controled by view:selected CSS property. 

I changed the regular text colors too, sMozFieldBackground and sMozFieldText also use the view text colors. But that can be reverted, the selection matters here.
Attachment #8525997 - Flags: review?(karlt)
Comment on attachment 8525997 [details] [diff] [review]
text-color.patch

The change from background to view style class makes sense, but I think
background makes more sense for eColorID_WindowBackground.

Please rename sMozWindowSelected* to sTextSelected* or similar if they are no
longer for the window background.

See also eColorID_highlight(text).  If they are the same, then they should
share the same code, and it may no longer be necessary to keep mViewStyle on
nsLookAndFeel.

The built-in theme in gtk2 has.

.view:selected {
  background-color: shade (@bg_color, 0.9);
  color: @fg_color;
}

.view:selected:focused {
  background-color: @selected_bg_color;
  color: @selected_fg_color;
}

eColorID_TextSelectBackgroundDisabled is for the unfocused version, so I think 
GTK_STATE_FLAG_FOCUSED | GTK_STATE_FLAG_SELECTED is required here.
Attachment #8525997 - Flags: review?(karlt) → review-
(Assignee)

Comment 8

3 years ago
Created attachment 8528372 [details] [diff] [review]
text color patch v.2

Thanks, there's an updated one. Merged colors, added the "FOCUSED" flag.
Attachment #8525997 - Attachment is obsolete: true
Attachment #8528372 - Flags: review?(karlt)
Comment on attachment 8528372 [details] [diff] [review]
text color patch v.2

Sorry I wasn't clear before.

eColorID__moz_fieldtext, eColorID_TextSelectBackground,
eColorID_TextSelectForeground should use GTK_STYLE_CLASS_VIEW, as in this
patch.  But eColorID_WindowBackground, and eColorID_WindowForeground should
continue to use GTK_STYLE_CLASS_BACKGROUND.  It is likely that a theme will
use different colors for view and window backgrounds, and this is true for the
built-in theme in GTK+-3.12.2.

The sTextSelected* changes look good.
Attachment #8528372 - Flags: review?(karlt) → review-
(Assignee)

Comment 10

3 years ago
Created attachment 8529096 [details] [diff] [review]
checkboxes.patch

Patch for check-box rendering in gtk 3.14. We need to add a new flag GTK_STATE_FLAG_CHECKED here.
Attachment #8529096 - Flags: review?(karlt)
Comment on attachment 8529096 [details] [diff] [review]
checkboxes.patch

>Bug 1073117 - Theme issues with GTK 3.14 - fixed checkbox rendering, r=?karlt

"Add new GTK_STATE_FLAG_CHECKED for checkbox rendering".

So this is fallout from
https://git.gnome.org/browse/gtk+/commit/gtk/gtktogglebutton.c?id=4e077d4638b4ecfaf67afbab5082b9d47499af4f

Having both active and checked set seems a reasonable compromise to handle the ABI change.  A possibly better alternative is to use the runtime gtk_check_version() to determine which to add, but I don't know whether themes are distinguishing between checked and depressed yet, so this is fine for now if you like.

Can you define GTK_STATE_FLAG_CHECKED in an enum or #define when !GTK_CHECK_VERSION(3,14,0) so that it can be set even in builds that are compatible with older GTK versions, please?
Attachment #8529096 - Flags: review?(karlt)
(Assignee)

Comment 12

3 years ago
(In reply to Karl Tomlinson (:karlt) from comment #11)
> Comment on attachment 8529096 [details] [diff] [review]
> checkboxes.patch
> 
> >Bug 1073117 - Theme issues with GTK 3.14 - fixed checkbox rendering, r=?karlt
> 
> "Add new GTK_STATE_FLAG_CHECKED for checkbox rendering".
> 
> So this is fallout from
> https://git.gnome.org/browse/gtk+/commit/gtk/gtktogglebutton.
> c?id=4e077d4638b4ecfaf67afbab5082b9d47499af4f
> 
> Having both active and checked set seems a reasonable compromise to handle
> the ABI change.  A possibly better alternative is to use the runtime
> gtk_check_version() to determine which to add, but I don't know whether
> themes are distinguishing between checked and depressed yet, so this is fine
> for now if you like.

Actually both flags are used (ACTIVE and CHECKED). ACTIVE means that the checkbox background looks like a pit and CHECKED state renders the check itself (both radio and checkbox).
(Assignee)

Comment 13

3 years ago
Created attachment 8529692 [details] [diff] [review]
checkbox v.2

Thanks, there's an updated one.
Attachment #8529096 - Attachment is obsolete: true
Attachment #8529692 - Flags: review?(karlt)
(Assignee)

Comment 14

3 years ago
Created attachment 8529730 [details] [diff] [review]
selected text color fix v.3

Thanks, there's the color fix.
Try: https://tbpl.mozilla.org/?tree=Try&rev=881caf54ee26
Attachment #8528372 - Attachment is obsolete: true
Attachment #8529730 - Flags: review?(karlt)
(In reply to Martin Stránský from comment #12)
> Actually both flags are used (ACTIVE and CHECKED). ACTIVE means that the
> checkbox background looks like a pit and CHECKED state renders the check
> itself (both radio and checkbox).

Yes, ACTIVE and CHECKED are both used, but for different things.  ACTIVE (like
a pit) indicates that the mouse button is down over the checkbox.

"The new :checked state replaces :active for the actual checkedness of the
widgets and :active is now used exclusively while the button is being
pressed."
Comment on attachment 8529692 [details] [diff] [review]
checkbox v.2

>+#if !GTK_CHECK_VERSION(3,14,0)
>+#define GTK_STATE_FLAG_CHECKED 0
>+#endif

Please define GTK_STATE_FLAG_CHECKED as "(1 << 11)" so that the checked flag
is passed even when built against older GTK.
(I'm expecting older themes to ignore irrelevant flags.)
Attachment #8529692 - Flags: review?(karlt) → review+
Comment on attachment 8529730 [details] [diff] [review]
selected text color fix v.3

Please leave eColorID__moz_fieldtext, sMozFieldText, and sMozFieldBackground,
using GTK_STYLE_CLASS_VIEW, as they are now.  Those colors are not necessarily
the same as the window colors.  (If there is a problem with those colors, then
we need to find a different solution.)
Attachment #8529730 - Flags: review?(karlt) → review-
(Reporter)

Comment 18

3 years ago
(In reply to Karl Tomlinson (:karlt) from comment #15)
> Yes, ACTIVE and CHECKED are both used, but for different things.  ACTIVE
> (like
> a pit) indicates that the mouse button is down over the checkbox.
> 
> "The new :checked state replaces :active for the actual checkedness of the
> widgets and :active is now used exclusively while the button is being
> pressed."

To clarify what I think is meant here: If GTK >=3.14 is detected at runtime, only CHECKED should be used, not ACTIVE|CHECKED. The difference isn't that great with the default theme, but it's better not to make assumptions about others.

Proper support for 3.14's ACTIVE to render the pressed state would be an enhancement that can be done at a later time.
(Assignee)

Updated

3 years ago
Duplicate of this bug: 1091136
(Assignee)

Comment 20

3 years ago
Created attachment 8531235 [details] [diff] [review]
checkbox-v3.patch

Okay, there's a patch with active or checked state.
Attachment #8529692 - Attachment is obsolete: true
Attachment #8531235 - Flags: review?(karlt)
Attachment #8531235 - Flags: review?(karlt) → review+
(Assignee)

Comment 21

3 years ago
Created attachment 8532538 [details] [diff] [review]
selected-text-color-v4.patch

Thanks, there's the new one.
Attachment #8529730 - Attachment is obsolete: true
Attachment #8532538 - Flags: review?(karlt)
Comment on attachment 8532538 [details] [diff] [review]
selected-text-color-v4.patch

>     sMozWindowBackground = GDK_RGBA_TO_NS_RGBA(color);
>+
>+    // Text colors
>     gtk_style_context_get_color(style, GTK_STATE_FLAG_NORMAL, &color);
>     sMozWindowText = GDK_RGBA_TO_NS_RGBA(color);

I think it is OK to leave this comment out, which would help clarify that this text color is only for use with window background.
Attachment #8532538 - Flags: review?(karlt) → review+
(Assignee)

Comment 23

3 years ago
Created attachment 8533089 [details] [diff] [review]
selected text color v.5 patch
Attachment #8532538 - Attachment is obsolete: true
(Assignee)

Comment 24

3 years ago
Comment on attachment 8533089 [details] [diff] [review]
selected text color v.5 patch

Try run: https://tbpl.mozilla.org/?tree=Try&rev=043e85cf8e49
(Assignee)

Updated

3 years ago
Keywords: checkin-needed
(Assignee)

Updated

3 years ago
Whiteboard: [leave open for more patches]
https://hg.mozilla.org/integration/mozilla-inbound/rev/276096579e7d
https://hg.mozilla.org/integration/mozilla-inbound/rev/3be54f6835a7
Keywords: checkin-needed
Attachment #8531235 - Flags: checkin+
Attachment #8533089 - Flags: checkin+
(Assignee)

Comment 26

3 years ago
We also need a fix for:

- Wrong buttons rendering (small size, small focus ring)
- Wrong tab rendering (gtk3 in 3.14 does not render tabs, just a small blue line under selected tab)
(Assignee)

Comment 27

3 years ago
- Also entry boxes are too hight.
https://hg.mozilla.org/mozilla-central/rev/276096579e7d
https://hg.mozilla.org/mozilla-central/rev/3be54f6835a7

Comment 29

3 years ago
Created attachment 8541306 [details]
Screenshoot with outdated widgets

Alle the widgets look like they are using outdated GTK
(Assignee)

Comment 30

3 years ago
Created attachment 8546573 [details] [diff] [review]
entry & button sizes

Theme borders has been removed in Gtk 3.14 so we can't use them to calculate button size. Should computed as border + padding. Also update nsNativeThemeGTK::GetWidgetPadding() for that. It also needs focus rendering update which will be addressed in another patch.
Attachment #8546573 - Flags: review?(karlt)
(Assignee)

Comment 31

3 years ago
Created attachment 8547463 [details] [diff] [review]
button focus

"interior-focus" and "focus-padding" has been removed in Gtk 3.14 so reflect that and use border as focus size like Gtk 3.14.
Attachment #8547463 - Flags: review?(karlt)
(Assignee)

Comment 32

3 years ago
Created attachment 8548919 [details] [diff] [review]
focus sizes

Removes obsoleted focus style properties and replaced by border and padding.
Attachment #8548919 - Flags: review?(karlt)
(Assignee)

Comment 33

3 years ago
Comment on attachment 8548919 [details] [diff] [review]
focus sizes

combobox size calculation is wrong here, 
calculate_button_inner_rect() may be the culprit.
Attachment #8548919 - Flags: review?(karlt)
(Assignee)

Comment 34

3 years ago
Created attachment 8550338 [details] [diff] [review]
no-gap-tab

Gtk 3.14 introduces new theme for tabs, without the expander.
Attachment #8550338 - Flags: feedback?(karlt)
Comment on attachment 8547463 [details] [diff] [review]
button focus

>Bug 1073117 - Fixed Theme issues with GTK 3.14 - GtkButtons - use border style 
>property for focus rendering. Don't follow interior-focus which is always true in Gtk 3.14.
>r=?karlt

Can you mention interior focus and focus padding on the first line, please?
Something like:

"
Bug 1073117 draw buttons with interior focus always and ignore focus-padding r=karlt

Ignore interior-focus style property and use only style context 
border for focus positioning, as in GTK 3.14.
"

(The interior-focus change to GTK may have been compatible with many existing
themes, but the focus-padding change would only be compatible with those that
changed the values of focus-line-width and focus-padding to zero.)
Attachment #8547463 - Flags: review?(karlt) → review+
Comment on attachment 8546573 [details] [diff] [review]
entry & button sizes

>Bug 1073117 - Theme issues with GTK 3.14 - fix gtk button and entry size, r=?karlt

Can you explain the effect on entry widgets, and the need for the change,
please?
Can that be a separate patch?

>Theme borders has been removed in Gtk 3.14 so we can't use them to calculate button size.

>+            style = gtk_widget_get_style_context(gButtonWidget);
>+            moz_gtk_add_style_border(style, left, top, right, bottom);
>+
>             /* Don't add this padding in HTML, otherwise the buttons will
>                become too big and stuff the layout. */
>             if (!inhtml) {
>-                moz_gtk_widget_get_focus(gButtonWidget, &interior_focus, &focus_width, &focus_pad);
>-                moz_gtk_button_get_inner_border(gButtonWidget, &inner_border);
>-                *left += focus_width + focus_pad + inner_border.left;
>-                *right += focus_width + focus_pad + inner_border.right;
>-                *top += focus_width + focus_pad + inner_border.top;
>-                *bottom += focus_width + focus_pad + inner_border.bottom;
>+                moz_gtk_add_style_padding(style, left, top, right, bottom);
>             }
> 
>             moz_gtk_add_style_border(gtk_widget_get_style_context(gButtonWidget), 
>                                      left, top, right, bottom);

inner-border hasn't been used in GTK widgets since 3.4, and, since 3.14, all
buttons are considered interior-focus, and focus-line-width and focus-padding
are ignored, so I understand replacing them with the padding.  Please be more
specific in the comment as I didn't understand "Theme borders".  (Themes can still specify style context (CSS) borders, which have not been removed.  Container borders also still exist, but I don't know whether or not themes control these.)

However, I don't understand adding the same style_border a second time.

>@@ -992,16 +992,21 @@ nsNativeThemeGTK::GetWidgetPadding(nsDev
>   switch (aWidgetType) {
>     case NS_THEME_BUTTON_FOCUS:
>     case NS_THEME_TOOLBAR_BUTTON:
>     case NS_THEME_TOOLBAR_DUAL_BUTTON:
>     case NS_THEME_TAB_SCROLLARROW_BACK:
>     case NS_THEME_TAB_SCROLLARROW_FORWARD:
>     case NS_THEME_DROPDOWN_BUTTON:
>     case NS_THEME_TOOLBAR_BUTTON_DROPDOWN:
>+    case NS_THEME_FOCUS_OUTLINE:
>+    case NS_THEME_NUMBER_INPUT:
>+    case NS_THEME_TEXTFIELD:
>+    case NS_THEME_TEXTFIELD_MULTILINE:
>+    case NS_THEME_BUTTON:

The effect of this change is to prevent content/chrome CSS from adding additional padding.
Is that the intention?  If so, why?
Attachment #8546573 - Flags: review?(karlt) → review-
(Assignee)

Comment 37

3 years ago
Created attachment 8561515 [details] [diff] [review]
button+entry size v.2

Thanks for the comments. I think this patch clears the logic behind it. It just use moz_gtk_get_widget_border() for the size (border+padding) computation for buttons and entries. Entries already do so and buttons are updated by this patch. 

When the final size is computed by moz_gtk_get_widget_border() we don't need nsNativeThemeGTK::GetWidgetPadding() to get the extra padding so that's why buttons and entries were added here to have a zero padding.
Attachment #8546573 - Attachment is obsolete: true
Attachment #8561515 - Flags: review?(karlt)
(Assignee)

Comment 38

3 years ago
btw. The double moz_gtk_add_style_border() call for buttons was a mistake here.
(Assignee)

Comment 39

3 years ago
Created attachment 8562699 [details] [diff] [review]
focus sizes v.2

This patch:

- removes obsoleted focus properties from gtk3 code, leaves that in gtk2 only (so those are removed from gtkdrawing.h)

- interior-focus is considered always on

- replaces focus-line-width and focus-padding with border and padding. Padding is applied in moz_gtk_get_widget_border() thus the padding is removed from nsNativeThemeGTK::GetWidgetPadding()

- new "outline-width" property is used for outline focus size
Attachment #8548919 - Attachment is obsolete: true
Attachment #8562699 - Flags: feedback?(karlt)
(Assignee)

Comment 40

3 years ago
Comment on attachment 8561515 [details] [diff] [review]
button+entry size v.2

Let's split this patch to button and entry parts.
Attachment #8561515 - Flags: review?(karlt)
Sorry about the slow response here.  I have a large font handling rewrite patch to review, and that is holding me up.
Comment on attachment 8562699 [details] [diff] [review]
focus sizes v.2

Removal of moz_gtk_button_get_inner_border requires attachment 8561515 [details] [diff] [review] for the
MOZ_GTK_BUTTON border, I assume.

>     gint focus_width = 0;

>+    gtk_style_context_get(style, 0, "outline-width", &focus_width, NULL);

The existing interior_focus code in moz_gtk_get_focus_outline_size doesn't
really make much sense.  The code is using GtkEntry to draw focus outlines,
which is not ideal (bug 1031662) and this widget uses focus-line-width only
with interior focus.

Similarly outline-width and outline-offset are used with only with
gtk_render_focus, but GtkEntry only used gtk_render_focus when not interior
focus.  Now it is always interior focus.

I think the focus outline code is disabled anyway, but given the current
GtkEntry implementation, removing the outline-width code would make the size
consistent with the implementation.

In calculate_button_inner_rect():

>     /* This mirrors gtkbutton's child positioning */
>-    moz_gtk_button_get_inner_border(button, &inner_border);
>-    moz_gtk_widget_get_focus(button, &interior_focus,
>-                             &focus_width, &focus_pad);
>-
>-    if (ignore_focus)
>-        focus_width = focus_pad = 0;
>-

>+    if (!ignore_focus)
>+        gtk_style_context_get_padding(style, 0, &padding);
>+

The "mirrors" comment applies to border and padding, rather than ignore_focus.

ignore_focus should be called inHTMLDropdown or similar now.
I suspect it has little to do with focus.

>-    inner_rect->x = rect->x + border.left + focus_width + focus_pad;

>+    inner_rect->x = rect->x + border.left;
>     inner_rect->x += direction == GTK_TEXT_DIR_LTR ?
>-                        inner_border.left : inner_border.right;

>+                        padding.left : padding.right;

gtkbutton.c doesn't use the padding differently according to direction.  I
suspect the gtk_widget_set_direction() call in moz_gtk_button_paint is
sufficient, and direction should not be used here.

>     case MOZ_GTK_CHECKBUTTON_LABEL:
>     case MOZ_GTK_RADIOBUTTON_LABEL:

>+            moz_gtk_add_style_border(gtk_widget_get_style_context(w), 
>+                                     left, top, right, bottom);

What was the reasoning for adding the border here?
Should padding be included also?
Attachment #8562699 - Flags: feedback?(karlt) → feedback+
(In reply to Karl Tomlinson (:karlt) from comment #42)
> >     case MOZ_GTK_CHECKBUTTON_LABEL:
> >     case MOZ_GTK_RADIOBUTTON_LABEL:
> 
> >+            moz_gtk_add_style_border(gtk_widget_get_style_context(w), 
> >+                                     left, top, right, bottom);
> 
> What was the reasoning for adding the border here?
> Should padding be included also?

IIUC the border and padding from the widget should be applied to the MOZ_GTK_CHECKBUTTON_CONTAINER, and the gtk_render_focus() call from moz_gtk_toggle_label_paint will probably draw outside the label rect.

I haven't seen any gtk_render_focus() calls in GTK matching those in moz_gtk_toggle_paint().
(Assignee)

Comment 44

3 years ago
Created attachment 8580694 [details] [diff] [review]
focus v.3

Thanks, there's the updated one. It also unifies the border code for button and entries, uses padding only for !inhtml elements.

> In calculate_button_inner_rect():
> The "mirrors" comment applies to border and padding, rather than ignore_focus.
> ignore_focus should be called inHTMLDropdown or similar now.
> I suspect it has little to do with focus.

IMHO that reduces element size when it's used in html. No idea if that actually has any effect and I can remove it.

> case MOZ_GTK_CHECKBUTTON_LABEL:
> case MOZ_GTK_RADIOBUTTON_LABEL:

Removed completely and moved to MOZ_GTK_CHECKBUTTON_CONTAINER only with the padding.
Attachment #8561515 - Attachment is obsolete: true
Attachment #8562699 - Attachment is obsolete: true
Attachment #8580694 - Flags: review?(karlt)
Comment on attachment 8580694 [details] [diff] [review]
focus v.3

r+ with a comment change indicated below.

(In reply to Martin Stránský from comment #44)
> It also unifies the border code for button
> and entries, uses padding only for !inhtml elements.

>     case MOZ_GTK_ENTRY:
>         {
>             ensure_entry_widget();
>             style = gtk_widget_get_style_context(gEntryWidget);
>             moz_gtk_add_style_border(style, left, top, right, bottom);
>-            moz_gtk_add_style_padding(style, left, top, right, bottom);
>+
>+            /* Don't add this padding in HTML, otherwise the entries will
>+               become too big and stuff the layout. */
>+            if (!inhtml) {
>+                moz_gtk_add_style_padding(style, left, top, right, bottom);
>+            }
>+

Not having the padding should be good I think, but the reasoning is different
from MOZ_GTK_BUTTON (or if the reasoning is the same, then the comment for
MOZ_GTK_BUTTON is not helpful).  (This would have been easier to review in a
separate patch, but it doesn't matter now.)

forms.css sets 1 pixel padding on input elements, so we have the choice of
either this padding or that from the GTK theme, but we shouldn't use both.

So please change the comment to "Use the document padding in HTML." or
similar.

Perhaps only document padding should be used in XUL also, and there is no need
for the !inhtml test.  textbox has plenty of padding.  I don't know, but feel
free to remove the !inhtml test and adjust the comment if you like.

> > In calculate_button_inner_rect():
> > The "mirrors" comment applies to border and padding, rather than ignore_focus.
> > ignore_focus should be called inHTMLDropdown or similar now.
> > I suspect it has little to do with focus.
> 
> IMHO that reduces element size when it's used in html. No idea if that
> actually has any effect and I can remove it.

The size has been determined at this stage using either
moz_gtk_get_widget_border for MOZ_GTK_DROPDOWN or
moz_gtk_get_combo_box_entry_button_size for MOZ_GTK_DROPDOWN_ARROW.
calculate_button_inner_rect() may have effect on the size of arrows and
separators in combo box buttons, so I expect it should be consistent with the
size calculations.

It may be fine to remove the ignore_focus variable and always add padding, but
the MOZ_GTK_DROPDOWN code in moz_gtk_get_widget_border() should be consistent.
(moz_gtk_get_combo_box_entry_button_size is currently inconsistent.)
Attachment #8580694 - Flags: review?(karlt) → review+
(Assignee)

Comment 46

3 years ago
(In reply to Karl Tomlinson (:karlt) from comment #45)
> > > In calculate_button_inner_rect():
> > > The "mirrors" comment applies to border and padding, rather than ignore_focus.
> > > ignore_focus should be called inHTMLDropdown or similar now.
> > > I suspect it has little to do with focus.
> > 
> > IMHO that reduces element size when it's used in html. No idea if that
> > actually has any effect and I can remove it.
> 
> The size has been determined at this stage using either
> moz_gtk_get_widget_border for MOZ_GTK_DROPDOWN or
> moz_gtk_get_combo_box_entry_button_size for MOZ_GTK_DROPDOWN_ARROW.
> calculate_button_inner_rect() may have effect on the size of arrows and
> separators in combo box buttons, so I expect it should be consistent with the
> size calculations.
> 
> It may be fine to remove the ignore_focus variable and always add padding,
> but
> the MOZ_GTK_DROPDOWN code in moz_gtk_get_widget_border() should be
> consistent.
> (moz_gtk_get_combo_box_entry_button_size is currently inconsistent.)

Sorry, I don't quite understand what do you mean here. 

moz_gtk_get_widget_border for MOZ_GTK_DROPDOWN uses a composition of borders and element sizes. 

moz_gtk_get_combo_box_entry_button_size() for NS_THEME_DROPDOWN_BUTTON just calls gtk_widget_get_preferred_size() which is supposed to compute the border+element sizes internally and return just the result.

You say that moz_gtk_get_combo_box_entry_button_size() is inconsistent, so do you suggest to replace gtk_widget_get_preferred_size() by a composition like we have for MOZ_GTK_DROPDOWN? Or would you like to use gtk_widget_get_preferred_size() for more items instead?
(Assignee)

Comment 47

3 years ago
(In reply to Karl Tomlinson (:karlt) from comment #45)
> The size has been determined at this stage using either
> moz_gtk_get_widget_border for MOZ_GTK_DROPDOWN or
> moz_gtk_get_combo_box_entry_button_size for MOZ_GTK_DROPDOWN_ARROW.
> calculate_button_inner_rect() may have effect on the size of arrows and
> separators in combo box buttons, so I expect it should be consistent with the
> size calculations.
> 
> It may be fine to remove the ignore_focus variable and always add padding,
> but
> the MOZ_GTK_DROPDOWN code in moz_gtk_get_widget_border() should be
> consistent.
> (moz_gtk_get_combo_box_entry_button_size is currently inconsistent.)

The thing is, would you like to correlate code for those two widgets in nsNativeThemeGTK::GetWidgetBorder() or nsNativeThemeGTK::GetMinimumWidgetSize()? That's the point I believe.
Yes, this is confusing.  I was referring to consistency in the drawing
functions (using calculate_button_inner_rect()) with the functions affecting
size.  I misunderstood the code in one place, sorry, and I may not have
understood what you meant by "remove it".  I am assuming you meant remove the
ignore_focus variable from calculate_button_inner_rect() and instead always
include the padding as if ignore_focus was false.

The NS_THEME_DROPDOWN_BUTTON/MOZ_GTK_DROPDOWN_ARROW drawing code is actually
consistent with the size code (despite what I said).  It is not considered a
container in Gecko.  i.e. its size is not determined by any child elements,
and so it's size is from GetMinimumWidgetSize() and
gtk_widget_get_preferred_size(), as you say.  gtk_widget_get_preferred_size()
is returning the appropriate size describing the outside of the border box
around gComboBoxEntryButtonWidget.  This size includes padding from the GTK
theme, and moz_gtk_combo_box_entry_button_paint() correctly tells
calculate_button_inner_rect() not to ignore the padding.  For this widget
type, I don't think it matters what moz_gtk_get_widget_border() or
GetWidgetBorder() return because there are no child elements affected.

For NS_THEME_DROPDOWN/MOZ_GTK_DROPDOWN, the drawing code is also currently
consistent with the size code.  This is a container widget in Gecko and so the
size is determined by the GetWidgetBorder(), the size of child elements (for
option text labels, I assume), and any document specified padding (because
GetWidgetPadding() returns false).  There is no default padding from forms.css
so the document padding is usually zero.  Currently GTK theme padding is
included only when not in HTML (i.e. only when in XUL), both in drawing and
border code.  My point is that if you want to always include GTK theme padding
in the drawing code, which sounds good to me, then I expect it should also
always be included in the border code.

Regardless, attachment 8580694 [details] [diff] [review] is good to land with just the comment change.
ignore_focus is a separate issue and can keep that name for now if you like.
(Assignee)

Comment 49

3 years ago
Created attachment 8583038 [details] [diff] [review]
focus-v.3 for check-in

Thanks for the explanation, there's the patch with adjusted comment for check-in. Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=5578eb2b0362
Attachment #8580694 - Attachment is obsolete: true
(Assignee)

Updated

3 years ago
Attachment #8580694 - Attachment is obsolete: false
(Assignee)

Updated

3 years ago
Keywords: checkin-needed
(Assignee)

Updated

3 years ago
Attachment #8547463 - Flags: checkin?
(Assignee)

Updated

3 years ago
Attachment #8583038 - Flags: checkin?
https://hg.mozilla.org/integration/mozilla-inbound/rev/e54525519dda
https://hg.mozilla.org/integration/mozilla-inbound/rev/7b69e3da1211
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/e54525519dda
https://hg.mozilla.org/mozilla-central/rev/7b69e3da1211
Attachment #8547463 - Flags: checkin? → checkin+
Attachment #8583038 - Flags: checkin? → checkin+
Comment on attachment 8550338 [details] [diff] [review]
no-gap-tab

gtknotebook considers initial-gap even when has-tab-gap is false, and so for
consistency, tabRect should be used here instead of rect.

Similarly, gtk_render_focus() should be passed the same rectangle irrespective
of the value has-tab-gap.  gtknotebook uses the tab label rect which is
related to the tab rect via tab padding as in the new code here, but the
has-tab-gap code should be the same and moz_gtk_get_tab_border should drop the
style_border too (https://bugzilla.mozilla.org/show_bug.cgi?id=877605#c24)

>+      return 0; /* don't use ythickness for tabs without a gap */

ythickness is not really a concept in GTK3 drawing.  Instead, say something
like "with no gap, tabs do not overdraw the tabpanel border".
Attachment #8550338 - Flags: feedback?(karlt) → feedback+
(Assignee)

Comment 53

3 years ago
Created attachment 8598041 [details] [diff] [review]
tab v.2 patch

Thanks, there's the updated one. It uses the same code to render the focus for gap/no gap code paths. All other remarks should be addressed.
Attachment #8550338 - Attachment is obsolete: true
Attachment #8598041 - Flags: review?(karlt)
Comment on attachment 8598041 [details] [diff] [review]
tab v.2 patch

>+    gtk_widget_style_get(gTabWidget, "has-tab-gap", &has_tab_gap, NULL);

"has-tab-gap" is new in 3.12.
I'd like this code to run against early versions of GTK3 without g_warning()S
(even if things don't look exactly right).

Can you use gtk_check_version() to determine whether to call
gtk_widget_style_get() to get this property, please, and arrange that
has_tab_gap is true if not?

>+      GtkWidget* tab_widget = gtk_notebook_get_nth_page(gTabWidget, 0);
>+      GtkWidget* tab_label = gtk_notebook_get_tab_label(gTabWidget, tab_widget);
>+      GtkStyleContext *style = gtk_widget_get_style_context(tab_label);
>+      gtk_style_context_get_padding(style, state_flags, &padding);

Sorry if what I said in comment 52 was confusing.
The version in attachment 8550338 [details] [diff] [review] got the padding from the correct widget, so
there is no need to add a tab label widget.

(The padding around the tab label rect is determined from the style context
 for the GtkNotebook using the equivalent of
 moz_gtk_tab_prepare_style_context() to distinguish from the padding internal
 the whole notebook border.)

>+    gtk_style_context_restore(style);
>+
>     if (state->focused) {

>     }
>-
>-    gtk_style_context_restore(style);

Having the gtk_style_context_restore() after drawing the focus was more
correct because the restored context represents that of the entire notebook,
while the context prior to restoring represented that of the tab.

However, what was not quite right before was that GTK_STYLE_REGION_TAB was
removed on selected tabs when has_tab_gap to draw the gap before drawing the
focus.  The changes to the style context are awkward to do tidily with the
different code paths here and the order used for drawing.
What might be easiest is to call moz_gtk_tab_prepare_style_context again if
drawing the focus.

>+    if (has_tab_gap) {
>+      gtk_widget_style_get (gTabWidget, "tab-curvature", &tab_curvature, NULL);
>+      *left += tab_curvature;
>+      *right += tab_curvature;
>+
>+      if (flags & MOZ_GTK_TAB_FIRST) {
>+        int initial_gap;
>+        gtk_widget_style_get (gTabWidget, "initial-gap", &initial_gap, NULL);
>+        if (direction == GTK_TEXT_DIR_RTL)
>+          *right += initial_gap;
>+        else
>+          *left += initial_gap;
>+      }
>+
>+      // Top tabs have no bottom border, bottom tabs have no top border
>+      if (flags & MOZ_GTK_TAB_BOTTOM) {
>+        *top = 0;
>+      } else {
>+        *bottom = 0;
>+      }
>     }

has-tab-gap affects drawing, but not positioning, so the value returned here should not depend on has-tab-gap.  I expect removing moz_gtk_add_style_border and the TODO reference to bug 877605 should be sufficient here.
Attachment #8598041 - Flags: review?(karlt) → review-
This bug should be closed once the no-gap tab patch lands.
A new bug should be used for any further work required.
Whiteboard: [leave open for more patches]
(Assignee)

Comment 56

3 years ago
Created attachment 8599284 [details] [diff] [review]
tab v.3 patch

Thanks, there's the updated one. There's one thing with moz_gtk_get_tab_border(), we should reset top/bottom border in has_tab_gap mode only.
Attachment #8598041 - Attachment is obsolete: true
Attachment #8599284 - Flags: review?(karlt)
Comment on attachment 8599284 [details] [diff] [review]
tab v.3 patch

(In reply to Martin Stránský from comment #56)
> There's one thing with
> moz_gtk_get_tab_border(), we should reset top/bottom border in has_tab_gap
> mode only.

I think you are right that we need to consider both top and bottom padding,
but I think these should both be kept even for has_tab_gap true.

>+    if (notebook_has_tab_gap) {
>+      // Top tabs have no bottom border, bottom tabs have no top border
>+      if (flags & MOZ_GTK_TAB_BOTTOM) {
>+        *top = 0;
>+      } else {
>+        *bottom = 0;
>+      }
>     }

Can you remove this whole block, please?

>     if (state->focused) {
>       /* Paint the focus ring */

>+      GtkBorder padding;
>+
>+      style = gtk_widget_get_style_context(gTabWidget);
>+      gtk_style_context_save(style);
>+      moz_gtk_tab_prepare_style_context(style, flags);
>+
>+      gtk_style_context_get_padding(style, GetStateFlagsFromGtkWidgetState(state), &padding);
>+
>+      focusRect.x += padding.left;

Please remove the "style = gtk_widget_get_style_context(gTabWidget);" line,
as this is the same style context.

>+static gboolean notebook_has_tab_gap;

"has-tab-gap" depends on the theme, which can change while the application is
running.  But this is good enough for now.
Attachment #8599284 - Flags: review?(karlt) → review+
(Assignee)

Comment 58

3 years ago
Created attachment 8599744 [details] [diff] [review]
patch v.4 patch

Thanks, there's a new one. Try:  https://treeherder.mozilla.org/#/jobs?repo=try&revision=0440e2d34824
Attachment #8599284 - Attachment is obsolete: true
(Assignee)

Updated

3 years ago
Keywords: checkin-needed
Attachment #8580694 - Attachment is obsolete: true
Attachment #8599744 - Flags: checkin+

Comment 59

3 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/1bc82fa21038
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/1bc82fa21038
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox40: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
Depends on: 1173907
Depends on: 1232208
You need to log in before you can comment on or make changes to this bug.