[HiDPI] Pinned tab separators are sometimes too thick

RESOLVED FIXED in Firefox 41

Status

()

Firefox
Theme
P3
normal
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: cwiiis, Assigned: dao)

Tracking

(Blocks: 1 bug)

Trunk
Firefox 43
All
Windows 10
Points:
---
Dependency tree / graph
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox40 wontfix, firefox41 fixed, firefox42 fixed, firefox43 fixed)

Details

Attachments

(3 attachments, 1 obsolete attachment)

(Reporter)

Description

3 years ago
Created attachment 8640969 [details]
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?)
(Reporter)

Updated

3 years ago
Blocks: 1077146

Comment 1

3 years ago
This is very likely caused by your DPI settings, I don't see this on 100% DPI.
Blocks: 1158143
No longer blocks: 1077146
Chris, what DPI setting is in use on your machine?
Flags: needinfo?(chrislord.net)
Another bug says 125%
Summary: Dividers in pinned tabs are inconsistent with dividers in the tab bar, and with themselves → Pinned tab separators are sometimes too thick
Summary: Pinned tab separators are sometimes too thick → [HiDPI] Pinned tab separators are sometimes too thick
(Reporter)

Comment 4

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

Indeed, 125%.
Flags: needinfo?(chrislord.net)
(Assignee)

Comment 5

3 years ago
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?
Flags: needinfo?(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?
Flags: needinfo?(shorlander)
(Assignee)

Comment 7

3 years ago
(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/
Flags: needinfo?(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!
Flags: needinfo?(shorlander)
(Assignee)

Updated

3 years ago
Attachment #8645676 - Flags: ui-review?(shorlander)
Kinda P4 polish, but 125% is our most common scaling factor on Windows (after 100%).
Priority: -- → P3
(Assignee)

Updated

3 years ago
Blocks: 1192839
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
Attachment #8645676 - Flags: ui-review?(shorlander) → ui-review+
(Assignee)

Comment 11

3 years ago
Created attachment 8650966 [details] [diff] [review]
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).
Assignee: nobody → dao
Attachment #8645676 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #8650966 - Flags: review?(gijskruitbosch+bugs)
(I'm sorry, I likely won't get to this review until Monday.)
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. :-\
Flags: needinfo?(dao)
(Assignee)

Comment 14

3 years ago
(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?
Flags: needinfo?(dao)
(Assignee)

Comment 15

3 years ago
(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)
(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 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?
Attachment #8650966 - Flags: review?(gijskruitbosch+bugs) → review+
(Assignee)

Comment 18

3 years ago
(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
(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.
https://hg.mozilla.org/mozilla-central/rev/11cfa127c503
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox43: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 43
(Assignee)

Updated

3 years ago
Depends on: 1198236
(Assignee)

Updated

3 years ago
Flags: qe-verify-
(Assignee)

Comment 22

3 years ago
(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.
(Assignee)

Updated

3 years ago
Blocks: 1173730
(Assignee)

Comment 23

3 years ago
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]:
Attachment #8650966 - Flags: approval-mozilla-aurora?
(Assignee)

Updated

3 years ago
status-firefox40: --- → wontfix
status-firefox41: --- → affected
(Assignee)

Comment 24

3 years ago
Comment on attachment 8650966 [details] [diff] [review]
patch v2

bug 1198236 is still an issue. will wait until that's resolved.
Attachment #8650966 - Flags: approval-mozilla-aurora?
(Assignee)

Updated

3 years ago
Blocks: 1179756
(Assignee)

Comment 25

3 years ago
Created attachment 8654053 [details] [diff] [review]
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]:
Attachment #8654053 - Flags: approval-mozilla-aurora?
(Assignee)

Comment 26

3 years ago
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
Attachment #8654053 - Flags: approval-mozilla-beta?
Comment on attachment 8654053 [details] [diff] [review]
patch for uplift

Early in the 42 cycle, taking it to polish the release.
Attachment #8654053 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
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!
Attachment #8654053 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Correction to my own comment, :( this is not just win10 specific.

Updated

3 years ago
Depends on: 1224732
You need to log in before you can comment on or make changes to this bug.