User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.3) Gecko/20041022 Firefox/1.0 Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.3) Gecko/20041022 Firefox/1.0 The recent work done on the browser tabs unscores the fact that the Page Info tabs are very ugly. Could the same work can be done on the Page Info window? Reproducible: Always Steps to Reproduce: 1. Click Tools -> Page Info 2. Bask in the tabular ugliness

A comparison of various tab interfaces. First off, the ugly Page Info tabs; then, the tabs from IE's Internet Options; finally, the new-and-improved Firefox browser tabs.

What is so ugly about them?

I was just pointing out how they: a) don't look like normal tabs and b) aren't physically attached to the panel they control, making the metaphor of tabs less 'real' to the user

Duplicate of bug 214265?

No, it isn't a dupe of that bug. It in fact concerns every tabs, as can be seen in the new prefs window. There is a real problem in the implementation of tabbox/tabs/-moz-appearance, and a bug highlighting some of its errors is 254763. There may be the need for another bug, though.

This bug should really belong to Toolkit/XUL widgets, with a summary involving native rendering of tab widgets... Heavily linked to bug 254763, as previously said. That said, while investigating the code, I saw the current implementation in gtk paints the left border of a selected tab in the selected tab if it is leftmost, or in the rightmost 2 pixels of its left neighbourgh if not, because of painting order problems. Couldn't it be that a selected tab always draws itself fully, and that nearby tabs, if present, get -2px margins where appropriate and just stay transparent where they could overlap the selected tab ? No need to really do the painting of a tab borders in neighbours, just let those border shine through where they would be drawn upon... Then the code would really be cleaner, and wouldn't so heavily resort on the first-child attribute (which is currently broken, since it is set to the first tab created and never modified after that, even if that tab is drag'n'dropped elsewhere).

Also notice that the two types of left selected edges (first inside, others drawn by predecessor) aren't consistent for the tabbrowser bar, as selected tabs are 1 pixel taller than the others, to overwrite the bottom background line. Back to the uglyness problem, which concerns at least Windows & GTK: The top edge of the tabpanels, currently drawn by the "-moz-appearance: tabpanels" code, runs through the whole width. Moreover, the tabs-right spacer has a non-native bottom border, which amounts solely to just a line above the normal native border... The solution would be to a) Do not draw the top edge of the tabpanels b) Draw it as a bottom border for the spacer and the unselected tabs c) adjust the rendering of selected tabs I'd have proposed a patch, but I am too new to this code; I will try to write one, but do not expect it soon, and the bug would certainly benefit from someone more skilled (I don't think it is that difficult, it is just I am that bad at Mozilla :( )

This has also become more critical as the new (1.5) Options dialog uses these heavily. Page Info was used little by average users, Options is used much more often.

The modifications I discuss here concerns gtk2 only, and take place in /mozilla/widget/src/gtk2/gtk2drawing.c, moz_gtk_tab_paint and moz_gtk_tabpanels_paint ---- Trying to implement what I said I am faced with a problem : how to know how much of the "box" rendered by the theme for the tabpanel is to be considered the border ? Furthermore, couldn't a GTK theme draw a spherical gradient on the back of the tabpanels whose center would be in the middle of the gap ? The correct way would be to draw the box with gtk_paint_box_gap, with the gap at the right place, facing the selected tab ; but it would imply calculate the right x-coordinate and gap width, values depending on the layout of the tabs... which is probably not wanted, or even impossible (having -moz-apperance: tabpanels; does not garantee we are a tabpanels, even less we have tabs to access and get position) Provided the size of the native-rendered top border is accessible to the code, an approximate solution is possible, but it could render wrong if the background of the extension (tab) is supposed to match the background of the box (tabpanels) near the gap, with a non uniform color. ------ Concerning the current way to manage the 2px overlap of tabs : - first there is a mistake, since the expression gboolean before_selected = ((flags & MOZ_GTK_TAB_BEFORE_SELECTED)!=0) will always evalutate to true (1), because of the surrounding if (flags & MOZ_GTK_TAB_BEFORE_SELECTED) - second, because every tab except the first is rendered as if 2px wider but with no left edge, one gets a greater perceived padding (by 2px) on the left side thus feeling the text ill-centered, except for the first tab causing an inconsistency. It would easily be solved with setting 2px more padding to the right than to the left for all but the first tab. Note that when the tab is selected, the left neighbourgh draws the left edge, thus the left padding is effectively seen as being 2px wider than it is really, so no need for a distinction on selected or not. - third, selected tabs get more padding, and thus inactive unselected tabs keep moving, whereas the gtk effect is only to change which tab overlaps which on those so special 2px. Easily solved by giving selected tabs no more padding. (I know "-moz-appearance" overrides "border", but my experiments contradict my remembering that it overrided also "padding"... My memory must be playing tricks on me.) An important factor concerning the look of the tabs is that they get too few padding. In the top right bottom left order, 5px 6px 3px 4px (thus 6px for the left padding of the first tab) gives pixel-perfect results in all font sizes, for all themes I tested but one, bluecurve, but for which the error is of 1px.

This patch works well with every tested theme. I assumed that the border-width of the tabpanels top edge is less than 2px, because when I tried to use style->ythickness which should contain the height of the border, I realized that a lot of themes do not set it to the correct value. It is a somewhat safe assumption, because a lot of themes won't want borders to be larger than the hardcoded overlap between tabs, for sake of clarity and design. Even if in some theme it could break, I think it would be better than the current behaviour. And in most themes (especially standard ones) it works. Another problem is that we draw 2px of the tabpanel left border in the first tab, and thus if the first tab isn't all the way left, the rendering is wrong (1px error only). It affects especially the tabbrowser tab bat, but I didn't try RTL which could be affected. Similarily, the last tab has no mean do detect if its right border is exactly in front of the tabpanel right one, and if so, there is a 1px gap in the right border. Last, but not least, a -moz-appearance: tabs-right-spacer or similar should be added, corresponding to the drawing of the remaining of the border. It isn't in this patch, as I'll have to dig into the style system before being able to write the corresponding code. Should I ask for review for this part ? Note: The mistake correction is included.

While testing more pixmap themes, I noticed a problem in my first try of removing the top edge of tabpanel : I had assumed that the gap demand would effectively result in a gap, and some themes, like tAqua, do not draw a gap under tabs. So I added a 2px shift up of the top border, to ensure that it wouldn't be rendered twice by such themes. I also encountered a strange and really annoying problem : the Clearlooks theme seem to not correctly clip some of its drawing primitives, so for example the background of the tabpanel bleeds outside of it and overwrite the tabs... Maybe it only is on my system. No problem encountered on pure gtk apps. The only way to get rid of those pbs would be to never resort on clipping, which is probably overkill to workaround a theme engine bug.

For the first two themes presented (ThinIce & Crux), 4 shots, top to bottom : - Current trunk rendering - Rendering with my patch #2 - Rendering with #2 patch and "padding: 5px 6px 5px 4px" on every tab (except 6px of left padding on first tab) - Some tabs in gimp You can see that the result is nearly pixel perfect (except that the padding I set seems to be a little too great), including little bugs in the crux theme : at the left of the first, selected, tab, there is a single pixel a bit darker than others on its vertical line. I didn't shot the default theme since it only has 1px tall top border for tabpanels, so nothing different from ThinIce under the sun. The only problem is the border under the right spacer : currently the old style is kept on the right spacer, which gives a single ThreeDHighlight line (it is the "odd border above tabpanels" of bug 254763). Also note that for tAqua and ClearLooks (3 & 4), with the patch it does the right thing (TM) concerning the rendering of the tabpanels. I just noticed a glitch on the selected tab for tAqua, I don't know why it doesn't render as in native gtk.

Correction : It isn't Clearlooks that is tested on previous attachment, it is BlueCurve. Clearlooks is broken on my system. For the same problems in Windows, I can try to write a patch, but I haven't any Windows build environment (My only windows has been reduced to the bare minimum, has only 200Mo free, and is solely used to flash my router when I turn it into a brick). The OS should be changed to All, by the way (or if there is no pb in other OSes, perhaps create a new bug for the GTK part ?)

I think separate bugs for Windows and GTK would make more sense. And your patch is unlikely to be looked into unless you request a review (roc and bryner were suggested on IRC).

Comment on attachment 204241 [details] [diff] [review] Better comments OK, so "roc" isn't considered a good way to refer to Robert O'Callahan. Trying its email address.

I've written a patch that a) does the same thing as the previous, plus b) adds a "tab-spacer-right" possible value to "-moz-appearance" c) adds support of this value to nsNativeThemeGTK I'll post it in two parts : the b) on one hand, and the remaining (platform-specific) on the other hand. Concerning the Windows-specific code, there is very few widgets that already need tweaks as tabs, and so there is no way to do already commonly used, that I could mimic. I can mockup something, but then it may be too far from the implicit guidelines followed by win32-widget folks. That and I repeat I have no windows testbed, and I am certainly not enough of a genius to be able to write code that deep into (sometimes unpredictable) platform api without any testing.

