Port GTK2 to GTK3 - tab widget rendering

RESOLVED FIXED

Status

()

Core
Widget: Gtk
RESOLVED FIXED
5 years ago
3 years ago

People

(Reporter: stransky, Assigned: stransky)

Tracking

Trunk
x86
Linux
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(5 attachments, 3 obsolete attachments)

(Assignee)

Description

5 years ago
+++ This bug was initially created as a clone of Bug #627699 +++

GTK+ 3.0 and GNOME 3 are approaching and we should get Firefox ready for them. This bug is aimed to correct rendering of tab widget.
(Assignee)

Comment 1

4 years ago
The relevant part of the GTK3 bug is https://bugzilla.mozilla.org/show_bug.cgi?id=627699#c263
(Assignee)

Comment 2

4 years ago
Created attachment 794598 [details] [diff] [review]
tab rendering, v2

A minimal tab patch without the overlapping. It will be addressed in another one.

> Why is drawing the gap no longer necessary here?

The gap is drawn by gtk_render_extension(). The gtk_render_background()/gtk_render_frame_gap() has no effect here, it only produces an artifact on the bottom of the tab. The artifact (a blue line in Awaidta theme).

> Why do top tabs no longer need focusRect adjustment, but bottom tabs still do?

I haven't found any bottom tabs in Firefox UI (is there any?) so I did the change for top tabs only. However the focus change is tied to wide and overlapped tabs, so let's address it there.
Attachment #794598 - Flags: review?(karlt)
(Assignee)

Comment 3

4 years ago
Karl, any update here?
Flags: needinfo?(karlt)
Comment on attachment 794598 [details] [diff] [review]
tab rendering, v2

