1189212 - [HiDPI] Pinned tab separators are sometimes too thick

Status: RESOLVED FIXED

(Firefox :: Theme, defect, P3)

Description

[Chris Lord [:cwiiis]]

Attachment: Screenshot 2015-07-30 09.36.01.png

The thickness/sharpness of the line that divides pinned tabs is inconsistent (possibly just for the first and last pinned tabs?)

Comment 1

[Tim Nguyen :ntim]

This is very likely caused by your DPI settings, I don't see this on 100% DPI.

Comment 2

[:Gijs (out for now; he/him)]

Chris, what DPI setting is in use on your machine?

Comment 3

[:Gijs (out for now; he/him)]

Another bug says 125%

Comment 4

[Chris Lord [:cwiiis]]

(In reply to :Gijs Kruitbosch from comment #2)
> Chris, what DPI setting is in use on your machine?

Indeed, 125%.

Comment 5

[Dão Gottwald [:dao]]

Attachment: patch?

Here's a patch that simplifies our tab separator code and should fix this bug along the way. This would make us use the solid tab separators on all platforms. Is this something UX would be OK with?

Comment 6

[Stephen Horlander [:shorlander]]

(In reply to Dão Gottwald [:dao] from comment #5)
> Created attachment 8645676 [details] [diff] [review]
> patch?
> 
> Here's a patch that simplifies our tab separator code and should fix this
> bug along the way. This would make us use the solid tab separators on all
> platforms. Is this something UX would be OK with?

I built this on OS X and it looks nice enough. I think it does have some drawbacks though. 

The contiguous lines from the tabstrip border helps with the z-ordering. It visually anchor everything. Without that the elements feel like they are floating. What might be an acceptable style trade-off on Windows 10 might not work everywhere else.

Would it be possible to get Try Builds that would work on Windows XP and 7 and Linux that I could use for a bit please?

Comment 7

[Dão Gottwald [:dao]]

(In reply to Stephen Horlander [:shorlander] from comment #6)
> Would it be possible to get Try Builds that would work on Windows XP and 7
> and Linux that I could use for a bit please?

Windows: http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-de2f60de6d30/try-win32/

Linux: http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-de2f60de6d30/try-linux64/

Comment 8

[Stephen Horlander [:shorlander]]

(In reply to Dão Gottwald [:dao] from comment #7)
> (In reply to Stephen Horlander [:shorlander] from comment #6)
> > Would it be possible to get Try Builds that would work on Windows XP and 7
> > and Linux that I could use for a bit please?
> 
> Windows:
> http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/dgottwald@mozilla.
> com-de2f60de6d30/try-win32/
> 
> Linux:
> http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/dgottwald@mozilla.
> com-de2f60de6d30/try-linux64/

Thank you!

Comment 9

[Justin Dolske [:Dolske]]

Kinda P4 polish, but 125% is our most common scaling factor on Windows (after 100%).

Comment 10

[Stephen Horlander [:shorlander]]

Comment on attachment 8645676 [details] [diff] [review]
patch?

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

After using it for a few days I think it works. Still a little concerned that it makes the tabs feel detached, but less so.

I did notice that the separators are an odd height so they look not quite vertically aligned with the rest of the tab contents: http://cl.ly/image/2J2V2x3l0D1o

Comment 11

[Dão Gottwald [:dao]]

Attachment: patch v2

> I did notice that the separators are an odd height so they look not quite
> vertically aligned with the rest of the tab contents:
> http://cl.ly/image/2J2V2x3l0D1o

I've made them one pixel shorter (removed another pixel from the top).

Comment 12

[:Gijs (out for now; he/him)]

(I'm sorry, I likely won't get to this review until Monday.)

Comment 13

[:Gijs (out for now; he/him)]

Did you test for bug 1121723 ? I'm worried that now using the same layout as devedition, we're going to run into that at some point. :-\

Comment 14

[Dão Gottwald [:dao]]

(In reply to :Gijs Kruitbosch from comment #13)
> Did you test for bug 1121723 ? I'm worried that now using the same layout as
> devedition, we're going to run into that at some point. :-\

This is a very old bug and I see it with and without my patch, so I'm not sure what exactly that code is solving if anything at all. In any case, I'm now using -moz-margin-start: -1px; for all tabs, so there's no need to switch margins for the last tab, right?

Comment 15

[Dão Gottwald [:dao]]

(In reply to Dão Gottwald [:dao] from comment #14)
> (In reply to :Gijs Kruitbosch from comment #13)
> > Did you test for bug 1121723 ? I'm worried that now using the same layout as
> > devedition, we're going to run into that at some point. :-\
> 
> This is a very old bug

(pre-Australis afaik, so it predates our tab separator implementation)

Comment 16

[:Gijs (out for now; he/him)]

(In reply to Dão Gottwald [:dao] from comment #14)
> (In reply to :Gijs Kruitbosch from comment #13)
> > Did you test for bug 1121723 ? I'm worried that now using the same layout as
> > devedition, we're going to run into that at some point. :-\
> 
> This is a very old bug and I see it with and without my patch, so I'm not
> sure what exactly that code is solving if anything at all.

That's very odd, because the devedition regression and fix in that bug were pretty real (ie I and several other people reproduced, tested, and checked that it was fixed).

> In any case, I'm
> now using -moz-margin-start: -1px; for all tabs, so there's no need to
> switch margins for the last tab, right?

I hope so.

Comment 17

[:Gijs (out for now; he/him)]

Comment on attachment 8650966 [details] [diff] [review]
patch v2

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

Do we have a bug + STR for the current version of the overflow jittering on file?

Comment 18

[Dão Gottwald [:dao]]

(In reply to :Gijs Kruitbosch from comment #17)
> Comment on attachment 8650966 [details] [diff] [review]
> patch v2
> 
> Review of attachment 8650966 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Do we have a bug + STR for the current version of the overflow jittering on
> file?

bug 648789

Comment 19

[:Gijs (out for now; he/him)]

(In reply to Dão Gottwald [:dao] from comment #18)
> (In reply to :Gijs Kruitbosch from comment #17)
> > Comment on attachment 8650966 [details] [diff] [review]
> > patch v2
> > 
> > Review of attachment 8650966 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > Do we have a bug + STR for the current version of the overflow jittering on
> > file?
> 
> bug 648789

That looks like a one-off issue during an animation. bug 1121723 was about jittering that switched between the overflown/non-overflown state constantly until you stopped hovering / selecting the tab "causing" the issue.

Comment 20

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/11cfa127c503

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/11cfa127c503

Comment 22

[Dão Gottwald [:dao]]

(In reply to :Gijs Kruitbosch from comment #19)
> (In reply to Dão Gottwald [:dao] from comment #18)
> > (In reply to :Gijs Kruitbosch from comment #17)
> > > Comment on attachment 8650966 [details] [diff] [review]
> > > patch v2
> > > 
> > > Review of attachment 8650966 [details] [diff] [review]:
> > > -----------------------------------------------------------------
> > > 
> > > Do we have a bug + STR for the current version of the overflow jittering on
> > > file?
> > 
> > bug 648789
> 
> That looks like a one-off issue during an animation. bug 1121723 was about
> jittering that switched between the overflown/non-overflown state constantly
> until you stopped hovering / selecting the tab "causing" the issue.

Not sure what an "one-off issue during an animation" is, but anyway it does seem that I misunderstood bug 1121723. I still can't reproduce it though. Please file a bug if you can.

Comment 23

[Dão Gottwald [:dao]]

Comment on attachment 8650966 [details] [diff] [review]
patch v2

Approval Request Comment
[Feature/regressing bug #]: bug 1173730
[User impact if declined]: see comment 0
[Describe test coverage new/current, TreeHerder]:
[Risks and why]: pretty low. Stephen reviewed the style on various platforms and it's a net code simplification that should make it easier to deal with regressions should there be any (such as bug 1198236)
[String/UUID change made/needed]:

Comment 24

[Dão Gottwald [:dao]]

Comment on attachment 8650966 [details] [diff] [review]
patch v2

bug 1198236 is still an issue. will wait until that's resolved.

Comment 25

[Dão Gottwald [:dao]]

Attachment: patch for uplift

This is this bug's and bug 1198236's patches merged together

Approval Request Comment
[Feature/regressing bug #]: bug 1173730
[User impact if declined]: see comment 0
[Describe test coverage new/current, TreeHerder]:
[Risks and why]: mostly code consolidation / simplification, should be low risk
[String/UUID change made/needed]:

Comment 26

[Dão Gottwald [:dao]]

Comment on attachment 8654053 [details] [diff] [review]
patch for uplift

already requesting beta approval too; since we're using the devedition theme on aurora, that channel doesn't give us significant testing exposure for this change

Comment 27

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8654053 [details] [diff] [review]
patch for uplift

Early in the 42 cycle, taking it to polish the release.

Comment 28

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/cb8eea12b505

Comment 29

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8654053 [details] [diff] [review]
patch for uplift

This patch mostly removes code (cleans up) and the fix was verified by Stephen (see comment 10). With that, it seems safe to uplift to Beta. We do want win10 UX to be awesome and this fix should help!

Comment 30

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Correction to my own comment, :( this is not just win10 specific.

Comment 31

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/f7bf6ce9f3f9