Forgot a file in the diff

Here what it looks like in the default theme, in ThinIce, and Bluecurve. Note that there is no missing pixel in the red circle; the default theme renders this corder exactly the same.

Note that to get the rendering of the screenshot, along with the patch must be added (in userChrome.css) tab { padding: 4px 6px 4px 4px ! important; } tab:first-child { padding-left: 6px ! important; } .tabs-right { -moz-appearance: tab-spacer-right; } ----- Also note that the solution used in this patch makes the native rendering via -moz-appearance match closely the way the borders are drawn without any -moz-appearance : Each tab is responsible for its part of tabpanels border, and the spacer .tabs-right draws the remaining part of the border. Thus, as a bonus side-effect the odd border above tab panels is removed if and only if the tabbox is natively drawn, which is exactly what is wanted.

If wanted, I can use max(ythickness,2px) in place of 2px for the height of the part of tabpanels top border drawn by tabs. Hopefully themes having large borders would correctly set this value. I am unsure if it is worth it, though. And if the size is variable, it means that the height of the tabs should vary accordingly.

Trying to automatically get the right border-size in layout for -moz-appearance: tab, I noticed there are two places where custom values are calculated for native border width rather than using gtk_ provided ones : in nsNativeThemeGTK::GetWidgetBorder, and moz_gtk_get_widget_border (gtk2drawing.c). I didn't succeed in determining which rule applies to what to put where. Furthermore, I noticed that a lot of widgets just don't fill in their border info, but rather put 0. So what should be done ? Should I return a good border value (which seems sane to me, instead of managing the border with ad-hoc padding values), and if yes, where should I do it ? Directly into GetWidgetBorder, or in moz_gtk_get_widget_border, which is called by the former ? Note that moz_gtk_get_widget_border returns only two values, vertical and horizontal, and that those are used to fill in the rect ; if non symetrical values are wanted, nsNativeThemeGTK::GetWidgetBorder is the way to go.

The review has been asked for some time now, and no response from bryner. I reckon that if roc bounced the review to another person, he has no time to review. bryner obviously has no time currently either... Should I ask for review to another one ? Or just wait for events to settle down and reviewers' free time to emerge anew from time to time ?

While testing the patch for using display lists in paint/events, I just noticed a bug that is present in both the current implementation, and the implementation proposed in this bug : The selected tab is 2px taller than the others, since every other has a 2px top margin. Then, if the selected tab isn't the first, its left edge is painted by it's left neighbour, in the overlap area. In order to paint it correctly, we enlarge the cliprect 2px up, and paint outside of the left neighbour border area. So we end up with something like that, zooming on the top of the edge between the selected tab and its left neighbour : .......... __:_________ corner -> | : .............|..: _____________| : | : | : The _ and | represents the drawn borders, and the dots represent actual widgets boundaries. The problem is that the corner is repainted if and only if the left neighbour is invalidated, but since it isn't in its boundaries, one can invalidate the corner without it being repainted. On edge cases, we thus get a 2px.2px square that isn't repainted. The solution is to make every tab the same height and put the difference of top into the border. But I am not sure it is really cool, since currently the code returning native border widths isn't passed an instance of the object, just its type, and thus cannot return different borders depending on the position of the widget, for example. It would be really usefull, because without it we must use padding values to replace native border widths that aren't powerful enough.

Comment on attachment 204275 [details] [diff] [review] Addition of "-moz-appearance" value, right one Clearing review request... I'm just not going to have time to get to this, sorry.

Comment on attachment 204277 [details] [diff] [review] GTK2 specific part Clearing review request... I'm just not going to have time to get to this, sorry.

You may be able to solve the problem in comment #27 by modifying nsITheme::GetWidgetOverflow. Please request review from me if you do that, or even if you don't.

I really lack time atm, but I will at least test if my patch still applies to trunk, and unbitrot, but may be unable to do more for now. I'll request review for a patch against current trunk. _FrnchFrgg_

I had inverted the two patches, so it is *this* one that should have obsoleted the previous layout part. Sorry.

I have added code to return good border values, and the rendering is far better without any tinkering of padding. Included in this patch is my attempt at using GetExtraSizeForWidget() to enlarge the overflowarea as suggested by roc, but it doesn't seem to correct the bug spoken about in comment 27.

Why's that? Is the left-tab not getting invalidated and redrawn when it should be?

(In reply to comment #35) > Why's that? Is the left-tab not getting invalidated and redrawn when it should > be? I can still erase the corner without it being redrawn. I didn't test to see if the tab receives an invalidate event, and if the repaint function is called (and the results somewhat lost because of another pb). I'll investigate, but I am pretty certain the _paint function isn't called.

I'll resend a patch for the skin part with correct spacing (the current one doesn't respect the surrounding style), but I think the current one is sufficient for review, and I don't want to play with flags now.

re comment #36, have you had a chance to investigate?

My _paint function isn't called; I am not sure about how to know an invalidate event is triggered (but eaten because of an intersection with the XUL object rect instead of the overflow rect, for example)

This is really quite complicated. I'd much prefer a solution where tabs don't need to paint parts of other tabs. + else + gapstart = rect->width + 10; What is this magic number 10?

This is an arbitrary value to put the gap outside of the widget. I agree that this is over-complicated. The problem is that if each tab painted itself (and the corresponding part of the tabpanel border), there would be no mean to ensure the correct overlapping, since (last I checked, and when the initial code was written) XUL doen't honnor z-index. Even if we can refactor the code not to draw parts of adjacent tabs, the code is still complicated because we need to tie the drawing of the tabpanel border to the selected tab. The patch I propose is somewhat of a hack (the tabpanel border should belong to the tabpanel) but it would probably be prohibitively complicated (and perhaps unwanted) to do otherwise given the layout model where the tabs & tabpanel are independent. So a tab will probably still draw parts of other widgets (unless we give up correctness of the appearance for tabs).

There are two + 10 in the code... There is another in the _paint call just after the setting of gapstart. It is also arbitrary, to ensure the border painted will be wide enough (in particular, it needs to be wider than the gap to be sure the end of the gap will be drawn in selected case, and it needs to be wider than the tab). It should be possible to replace those "10"s by the smallest value possible each time, while ensuring correctness.

Same thing in moz_spacer_right_paint (not sure about the name), the "10"s are arbitrary (but need to be the same value to align the end of the box_gap drawing with the right of the spacer).

Can you add comments to those to describe what they're for? How much better would it be if we allowed tabs to z-order properly so the active tab was always drawn last? If it makes things better, it might be worth doing even if it doesn't solve all problems.

If the tabs could z-order properly (not only the selected tab last, but ensure that they are painted left-to-right — or better, in the currently defined direction) then each tab would only need to paint itself, and the part of the tabpanel border underneath itself. It would simplify the code with only two states: selected or not. Perhaps we could even do something as a box_gap painted by the tabpanel with no gap at all, and just a gap overwritten on it by the selected tab, but it wouldn't simplify greatly the code, since a tab would still need to sometimes draw a box_gap.

I'm developing a XUL application and I've been bothered by this bug for years now. Until today I assumed that there was some wrong detail in my XUL files (a missing attribute/class/whatever) and that as soon as I fixed it my application, which has a tabbed interface, would look fine. It seems that the way to have good-looking tabs is to use the tabbrowser tag. I've looked there with the DOM inspector and it uses nested boxes with stacks etc. to achieve the nice looks it has. However using it is not a good solution for me since only some of my tabs contain documents in iframes. There are a couple more that have XUL content directly inside the tabpanel, and I'd like to keep it that way. I've been fumbling around with my tabbox trying to make it look decent, and I've got some partial workaround. It seems that the whole thing revolves around margins and drawing order (z-order as mentioned by FrnchFrgg above). First I tried setting the bottom-margin of the tabs element to -2px, to bridge the gap (setting the top margin of the tabpanels element to -2px gives the same result). This put the tabs in the right position, but the top border of the tabpanels element was painted over them. Then I switched the tabs and tabpanels, so that the tabs were drawn last and thus over the tabpanels, and finally set the attribute dir to "reverse" to put the tabs back on top. This way, the bottom of the tabs covers the top of the tabpanels, and the selected tab gets connected to its tabpanel. I'll try to post an example file as an attachment. This seems to work in Firefox in both MacOS X and Windows XP, although in Windows the top border (a thin line) of the tabpanels gets clipped out, so it does not look perfect. In SeaMonkey, the tabs element does not span the whole width, so even in MacOS X the top line gets clipped outside of the tabs. Would it be hard to make a special case for the rendering of tabboxes so that the tabs were drawn after the tabpanels regardless of the "dir" attribute, with the 2 pixel overlap? While I'm asking for things, it would also be nice if the tabs adapted to the "dir" and "orient" attributes, so that we could use tabs in the other three sides of the tabpanels. Thanks for taking the time to read this report. I'm quite annoyed by this bug, but I understand we are all busy, and I'm very happy anyway seeing bugs being corrected all the time. :)

