271146 - Tabs do not connect into tab panels under Luna

Status: RESOLVED INCOMPLETE

(Core :: XUL, defect, P3)

Description

[Ben Goodger (use ben at mozilla dot org for email)]

Open a Page Info window or a Bookmark Properties dialog in Firefox and you'll
see that the selected tab in the row of tabs along the top of the window does
not connect into the contents of the tab panel.

Comment 1

[Ben Goodger (use ben at mozilla dot org for email)]

Attachment: partial work

visual blemishes, license issues, remain, needs more careful documentation,
removal of pixel hacks.

Comment 2

[Ben Goodger (use ben at mozilla dot org for email)]

Attachment: more work

not well tested (needs testing on 2k/98/mac/linux), and still one or two
issues, but a marked improvement over the current state of affairs

Comment 3

[Ben Goodger (use ben at mozilla dot org for email)]

Attachment: patch that works perfectly

no pixel artifacts here!

Comment 6

[Julien "_FrnchFrgg_" RIVAUD]

I think the patch in this bug is not the right thing to do. The result can be achieved with only CSS and a tiny modification of the native drawing code (mostly reverting the hacks in there).

Comment 7

[Julien "_FrnchFrgg_" RIVAUD]

Attachment: Redo CSS for tabboxes in winstripe [v1]

Here is a probably almost good patch; it's based on the work I did for gnomestripe. But it should be considered more as a proof of concept since I don't have Windows and as such cannot test pixel perfectness.

Comment 8

[Dão Gottwald [:dao]]

How about this:

 .tab-bottom[selected="true"] {
-  margin-bottom: 0;
+  margin-bottom: -2px;
+  position: relative;
   -moz-border-top-colors: -moz-Dialog;
   padding: 4px 6px 1px 6px;
 }

Comment 9

[Dão Gottwald [:dao]]

err, not .tab-bottom, but tab:

 tab[selected="true"] {
   -moz-user-focus: normal;
   margin-top: 0;
+  margin-bottom: -2px;
+  position: relative;
   border-bottom-color: transparent;
   padding: 1px 6px 4px 6px;
 }

Comment 10

[Julien "_FrnchFrgg_" RIVAUD]

Yeah, it's the heart of the patch. But you need also to add a margin-left: -2px and margin-right: -2px, and then add the necessary adjustments to the padding and co to ensure you still get pixel-correctness...

You can reimplement it if you prefer, I won't have the time to nurtur this patch trough review anyway.

Comment 11

[Dão Gottwald [:dao]]

Comment on attachment 324615 [details] [diff] [review]
Redo CSS for tabboxes in winstripe [v1]

I don't understand why the tabs element and all the tabs need position:relative, let alone all the twiddling with the borders, paddings and margins .... Why do tabpanels need 4 borders instead of 3? What about bug 265698 comment 208 and bug 265698 comment 213? Why should we stop using the tab-left-edge and tab-right-edge appearances?
It strikes me that no toolkit peer reviewed this in bug 265698. See my previous comment for what I think would be a more trivial approach. Not sure what's missing there.

If there are really good reasons for your radical changes, please try to explain them.

Comment 12

[Dão Gottwald [:dao]]

Attachment: three issues with the latest patch

(2) and (3) only show up with a classic theme, and (2) is also an issue with the pseudo patch from comment 9.

Comment 13

[Julien "_FrnchFrgg_" RIVAUD]

There are good reasons for the changes:
For the borders, when the tabpanel is native themed, the theme engine draws all
four borders, so we should also do the same when simulating classic.
Then the tab-left-edge and tab-right-edge are just hacks to simulate the fact
that the selected tab overwrites the adjacent tabs without using real z-index.
So if we put the selected tab in front with a z-index and position: relative;
we can set the adhoc margins and forget all those hackish hacks.
So all the tabs need position: relative, or at least the selected one.

As for 265698 comment 208, Jakub has a lot of false positives since he uses
custom patched version of his themes. I'll be perfectly happy to handle a new
bug if his problem is reproduceable. Comment 213 is probably about the browser
tab bar (which I can reproduce), not about normal tabs (which I cannot).

Comment 14

[Dão Gottwald [:dao]]

