Martin Stránský [:stransky] (ni? me) Assignee
	Description • 11 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.

	Martin Stránský [:stransky] (ni? me) Assignee
	Comment 1 • 11 years ago

The relevant part of the GTK3 bug is https://bugzilla.mozilla.org/show_bug.cgi?id=627699#c263

	Martin Stránský [:stransky] (ni? me) Assignee
	Comment 2 • 11 years ago

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.

	Martin Stránský [:stransky] (ni? me) Assignee
	Comment 3 • 11 years ago

Karl, any update here?

	Karl Tomlinson (:karlt)
	Comment 4 • 11 years ago

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.

	Martin Stránský [:stransky] (ni? me) Assignee
	Comment 5 • 11 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.

	Karl Tomlinson (:karlt)
	Comment 6 • 11 years ago

(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

	Martin Stránský [:stransky] (ni? me) Assignee
	Comment 7 • 11 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.

	Martin Stránský [:stransky] (ni? me) Assignee
	Comment 8 • 11 years ago

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.

	Karl Tomlinson (:karlt)
	Comment 9 • 11 years ago

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().

	Karl Tomlinson (:karlt)
	Comment 10 • 11 years ago

(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().

	Martin Stránský [:stransky] (ni? me) Assignee
	Comment 11 • 11 years ago

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

	Martin Stránský [:stransky] (ni? me) Assignee
	Comment 12 • 11 years ago

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.

	Martin Stránský [:stransky] (ni? me) Assignee
	Comment 13 • 11 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.

	Karl Tomlinson (:karlt)
	Comment 14 • 11 years ago

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.

	Martin Stránský [:stransky] (ni? me) Assignee
	Comment 15 • 11 years ago

What about this one?

	Ryan VanderMeulen [:RyanVM]
	Comment 16 • 11 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/2c9fa83a8dc5

	Ryan VanderMeulen [:RyanVM]
	Comment 17 • 11 years ago

https://hg.mozilla.org/mozilla-central/rev/2c9fa83a8dc5

	Morgane ”Sardem FF7” Glidic
	Comment 18 • 11 years ago

Is this patch the only one needed to close this bug or are we waiting for another one?

	Martin Stránský [:stransky] (ni? me) Assignee
	Comment 19 • 11 years ago

Yes, we need more patches here - at least for a tab size.

	Martin Stránský [:stransky] (ni? me) Assignee
	Comment 20 • 10 years ago

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.

	Karl Tomlinson (:karlt)
	Comment 21 • 10 years ago

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).

	Karl Tomlinson (:karlt)
	Comment 22 • 10 years ago

(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

	Martin Stránský [:stransky] (ni? me) Assignee
	Comment 23 • 10 years ago

Thanks! There's an updated one.

	Karl Tomlinson (:karlt)
	Comment 24 • 10 years ago

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.

	Martin Stránský [:stransky] (ni? me) Assignee
	Comment 25 • 10 years ago

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

	Martin Stránský [:stransky] (ni? me) Assignee
	Comment 26 • 10 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

	Carsten Book [:Tomcat]
	Comment 27 • 10 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/c09023e70b16

	Ryan VanderMeulen [:RyanVM]
	Comment 28 • 10 years ago

https://hg.mozilla.org/mozilla-central/rev/c09023e70b16

	Mike Hommey [:glandium]
	Comment 29 • 10 years ago

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

	Martin Stránský [:stransky] (ni? me) Assignee
	Comment 30 • 10 years ago

I see. Okay, will look at it.

	Martin Stránský [:stransky] (ni? me) Assignee
	Comment 31 • 10 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).

	Mike Hommey [:glandium]
	Comment 32 • 10 years ago

(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.

	Karl Tomlinson (:karlt)
	Comment 33 • 10 years ago

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.

	Mike Hommey [:glandium]
	Comment 34 • 10 years ago

Is this fixable, or do we need to start building against gtk+ 3.4?

	Karl Tomlinson (:karlt)
	Comment 35 • 10 years ago

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

	Mike Hommey [:glandium]
	Comment 36 • 10 years ago

(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

	Mike Hommey [:glandium]
	Comment 37 • 10 years ago

This is all good, elm is now fixed with this.

	Martin Stránský [:stransky] (ni? me) Assignee
	Comment 38 • 9 years ago

I think we're done here.