546178 - Spaces are painted under GTK tabs when not needed

Status: RESOLVED FIXED

(Core :: Widget: Gtk, defect)

Description

[Michael Ventnor]

Attachment: Patch

This bug was hidden by the widget-ised browser, but bug 130078 revealed it. We only painted spaces under selected tabs to work around differences between how GTK paints tabpanels and how we paint them. When that bug is fixed, the tabbrowser tabs show this bug.

This is not a bad solution, but some people might consider it hacky.

Comment 1

[Robert O'Callahan (:roc) (email my personal email if necessary)]

This is rather hacky indeed.

Basically the problem is that we need to know the relationship between tabs and tabpanels. Either when we paint a tabpanels, we need to locate the active tab so we can cut a hole in the border under the active tab, or when we paint the active tab we need to find whether there is a tabpanels so that we can know whether to overpaint the tabpanel border or not. It looks like currently XUL doesn't give us enough information to do this.

Your patch hacks around this problem by looking in the frame tree for where you expect the tabpanels to be, to see if they're there. That seems overly fragile.

I think probably the simplest and most backward-compatible solution is to introduce a "notabpanels" content attribute that tabs should use where there is no associated tabpanels, and don't try to overpaint the tabpanels when that attribute is set. Anyone got any better ideas?

Comment 2

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Hmm, well, in the browser there *is* a tabpanels, it just contains the content document and isn't styled with -moz-appearance:tabpanels.

So maybe a cleaner thing to do that doesn't require new API is for NS_THEME_TAB_PANELS painting to paint the gap under the active tab. That means we have to find the active tab. It looks like we can do that by finding the "tabbox" ancestor of the tabpanels, then find the first "tabs" descendant of the tabbox, and then QI that to nsIDOMXULSelectControlElement, then call selectedItem on that. Not exactly simple, but a lot more robust.

Comment 3

[Robert O'Callahan (:roc) (email my personal email if necessary)]

We can also verify that the bottom (or top) edge of the tabs is at the edge of the tabpanels.

Comment 4

[Michael Ventnor]

(In reply to comment #2)
> It looks like we can do that by finding the
> "tabbox" ancestor of the tabpanels, then find the first "tabs" descendant of
> the tabbox

You mean direct descendant here, right? I don't want to do anything that may become a bit slow.

Comment 5

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I'm not worried about that since tabpanels aren't very common. The browser window doesn't have any.

Comment 6

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 426936 [details] [diff] [review]
Patch

see comments

Comment 7

[Julien "_FrnchFrgg_" RIVAUD]

IIRC, the same choice presented itself when I rewrote the tabpanels rendering (before, the tabpanel's border was continuous and tabs didn't look attached, and gtk styling was wrong too)

There were (and are) three solutions:

a) Let the tabpanel draw its borders fully without any gap, then make the tabs overdraw with a gap if needed (note that it isn't just a matter of erasing)

b) Let the tabpanel draw only 3 of its borders, and let the tabs and the spacer draw the remaining border piecewise.

c) Have a way to find out where the gap should be when drawing the tabpanel borders. It may imply:
    c1) Walk a bit the DOM at painting time and calculate the parameter then
    c2) Manage to have the calculation done during layout (using a hook similar to the "getsizeforwidget" and co)

At the time of the rewrite, I think c) was striken because it was too complex even if it was considered the most correct (and the most faithful to actual gtk drawing code). b) is IMHO bad, cause it requires a lot of different calls to the gtk API and hope that all those pieces stich well, and I don't really remember but there was other problems about this solution (the WIN95 look was draw that way at the time, so it was the solution considered and abandoned first)

I still think that c) is the best way to go, and probably c2 if you ask me. It needs careful thinking about what asumptions we want to make about the XUL structure of tabs.

Two notes:
- What is the link between this and bug 130078 ? The manually painted separation between the tabstrip and the viewport is too small and then the gap is painted on top of the HTML page because the viewport now obeys z-index ?
If so the solution is probably simpler than implementing c. Note that with a bit of luck, a) can hold tight until we get a better theming API from gtk.
Or am I completely misreading what bug 130078 is about ?

- I'd really like if we could have native tabs attached to the left/right/bottom of tabpanels. Sure it is not used, and not a priority, but if the refactoring makes it really hard to support that, I'd be sad :)

Comment 8

[mmkfly]

I've been playing with Stylish, attempting to style tabs closer to what I see in native GTK apps.  I've come very close, using the a) "Let the tabpanel draw its borders fully without any gap, then make the tabs overdraw with a gap if needed" method.

I'm not sure if it's related to this bug, but in situtations like the following, consisting of a few inactive tabs followed by the active tab (which draws an overlap as intended):

[i1][i2][i3][A]

If I close one of the inactive tabs via it's close/X button, the overlap from the 4th slot (where the active tab was before), sticks around where there is now NO tab, and creates a hole in the border.  I can't seem to work around this at all.

Comment 9

[Markus Stange [:mstange]]

(In reply to comment #8)
> If I close one of the inactive tabs via it's close/X button, the overlap from
> the 4th slot (where the active tab was before), sticks around where there is
> now NO tab, and creates a hole in the border.  I can't seem to work around this
> at all.

What happens if you increase the left and right margin that is used for active tabs in nsNativeThemeGTK::GetWidgetOverflow?

Comment 10

[Markus Stange [:mstange]]

Maybe you have to apply the patches in bug 444870 in order for that to work correctly.

Comment 11

[Will Roberts [:bws42]]

This seems to WFM since the landing of bug 444870

Comment 12

[Dão Gottwald [:dao]]

http://hg.mozilla.org/mozilla-central/rev/13c6e0ecf8a7 appears to have fixed this.