Client side decoration in Firefox with Pop theme has a slight imperfection

[Franck]

With Firefox new client-side decoration, when using Pop theme, there is a small white line upon the first tab that looks unclean.
Also, don't use the whole title bar height, which might look cleaner, or not :-)

ProblemType: Bug
DistroRelease: Ubuntu 19.04
Package: firefox 66.0.3+build1-0ubuntu1
ProcVersionSignature: Ubuntu 5.0.0-8.9-generic 5.0.1
Uname: Linux 5.0.0-8-generic x86_64
NonfreeKernelModules: zfs zunicode zavl icp zcommon znvpair
AddonCompatCheckDisabled: False
ApportVersion: 2.20.10-0ubuntu26
Architecture: amd64
AudioDevicesInUse:
 USER PID ACCESS COMMAND
 /dev/snd/controlC1: franck 3795 F.... pulseaudio
 /dev/snd/pcmC1D0c: franck 3795 F...m pulseaudio
 /dev/snd/controlC0: franck 3795 F.... pulseaudio
BuildID: 20190410124846
Channel: Unavailable
CurrentDesktop: GNOME
Date: Thu Apr 11 20:44:57 2019
EcryptfsInUse: Yes
Extensions: extensions.sqlite corrupt or missing
ForcedLayersAccel: False
IfupdownConfig:
 # interfaces(5) file used by ifup(8) and ifdown(8)
 auto lo
 iface lo inet loopback
IncompatibleExtensions: Unavailable (corrupt or non-existant compatibility.ini or extensions.sqlite)
IpRoute:
 default via 10.0.0.1 dev enp0s25 proto dhcp metric 100
 default via 10.0.0.1 dev wlp3s0 proto dhcp metric 600
 10.0.0.0/24 dev enp0s25 proto kernel scope link src 10.0.0.14 metric 100
 10.0.0.0/24 dev wlp3s0 proto kernel scope link src 10.0.0.246 metric 600
 10.204.48.0/24 dev lxdbr0 proto kernel scope link src 10.204.48.1
Locales: extensions.sqlite corrupt or missing
PrefSources: prefs.js
ProcEnviron:
 TERM=xterm-256color
 PATH=(custom, no user)
 XDG_RUNTIME_DIR=<set>
 LANG=fr_FR.UTF-8
 SHELL=/bin/bash
Profiles: Profile0 (Default) - LastVersion=66.0.3/20190410124846 (In use)
RunningIncompatibleAddons: False
SourcePackage: firefox
Themes: extensions.sqlite corrupt or missing
UpgradeStatus: No upgrade log present (probably fresh install)
dmi.bios.date: 09/27/2017
dmi.bios.vendor: LENOVO
dmi.bios.version: G7ETA9WW (2.69 )
dmi.board.asset.tag: Not Available
dmi.board.name: 2353CTO
dmi.board.vendor: LENOVO
dmi.board.version: Not Defined
dmi.chassis.asset.tag: No Asset Information
dmi.chassis.type: 10
dmi.chassis.vendor: LENOVO
dmi.chassis.version: Not Available
dmi.modalias: dmi:bvnLENOVO:bvrG7ETA9WW(2.69):bd09/27/2017:svnLENOVO:pn2353CTO:pvrThinkPadT430s:rvnLENOVO:rn2353CTO:rvrNotDefined:cvnLENOVO:ct10:cvrNotAvailable:
dmi.product.family: ThinkPad T430s
dmi.product.name: 2353CTO
dmi.product.sku: LENOVO_MT_2353
dmi.product.version: ThinkPad T430s
dmi.sys.vendor: LENOVO

[Jhofmann-d (jhofmann-d)]

Created attachment 8912611
Screen Shot 2017-09-27 at 10.46.56.png

See screenshot, the first tab separator should have the same height as the other separators. We forgot to do that in bug 1390025.

[Jhofmann-d (jhofmann-d)]

