579683 - App tabs draw into the toolbar below or the content area

Status: RESOLVED FIXED

(Firefox :: Theme, defect)

Description

[Will Roberts [:bws42]]

User-Agent:       Mozilla/5.0 (X11; Linux x86_64; en-US; rv:2.0b2pre) Gecko/20100717 Minefield/4.0b2pre
Build Identifier: Mozilla/5.0 (X11; Linux x86_64; en-US; rv:2.0b2pre) Gecko/20100717 Minefield/4.0b2pre

With tabs on top, app tabs draw into the toolbar below them. I thought it might have been the same as bug 546178 but normal tabs no longer overflow and app tabs still do.

Also noticed that the tab bar shrinks by a pixel if you only have app tabs open.

Reproducible: Always

Comment 1

[Will Roberts [:bws42]]

Attachment: screenshot

Comment 2

[:Gavin Sharp [email: [e-mail]]]

Why the dependency on bug 363249?

Comment 3

[Dão Gottwald [:dao]]

-moz-calc() allows us to specify the correct tab height.

Comment 4

[:Gavin Sharp [email: [e-mail]]]

Why does that require moz-calc?

Comment 5

[Dão Gottwald [:dao]]

Because the text size might be larger than 16px, which I'd like to take into account rather than hardcoding a height. E.g. I'd like to use something like height: -moz-calc(max(16px, 1em) + 7px), where 7px would be the sum of any vertical border and padding. Of course we could fix this right now /for most users/ by using height: 23px; (assuming 7px was the correct sum).

Comment 7

[Biju]

Why is this not a problem on normal tab? normal tab only have Text in it...

Comment 8

[Dão Gottwald [:dao]]

App tabs aren't constrained by the tab container, but I'm not sure what makes them consume more height either.

Comment 10

[John Drinkwater (:beta)]

This also occurs when notification bars pop-up, the app tabs that otherwise look ok overlap into the icon & message.

Comment 11

[John Drinkwater (:beta)]

Attachment: AppTabs overlap ‘too many pop-ups’ alert.

Comment 13

[Please Ignore This Troll (Account Disabled)]

tab[pinned="true"] {height: 23px!important;}
this fixes 1 issue with lines (from borders of the pinned tabs) being shown lower than needed.
And I think it's impossible to fix the issue with tab bar losing it's height if all the opened tabs are pinned - simply because if you pick element
#tabbrowser-tabs.tabbrowser-tabs
in DOMi - it highlights the part of the tab bar _WITHOUT_ pinned tabs. That's weird, since the pinned tabs are shown as (not direct, but still) children to that element.

Comment 14

[Please Ignore This Troll (Account Disabled)]

tab[pinned="true"] {height: 23px!important;} /* for Win XP */
tab[pinned="true"] {height: 25px!important;} /* for Win 7 */

Comment 15

[Please Ignore This Troll (Account Disabled)]

#TabsToolbar {height: 26px!important;} /* Win 7 */
this fixxes the issue for tabbar losing it's height, if all the opened tabs are made into app tabs.
It's time for someone to make a diff patch.

Comment 16

[Please Ignore This Troll (Account Disabled)]

just noticed: in this case the favicon of the apptab is 2px higher than regular tab's favicon.
so here is a fix:
tab[pinned="true"]>.tab-icon-image {position: relative !important; top: 2px!important;}

Comment 18

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