(In reply to Martin Stránský from comment #2)
> The gap is drawn by gtk_render_extension(). The
> gtk_render_background()/gtk_render_frame_gap() has no effect here, it only
> produces an artifact on the bottom of the tab. The artifact (a blue line in
> Awaidta theme).

Gecko calls moz_gtk_tabpanels_paint to draw the background and frame, and then
moz_gtk_tab_paint for each tab.  However, moz_gtk_tabpanels_paint doesn't know
where the gap needs to be so we are depending on moz_gtk_tab_paint to paint
the gap for the selected tab.

gtk_render_background and gtk_render_frame_gap is the right way to draw that
gap and consistent with gtk_notebook_paint.

Even if the Adwaita theme engine draws outside the rectangle of
gtk_render_extension, we can't assume that other engines will do this, as it
is not really the right thing for them to do.

One thing that is not consistent is the order of the function calls.
Does moving gtk_render_extension to after gtk_render_frame_gap make things
work better?

> I haven't found any bottom tabs in Firefox UI (is there any?) so I did the
> change for top tabs only. However the focus change is tied to wide and

Firefox may not use them, but other apps might, and we should keep these
consistent.

> moz_gtk_get_tab_thickness(void)

>+    gtk_style_context_get_padding(style, 0, &padding);
>+
>+    int thickness = border.top + padding.top;

The idea of moz_gtk_get_tab_thickness() is to return the thickness of the
border of the notebook, so that the moz_gtk_tab_paint() can draw over this
border and make a "gap" in it.

The padding is the distance between the border and the notebook content area,
and so I would not expect it to be necessary to draw over the padding, but
gtk_notebook_redraw_tabs_junction and gtk_notebook_redraw_tabs both seem to
invalidate this padding area, so this is consistent in some ways.

>+    /* Adwaita theme engine needs to call gtk_style_context_add_region() 
>+     * to pick up correct tab background.

How about "The Adwaita theme engine uses gtk_theming_engine_has_region() with
GTK_STYLE_REGION_TAB to detect Notebook tabs and appropriately size the focus
indicator"?

>+    if ((flags&MOZ_GTK_TAB_FIRST))

Spaces around the '&' operator, please.
And remove extra parentheses.
Attachment #794598 - Flags: review?(karlt) → review-
Flags: needinfo?(karlt)
(Assignee)

Comment 5

4 years ago
(In reply to Karl Tomlinson (:karlt) from comment #4)
> Gecko calls moz_gtk_tabpanels_paint to draw the background and frame, and
> then
> moz_gtk_tab_paint for each tab.  However, moz_gtk_tabpanels_paint doesn't
> know
> where the gap needs to be so we are depending on moz_gtk_tab_paint to paint
> the gap for the selected tab.
> 
> gtk_render_background and gtk_render_frame_gap is the right way to draw that
> gap and consistent with gtk_notebook_paint.
 
My point is that gtk_render_extension() already draws the background and thus we don't need to do so by gtk_render_background()/gtk_render_frame_gap() combo...see gtknotebook.c:gtk_notebook_draw_tab() function.

> One thing that is not consistent is the order of the function calls.
> Does moving gtk_render_extension to after gtk_render_frame_gap make things
> work better?

Yes, because gtk_render_extension() overwrite it.

> Even if the Adwaita theme engine draws outside the rectangle of
> gtk_render_extension, we can't assume that other engines will do this, as it
> is not really the right thing for them to do.

gtk_render_extension() does not paint outside the given rectangle.
(In reply to Martin Stránský from comment #5)
> My point is that gtk_render_extension() already draws the background and
> thus we don't need to do so by
> gtk_render_background()/gtk_render_frame_gap() combo...see
> gtknotebook.c:gtk_notebook_draw_tab() function.

gtk_notebook_draw_tab() is only for drawing the tab.

moz_gtk_tab_paint() doesn't need to draw the background in the tab rectangle for gtk_render_extension(), but does need to draw the gap, which is outside this rectangle, adjacent to the selected tab.

gtk_notebook_paint() calls gtk_render_background() and gtk_render_frame_gap() before calling gtk_render_extension() via gtk_notebook_draw_tab().

> > Does moving gtk_render_extension to after gtk_render_frame_gap make things
> > work better?
> 
> Yes, because gtk_render_extension() overwrite it.

Sounds like this would be the appropriate solution.
It overwrites it because Adwaita is drawing outside the rectangle.

> > Even if the Adwaita theme engine draws outside the rectangle of
> > gtk_render_extension, we can't assume that other engines will do this, as it
> > is not really the right thing for them to do.
> 
> gtk_render_extension() does not paint outside the given rectangle.

gtk_render_extension() is not designed to draw the gap and so does not need to draw outside the rectangle, but Adwaita does draw outside this rectangle to draw the gap from gtk_render_extension() instead of from gtk_render_frame_gap().
See the "+ 1.0" at
https://git.gnome.org/browse/gnome-themes-standard/tree/src/adwaita_engine.c?h=gnome-3-6#n363
(Assignee)

Comment 7

4 years ago
(In reply to Karl Tomlinson (:karlt) from comment #6)
> gtk_notebook_draw_tab() is only for drawing the tab.
> 
> moz_gtk_tab_paint() doesn't need to draw the background in the tab rectangle
> for gtk_render_extension(), but does need to draw the gap, which is outside
> this rectangle, adjacent to the selected tab.
> 
> gtk_notebook_paint() calls gtk_render_background() and
> gtk_render_frame_gap() before calling gtk_render_extension() via
> gtk_notebook_draw_tab().

Well, yes. But gtk3 uses render_background/render_frame_gap for actual notebook page rendering, then the style is modified and gtk_render_extension() draws the tab with different style.

We use the same style context for gtk_render_background()/gtk_render_frame_gap()/ gtk_render_extension() to simulate it. Unfortunately it provides the artifacts - background is rendered as a tab background - and gtk_render_frame_gap() renders nothing at all.

The solution here may be to use different style for the gap rendering - the same we use for notebook page.

> > > Does moving gtk_render_extension to after gtk_render_frame_gap make things
> > > work better?
> > 
> > Yes, because gtk_render_extension() overwrite it.
> 
> Sounds like this would be the appropriate solution.
> It overwrites it because Adwaita is drawing outside the rectangle.

Unfortunately there are still some artifacts left, the extension height is shrink by gap_voffset.
(Assignee)

Comment 8

4 years ago
Created attachment 832883 [details] [diff] [review]
tab rendering, v.3

This one uses background_style to render the gap by gtk_render_background(). I removed the gtk_render_frame_gap(), it useless here.

As far as the lazy style resolving is performed by gtk3 we have to call the gtk_style_context_save()/gtk_style_context_restore() to flush style cache and actually switch the context.
Attachment #794598 - Attachment is obsolete: true
Attachment #832883 - Flags: review?(karlt)
Comment on attachment 832883 [details] [diff] [review]
tab rendering, v.3

(In reply to Martin Stránský from comment #8)
> This one uses background_style to render the gap by gtk_render_background().

Yes gtk_render_background() needs the StyleContext to be in a different state
for the notebook from what gtk_render_extension() needs to draw the tab, but
here |background_style| is the same StyleContext as |style|.  Please use a
single variable, because having one modified by changes to the other is
confusing.

Consider using gtk_style_context_remove_region from within a save/restore
block for the gap drawing.

> I removed the gtk_render_frame_gap(), it useless here.

It is not useless here.  Please don't look at what Adwaita draws.
The goal is to mimic what GtkNotebook does.  The default theme engine
(see gtk_theming_engine_render_frame_gap()), for example, draws the frame
border_width pixels into the gap.

I'd prefer to leave the gtk_render_focus code outside the (flags &
MOZ_GTK_TAB_SELECTED) block, for consistency with gtk_notebook_draw_tab().
Attachment #832883 - Flags: review?(karlt) → review-
(In reply to Martin Stránský from comment #7)
> Unfortunately there are still some artifacts left, the extension height is
> shrink by gap_voffset.

I couldn't make sense of the gap_voffset code, but this can be fixed separately.
Which bug report introduced that code?  I wonder whether that was before we
had GetExtraSizeForWidget().
(Assignee)

Updated

4 years ago
Whiteboard: [leave open for remaining patches][check linux try build before requesting checkin] → [leave open for remaining patches]
(Assignee)

Comment 11

4 years ago
Created attachment 8337736 [details]
gtk_render_frame_gap artifact

Yes, I understand not what do you mean, but unfortunately the gtk_render_frame_gap() produces this artifacts.
(Assignee)

Updated

4 years ago
Attachment #8337736 - Attachment description: artifact.png → gtk_render_frame_gap artifact
(Assignee)

Comment 12

4 years ago
Created attachment 8337742 [details] [diff] [review]
tab clipping

What about something like that? Clipping the bottom part of the frame. The tricky part is the border size...I used the style border here which is "1" in this theme.
Attachment #8337742 - Flags: feedback?(karlt)
(Assignee)

Comment 13

4 years ago
(In reply to Martin Stránský from comment #12)
> Created attachment 8337742 [details] [diff] [review]
> tab clipping
> 
> What about something like that? Clipping the bottom part of the frame. The
> tricky part is the border size...I used the style border here which is "1"
> in this theme.

Actually we can also add two more clipping areas to clip the left and right side of the rendered frame. Then the the border size does not matter here because we can just draw the upper line with a gap.
Comment on attachment 8337742 [details] [diff] [review]
tab clipping

Yes, gtk2drawing.c had a cliprect, which made this work.

It should be possible to use a single rectangle.
rect->x and rect->width can be used for the horizontal details of the
rectangle, consistent with the background drawing.

gap_height should be used in the calculation for the bottom/height of the clip
rectangle, because that is used in GetExtraSizeForWidget().  In fact, the
bottom of the rectangle should be the same as for the rectangle used for
gtk_render_background.  Making the top the same is also probably best.
Attachment #8337742 - Flags: feedback?(karlt) → feedback+
(Assignee)

Comment 15

4 years ago
Created attachment 8339910 [details] [diff] [review]
tab rendering v4

What about this one?
Attachment #832883 - Attachment is obsolete: true
Attachment #8337742 - Attachment is obsolete: true
Attachment #8339910 - Flags: review?(karlt)
Attachment #8339910 - Flags: review?(karlt) → review+
(Assignee)

Updated

4 years ago
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/2c9fa83a8dc5
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/2c9fa83a8dc5
Is this patch the only one needed to close this bug or are we waiting for another one?
(Assignee)

Comment 19

4 years ago
Yes, we need more patches here - at least for a tab size.
(Assignee)

Comment 20

4 years ago
Created attachment 8397159 [details] [diff] [review]
tab size patch

This patch implements specific tab sizes which depends on tab position (first tab) and active (selected) tab. It also implements "initial-gap" style property - works for both LTR and RTL locales.

Karl, what do you think about it? I guess it's the maximum we can do without massive layout changes. The original Gtk3 tab also implements tab overlapping which is IMHO unsupported by gecko now.
Attachment #8397159 - Flags: feedback?(karlt)

Updated

4 years ago
Blocks: 1034064

Updated

4 years ago
No longer blocks: 1034064
Attachment #8339910 - Flags: checkin+
Comment on attachment 8397159 [details] [diff] [review]
tab size patch

Yes, it wasn't right for moz_gtk_get_widget_border to return the notebook
(TABPANELS) padding for the tab, and if the padding is different for
active/inactive tabs then this looks like the way to do it.

Sorry about the slow response here.  I find the layout here hard to understand
each time I think about it. 

Can you use a helper function to set up the style context for the tab, please,
so that moz_gtk_get_tab_border() and moz_gtk_tab_paint() are consistent and we
need only get it right once?  This would be the equivalent of
notebook_tab_prepare_style_context() in gtknotebook.c.

Also, please leave "initial-gap" support out of this patch.  It is really an initial and final gap as it is applied at both ends of the tab panel.  Applying it on the tabpanel would be more consistent with GTK's design.  Although it is a padding, it looks like moz_gtk_get_widget_border() is the function where
containers usually return padding so that CSS can still modify the padding
if necessary (when GetWidgetPadding() returns false).
Attachment #8397159 - Flags: feedback?(karlt) → feedback+
(In reply to Karl Tomlinson (:karlt) from comment #21)
> Also, please leave "initial-gap" support out of this patch.  It is really an
> initial and final gap as it is applied at both ends of the tab panel. 
> Applying it on the tabpanel would be more consistent with GTK's design. 

Sorry I'm wrong there.  tabpanels is quite different.  The "tabs" element is what I was thinking of and has spacers in the right place [1], but using nsLookAndFeel to size these may not be easy.  So using "initial-gap" in the border and tab drawing might, as you do, might be easiest, and it will do the right thing when there are not so many tabs as to fill up the tabbar.

[1] http://hg.mozilla.org/mozilla-central/annotate/738469449872/toolkit/content/widgets/tabbox.xml#l247
(Assignee)

Comment 23

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

Thanks! There's an updated one.
Attachment #8483532 - Flags: review?(karlt)
Comment on attachment 8483532 [details] [diff] [review]
tab v.2

>+    moz_gtk_add_style_border(style, left, top, right, bottom);
>+    moz_gtk_add_style_padding(style, left, top, right, bottom);

gtk_notebook_get_preferred_tabs_size() and gtk_notebook_page_allocate()
actually include only the padding, without any border, around the tab labels.
(gtknotebook's get_padding_and_border() is used only for the entire notebook,
not the individual tabs.)  Instead, they add the dimensions from the
focus-line-width and focus-padding style properties.

There is an existing issue with the gtk_render_focus() call in
moz_gtk_tab_paint() using the border, and using the style context for the
notebook instead of the tab.  Including the border in moz_gtk_get_tab_border
for now would be semi-consistent with moz_gtk_tab_paint(), so I'm OK with it
if you like, but can you include a TODO comment please indicating that add_style_border should be replaced with focus-line-width and focus-padding?

>+      GtkWidgetState state;
>+      GtkThemeWidgetType gtkWidgetType;
>+      gint flags;
>+
>+      if (!GetGtkWidgetAndState(aWidgetType, aFrame, gtkWidgetType, &state,
>+                                &flags))
>+        return NS_OK;
>+
>+      moz_gtk_get_tab_border(MOZ_GTK_TAB, &aResult->left, &aResult->top,
>+                             &aResult->right, &aResult->bottom, direction,
>+                             (GtkTabFlags)flags);

Please remove the GtkThemeWidgetType parameter to moz_gtk_get_tab_border().
There's no need for it because it is always MOZ_GTK_TAB.

Also, please pass nullptr for the aState parameter of GetGtkWidgetAndState(),
so that the |state| variable can be removed here.
Attachment #8483532 - Flags: review?(karlt) → review+
(Assignee)

Comment 25

3 years ago
Created attachment 8484130 [details] [diff] [review]
tab v.2 for check-in

Thanks. Let's target the border issue in another patch with the gtk2 tabs.
(Assignee)

Comment 26

3 years ago
Comment on attachment 8484130 [details] [diff] [review]
tab v.2 for check-in

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

Updated

3 years ago
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/c09023e70b16
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/c09023e70b16
This broke GTK+3 builds on elm:

/builds/slave/elm-lx-00000000000000000000000/build/widget/gtk/gtk3drawing.c:2119:41: error: 'GTK_STYLE_CLASS_BOTTOM' undeclared (first use in this function)
/builds/slave/elm-lx-00000000000000000000000/build/widget/gtk/gtk3drawing.c:2120:41: error: 'GTK_STYLE_CLASS_TOP' undeclared (first use in this function)
https://tbpl.mozilla.org/php/getParsedLog.php?id=47516794&tree=Elm
(Assignee)

Comment 30

3 years ago
I see. Okay, will look at it.
(Assignee)

Comment 31

3 years ago
Isn't the try build broken somehow? GTK_STYLE_CLASS_* is defined in gtkstylecontext.h which may be pulled from gtk.h. (I checked gtk 3.4 which should be on the try).
(In reply to Martin Stránský from comment #31)
> Isn't the try build broken somehow? GTK_STYLE_CLASS_* is defined in
> gtkstylecontext.h which may be pulled from gtk.h. (I checked gtk 3.4 which
> should be on the try).

We're building against GTK+ 3.0.12.
Although Mozilla's test machines run with GTK+ 3.4 and bug 1027138 seems to conclude that there is no need to support older versions, attachment 8444888 [details] uses 3.0.12 to set up the build machines.
Is this fixable, or do we need to start building against gtk+ 3.4?
Flags: needinfo?(stransky)
Flags: needinfo?(karlt)
It should be fixable either through a widget/gtk/compat-gtk3/gtk/gtkstylecontext.h
or #defines in gtk3drawing.c.

I don't think Firefox is likely to run well against early GTK+3 versions, so building against 3.4 may be a sensible thing to do, if it is not too difficult to set up.

There is also an existing GTK_CHECK_VERSION(3,4,0) check for compiling smooth scroll support which I thought was not an issue because I mis-assumed the version on the build machines.

http://hg.mozilla.org/mozilla-central/annotate/f4037194394e/widget/gtk/nsWindow.cpp#l3096
Flags: needinfo?(karlt)
(In reply to Karl Tomlinson (:karlt) from comment #35)
> I don't think Firefox is likely to run well against early GTK+3 versions, so
> building against 3.4 may be a sensible thing to do, if it is not too
> difficult to set up.

Trying that now:
https://tbpl.mozilla.org/?tree=Try&rev=f844f3f394f3
This is all good, elm is now fixed with this.
Flags: needinfo?(stransky)
(Assignee)

Comment 38

3 years ago
I think we're done here.
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
(Assignee)

Updated

3 years ago
Whiteboard: [leave open for remaining patches]
You need to log in before you can comment on or make changes to this bug.