(In reply to comment #13)
> There are good reasons for the changes:
> For the borders, when the tabpanel is native themed, the theme engine draws all
> four borders, so we should also do the same when simulating classic.
> Then the tab-left-edge and tab-right-edge are just hacks to simulate the fact
> that the selected tab overwrites the adjacent tabs without using real z-index.
> So if we put the selected tab in front with a z-index and position: relative;

I'd prefer a separate bug for this. If the current solution really is the wrong approach in this regard, we should also remove tab-*-edge from the widget code and before-/afterselected from tabbox.xml and tabbrowser.xml.

> So all the tabs need position: relative, or at least the selected one.

Yep, I think the selected one should be enough.

> Comment 213 is probably about the browser
> tab bar (which I can reproduce), not about normal tabs (which I cannot).

Still a regression, isn't it?

Comment 15

[Julien "_FrnchFrgg_" RIVAUD]

(In reply to comment #12)
> Created an attachment (id=324633) [details]
> three issues with the latest patch
> 
> (2) and (3) only show up with a classic theme, and (2) is also an issue with
> the pseudo patch from comment 9.
> 

(1) is because the tab bar is not a normal tab bar and is hacked a lot, the margins would need to be ensured equal to 0 in browser.css

(2) is exactly what the widget code would be useful for (for themed part), for the classic pure CSS I think we can get the correct rendering by using intelligently border radius and the fact that there's no bottom border.

I don't understand (3) since I should have the full 4 borders.

Comment 16

[Dão Gottwald [:dao]]

(In reply to comment #15)
> (In reply to comment #12)
> > Created an attachment (id=324633) [details] [details]
> > three issues with the latest patch
> > 
> > (2) and (3) only show up with a classic theme, and (2) is also an issue with
> > the pseudo patch from comment 9.
> > 
> 
> (1) is because the tab bar is not a normal tab bar and is hacked a lot, the
> margins would need to be ensured equal to 0 in browser.css

Right, your patch would need to do this, assuming the underlying toolkit code was changed for good reasons, which I'm not convinced of. It's hard to oversee this since you basically reinvent the entire tabbox stylesheet. Like I said, better break it down into several bugs.

Comment 17

[Julien "_FrnchFrgg_" RIVAUD]

(In reply to comment #14)

> Still a regression, isn't it?

I don't think so. I'm pretty sure the tabbrowser strip height changes depending of whether there is one tab or more, even on Firefox 2. It comes from the top 2px margin which is added to unselected tabs (so only when there is at least 2 tabs)

Comment 18

[Dão Gottwald [:dao]]

I'm pretty sure Firefox 2 didn't behave like that. So it's a regression, as far as I can tell, but the question is whether bug 265698 regressed it.

Comment 19

[James Ross]

(In reply to comment #14)
> (In reply to comment #13)
> > So all the tabs need position: relative, or at least the selected one.
> Yep, I think the selected one should be enough.

In my work a number of months back, I found that you needed to set position on all tabs (or none), otherwise Gecko starts messing up the order of the layout frames (and thus the tabs display in the wrong order, and actually change order as you change tab). See the dependency tree for bug 307394 for all the fun.

Comment 20

[Julien "_FrnchFrgg_" RIVAUD]

 
> Right, your patch would need to do this, assuming the underlying toolkit code
> was changed for good reasons, which I'm not convinced of.

To remove the hacks present currently in the theme code and get more correct rendering is a convincing enough reason for me.
I don't mean the implementation at all but what I think is the right thing to do is:

- remove the CSS that makes the top border of a tab panel be drawn piecewise by the tabs and the spacer, and replace it by CSS making the tob border be painted as a whole as a real top border of the tabpanels.
Reason: simpler code, more coherent with the underlying native model of widgets

- make the selected tab overlap this top border, to cut it at the right place; while we're at it, why not use this new z-index ordering to make the selected tab overlap its neighbours instead of simulating that by changing rendering of nearby tabs (either by CSS in the classic mode, or by custom hacks in widget code).
It also simplifies the CSS since there no longer needs to have special CSS for the tabs nearing the selected tab. Eventually this information wouldn't need to be maintained anymore, which simplifies the toolkit js...

> It's hard to oversee
> this since you basically reinvent the entire tabbox stylesheet. Like I said,
> better break it down into several bugs.

I won't have time to adress this bug until the Firefox summit. I'll let you handle this bug as pleases you, no problem.

Comment 21

[Julien "_FrnchFrgg_" RIVAUD]

(In reply to comment #18)
> I'm pretty sure Firefox 2 didn't behave like that. So it's a regression, as far
> as I can tell, but the question is whether bug 265698 regressed it.

You're right, I cannot reproduce in Firefox 2. But it was here before bug 265698, I'm pretty sure of it.

Comment 22

[Sylvestre Ledru [:Sylvestre]]

Moving to p3 because no activity for at least 1 year(s).
See https://github.com/mozilla/bug-handling/blob/master/policy/triage-bugzilla.md#how-do-you-triage for more information

Comment 23

[Julien "_FrnchFrgg_" RIVAUD]

I am pretty sure this bug is completely moot with the GTK3 migration. Even if some "Luna" theme would still exist, it wouldn't be implemented the same way.

And most tabs in Firefox are not system-themed anymore anyway, since preferences are now actual tabs with their own style.

That said, the tabs in Thunderbird's preferences have mostly broken rendering here (it might not yet be GTK3).

Comment 24

[Stephen A Pohl [:spohl]]

Closing, since no longer applicable.