This XUL file shows the default tabs rendering on the left, and the modified one in the right, done by inverting the order of tabs and tabpanels, and joining them with a negative margin.

I've found a problem with the workaround in comment 47 when a tabbox is inside the tabpanel of another, and reported it as bug 384773. If you have no nested tabboxes, it should work fine. However this means that I can't use it in my application.

Michael might be interested in this.

RoC: What do you mean by "interested in this" ? Should I unbitrot the patches and ask him for review, or ask him for advice, or let him take over this bug ?

Thinking of comment 45, I think it would be a good solution to make the tabpanel paint its top border, like before, and then make only the selected tab overwrite it with a gap. Advantages are: - A tabpanel alone has its 4 borders fully painted - no painting of the top border of the tabpanel in chunks which is simpler, faster, and more correct (so if the theme draws a gradient for the border, for instance, visual correctness is ensured) - probably no need for the new -moz-appearance (if we rewrite the unthemed appearance to use the same ideas, with overwriting of a gap; else, the added -moz-appearance is still needed, if only to suppress the unthemed drawing taking place there) - others that I don't have in minde Disavantages are: - Still need to paint a "gap", so some tabpanel drawing in the code of tabs - If the border of the tabpanel is painted wider than 2px by the theme, and the theme doesn't expose this width, like a lot do, we cannot make the "gap" sufficiently thick to overwrite the whole border. Perhaps not a great problem, because for now, in the themes I encountered that could be affected, the "gap" and the normal border had no visual differences outside of those 2px (see https://bugzilla.mozilla.org/attachment.cgi?id=204234, the aqua-like theme for example) - Need to get z-index to work properly in XUL, and I don't know how to do that (in fact I suspect it is difficult, or else the current solution wouldn't have been used at all)

I mean, ask him for advice or let him take over this bug.

He can take over the bug if it means that it is solved quicker and better (especially on Windows since I have no means to get hands on a Windows with a build environment) The only thing which annoy me is that my first chance to get code checked in Gecko evaporates... :'-( It's difficult to relinquish ownership of a pet bug, especially since a lot of lost time came from my not knowing who to ask review to, etc... I still think that my patches are good to base upon, I can even attach a new patch better documented, unless the route of comment 53 is choosen. The proper patchset would then remove a lot of code and add few (not counting what is needed to get reliable z-index). My bug being stolen (hehe ;) won't discourage me from trying to annoy you elsewere... You'll have to bear my chattiness (in comments) a bit longer; and I'll continue to watch this very bug. It's probably better like this, because for the upcoming year I will be very busy preparing a "only 180 best accepted" exam (to earn the right to teach Maths and Computer Science at university). Thanks for all the fish !

FrnchFrgg, if you want to please go ahead and continue with this bug. I'm a little busy with other things at the moment anyway.

I'm not asking Michael to take over this bug if you want to keep working on it. I'm mainly hoping he can provide better feedback that I have (i.e., almost none).

As I said, I'd like to keep working on this bug, but: - I only have PCs, and I haven't Windows - I don't know how much time I'll be able to devote to leisure (gecko is leisure to me) this year - The correct resolution of the bug entails a correction of z-index behaviour which I am not sure I can tackle That said, with advice and pointers I could dig into the z-index pb, and write a proper solution, fully documented, for Gtk. The theme part wouldn't need a lot of time and I'm sure I can do it before christmas (probably sooner, but I prefer to be conservative). OTOH, the z-index part is unknown to me, so I cannot predict a timeframe. If having me to tackle this, even if I work slowly, suits you better than having to devote your time coding, I can do the dirty work while you give me hints. What do you think about it ?

I meant "devote your time coding _on this issue rather than where you're more needed/gifted_"

We should add the z-index capability you need, but I don't have time right now to figure out the best way to do that :-(.

Changes since last version: - unbitrotted - better doc, especially for arbitrary values to avoid border effects - introduction of a const gint (hopefully optimized away by gcc) "overlap_width" instead of using "2" everywhere so formulas & gdk api calls are more readable, and update is easier, should this value be no longer hardcoded in GTK in the future. Still no windows/mac os implementation. Applying those paches shouln't break any platform, because even if I added a new "-moz-appearance", platforms should gracefully ignore unknown ones, though nsNativeTheme::ThemeSupportsWidget() I am not sure, though. Note that this is not the way I eventually want to do this. When z-index is corrected, I'll first change the non-native way to draw all four borders for tabpanels, and overwrite with a gap for the selected tab (should also simplify the introduction of tabs attached to another border than the top), then make the native themed drawing work alike. This solution avoids the introduction of a new "-moz-appearance: tab-spacer-right", and greatly cleans the code (there will no longer be the "overlap" which is rather convoluted; the windows code has a different and less akward, but still unsatisfactory way of dealing with the z-index pb) It would be also simpler to update the native drawing for other platforms, since there won't need to introduce the implementation of a new "-moz-appearance" everywhere. It could then be progressive, especially since it wouldn't break native themin a lot visually (just eat two more pixels where a fake overlap has been coded).

FrnchFrgg, I'm not a qualified reviewer. You should ask Robert O'Callahan instead. The patch looks OK to me, but I'd like to know what you're going to do about the actual CSS. I suggest you copy the Winstripe tab.css into the Gnomestripe folder, add it to jar.mn, then add the -moz-appearance CSS rule there.

I thougth I could use the winstripe tabbox.css, because if the win32 nsINativeTheme says "I don't support -moz-appearance: tab-spacer-right", then it is drawn with the fallback CSS (IIRC), as if nothing changed. That's why I was happy with attachment 248852 [details] [diff] [review] (which didn't bitrot; sorry I didn't make clear that it was part of the fix)

(In reply to comment #64) > I thougth I could use the winstripe tabbox.css, because if the win32 > nsINativeTheme says "I don't support -moz-appearance: tab-spacer-right", then > it is drawn with the fallback CSS (IIRC), as if nothing changed. > > That's why I was happy with attachment 248852 [details] [diff] [review] (which didn't bitrot; sorry I > didn't make clear that it was part of the fix) > That's fine, but if you do make a separate Gnomestripe version then you can also add that padding to make the tabs look like The GIMP, as shown in attachment 204234 [details]

Is it worth doing this interim thing? why not just go straight to the z-index solution?

It depends on the difficulty and time needed to fix the z-index bug. I'd say it's not worth it, but it was easy to add doc and unbitrot, so I did that. What do you think ?

I'd rather not add something that we then plan to take away soon.

I'll then post a patch set that will make tabs responsible only for their rendering, and, if selected, the corresponding gap. Should I use CSS or javascript (or C++) to make tabs get a z-index corresponding to their position (to get the proper effect, z-index should be raising as we consider tabs start to end, except for the selected one which should be topmost) ? I think a pure CSS solution, while possible, would be rather bulky and wouln't scale to an arbitrary tab count. This patch set would unfortunately break the rendering more than it currently is, until the z-index problem is sorted out. I probably could deliver one week from now. Does that sound good to you ?

Make the tabs position:relative and use CSS z-index on them. You probably want to make all the tabs z-index:0 except for the active tab which can be z-index:1. Hmm, that might even work already!

I changed the code in my tree so that a tab only draws itself and optionally the gap below it. I then used a userChrome.css (changing tabbox.css & recompiling is tedious) to make the tabs overlap, and the selected tab overlap the tabpanels. Conclusion: z-index is ignored, and what is drawn later is on top; the tabpanels aren't overwritable, and the perceived bottom-most tab is the leftmost one. So currently everything's backwards...

can you give me a standalone XUL testcase with overlapping elements and z-index and I'll try to come up with a patch to make z-index work?

I'll do that, yes. I'm afraid there would be no mean to make the tabs overlap properly without assigning a different z-index for each one (descending from left to right). Then comes the problem of doing that for an arbitrary number of tabs (I'm not aware of a way to do that with CSS selectors). And assign a value higher than all to the selected tab. It could be easier if we could have z-index: 0 for the selected one and going into negative z-indexes, but it seems the body has its z-index equal to 0 so every negative will end up hidden ?

It just occured to me that getting z-index to work and make the tabs overlap (and the selected one overlap the panels) would no almost the right thing on every platform. The only glitch remaining would be the gap which wouldn't be über-pretty (the borders of the tab wouldn't join the top border of the tabpanel in a neat corner). Then we could add platform specific code to get this last effect right, but in the interim it would be better than what we have in current trunk.

> (descending from left to right). So the left-most tab appears on top? That sounds weird. If we have to assign a different z-index to each tab, that's not really too hard, just some Javascript code that has to be called whenever a tab is added or removed or when a tab is activated. > it seems the body has its > z-index equal to 0 so every negative will end up hidden ? Doesn't have to be, the tab strip can be given its own stacking context so it will appear in front of the body.

(In reply to comment #75) > So the left-most tab appears on top? That sounds weird. Yes, it's the left-most tab that's on top. See attachment 204279 [details]. It mimics the tabbed filers in real life. > If we have to assign a different z-index to each tab, that's not really too > hard, just some Javascript code that has to be called whenever a tab is added > or removed or when a tab is activated. I know it won't be very hard, just not in pure CSS. > > it seems the body has its > > z-index equal to 0 so every negative will end up hidden ? > > Doesn't have to be, the tab strip can be given its own stacking context so it > will appear in front of the body. Ah, I forgot that thing about "sub-stacking-contexts". So it ends up rather simple, provided the z-index bug is not to hard to correct. BTW, there's bug 84687 on z-index not working in xul, but it's been WONTFIXed. I think it's more specific than the title suggests, because the comments focus on stacks. The testcases there are not useful, I think, so I'll come up with one. Should I attach it there, or do you want to open a new fresh bug for z-index-ness ?

sorry, the bug is bug 94687, not 84687

What about using the xul ordinal attribute instead of z-index? You would still have to give each tab a different ordinal, but it might work without having to fix the z-index bug.

Won't that cause the tabs to appear in a different horizontal position?

Yea, that won't work. They're not in a stack, so it would have too much of an effect.

While writing the testcase, I had the idea of getting inspiration from http://dbaron.org/css/test/z-index/01-basic/ and as I saw http://dbaron.org/css/test/z-index/01-basic/05.html "z-index does not apply when position is static", I tried to put "position: relative" on xul objects, and then they seem to obey z-index... Are there good reasons not to use "position: relative" on tabs ?

See comment #70 where I mentioned making the elements "position:relative". If you don't set "left" and "top" then setting position:relative will have no effect, so just do it!

I didn't see it at the time... Perhaps I ought to sleep at night, instead of reading bugmail... I have a patchset nearly ready, I just need to tighten it a bit. One of the last problems I have is about the 2px of overlap. Indeed, I think that winstripe doesn't want it everywhere (but we still want the selected tab to overlap it's neighboors and the tabpanels), be it native themed or not. OTOH, the rendering of non-native with a 2px overlap is beautiful too, so perhaps that we just should make gnomestripe have this overlap, whether or not it will be native-themed (since IIRC -moz-appearance cannot override margins, it would probably be difficult to make tabs overlap only if native themed). But the only difference between a winstripe "tabbox.css" and a (to be created) gnomestripe "tabbox.css" would be this overlap... I am somewhat reluctant to clone this file (which means maintaining separately nearly identical files in the future) because of so little a difference. Is there a clean mean to do ? I didn't investigate the mac theme (but the advantage of the new technique is that the old way can coexist with the new...)

This part shouln't change a lot from now on (appart to address review comments). It removes a greeeeeaaat deal of code (Yay !)

(OK, it removes more comments than code, but it's because it way simpler, so no need to explain the hand-made overlap)

This skin part has several shortcommings (appart from those discussed in comment 83) : I didn't touch the "tabs-bottom" part, and I didn't test RTL (but I should have taken care enough to make it work) I didn't write the js code to set z-index, so for now I added the following in my userChrome.css: tab { z-index: -1; } tab + tab { z-index: -2; } tab + tab + tab { z-index: -3; } tab + tab + tab + tab { z-index: -4; } tab + tab + tab + tab + tab { z-index: -5; } tab + tab + tab + tab + tab + tab { z-index: -6; } tab + tab + tab + tab + tab + tab + tab { z-index: -7; } tab + tab + tab + tab + tab + tab + tab + tab { z-index: -8; } tab + tab + tab + tab + tab + tab + tab + tab + tab { z-index: -9; } It's ugly (especially since nth-child isn't handled by gecko), but it's good for testing.

If we want to preserve Windows2000 rendering when non-native in winstripe, then there would be more differences than just the overlapping between gnomestripe and winstripe (paddings would also be different, at least). A fork of tabbox.css would be more justifiable then, wouldn't it ?

(In reply to comment #83) > One of the last problems I have is about the 2px of overlap. Indeed, I think > that winstripe doesn't want it everywhere (but we still want the selected tab > to overlap it's neighboors and the tabpanels), be it native themed or not. > OTOH, the rendering of non-native with a 2px overlap is beautiful too, so > perhaps that we just should make gnomestripe have this overlap, whether or not > it will be native-themed (since IIRC -moz-appearance cannot override margins, > it would probably be difficult to make tabs overlap only if native themed). You can modify GetWidgetOverflow to allow your element to draw outside its frame rectangle by some amount.

I didn't think of that. I suppose the end result is like a negative margin (but it will complicate calculus of padding, since the width must be reduced to get the same rendering; currently I defer to GTK which tells me what size the borders are). I think I'll fork tabbox.css, because it'll permit to get padding and overlap more gtk-like even without native theming, without compromizing the appearance in windows. What do you think I should do ? (is it sufficient to put a modified tabbox.css in the gnomestripe folder, or is there some Makefile magic to cast ?)

You'll have to add the new file to the jar.mn file, or it won't get included in the build.

Daniel: Thanks. roc: It seems that making tabs position: relative breaks the tab strib of the browser; there's no mean to change the active tab by clicking. No problem if I let them be position: static. When I wrote a userChrome.css to put tabs on the right of the browser, stacked vertically, I noticed that this stuff is really fragile... Applying some very simple styles could even break the "undo close tab" functionnality. I'm not sure I'm able to fix that.

That sounds like a bug, nothing more. Can you make a small XUL test case?

I think it's because of the negative z-index. There is a lot of things overlaid on top of each other in this tabstrip. Maybe some of them has a stacking context and is transparent but not click-transparent... Anyway, the simplest way to correct that is to put .tabbrowser-tab { position: static } somewhere appropriate (probably in browser.css)

I have a little problem, and I need your advice: In the native theming code, I get the height H of the border with ythickness, and if it is less than 2px, I set it to 2px. Then I put the "gap" in the H bottom pixels of the tab. Those pixels overlap the tabpanels border because of the negative margin. Problem: if H > 2, the negative margin does not match, and the gap is misaligned. The only solution I see for now is to draw the gap always 2px from the bottom, and let it overflow H - 2 pixels, which I can advertise with nsITheme::GetExtraSizeForWidget(). But I find it clunky, with anew those hardcoded 2px. That means the c code has to be changed everytime the margin is changed... What do you think ? On an unrelated note, I have a gnomestripe tabbox.css which renders tabs with nearly pixel for pixel match between non-native and native with default GTK theme (not clearlooks, but the old one), and a winstripe tabbox.css using the new method (with negative margin under selected tab) but achieves the exact same result visually than the current tabbox.css.

I don't have any better ideas about that.

For the record, here is where I am currently heading: If selected, draw a "gap" to overwrite the tabpanel top border, and draw it |gap_voffset| pixels from the bottom of the tab, where |gap_voffset| is |-bottom margin|, but ensured between 0 and |gap_height|, where |gap_height| is essentially the YTHICKNESS advertized by GTK. This means that the gap is ensured to be correctly aligned with the tabpanel top border. This also means that the gap can overflow the tab (of gap_height - gap_voffset), so we need to advertise that via GetExtraSizeForWidget. In particular, if the border of a tab is 2px like in Clearlooks, and the bottom margin is -2px like in the planned replacement of tabbox.css, everything is rendered like in the currently attached series of patches, but if tab border is different, the rendering is now correct. My current problem is that moz_gtk_tab_paint() seems to be called from moz_gtk_widget_paint(), which is called from ThemeRenderer::NativeDraw(), which is probably called from ThemeRenderer::Draw() [inherited from gfxXlibNativeRenderer], called in nsITheme::DrawWidgetBackground, and only there I find a frame to fetch the bottom margin from. What should I do ?

Note that I encountered real themes that have more than 2px thickness, and are thus broken without what I am trying to do. We could decide that since such themes are more borked currently, it's not that important, especially since a lot of themes have only 2px borders. But it's less satisfactory; it feels like hiding monsters under the carpet

DrawWidgetBackground calls GetGtkWidgetAndState, where one of its OUT parameters is a general-purpose gint (aWidgetFlags) you can set. This is passed to moz_gtk_widget_paint in gtk2drawing.c. So maybe you can use the flags parameter in GetGtkWidgetAndState to set the value of the bottom margin.

Yeah, but it's currently used to pass along some info as to whether the tab is selected, and/or the first of the tabs. Perhaps I could use some macros to bundle this info into the gint, or use a struct that fits into the gint...

Make tabpanels draw their 4 borders (instead of 3), then use z-index and negative margin on selected tab to overwrite the border (and get a gap). Do that in a new gnomestripe file, to allow divergence of default rendering on linux and windows.

Since the new CSS takes care of tab overlapping with z-index, remove all ad-hoc code which was simulating that. Also add the drawing of a gap with box_gap under selected tabs, to get pixel-perfect rendering. Make GetWidgetAndState() pass margin info along with NS_THEME_TAB flags. Also remove no longer used flags. Use margin info from GetWidgetAndState() to correctly position the tab gap. Since it may make the gap overflow the frame, indicate so with GetWidgetOverflow()

A refactor of CSS for tabboxes for Win32 is coming, the goal is to have no change in appearance (when not natively themed). This first patch would make the native Win32 rendering almost correct, but a followup patch could/should be written to get a pixel-perfect imitation of how native windows tabs look. NOTE 1: On gtk, the tabs are drawn with the leftmost one on top. It means that the n-th tab (from left) should have a "-n" z-index; it would be best done in javascript, which would permit an arbitrary number of tabs; a solution in pure CSS would be like: tab:first-child { z-index: -1 } tab:first-child + tab { z-index: -2 } and so on. NOTE 2: This approach revealed at least one bug in gecko: bug 399532, for which I have a patch, but the graphical quirk is still there even with this fix (you can see it by hovering the tabpanels, then move the mouse out of it at the bottom; you'll see the tabpanels redrawing over the tabs whose stacking context is supposed to be higher). It seems the dirty area is intersected with the widget's rect before the call to nsLayoutUtils::Paint(), and so before the creation of the display list; I didn't succeed in finding where [see bug 408059]

Addendum to NOTE2: The bugs spoken about are only visible if the gtk_box_gap YTHICKNESS is greater than the negative margin set on selected tabs (if not, the tab "gap" never overflows the widget's rect thus no room for the bugs). To see the bugs you should force margin-bottom to 0. It is good news since 90% of users will never encounter those bugs with tabs (which in any case don't break the rendering more than it currently is without the patches). NOTE 3: In its current form, the CSS patch breaks the tabbrowser strip; it seems not to like the z-indexes.

Addendum to NOTE3: It's not the CSS patch that breaks the tabbrowser strip (inactive tabs don't respond to click), it's setting negative z-indexes. tabbrowser box { position: relative; z-index: 0 } solves this problem. I tested and didn't find broken things anymore (undo close tab still works, etc...)

Note that this removes the need for -moz-appearance: tab-right-edge and tab-left-edge. I'll post a "cleanup unused cruft" patch when everything has settled.

Comment on attachment 292755 [details] [diff] [review] [2 of 2] Refactor native GTK drawing of tabs, taking advantage of new CSS >+ if (flags & MOZ_GTK_TAB_SELECTED) >+ { >+ if (flags & MOZ_GTK_TAB_FIRST) >+ gap_hoffset = 0; For RTL layouts the gap_hoffset should be 10 on the first tab, and the width should then be like below, because the first tab is drawn to the right edge. >+ else >+ gap_hoffset = 10; /* 10px should be enough */ >+ >+ TSOffsetStyleGCs(style, rect->x - gap_hoffset, >+ rect->y + rect->height - gap_voffset); >+ gtk_paint_box_gap(style, drawable, GTK_STATE_NORMAL, GTK_SHADOW_OUT, >+ cliprect, gTabWidget, "notebook", >+ rect->x - gap_hoffset, >+ rect->y + rect->height - gap_voffset, >+ rect->width + gap_hoffset + 10, rect->height, For the first tab on RTL layouts (gap_hoffset = 10): rect->width + gap_hoffset + 0 >+ GTK_POS_TOP, gap_hoffset, rect->width); > } > > return MOZ_GTK_SUCCESS; > } You can get the text direction the way I was going to do in attachment 292874 [details] [diff] [review] (bug 402247), once the patch for bug 316748 has been checked in.

This patch takes the comments of Teune into account, and is to be applied on top of the patch of bug 316748. Teune, on a side note, could you review both CSS sheets (GTK and Windows) regarding RTL ?

> Teune, on a side note, could you review both CSS sheets (GTK and Windows) > regarding RTL ? > I see no problems in the CSS sheets, but I must say I don't know much of RTL handling on Windows (whether CSS may require hacks). However there is something else I noticed today: some themes draw rounded corners for notebook widgets, leaving the topleft corner square for LTR and the topright corner for RTL. But to let the theme engine know which direction we are displaying our widgets you should use gtk_widget_set_direction() in both the tab_paint and tabpanels_paint functions. Note also that not all theme engines check for RTL when drawing rounded corners (Nodoka for instance), but that is not our problem.

In fact, it may be worthwhile to use gtk_widget_set_direction for every type of widget... I just thought about one thing: It may be better to use -moz-margin-start, -moz-padding-end, and co instead of overloading margins with [chromedir="rtl"] What do you think about it ? By the way, roc, who should review the skin patches ?

I can do that I think.

Redone with -moz-margin-start, far prettier (the code). Rendering not changed.

Redone with -moz-margin-start, far prettier (the code). Rendering not changed. I have also tested more and corrected the tabs-bottom rendering; I have tested the RTL also, works without glitch with or without -moz-appearance.

What remains to be done: - Set "z-index: -n" on nth tab (shouldn't be necessary on winstripe since neighbouring tabs don't overlap there) I don't know if it's better do do that in script or with CSS, if script i'll need tips. - set the correct orientation on the underlying gtk widgets, best done in a followup bug (especially since it concerns a lot more than tabs). - modify the tabbrowser code so that the box enclosing the tabs has "position: relative; z-index: 0" since tabbrowser inserts some things in the "tabs" object which obscures (event-wise) the tab widgets when they have negative z-index. Creating a new stacking context only for them solves it. In my userChrome.css, I use the selector "tabbrowser box", which is probably overreaching.

Only change with previous version: z-index of selected tab is 1, not 0. This permits a aesthetical rendering event without the negative z-indexes on other tabs (only "glich" remaining without those z-indexes: unselected tabs are not raising from end to start as in gtk).

Only change with previous version: z-index of selected tab is 1, not 0. This permits a aesthetical rendering even without the negative z-indexes on other tabs; the negative z-indexes are then not needed, since unselected tabs don't overlap on windows (thus no need to z-order them).

Followup bug about orientation is bug 408620. I need advice about how to do the automatic negative z-index part, and about what to do with tabbrowser code.

Do we need to change winstripe at all? This patch would be easier to get approved if we didn't. + /* First six bits will be used to store max(0,-bmargin) + * where bmargin is the bottom margin of the tab in pixels */ Instead of doing this bit thing, how about just make GtkTabFlags a struct, e.g. struct GtkTabFlags { PRUint32 mMargin; PRPackedBool mFirst; PRPackedBool mSelected; };

I was told on IRC I wasn't clear enough, so here is a new take: - the CSS patch for winstripe has been tested (on linux without native theming, which should be the same as on windows without native theming) and renders nearly pixel-perfectly as without the patch applied. The goal is getting the same rendering while always drawing the top of the tabpanel (and then overwriting it) to mimic what the native theming does. With native theming applied, this CSS patch should make the Windows tabs be correctly linked to the tabpanel, and some other glitches outlined in the bugs here should be also corrected. This wouldn't bring pixel-perfect imitation of native windows since I cannot write the windows native theming code which would draw the beautiful native gap as I've done in GTK. - The CSS patch for gnomestripe is nearly the same as the winstripe one, with some differences: The tab padding on GTK is greater, and neighbouring tabs overlap a bit (2px). Without native theming it achieves a really good imitation of the default (plain) gtk theme (nowadays rarely used). The only problem is that in GTK tabs are raising from end to start, that is the first tab is topmost. To achieve that, there should be decreasing z-index from start to end, and I think the best is to set "z-index: -n" on the n-th tab (when not overriden because the tab is selected). It can be done in pure CSS, but is then limited to a small (10, 15, ...) number of tabs, and is potentially slow. It can also be done in javascript with updates each time a tab is added, moved, etc... For this solution I need advice. Last problem of this negative z-index is that for the tabbrowser strip creating a new stacking context on the "tabs" element is no longer sufficient, since the bindings introduce other things inside it that then obscure event-wise the tabs. That's why setting "position: relative; z-index: 0" on the enclosing box of the tabs is needed, and I too need advice on where to do that. - The GTK native drawing code's role is to get the last pixel-perfectness by drawing the gap part of the tabpanels top border when drawing the selected tab. the vertical positioning of this "gap" is calculated so that the gap is correctly aligned with the tabpanel border even if YTHICKNESS is different from the negative margin used to make the selected tab overdraw the tabpanels border. In effect it simulates the ability to override margins with native theming (and it is needed for a lot of themes, especially aqua-like ones).

Note that the patches are independant, even if the gtk theming part has no sense without the gnomestripe modification. In particular, the winstripe patch could be thrown away, or used as a basis for the correction of the bug on Windows, or anything else that you see fit. I just put it here because it was so little more work to do when the gnomestripe patch was done.

What about testing on Windows *with* native theming? IIRC, Windows's native theming for tabs works a good bit differently than GTK's.

I know what does Windows' native theming for tabs, so I reckon it will do what I expect, but I have no windows at home (nor do I plan to take the plunge). You could try the patch if you want, and decide if it's good enough as-is (considering the current rendering is really ugly, see attachment 163087 [details] on windows). In fact, when I look at this screenshot, it seems that the beautiful gap is already drawn by windows at the bottom of the tab, so the winstripe patch could be the only thing needed (there could then be a cleanup patch because the test for after selected and before selected in native theming is then unneeded). But again, I cannot test, and I'd like if someone else could take over the windows part.

This patch addressed roc comments about GtkTabFlags, based on IRC discussion. A struct was not used cause fitting in a gint wouldn't be portable (says roc).

Let's handle Windows in a new bug. Looking at the GTK themes I have on my Linux VM (which are mostly based on Clearlooks), it seems to me that changing the z-order of inactive tabs will have hardly any visible effect. So let's not do that right now; let's just let inactive tabs have z-index 0 and the active tab have z-index 1. (If we want to implement different z-indexes for inactive tabs later, I think we'll need Javascript code that runs whenever a tab is added, removed, or activated, which explicitly sets the z-index of each tab.) Then we don't need to modify the tabbrowser code, right, because nothing will have negative z-index?

Exactly. If you don't want to change the z-order of tabs, the current set of patches is enough.

Good, that's very good. + /* If the tab is selected, draw a gap at the bottom; it will overwrite the + * top border of the tabpanel below it, because selected tabs have a + * negative bottom margin (let's call it bmargin). We want to align the gap + * drawing with the overwritten border, even if YTHICKNESS is not equal to + * -bmargin (we cannot ensure they will be equal, since margins aren't + * overridable by native theming). + * To that effect, nsNativeThemeGtk passes us |min(0,-bmargin)| in the + * 6 lowest bits of |flags|. It is extracted into |gap_voffset| which + * is then caped to YTHICKNESS. The gap is finally drawn |gap_voffset| + * pixels from the bottom of the rect, possibly overflowing a bit. This + * overflow is advertised with GetWidgetOverflow(). + * This removes |gap_voffset| pixels to the height of the tab itself, so + * take it into account when drawing */ This comment is hard to follow. An ASCII-art diagram might be useful here. Maybe Teune can help simplify it.

Here's my take: """ ___________________________ / \ | | ----|. . . . . . . . . . . . . . .|----- top of tabpanel | ^ bmargin | ^ | v (-negative margin) | | |.............................| | YTHICKNESS (height of gap) ^ | v overflow v ---------------------------------------- The "gap" needs to be perfectly aligned with the apparent top border of the tabpanel, that is it needs to span exactly the YTHICKNESS arrow. To achieve that, we need to take into account the negative margin already present, and overflow the widget's rect for the remainer. We introduce a supplementary constraint to avoir surprising renderings with big negative of positive margins: the gap always touches the bottom tab border. In other words, 0 < gap_voffset < YTHICKNESS. The 0 < gap_voffset part is already done in nsNativeThemeGTK to enable us to pass an unsigned value. """ Is it better ?

While testing these patches, I encountered a few problems: - with ChatZilla, the selected tab is drawn 2px too low (It has the correct height but the top of the selected tab is at the same position as the top of the non-selected tabs; The bottom is positioned 2px too low). ChatZilla code: http://lxr.mozilla.org/mozilla/source/extensions/irc/xul/content/chatzilla.xul#92 - with the Industrial theme, the selected tab does not seem to cover the tab panel border beneath it, so that the tab and panel do not look like one single element as with native GTK tabs or with Clearlooks for example. (In reply to comment #130) > Here's my take: """ > ___________________________ > / \ > | | > ----|. . . . . . . . . . . . . . .|----- top of tabpanel > | ^ bmargin | ^ > | v (-negative margin) | | > |.............................| | YTHICKNESS (height of gap) > ^ | > v overflow v > ---------------------------------------- > > The "gap" needs to be perfectly aligned with the apparent top border of the > tabpanel, that is it needs to span exactly the YTHICKNESS arrow. > To achieve that, we need to take into account the negative margin already > present, and overflow the widget's rect for the remainer. > We introduce a supplementary constraint to avoir surprising renderings with big > negative of positive margins: the gap always touches the bottom tab border. In > other words, 0 < gap_voffset < YTHICKNESS. The 0 < gap_voffset part is already > done in nsNativeThemeGTK to enable us to pass an unsigned value. """ > > Is it better ? > If I understand your explanation correctly, I would use something like this: For the selected tab, we need to draw a gap that covers the top border (ythickness) of the tab panel. The top position of the gap is obtained from the negative bottom margin of the tab set in tabbox.css (bmargin which is passed in the 6 lowest bits of |flags| as gap_voffset). nsNativeTheme::GetWidgetOverlow removes gap_voffset px from the tab's height so that the gap is not drawn over the tab. which makes me wonder: should rect->height here not be replaced with gap_height? >+ gtk_paint_box_gap(style, drawable, GTK_STATE_NORMAL, GTK_SHADOW_OUT, >+ cliprect, gTabWidget, "notebook", >+ rect->x - gap_loffset, >+ rect->y + rect->height - gap_voffset, >+ rect->width + gap_loffset + gap_roffset, >+ rect->height, GTK_POS_TOP, gap_loffset, rect->width);

> - with ChatZilla, the selected tab is drawn 2px too low (It has the correct > height but the top of the selected tab is at the same position as the top of > the non-selected tabs; The bottom is positioned 2px too low). That's strange, since I didn't change the way the selected tab is made taller ("margin-top: 2px" for all tabs except selected, idem with s/top/bottom/ for .tabs-bottom as the code seems to indicate). Perhaps chatzilla forces a given height and a margin-top: 0... > - with the Industrial theme, the selected tab does not seem to cover the tab > panel border beneath it, so that the tab and panel do not look like one single > element as with native GTK tabs or with Clearlooks for example. The problem with industrial may be that is assumes that the background is already painted, and thus does not paint anything in the inner part of the tab, which in turns prevent us to get our overwriting. Drawing a rect with the background color prior to painting the gap might do the trick... > If I understand your explanation correctly, I would use something like this: > [...] The begining is good, but I think we should explain the additionnal constraint, and you are wrong about ::GetWidgetOverflow. The gap is drawn partly over the tab, partly outside the tab (only if ythickness > bmargin). ::GetWidgetOverflow just tells the rest of Gecko that the tab's "paint bounds" are bigger than its frame rect, which has an influence on redrawing, for instance (if just the overflowing part is erased we still want gecko to tell us to redraw, even if it's not in our frame rect). > which makes me wonder: should rect->height here not be replaced with > gap_height? No, because gtk_paint_box_gap() draws a whole tabpanels, with a gap where indicated. If we make it |gap_height| tall, the bottom border of it will be visible, which we don't want. rect->height is a good candidate since it's tall enough in non pathological cases. Another value could be chosen, like max(10px, 2 * gap_height), or 5 * gap_height, but why bother ? I can change it if you want. I'll try to write a new comment for the whole explanation including your good ideas.

(In reply to comment #132) > > - with ChatZilla, the selected tab is drawn 2px too low (It has the correct > > height but the top of the selected tab is at the same position as the top of > > the non-selected tabs; The bottom is positioned 2px too low). > > That's strange, since I didn't change the way the selected tab is made taller > ("margin-top: 2px" for all tabs except selected, idem with s/top/bottom/ for > .tabs-bottom as the code seems to indicate). > Perhaps chatzilla forces a given height and a margin-top: 0... Never mind this bit: I had forgotten I had removed -moz-appearance: none; for .tab-bottom (which chatzilla uses).

Comment on attachment 293602 [details] [diff] [review] Refactor native GTK drawing of tabs > if (aWidgetFlags) { >- *aWidgetFlags = 0; >+ /* First six bits will be used to store max(0,-bmargin) >+ * where bmargin is the bottom margin of the tab in pixels */ >+ *aWidgetFlags = PR_MIN(MOZ_GTK_TAB_MARGIN_MASK, PR_MAX(0, >+ aFrame->PresContext()->AppUnitsToDevPixels( >+ -aFrame->GetUsedMargin().bottom) )); > > if (aWidgetType == NS_THEME_TAB && > CheckBooleanAttr(aFrame, nsWidgetAtoms::selected)) > *aWidgetFlags |= MOZ_GTK_TAB_SELECTED; >- else if (aWidgetType == NS_THEME_TAB_LEFT_EDGE) >- *aWidgetFlags |= MOZ_GTK_TAB_BEFORE_SELECTED; > > if (aFrame->GetContent()->HasAttr(kNameSpaceID_None, > nsWidgetAtoms::firsttab)) > *aWidgetFlags |= MOZ_GTK_TAB_FIRST; You can make this cleaner by using IsSelectedTab and IsFirstTab How much extra work would it be to theme bottom tabs natively (using IsBottomTab, getting margin top and drawing the gap at the top of the tab)?

Not much for the tab drawing itself (just ask gtk to draw a tab extension attached by its top). For the gap, it is a matter of having a changed gap_voffset calculation (offset from top and take ythickness into account). But then we will have to complexify ::GetWidgetOverflow, etc... For themes which advertize the wrong ythickness, instead of having a vertically cut gap there will be a misalignment, but broken themes get what they deserve. Perhaps best done in a followup bug, as it's not a fix but a feature request ? And then tabs to the right or left could be wanted (personally I put the tabbrowser strip at the right of the screen, having 16:10 ratio) ? I'll try to write something for .tabs-bottom, at first a separate patch to put on top, and if it looks good I'll merge the patches. How do that sound to you ?

wrong focus, wrong height...

Jakub: Please try with attachment 293446 [details] [diff] [review] at least and preferably attachment 293602 [details] [diff] [review] too, on latest trunk (attachment 293602 [details] [diff] [review] needs the checkins of bug 408620)

Trunk is already patched or not? I don't want to compile it on myself.

Jakub: no, trunk isn't patched (yet). But I hope it'll make it for Firefox 3. I cannot ensure it will, though, cause there could be a lot of very good reasons to refrain from checking in those patches (the more time passes, the nearer we are to Firefox 3 release, and the lesser risky patches need to be to be accepted).

Address Teune's comments

Only difference with previous patch: .tab-bottom isn't forced to -moz-appearance: none.

Rewritten moz_gtk_tab_paint() in a cleaner way, more adapted to the top/bottom tabs dichotomy. I paid a lot of attention to comments, which should be clear and detailed. Implementation of bottom tabs is included. Not included yet: the background drawing for themes like industrial, but hopefully the review of this patch won't be invalidated by the addition.

This time with a call to gtk_style_apply_default_background before the paint of the gap. Tested with Industrial, Mist, ThinIce, Redmond, and Clearlooks. I'll try to test with an aqua-like pixmap theme with big ythickness if I have time, but since it worked for top tabs and I used the same idea, it should work too.

Comment on attachment 293447 [details] [diff] [review] refactored winstripe tabbox.css we're not doing this

+ DevPixelsToAppUnits(moz_gtk_get_tab_thickness()) + - PR_MAX(0, -aFrame->GetUsedMargin().bottom)); Wouldn't this be simpler as + PR_MIN(0, aFrame->GetUsedMargin().bottom)? + (aFrame ? aFrame->GetContent()->IsNodeOfType(nsINode::eHTML) : FALSE)); aFrame && aFrame->GetContent()->IsNodeOfType(nsINode::eHTML). I know you just copied it, but still... fix the other one while you're here. + case NS_THEME_TAB: + // Top tabs have no bottom border, bottom tabs have no top border + moz_gtk_get_widget_border(MOZ_GTK_TAB, &aResult->left, &aResult->top, + &aResult->right, &aResult->bottom, direction, + (aFrame ? aFrame->GetContent()->IsNodeOfType(nsINode::eHTML) : FALSE)); + if (IsBottomTab(aFrame)) + aResult->top = 0; + else + aResult->bottom = 0; default: You're missing a "break;" case NS_THEME_TAB_LEFT_EDGE: case NS_THEME_TAB_RIGHT_EDGE: We're not using LEFT_EDGE or RIGHT_EDGE anymore, right? might as well get rid of these + *aWidgetFlags = PR_MIN(MOZ_GTK_TAB_MARGIN_MASK, + PR_MAX(0, aFrame->PresContext()-> + AppUnitsToDevPixels( + -aFrame->GetUsedMargin().top) )); Indent the PR_MAX. Or better still, set a local variable to aFrame->GetUsedMargin().top or .bottom and then do the rest in shared code. + gap_height = YTHICKNESS(style); + if (gap_height < 2) + gap_height = 2; /* some themes don't set ythickness */ Just call moz_gtk_get_tab_thickness here. + cliprect->x -= gap_height - gap_voffset; Did you mean cliprect->y? Other than that, great stuff!!!

Addressed roc's comments.

I wanted to be too quick, and didn't try to compile the changes. Silly me, a trivial stupid syntax error lurking. This is the correct version.

Comment on attachment 294021 [details] [diff] [review] Refactor native GTK drawing of tabs, taking advantage of the new CSS, v5bis I agree with roc: great work! I have a few small nits: >+ else >+ return YTHICKNESS(gTabWidget->style); >+} >+ else is not needed here, you could just return YTHICKNESS(gTabWidget->style) at this point. >+ * :.............................: | (this is the size of the >+ * ^ | tabpanel top border) >+ * v overflow v >+ * ---------------------------------------- you mention overflow in your picture, but you never mention it in your comment, maybe just remove the overflow wording here, to avoid confusion. >+ if (flags & MOZ_GTK_TAB_FIRST) >+ if (direction == GTK_TEXT_DIR_RTL) >+ gap_roffset = 0; >+ else >+ gap_loffset = 0; >+ Use braces here (for the top if statement) >+ >+ *aWidgetFlags |= PR_MIN(MOZ_GTK_TAB_MARGIN_MASK, >+ PR_MAX(0, aFrame->PresContext()-> PR_MAX needs just a little more indentation. >+ case NS_THEME_TAB: >+ // Top tabs have no bottom border, bottom tabs have no top border >+ moz_gtk_get_widget_border(MOZ_GTK_TAB, &aResult->left, &aResult->top, >+ &aResult->right, &aResult->bottom, direction, >+ aFrame && aFrame->GetContent()-> >+ IsNodeOfType(nsINode::eHTML)); since a tab is never a HTML element, the last argument will always be false. So you can just replace it with FALSE here. >+ } >+ } >+ else >+ if (!GetExtraSizeForWidget(aWidgetType, &extraSize)) >+ return PR_FALSE; else if can be on one line

Addressed Teune's comments

I attached the very same attachment as v5 for the first v6. This is the real v6. Hopfully I didn't goof two times a row.

Comment on attachment 294062 [details] [diff] [review] Refactor native GTK drawing of tabs, taking advantage of the new CSS, v6 without goofing Congratulations!

Comment on attachment 294062 [details] [diff] [review] Refactor native GTK drawing of tabs, taking advantage of the new CSS, v6 without goofing Refactor native GTK drawing of tabs to help render XUL tab widgets better.

Comment on attachment 293960 [details] [diff] [review] Redo CSS for tabboxes, gnomestripe only, v3 Redo CSS for tabboxes in order to refactor XUL tab widgets.

Comment on attachment 293960 [details] [diff] [review] Redo CSS for tabboxes, gnomestripe only, v3 Yippee!

Comment on attachment 294062 [details] [diff] [review] Refactor native GTK drawing of tabs, taking advantage of the new CSS, v6 without goofing Yahooo!

Thanks for having led me through this... I need a new pet bug ;)

Comment on attachment 293960 [details] [diff] [review] Redo CSS for tabboxes, gnomestripe only, v3 Checking in toolkit/themes/gnomestripe/global/jar.mn; /cvsroot/mozilla/toolkit/themes/gnomestripe/global/jar.mn,v <-- jar.mn new revision: 1.26; previous revision: 1.25 done RCS file: /cvsroot/mozilla/toolkit/themes/gnomestripe/global/tabbox.css,v done Checking in toolkit/themes/gnomestripe/global/tabbox.css; /cvsroot/mozilla/toolkit/themes/gnomestripe/global/tabbox.css,v <-- tabbox.css initial revision: 1.1 done

Comment on attachment 294062 [details] [diff] [review] Refactor native GTK drawing of tabs, taking advantage of the new CSS, v6 without goofing Checking in widget/src/gtk2/gtk2drawing.c; /cvsroot/mozilla/widget/src/gtk2/gtk2drawing.c,v <-- gtk2drawing.c new revision: 1.60; previous revision: 1.59 done Checking in widget/src/gtk2/gtkdrawing.h; /cvsroot/mozilla/widget/src/gtk2/gtkdrawing.h,v <-- gtkdrawing.h new revision: 1.47; previous revision: 1.46 done Checking in widget/src/gtk2/nsNativeThemeGTK.cpp; /cvsroot/mozilla/widget/src/gtk2/nsNativeThemeGTK.cpp,v <-- nsNativeThemeGTK.cpp new revision: 1.129; previous revision: 1.128 done

Any non-gtk changes need to be moved to another bug (such as the Windows changes).

So, now look into my bugs [1] and fix them :> . 1| https://bugzilla.mozilla.org/buglist.cgi?bug_status=UNCONFIRMED&bug_status=NEW&bug_status=ASSIGNED&bug_status=REOPENED&emailassigned_to1=1&emailreporter1=1&emailtype1=exact&email1=liviopl.pl%40gmail.com&field0-0-0=bug_status&type0-0-0=notequals&value0-0-0=UNCONFIRMED&field0-0-1=reporter&type0-0-1=equals&value0-0-1=liviopl.pl%40gmail.com

Hmmm, tabs in preferences are nice, but focus there is not the GTK one, ane tabs in main windows have strange padding on the right and too small padding on the bottom. I have two screenshots.

Found next misbehave - native GTK have no styling for text on active tab.

The patch clearly mixes up app units and device pixels in nsNativeThemeGTK::GetWidgetOverflow(). extraSpace is in device pixels, not app units. I'll see if I can attach a patch in a little bit... -Jeremy

I think this is what was intended... Is aFrame->PresContext()->DeviceContext() the same as aContext? -Jeremy

Comment on attachment 294284 [details] [diff] [review] Fix unit confusion > I think this is what was intended... Thanks, I'm sure this is what was intended. :) >+ nscoord p2a = aContext->AppUnitsPerDevPixel(); >+ This should definitely be PRInt32 >+ extraSize = nsMargin(0, moz_gtk_get_tab_thickness() >+ + PR_MIN(0, NSAppUnitsToIntPixels(aFrame->GetUsedMargin().top,p2a)), 0, 0); >+ extraSize = nsMargin(0, 0, 0, moz_gtk_get_tab_thickness() >+ + PR_MIN(0, NSAppUnitsToIntPixels(aFrame->GetUsedMargin().bottom,p2a))); please put a space between top,/bottom, and p2a Otherwise this looks good. > Is aFrame->PresContext()->DeviceContext() the same as aContext? What do you mean the same? It's both nsIDeviceContext

Look at my comments please, I've found two issues in tabs. They should be fixed to provide native look.

this is how tabs in firefox main window should look.

this is how tabs in firefox main window should look.

Not that this focus effect is only present in the current development series of gtk-engines and will probably change quite a bit until it goes stable... I don't know how much sense it would make to do much work for this right now.

GTK should draw focus, not Firefox, so it doesn't matter, what style of focus is provided by theme.

Firefox Trunk has problems with Human theme.

From further investigation, it is broken on every theme using the ubuntulooks engine

Ubuntulooks is old and not developed from over year fork of Clearlooks. It should die.

There's also problem with Rezlooks themes (only in preferences).

Hi, Hmmm, if you only attach a patch and don't comment you don't get added to the CC list... (In reply to comment #172) > >+ nscoord p2a = aContext->AppUnitsPerDevPixel(); > >+ > This should definitely be PRInt32 Only in the current (very broken) state of affairs... > > Is aFrame->PresContext()->DeviceContext() the same as aContext? > > What do you mean the same? It's both nsIDeviceContext But are they the same device context? Or is one the printer and other the screen in some cases... -Jeremy

Comment on attachment 294708 [details] [diff] [review] Address review comments Don't forget to request superreview from roc.

(In reply to comment #183) > Hmmm, if you only attach a patch and don't comment you don't get added to the > CC list... Correct, but you added yourself to the CC list a while ago on 2007-12-21 13:49:04 PST.

Sorry, I was offline for a while, I'll try to address issues. No time to do this for now, though.

(In reply to comment #178) > Created an attachment (id=294362) [details] > screenshot of firefox trunk with human gtk theme applied > > Firefox Trunk has problems with Human theme. > This is because Ubuntulooks doesn't obey the clip rect. This bug has been reported and corrected in Clearlooks ages ago.

+ nscoord p2a = aContext->AppUnitsPerDevPixel(); I thought this way of doing was deprecated and we should call conversion functions called UnitToUnit() on the prescontext ? The "from" and "to" units are probably wrong in my patch, though, if you say so, since I don't really know in which unit space (scaled, unscaled, etc.) we are supposed to call the native theming funcs.

For native theming of focus, this is a problem that is not specific to tabs AFAIK, and there should be a new bug on that. Please CC me on it after having filed it. Jakub: Your search url doesn't work for me ?

Try again, with this link: http://tnij.org/jrusinek.bugs .

+ extraSize = nsMargin(0, moz_gtk_get_tab_thickness() + + PR_MIN(0, NSAppUnitsToIntPixels(aFrame->GetUsedMargin().top, p2a)), 0, 0); + extraSize = nsMargin(0, 0, 0, moz_gtk_get_tab_thickness() + + PR_MIN(0, NSAppUnitsToIntPixels(aFrame->GetUsedMargin().bottom, p2a))); While you're at it, these constructors should be nsIntMargin. But wouldn't it make more sense to just set "m" directly here instead of converting appunits to pixels to set extraSize, and then converting extraSize back to appunits in "m"?

I don't know why, but tabs in windows looks still out of place... Right padding especially is strangely big.

Any update on getting a new patch that addresses the review comments?

I think that's what I had originally in mind... I can change the DevPixelsToAppUnits to a NSIntPixelsToAppUnits call if you want to... I'd like an explanation on when to use which, though.

I think the bug should not be closed as it's not fixed yet :> .

Comment on attachment 295910 [details] [diff] [review] Correct use of units in nsNativeThemeGTK::GetWidgetOverflow() what about this part of Jeremy's patch? > - gint margin; > + nscoord margin; > if (IsBottomTab(aFrame)) { >+ PRInt32 p2a = aContext->AppUnitsPerDevPixel(); >+ m = NSMargin(NSIntPixelsToAppUnits(extraSize.left, p2a), this should be nsMargin (In reply to comment #194) > I can change the > DevPixelsToAppUnits to a NSIntPixelsToAppUnits call if you want to... I'd like > an explanation on when to use which, though. > Because NSIntPixelsToAppUnits is used everywhere else here, probably because aFrame->PresContext() has to be called every time otherwise, I personally would go for NSIntPixelsToAppUnits. On the other hand you mention in comment #188 that using AppUnitsPerDevPixel() is deprecated... I expect roc to know what would be preferred.

be consistent, use NSIntPixelsToAppUnits. We may want to change them all to AppUnitsPerDevPixel later.

Jakub, you spoke about wrong rendering still happening in Windows ? Is it the reason you reopened this bug (in that case, you probably want bug 358632, and close again this bug) ? I think that little glitches still remaining would best be handled in followup bugs, this bug was for the necessary refactoring to get an (hopefully more than) acceptable rendering.

(In reply to comment #196) > (From update of attachment 295910 [details] [diff] [review]) > what about this part of Jeremy's patch? > > - gint margin; > > + nscoord margin; > > if (IsBottomTab(aFrame)) { I think margin should be a gint here, since we want to pass an integer to moz_gtk_tab_paint(); I think having subpixel precision isn't necessary (please tell if you think I'm wrong). Perhaps you want an explicit cast, tough.

Comments addressed