Let's do this after bug 1391539, otherwise we'll have to file another bug to adjust the height of this border when hovering (I'm happy to pick it up once bug 1391539 is done).

[Jhofmann-d (jhofmann-d)]

*** Bug 1406681 has been marked as a duplicate of this bug. ***

[Mehmet-sahin (mehmet-sahin)]

Created attachment 8916677
Bildschirmfoto 2017-10-09 um 18.39.58.png

Hi Johann,

I noticed that the overflow separator has also the bug.

Please see the screenshot. Thanks.

[Nhnt11 (nhnt11)]

Created attachment 8917126
Bug 1403483 - Set a border on the first tab instead of at the end of the pre-tabs drag space.

Review commit: https://reviewboard.mozilla.org/r/188134/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/188134/

[Nhnt11 (nhnt11)]

Comment on attachment 8917126
Bug 1403483 - Set a border on the first tab instead of at the end of the pre-tabs drag space.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/188134/diff/1-2/

[Nhnt11 (nhnt11)]

This works well for me at the moment on current central, but I need to confirm with Johann whether some changes around this code he was talking about have landed already or not. Also, I'm not entirely sure this works for RTL, I'm going to investigate this tomorrow.

[Nhnt11 (nhnt11)]

Hmm, and I think I need to make the Windows-specific selector account for the menubar as well as maximized mode. Canceling review for now.

[Dao+bmo (dao+bmo)]

*** Bug 1408953 has been marked as a duplicate of this bug. ***

[Nhnt11 (nhnt11)]

Created attachment 8923047
Bug 1403483 - Show tab separator on first tab instead of on the pre-tabs whitespace.

Review commit: https://reviewboard.mozilla.org/r/194262/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/194262/

[Nhnt11 (nhnt11)]

Comment on attachment 8923047
Bug 1403483 - Show tab separator on first tab instead of on the pre-tabs whitespace.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/194262/diff/1-2/

[Nhnt11 (nhnt11)]

Dão, the only problem (as far as I can tell) with this patch is that there is no tab separator before the first non-pinned tab when the tabstrip is overflowing. A CSS-only solution might be to use a selector like .tabbrowser-tab[pinned] + .tabbrowser-tab:not([pinned]), but I worry about hidden tabs in between. Do you think this would be an OK solution or should we try and add an attribute to the first visible non-pinned tab in JavaScript for styling convenience?

[Nhnt11 (nhnt11)]

Comment on attachment 8923047
Bug 1403483 - Show tab separator on first tab instead of on the pre-tabs whitespace.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/194262/diff/2-3/

[Nhnt11 (nhnt11)]

TODO: Add a rule to hide the first tab separator when sizemode != normal on Windows.

[Nhnt11 (nhnt11)]

Comment on attachment 8923047
Bug 1403483 - Show tab separator on first tab instead of on the pre-tabs whitespace.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/194262/diff/3-4/

[Nhnt11 (nhnt11)]

(In reply to Nihanth Subramanya [:nhnt11] from comment #13)
> TODO: Add a rule to hide the first tab separator when sizemode != normal on
> Windows.

Latest patch takes care of this. Need to investigate if Linux needs a similar rule.

[Dao+bmo (dao+bmo)]

Comment on attachment 8923047
Bug 1403483 - Show tab separator on first tab instead of on the pre-tabs whitespace.

https://reviewboard.mozilla.org/r/194262/#review200420

It's not clear to me why this patch moves the border away fron the pre-tabs whitespace. Why can we not just fix that border's height?

[Nhnt11 (nhnt11)]

(In reply to Dão Gottwald [::dao] from comment #16)
> Comment on attachment 8923047
> Bug 1403483 - Show tab separator on first tab instead of on the pre-tabs
> whitespace.
>
> https://reviewboard.mozilla.org/r/194262/#review200420
>
> It's not clear to me why this patch moves the border away fron the pre-tabs
> whitespace. Why can we not just fix that border's height?

The border styling needs to change when the first tab is hovered/selected: is there an easy way to style the border of the pre-tabs whitespace depending on the :hover/visuallyselected state of the first tab? I don't see one.

[Jhofmann-d (jhofmann-d)]

In support of Nihanths current solution, it's probably what we need to do to resolve bug 1403520 as well.

[Jhofmann-d (jhofmann-d)]

I'm setting this to block bug 1403520 for now, as we probably shouldn't act there without solving this first.

[Jhofmann-d (jhofmann-d)]

Hey folks, is there anything we can do to resolve this anytime soon? I'm happy to take the review if you're busy right now, Dão.

Thanks!

[Nhnt11 (nhnt11)]

Comment on attachment 8923047
Bug 1403483 - Show tab separator on first tab instead of on the pre-tabs whitespace.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/194262/diff/4-5/

[Nhnt11 (nhnt11)]

(In reply to Nihanth Subramanya [:nhnt11] from comment #21)
> Comment on attachment 8923047
> Bug 1403483 - Show tab separator on first tab instead of on the pre-tabs
> whitespace.
>
> Review request updated; see interdiff:
> https://reviewboard.mozilla.org/r/194262/diff/4-5/

Rebased and added the sizemode != normal rule for Linux (re. comment 15 quoted below)

(In reply to Nihanth Subramanya [:nhnt11] from comment #15)
> (In reply to Nihanth Subramanya [:nhnt11] from comment #13)
> > TODO: Add a rule to hide the first tab separator when sizemode != normal on
> > Windows.
>
> Latest patch takes care of this. Need to investigate if Linux needs a
> similar rule.

(In reply to Johann Hofmann [:johannh] from comment #20)
> Hey folks, is there anything we can do to resolve this anytime soon? I'm
> happy to take the review if you're busy right now, Dão.
>
> Thanks!

Dão, re-flagged you for r?, let me know if I should redirect this to Johann.

[Nhnt11 (nhnt11)]

Created attachment 8975470
Bug 1403483 - Show tab separator on first tab instead of on the pre-tabs whitespace.

Review commit: https://reviewboard.mozilla.org/r/243752/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/243752/

[Jhofmann-d (jhofmann-d)]

Comment on attachment 8975470
Bug 1403483 - Show tab separator on first tab instead of on the pre-tabs whitespace.

https://reviewboard.mozilla.org/r/243752/#review253754

This is pretty fiddly to get right but I feel like you did a great job, although I'm not as immersed into this code as I was half a year ago :)

Generally this looks pretty good to me. I have one small complaint: I don't think the separator looks good on Windows 7 (non-maximized) like this. I can attach a screenshot. Considering that we're both not really working on tabs full-time anymore it's probably a good idea not to defer that to a follow-up bug.

We could consider trying to entirely hide the separator on Windows 7 (would need to check how that looks with light/dark theme, too). Let me know if you want to borrow my VM for that...

[Jhofmann-d (jhofmann-d)]

Created attachment 8981781
Windows 7

Sorry for the small image, might have to zoom in.

[Laszlo-bialis (laszlo-bialis)]

*** Bug 1464373 has been marked as a duplicate of this bug. ***

[Smlombardi (smlombardi)]

I see the same issue on the Mac, 10.14.3 with FF developer edition 67.0b3


[Dao+bmo (dao+bmo)]

*** Bug 1538767 has been marked as a duplicate of this bug. ***

[Olivier Tilloy (osomon)]

Thanks for the report Franck.
This sounds like it might be an upstream bug. Would you mind filing it at https://bugzilla.mozilla.org/enter_bug.cgi#h=dupes%7CFirefox, and sharing the link to it here? Thanks!

[Franck (alci)]

Created attachment 9079109
slight_imperfection.png

User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:68.0) Gecko/20100101 Firefox/68.0

Steps to reproduce:

With CSD, title bar has a slight imperfection when the title is higher than the tabs (due to the gnome theming). For example, you can apply Pop! theme.

Actual results:

on the upper left of the first tab, you can see a small vertical blank line (see attached screenshot)

Expected results:

No vertical blank line should appear.

[Franck (alci)]

Bug filled upstream: https://bugzilla.mozilla.org/show_bug.cgi?id=1567246

[Stransky (stransky)]

Dao should know more, I hope it's the right component.

[Dao+bmo (dao+bmo)]

*** This bug has been marked as a duplicate of bug 1147847 ***

[Dao+bmo (dao+bmo)]

*** Bug 1567705 has been marked as a duplicate of this bug. ***

[Stransky (stransky)]

I don't think this is about scrollbars but about titlebar / tabs.

[Stransky (stransky)]

I think this is related to left button box position - can you try to move the buttons to right and check if you still see this? Thanks.

[Dao+bmo (dao+bmo)]

(In reply to Martin Stránský [:stransky] from comment #3)
> I don't think this is about scrollbars but about titlebar / tabs.

Copy/paste failure.

*** This bug has been marked as a duplicate of bug 1403483 ***

[Dao+bmo (dao+bmo)]

*** Bug 1567246 has been marked as a duplicate of this bug. ***

[Franck (alci)]

Moving the buttons to the right certainly hides (solves ?) the problem, as then the tab in drawn to the very left of the title bar...

[Lhenry (lhenry)]

Is this still something we want to land? We have a couple of duplicates so I think it's an issue that bothers users and it's also marked P1.

[Dao+bmo (dao+bmo)]

(In reply to Liz Henry (:lizzard) (use needinfo) from comment #31)
> Is this still something we want to land?

It's not ready to land, might not be even close.

[Virtual-q (virtual-q)]

*** Bug 1640177 has been marked as a duplicate of this bug. ***

[Nhnt11 (nhnt11)]

I've lost context.

[Release-mgmt-account-bot (release-mgmt-account-bot)]

The severity field for this bug is relatively low, S3. However, the bug has 7 duplicates.
:dao, could you consider increasing the bug severity?

For more information, please visit [auto_nag documentation](https://wiki.mozilla.org/Release_Management/autonag#severity_underestimated.py).

[Autonag-nomail-bot (autonag-nomail-bot)]

The last needinfo from me was triggered in error by recent activity on the bug. I'm clearing the needinfo since this is a very old bug and I don't know if it's still relevant.