(In reply to comment #16)
> just noticed: in this case the favicon of the apptab is 2px higher than regular
> tab's favicon.
> so here is a fix:
> tab[pinned="true"]>.tab-icon-image {position: relative !important; top:
> 2px!important;}

Looks like Bug 587576's patch should take care of this:
 .tabbrowser-tab[pinned] > .tab-icon-image {
-  margin: 2px 0 0;
+  margin: 2px 2px 0;
 }

(In reply to comment #14)
> tab[pinned="true"] {height: 23px!important;} /* for Win XP */
> tab[pinned="true"] {height: 25px!important;} /* for Win 7 */

Dao said that we shouldn't use specific heights for this (bug 579373 comment 16), as different font sizes could be used. Which I guess means we can't have a specific height for the #TabsToolbar, either...

Comment 19

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Is this waiting for -moz-calc, or can we just take an explicit height for now, and deal with different font sizes later?

Comment 20

[Dão Gottwald [:dao]]

This is waiting for calc, which I think landed already but apparently isn't 100% done.

Comment 21

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Attachment: use -moz-calc to determine app tab height

Uses height: -moz-calc(max(16px, 1em) + 7px); as described in an earlier comment in this bug. Seems to fix the problem of drawing into the toolbar/notificationbar. Not sure if it works with the different font sizes.

Comment 22

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Attachment: screenshot

Screenshot of what it looks like for both tabs on top and tabs on bottom.

Comment 23

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

(In reply to comment #22)
> Created attachment 469777 [details]
> screenshot
> 
> Screenshot of what it looks like for both tabs on top and tabs on bottom.

Er, that screenshot apparently included some other changes that I thought I had reverted.

The patch does work, though. It also seems to align the favicon properly with regular tabs (part of 587576?), unless something else landed that fixed.

Comment 24

[Dão Gottwald [:dao]]

Comment on attachment 469773 [details] [diff] [review]
use -moz-calc to determine app tab height

basically okay, but belongs in themes, not in content

Comment 25

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Just to be sure, this is needed in all three default themes, yes? (Gnomestripe/Winstripe/Pinstripe)

Comment 26

[Please Ignore This Troll (Account Disabled)]

(In reply to comment #25)
> Just to be sure, this is needed in all three default themes, yes?
> (Gnomestripe/Winstripe/Pinstripe)

Look at the bug's info:
Platform:  	All All

for windows - there are 2 default themes: for classic (wind xp) style, and for aero (vista, 7) - IIRC they are in 2 different folders of the same archive.

Comment 27

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Attachment: patches themes instead

Comment 28

[Please Ignore This Troll (Account Disabled)]

(In reply to comment #27)
> Created attachment 469909 [details] [diff] [review]
> patches themes instead

Result of applying the rule from the suggested patch on Win7 Aero
http://img819.imageshack.us/img819/2128/94195735.png

Comment 29

[Please Ignore This Troll (Account Disabled)]

(In reply to comment #28)
> (In reply to comment #27)
> > Created attachment 469909 [details] [diff] [review] [details]
> > patches themes instead
> 
> Result of applying the rule from the suggested patch on Win7 Aero
> http://img819.imageshack.us/img819/2128/94195735.png

to fit apptabs' height it's better to use
+ 11px to -moz-calc, not + 7px.
But even after changing 7 to 11 the apptabs' favicons will be a few pixels higher than normal tabs' favicons.
So I guess this line is still needed:
tab[pinned="true"]>.tab-icon-image {position: relative !important; top: 2px!important;}

Comment 30

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

(In reply to comment #29)
> to fit apptabs' height it's better to use
> + 11px to -moz-calc, not + 7px.
I'll try to get an updated patch up later today. Anyone know what Mac/WinClassic looks like with this patch?
> But even after changing 7 to 11 the apptabs' favicons will be a few pixels
> higher than normal tabs' favicons.
> So I guess this line is still needed:
> tab[pinned="true"]>.tab-icon-image {position: relative !important; top:
> 2px!important;}
That would be bug 587576.

Comment 31

[Please Ignore This Troll (Account Disabled)]

(In reply to comment #30)
> That would be bug 587576.

Why? These things are completely different. I'm talking about your patch, that fixes nothing. I suggested the changes for the patch to fix that bug completely (at least for Win7 aero).
And the bug you mentioned - is about apptabs' width, not height and not the padding between it's top border and favicon.
Why don't you want just to add the line of code I suggested?

Comment 32

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Attachment: updated

Changes winstripe's to + 11px as per  drug0y's comment.

Removed this from pinstripe, as it already doesn't bleed into neighboring toolbars. Can put it back if needed.

Still don't know if the classic theme needs anything different than what's in the patch now.

Comment 33

[Please Ignore This Troll (Account Disabled)]

(In reply to comment #32)
> Still don't know if the classic theme needs anything different than what's in
> the patch now.

Tested on classic theme on xp - it requires + 8 px, but (!) in case there is 0 normal tabs on tab bar, (e.g. all the tabs are made into apptabs) than the bug re-appears:
http://img137.imageshack.us/img137/2808/30507888.png

So just like I said in comment #15 - we also need to change height of #TabsToolbar (26px for Win7 aero and 24px for classic theme on XP).

You all don't listen to me and ignore my posts. I've already offered the style to fix this issue on xp classic and 7 aero, all you needed to do is to adjust the values for other platforms.

Comment 34

[Please Ignore This Troll (Account Disabled)]

(In reply to comment #33)
> Tested on classic theme on xp - it requires + 8 px

+ 8px instead of + 11px, I didn't mean to sum.

Comment 36

[Dão Gottwald [:dao]]

Comment on attachment 470124 [details] [diff] [review]
updated

This is going to be slightly more complicated... I'll just take this bug.

Comment 37

[Dão Gottwald [:dao]]

While it's possible to kind of fix this with calc(), it ends up being pretty complex in order to get reasonable (but still not perfect) results for different scenarios.

I've discovered that we basically just need to specify a min-height to fix this bug.

Comment 38

[Dão Gottwald [:dao]]

Attachment: patch

The min-height is the maximum height of the non-textual tab contents, i.e. the tab close button on Linux (20px) and the favicon on Windows (16px). I had to move the focus ring on Windows from the label to the icon, otherwise a large font would make non-pinned tabs 2px taller than pinned tabs. We had to do something about the focus ring anyway, since putting it around the label obviously doesn't work for tabs a hidden label.

Comment 39

[Dietrich Ayala (:dietrich)]

Blocks a b5 blocker, so blocking+. (going to unset blocking on that meta bug)

Comment 40

[Dão Gottwald [:dao]]

Since bug 130078 landed, this affects tabs on bottom as well.

Comment 41

[Please Ignore This Troll (Account Disabled)]

The whole idea with using min-height is good, but why not change 
.tabbrowser-tab[pinned]>.tab-icon-image {vertical-align: middle;}
to
.tabbrowser-tab>.tab-icon-image {vertical-align: middle;}
? 
Mid alignment is needed for both cases, so we can shorten the rule without hurting the functionality.
and what about 
+.tabbrowser-tab:focus > .tab-icon-image {
+  outline: 1px dotted;
shouldn't the outline be also transparent?

Comment 42

[Please Ignore This Troll (Account Disabled)]

Oh, and as for Win7 with Aero - 19px min-height is better, if it's 16 px, than pinned tabs have a free space under them, and if all the tabs on the tabbar are pinned - then the tabbar is also losing it's height, which causes choppu UI repainting.
Your measurements are somehow wrong. Favicon's height is exactly 16px, but min height for pinned tab should be more than just favicon's height.

Comment 44

[Mike Beltzner [:beltzner, not reading bugmail]]

Don't think this is a b5+ blocker; we can wait until b6+. That said, if gavin reviews this patch and checks it in before we tag, more's the better.

Comment 45

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #38)
> The min-height is the maximum height of the non-textual tab contents, i.e. the
> tab close button on Linux (20px) and the favicon on Windows (16px).

Add comments accordingly? I wish our CSS rules were better documented in general.

> I had to
> move the focus ring on Windows from the label to the icon, otherwise a large
> font would make non-pinned tabs 2px taller than pinned tabs. We had to do
> something about the focus ring anyway, since putting it around the label
> obviously doesn't work for tabs a hidden label.

Can't you make this only apply to pinned tabs?

Comment 46

[Dão Gottwald [:dao]]

(In reply to comment #45)
> (In reply to comment #38)
> > The min-height is the maximum height of the non-textual tab contents, i.e. the
> > tab close button on Linux (20px) and the favicon on Windows (16px).
> 
> Add comments accordingly? I wish our CSS rules were better documented in
> general.

sure

> > I had to
> > move the focus ring on Windows from the label to the icon, otherwise a large
> > font would make non-pinned tabs 2px taller than pinned tabs. We had to do
> > something about the focus ring anyway, since putting it around the label
> > obviously doesn't work for tabs a hidden label.
> 
> Can't you make this only apply to pinned tabs?

No, the border around the label makes normal tabs taller but not pinned ones as they don't have a label.

Comment 47

[Dão Gottwald [:dao]]

(In reply to comment #46)
> > > I had to
> > > move the focus ring on Windows from the label to the icon, otherwise a large
> > > font would make non-pinned tabs 2px taller than pinned tabs. We had to do
> > > something about the focus ring anyway, since putting it around the label
> > > obviously doesn't work for tabs a hidden label.
> > 
> > Can't you make this only apply to pinned tabs?
> 
> No, the border around the label makes normal tabs taller but not pinned ones as
> they don't have a label.

I could compensate this with negative top and bottom margins, though...

Comment 48

[Dão Gottwald [:dao]]

Attachment: patch v2

comments addressed

Comment 49

[Dão Gottwald [:dao]]

http://hg.mozilla.org/mozilla-central/rev/bd0760227fc8

Comment 50

[Please Ignore This Troll (Account Disabled)]

(In reply to comment #48)
> Created attachment 470488 [details] [diff] [review]
> patch v2
> 
> comments addressed

since the bug became "resolved fixed" may I ask you about the code?
1. Why did you leave it so:
.tabbrowser-tab > .tab-text {
   border: 1px dotted transparent;
+  margin: -1px !important; /* let the border not occupy any space, like outline */}

why not just use outline instead of border? 83 bytes economy.

2. .tabbrowser-tab[pinned]:focus > .tab-icon-image {
  outline: 1px dotted;}

What does it do? It's supposed to draw a dotted border over the focused apptab, right? I tried it and it does nothing. Why? I suggest to remove this rule.

3. .tabbrowser-tab[pinned] > .tab-icon-image {
 vertical-align: middle;}

why did you include [pinned] in this rule? why not remove it and make the rule affect ALL tabs, not just apptabs?

I also request to reopen this bug:
a. Look at the active normal tab and active pinned tab with your bugfix and compare them: pinned tab is still drawing into the toolbar below, but just with 1 px.
b. Compare position of the favicon for normal and pinned tabs - they are different, normal tab's favicon is lower than pinned one's.

+ your code also decreases height of the whole tabbar, but I think it's ok.

Comment 51

[Please Ignore This Troll (Account Disabled)]

Whom should I write to request change the status for this bug from RESOLVED FIXED to REOPENED?

Comment 52

[Dão Gottwald [:dao]]

It won't be reopened unless the patch gets backed out. File a new bug if you're seeing an issue that should be addressed.

Comment 53

[Please Ignore This Troll (Account Disabled)]

(In reply to comment #52)
> It won't be reopened unless the patch gets backed out. File a new bug if you're
> seeing an issue that should be addressed.

then back out the patch, as it is not fixing the bug!

Comment 54

[Mike Beltzner [:beltzner, not reading bugmail]]

How is it not fixing the bug? Please provide screenshots illustrating what is not fixed, or as suggested, please file a follow-up bug illustrating what is left to be done.

Comment 55

[Please Ignore This Troll (Account Disabled)]

(In reply to comment #54)

Proofpic #1: http://img137.imageshack.us/img137/9822/apptabis1pxlower.png
Left part of the image represents the state when the apptab is active, while the suggested patch was applied.
Right part of the image represents the state when normal tab is active, while the patch is also applied.
Compare them and note, that apptab is 1px lower than needed.

Proofpic #2: http://img831.imageshack.us/img831/5872/faviconis1pxlower.png
AppTab's favicon is 1px lower than normal tab's favicon.

+ #3 (comment #50): Though Dao may just ignore my questions, I still think that some parts in the code of the suggested patch are useless.

Comment 56

[Dão Gottwald [:dao]]

(In reply to comment #55)
> Proofpic #1: http://img137.imageshack.us/img137/9822/apptabis1pxlower.png
> Left part of the image represents the state when the apptab is active, while
> the suggested patch was applied.
> Right part of the image represents the state when normal tab is active, while
> the patch is also applied.
> Compare them and note, that apptab is 1px lower than needed.

WFM. You should double-check that you haven't messed this up with your userChrome.css and then file a bug.

> Proofpic #2: http://img831.imageshack.us/img831/5872/faviconis1pxlower.png
> AppTab's favicon is 1px lower than normal tab's favicon.

You should file a bug.

> + #3 (comment #50): Though Dao may just ignore my questions, I still think that
> some parts in the code of the suggested patch are useless.

They aren't.

Comment 57

[Dão Gottwald [:dao]]

(In reply to comment #56)
> (In reply to comment #55)
> > Proofpic #1: http://img137.imageshack.us/img137/9822/apptabis1pxlower.png
> > Left part of the image represents the state when the apptab is active, while
> > the suggested patch was applied.
> > Right part of the image represents the state when normal tab is active, while
> > the patch is also applied.
> > Compare them and note, that apptab is 1px lower than needed.
> 
> WFM. You should double-check that you haven't messed this up with your
> userChrome.css and then file a bug.

I'm seeing this in the nightly that just came out, appears to be a regression from bug 591835.

Comment 58

[Please Ignore This Troll (Account Disabled)]

(In reply to comment #57)
> I'm seeing this in the nightly that just came out, appears to be a regression
> from bug 591835.

Q.E.D.

Comment 59

[Dão Gottwald [:dao]]

(In reply to comment #58)
> (In reply to comment #57)
> > I'm seeing this in the nightly that just came out, appears to be a regression
> > from bug 591835.
> 
> Q.E.D.

You do realize that bug 591835, having landed last night, couldn't be the reason for what you were seeing, unless you played with userChrome.css?

Comment 60

[Please Ignore This Troll (Account Disabled)]

(In reply to comment #59)
> You do realize that bug 591835, having landed last night, couldn't be the
> reason for what you were seeing, unless you played with userChrome.css?

I don't use customizations of userChrome.css, I use stylish instead and I turned off all the styles and restarted the browser before I tested your patch.
I don't use any extensions that might modify the appearance of the browser, I don't even have any personas/themes.
So either my profile is corrupted, or it's a Poltergeist.

Comment 61

[Mike Beltzner [:beltzner, not reading bugmail]]

Guys, please take your fight to private email or IRC. This bug remains fixed, someone please file a bug on the regression from bug 591835 or re-open that bug to track the real fix.

Also: I *strongly* suggest that you resist getting into competitions about proving each other right or wrong. drug0y, dao's a longtime contributor who knows what he's talking about, what he's doing, and in my experience is very good about admitting when he's made a mistake and double checking his work when asked to. Please try to respect that.

Comment 62

[Please Ignore This Troll (Account Disabled)]

(In reply to comment #61)
Ok.
My apologies to you, Dao: I might be wrong, but I really experienced what I've described though I've triple checked everything and I still don't know what is wrong, but that's my problem, not yours.

Comment 63

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

I'm seeing this bug again on the latest nightly.

Comment 64

[Please Ignore This Troll (Account Disabled)]

well, I have re-opened bug 585164 for that, but who cares?
mike beltzner doesn't trust screenshots from users.