789007 - Unwanted border between pinned tabs and navigation toolbar when tabstrip overflows

Status: RESOLVED FIXED

(Firefox :: Theme, defect)

Description

[Tim Taubert [:ttaubert] (inactive)]

About two weeks ago something landed that causes my Ubuntu build to have a border that visually separates pinned tabs from the navigation toolbar:

http://imgur.com/aGg7W

This happens only when toolbar overflows. The regression range is:

http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=8c85c83068e7&tochange=c676b554c7bb

Trying to narrow it down further.

Comment 1

[Tim Taubert [:ttaubert] (inactive)]

(In reply to Tim Taubert [:ttaubert] from comment #0)
> This happens only when toolbar overflows. The regression range is:

Sorry, I mean it happens only when the tabstrip overflows.

This does not happen on Mac or Windows.

Comment 2

[Tim Taubert [:ttaubert] (inactive)]

Caused by bug 776265.

Comment 3

[Tim Taubert [:ttaubert] (inactive)]

Adding some people from bug 776265 that hopefully can help investigating this regression.

Comment 4

[Lukas Blakk [:lsblakk] use ?needinfo]

Bug 776265 was uplifted to 16 so let's look at getting a fix for this or backing bug 776265 out of Beta.

Comment 5

[Mats Palmgren (inactive)]

I think bug 776265 / bug 785754 fixed some bug that the tab strip styles
depended on.  I can't reproduce the bug myself (on KDE, OSX), so it's hard
to say.  I would suggest playing around with the styles
(browser/themes/gnomestripe/browser.css is it?), especially anything with
min-/max-height.  This one looks like a candidate:

#tabbrowser-tabs[positionpinnedtabs] > .tabbrowser-tab[pinned] {
  min-height: 18px; /* corresponds to the max. height of non-textual tab contents, i.e. the tab close button */
}

Comment 6

[Dão Gottwald [:dao]]

(In reply to Mats Palmgren [:mats] from comment #5)
> I think bug 776265 / bug 785754 fixed some bug that the tab strip styles
> depended on.

Please elaborate. What bug has been fixed that could affect pinned tabs?

Comment 7

[Mats Palmgren (inactive)]

Some classes didn't implement min-/max-height correctly when used in combination
with -moz-box-sizing:border-box.   The error was that the min-/max-height affected
the content-box height, not the border-box height.  I'm not sure exactly which XUL
elements were affected.  For some XUL elements there were also the opposite error,
they got the -moz-box-sizing:content-box case wrong, <xul:image> had this bug for
example.

That said, in this case it seems there is a regression if I'm not mistaken.
Testcase coming up...

Comment 8

[Mats Palmgren (inactive)]

Attachment: Testcase #1

I would expect the 1st and 3rd boxes from the top to display the same.
(they do in Fx15)

Comment 9

[Mats Palmgren (inactive)]

Attachment: Testcase #2 (same test as HTML with display:-moz-box)

Comment 10

[Mats Palmgren (inactive)]

Attachment: Testcase #3

Comment 11

[Mats Palmgren (inactive)]

Attachment: fix

Tim, does this patch fix the problem?

Comment 12

[Tim Taubert [:ttaubert] (inactive)]

(In reply to Mats Palmgren [:mats] from comment #11)
> Tim, does this patch fix the problem?

No, the border is still there, unfortunately.

Comment 13

[Mats Palmgren (inactive)]

OK, I spawned a separate bug for my patch, bug 789824.

Comment 14

[Dão Gottwald [:dao]]

Attachment: patch

Seems like layout is behaving correctly here, given -moz-box-sizing:border-box.

Comment 15

[Dão Gottwald [:dao]]

Note that winstripe has a similar min-height, set to 16px. Theoretically it would need the same fix, assuming it needs the min-height in the first place, which I think was the case once, but seemingly isn't anymore... Which is good, because this minimum height should be implied by .tab-throbber and .tab-icon-image having a preset height of 16px.

Comment 16

[Dão Gottwald [:dao]]

(In reply to Dão Gottwald [:dao] from comment #15)
> Note that winstripe has a similar min-height, set to 16px. Theoretically it
> would need the same fix, assuming it needs the min-height in the first
> place, which I think was the case once, but seemingly isn't anymore... Which
> is good, because this minimum height should be implied by .tab-throbber and
> .tab-icon-image having a preset height of 16px.

filed bug 789833

Comment 17

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 659596 [details] [diff] [review]
patch

Review of attachment 659596 [details] [diff] [review]:
-----------------------------------------------------------------

That indeed seems like the right fix to me and it looks good. Thank you for investigating it!

Comment 18

[Dão Gottwald [:dao]]

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

Comment 19

[Ryan VanderMeulen [:RyanVM]]

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

Comment 20

[Dão Gottwald [:dao]]

Comment on attachment 659596 [details] [diff] [review]
patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 776265
User impact if declined: visual glitch in primary UI
Testing completed (on m-c, etc.): manual on m-c
Risk to taking this patch (and alternatives if risky): low risk
String or UUID changes made by this patch: none

Comment 21

[Alex Keybl [:akeybl]]

Comment on attachment 659596 [details] [diff] [review]
patch

[Triage Comment]
Low risk fix for a FF16 regression. Approving for branches.

Comment 22

[Dão Gottwald [:dao]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/8a8505859cd1
https://hg.mozilla.org/releases/mozilla-beta/rev/e88637dd3514

Comment 23

[Scoobidiver (away)]

Backout from Beta: http://hg.mozilla.org/releases/mozilla-beta/rev/93b1aaf8376b

Comment 24

[Boris Zbarsky [:bzbarsky]]

You meant "unaffected": the bug that caused this one was backed out too.

Comment 25

[Boris Zbarsky [:bzbarsky]]

But maybe it should have stayed "fixed".  Who knows.  In any case, "affected" is wrong.

Comment 26

[Virgil Dicu [:virgil] [QA]]

Mozilla/5.0 (X11; Linux i686; rv:17.0) Gecko/17.0 Firefox/17.0

All looks good on the Ubuntu front (17 beta 1) with pinned tabs and normal tabs, no borders above the navigation toolbar.