[Australis] curved tabs looks ugly when scaled up (hi-dpi)

VERIFIED FIXED in Firefox 29

Status

()

Firefox
Theme
VERIFIED FIXED
3 years ago
2 years ago

People

(Reporter: smaug, Assigned: MattN)

Tracking

(Blocks: 3 bugs, {regression})

unspecified
Firefox 31
All
Windows 8
regression
Points:
---
Dependency tree / graph
Bug Flags:
in-qa-testsuite ?
in-testsuite -

Firefox Tracking Flags

(firefox29+ fixed, firefox30+ verified, firefox31 verified)

Details

(Whiteboard: [Australis:P3])

Attachments

(5 attachments, 7 obsolete attachments)

4.43 KB, image/png
Details
793 bytes, text/html
Details
83.26 KB, patch
Details | Diff | Splinter Review
24.46 KB, patch
mconley
: review+
Dolske
: ui-review+
Details | Diff | Splinter Review
3.49 KB, image/png
Details
(Reporter)

Description

3 years ago
Created attachment 8343383 [details]
ugly_tab.png

For some reason curved tabs look rather ugly on Win8. They look fine on Win7 (and on Linux)

Updated

3 years ago
Blocks: 939862, 738491

Updated

3 years ago
Component: General → Theme

Updated

3 years ago
Summary: [Australis] curved tabs looks ugly on Windows 8 → [Australis] curved tabs looks ugly when scaled up (hi-dpi)
We're planning on working on HiDPI support for our UI on Windows after Australis.
Hardware: x86_64 → All
Whiteboard: [Australis:P4]

Comment 2

3 years ago
(In reply to Matthew N. [:MattN] from comment #1)
> We're planning on working on HiDPI support for our UI on Windows after
> Australis.

Indeed, but I have to agree with Olli that this looks pretty terrible. Why does this look OK on OS X hidpi, and isn't there "just" some CSS we can port to make the tabs look less affrontingly awful? :-)

Comment 3

3 years ago
I'm upgrading this to P3- to at least get this on the radar for this week.
Whiteboard: [Australis:P4] → [Australis:P3-]
(Reporter)

Comment 4

3 years ago
Created attachment 8385295 [details]
up-to-date ugly tab
(Reporter)

Comment 5

3 years ago
I guess the tab a bit less ugly these days, now that we use right colors, but it still
looks very unfinished.

Comment 6

3 years ago
If someone can create some high-dpi version of these images, we can fix this. In metro we're doing 1.0x, 1.4x, and 1.8x.

chrome://browser/skin/tabbrowser/tab-background-start.png

Who is the ux lead for desktop currently?

Comment 7

3 years ago
(In reply to :Gijs Kruitbosch from comment #2)
> (In reply to Matthew N. [:MattN] from comment #1)
> > We're planning on working on HiDPI support for our UI on Windows after
> > Australis.
> 
> Indeed, but I have to agree with Olli that this looks pretty terrible. Why
> does this look OK on OS X hidpi, and isn't there "just" some CSS we can port
> to make the tabs look less affrontingly awful? :-)

Because someone took the time to generate 2.0x images for osx -  

http://mxr.mozilla.org/mozilla-central/source/browser/themes/osx/jar.mn#246
(In reply to Jim Mathies [:jimm] from comment #6)
> If someone can create some high-dpi version of these images, we can fix
> this. In metro we're doing 1.0x, 1.4x, and 1.8x.
> 
> chrome://browser/skin/tabbrowser/tab-background-start.png
> 
> Who is the ux lead for desktop currently?

I think shorlander is the person to talk to here.
Flags: needinfo?(shorlander)

Comment 9

3 years ago
Created attachment 8385351 [details]
check your dots per px

Comment 10

3 years ago

We should probably morph this bug to a more general "update images resources for high-dpi on Windows" unless something like that already exists?

I can take the time to put together a patch now that metrofx is out the door.

Comment 11

3 years ago
(In reply to Jim Mathies [:jimm] from comment #10)
> 
> We should probably morph this bug to a more general "update images resources
> for high-dpi on Windows" unless something like that already exists?
> 
> I can take the time to put together a patch now that metrofx is out the door.

Updating all our images is significantly more work and isn't an Australis regression. There is a separate bug for it (bug 878288). I think we should focus this bug to be about the tabstrip. If we have time to fix other icons we can do that, but there are too many more pressing issues to fix for Australis right now.

Updated

3 years ago
Blocks: 878288

Comment 12

3 years ago
wfm, if someone can post the images, I can put together a patch.
Created attachment 8385386 [details]
windows-hidpi-tab-assets-01.zip

Windows Tab Assets for 1.25x, 1.5x and 2x.

The 2x obviously scaled fine. For 1.25 and 1.5 I had to round up. Not sure what will happen at those stops; the CSS might need further tweaks.
Flags: needinfo?(shorlander)
Comment on attachment 8385386 [details]
windows-hidpi-tab-assets-01.zip

Tested these, don't think they will work as is. Going to need to tweak them first.
Attachment #8385386 - Attachment is obsolete: true
I don't think we want to end up with four versions of these images. Would scaling down the 2x images for 1.25x & Co. look better than scaling up the 1x images?
(In reply to Dão Gottwald [:dao] from comment #15)
> I don't think we want to end up with four versions of these images. Would
> scaling down the 2x images for 1.25x & Co. look better than scaling up the
> 1x images?

I tried that. It didn't look great.
I also wonder how scaling down would affect our TART numbers...
(In reply to Stephen Horlander [:shorlander] from comment #16)
> (In reply to Dão Gottwald [:dao] from comment #15)
> > I don't think we want to end up with four versions of these images. Would
> > scaling down the 2x images for 1.25x & Co. look better than scaling up the
> > 1x images?
> 
> I tried that. It didn't look great.

Does it look better than upscaling? :)

(In reply to Mike Conley (:mconley) from comment #17)
> I also wonder how scaling down would affect our TART numbers...

Probably not worse than the upscaling we're doing today.

Also, do we have any idea whether some scaling factors might be more common than others?
(In reply to Dão Gottwald [:dao] from comment #18)
> (In reply to Stephen Horlander [:shorlander] from comment #16)
> > (In reply to Dão Gottwald [:dao] from comment #15)
> > > I don't think we want to end up with four versions of these images. Would
> > > scaling down the 2x images for 1.25x & Co. look better than scaling up the
> > > 1x images?
> > 
> > I tried that. It didn't look great.
> 
> Does it look better than upscaling? :)

Well, yes ;)

Here is what it looks like scaled to 150%. Top is upscaled with the current 1x image, second is downscaled with the 2x image and the bottom is a properly sized 1.5x image.

http://cl.ly/image/440T2g1d2M2V

Downscaling does look nicer than upscaling but still fuzzy. The specifically sized image looks a lot better.
Duplicate of this bug: 879583

Comment 21

3 years ago
Created attachment 8386364 [details] [diff] [review]
patch
Assignee: nobody → jmathies
Attachment #8386364 - Flags: review?(dao)
Created attachment 8386436 [details] [diff] [review]
Make Start and End of Tab 32x32

(In reply to Jim Mathies [:jimm] from comment #21)
> Created attachment 8386364 [details] [diff] [review]
> patch

I checked your patch and it has the same problems I was running into. Namely rounding errors causing gaps and overlap between the start, middle and end of the tab.

I think before we can cleanly support 100%, 125%, 150% and 200% stops the base image will have to evenly multiply by those stops.

Currently the start and end elements are 31 x 30. This patch changes tab start and end to 32 x 32. This also even scaling. On OS X it does have a bug that I can't figure out. Increasing the tab height and the negative margin on the nav-bar breaks in Customize Mode, with lightweight themes and in fullscreen. Not sure where we are doing those calculations though.
Attachment #8386436 - Flags: feedback?(MattN+bmo)
Created attachment 8386437 [details]
windows-hidpi-tab-assets-02.zip

New 32x32 base images.

Updated

3 years ago
Attachment #8386364 - Flags: review?(dao)
Wouldn't using an SVG as the primary asset ease the adoption of different pixel densities here? I'm well aware of SVG not being "the magic bullet" (as in "one size fits all"), but should be easier to customize (or is it?) to acting pixel density on launch/runtime.

As it was mentioned in the sister bug 878288, comment 7 even performance shouldn't degrade too much, due to improved runtime caching of SVG assets (bug 764299), but we could always generate and cache these images on (first) launch, if runtime caching turns out to be less performant than ideal.

The gains would, then be obvious, not having to re-generate (and store) all the assets for the myriad of different densities. Might this work? I'm not sure what are the exact tweaks needed for specific densities, is it possible that these could be automated/algorithmized?
(In reply to Szmozsánszky István [:flaki] from comment #24)
> Wouldn't using an SVG as the primary asset ease the adoption of different
> pixel densities here? I'm well aware of SVG not being "the magic bullet" (as
> in "one size fits all"), but should be easier to customize (or is it?) to
> acting pixel density on launch/runtime.
>
> As it was mentioned in the sister bug 878288, comment 7 even performance
> shouldn't degrade too much, due to improved runtime caching of SVG assets
> (bug 764299), but we could always generate and cache these images on (first)
> launch, if runtime caching turns out to be less performant than ideal.

If I recall correctly we were using SVG for the image and not just the clip-path during on of the iterations. I think it caused a performance hit though. Maybe something to try again with the improvements to SVG, but not at this stage.

> The gains would, then be obvious, not having to re-generate (and store) all
> the assets for the myriad of different densities. Might this work? I'm not
> sure what are the exact tweaks needed for specific densities, is it possible
> that these could be automated/algorithmized?

If the base SVG image is designed correctly it should just scale correctly. Would have to try it though to be sure because rounding issues and pixel snapping are often issues when trying to scale SVG.
(In reply to Stephen Horlander [:shorlander] from comment #22)
> Currently the start and end elements are 31 x 30. This patch changes tab
> start and end to 32 x 32. This also even scaling. On OS X it does have a bug
> that I can't figure out. Increasing the tab height and the negative margin
> on the nav-bar breaks in Customize Mode, with lightweight themes and in
> fullscreen. Not sure where we are doing those calculations though.

Change 1px to 2px at https://mxr.mozilla.org/mozilla-central/source/browser/themes/osx/browser.css?rev=cc1fdefca201&mark=2773#2759

There will be a new problem with the new customize mode outlines though.

I'll run my screenshot tool to try find problems and look at the patch some more.
Blocks: 820679
No longer blocks: 878288
(Reporter)

Updated

3 years ago
tracking-firefox29: --- → ?
tracking-firefox30: --- → ?
Depends on: 983761
Created attachment 8391774 [details] [diff] [review]
MattN WIP v.1

shorlander: the new Windows curves don't connect as nicely to the nav-bar border (at the bottom of the curve). 
* Current 1.0x: http://grab.by/vaEA 
* 32x32  1.25x: http://grab.by/vaEG
* 32x32   2.0x: http://grab.by/vaEy

On 1.25x it's very noticeable that the highlight on the nav-bar is twice the thickness as the highlight at the bottom of the tab curve. You can see the difference in the images alone. The 1.25x curve goes to the edge of the image whereas it doesn't with @1x and @2x.

Stephen, can you fix the new images so that the highlight lines up better on the bottom? It's easiest to see on black themes.
Flags: needinfo?(shorlander)
Attachment #8343383 - Attachment is obsolete: true
Assignee: jmathies → shorlander
Status: NEW → ASSIGNED
status-firefox29: --- → affected
status-firefox30: --- → affected
Attachment #8386436 - Flags: feedback?(MattN+bmo) → feedback+
tracking-firefox30: ? → +

Updated

3 years ago
Duplicate of this bug: 937850
tracking-firefox29: ? → +

Updated

3 years ago
Duplicate of this bug: 844795
Created attachment 8397531 [details] [diff] [review]
Folded WIP with new path

Updated images. Just folded all the various WIP patches into one.
Attachment #8386436 - Attachment is obsolete: true
Attachment #8386437 - Attachment is obsolete: true
Flags: needinfo?(shorlander)
Flags: needinfo?(MattN+bmo)
This will be prioritized by the desktop team along with the rest of the Australis follow-up work. The desktop team will not fix this in 29 or 30. As such, dropping the tracking flags.
tracking-firefox29: + → ---
tracking-firefox30: + → ---
Flags: needinfo?(MattN+bmo)
Keywords: regression
There was some confusion by a few of us in our earlier conversation about this bug. MattN is on this and it is targeted at 29. Restoring the tracking flags that I removed.
tracking-firefox29: --- → +
tracking-firefox30: --- → +
Blocks: 990218
Attachment #8386364 - Attachment is obsolete: true
Attachment #8391774 - Attachment is obsolete: true
Attachment #8397531 - Attachment description: 946987_add-hidpi-tabs-WIP.patch → Folded WIP with new path
Depends on: 990384
No longer blocks: 990218
Created attachment 8399818 [details] [diff] [review]
WIP 3 - Split some changes into bug 990384 and bug 990387
Attachment #8397531 - Attachment is obsolete: true
Whiteboard: [Australis:P3-] → [Australis:P3]
Matthew, are you still targeting 29 for this bug? Thanks
Flags: needinfo?(MattN+bmo)

Updated

3 years ago
Duplicate of this bug: 988868
Created attachment 8405754 [details] [diff] [review]
Approach 2 - v.1 Downscale @2x images for 1.25dppx and higher with the existing tab size

(In reply to Sylvestre Ledru [:sylvestre] from comment #34)
> Matthew, are you still targeting 29 for this bug? Thanks

We're going to make a decision by Monday as to whether we'll do anything for 29.
--
This patch definitely makes the stroke crisper although it still makes the connection problem on the bottom of the curve look somewhat worse than the blurry connection IMO. This patch is much lower risk though so if people think this is better than m-c then I'm fine with taking the patch.

Screenshots of stock m-c and with this patch applied are here (207 MB zip file): https://drive.google.com/file/d/0B8j8iLTl0K0UOUhwQXU3ekVzenc/edit?usp=sharing
Assignee: shorlander → MattN+bmo
Attachment #8405754 - Flags: ui-review?(shorlander)
Attachment #8405754 - Flags: ui-review?(madhava)
Attachment #8405754 - Flags: ui-review?(dolske)
Flags: needinfo?(MattN+bmo)
OK. Do you know at what time? I am going to launch the go to build for beta 8 Monday. If it does not make it for beta 8, it is going to be too late for 29.
(Reporter)

Comment 38

3 years ago
MattN, by any chance are there tryserver builds for the approach 2?
(In reply to Olli Pettay [:smaug] from comment #38)
> MattN, by any chance are there tryserver builds for the approach 2?

I just happened to push them: https://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/mozilla@noorenberghe.ca-d5ca06b23110/
(In reply to Sylvestre Ledru [:sylvestre] from comment #37)
> OK. Do you know at what time? I am going to launch the go to build for beta
> 8 Monday. If it does not make it for beta 8, it is going to be too late for
> 29.

What time would you like it landed by? Decision makers are in Eastern and Pacific timezones.
Before 11PM PDT would be great!
Comment on attachment 8405754 [details] [diff] [review]
Approach 2 - v.1 Downscale @2x images for 1.25dppx and higher with the existing tab size

I went thought the pile of screenshots Matt generated for this patch. I found of number of small glitches, but they're all present in the screenshots of m-c today. So this patch is improving the tabs, but I don't see any significant visual regressions. We'll come back post-29 to improve Windows hidpi further.
Attachment #8405754 - Flags: ui-review?(shorlander)
Attachment #8405754 - Flags: ui-review?(madhava)
Attachment #8405754 - Flags: ui-review?(dolske)
Attachment #8405754 - Flags: ui-review+
Comment on attachment 8405754 [details] [diff] [review]
Approach 2 - v.1 Downscale @2x images for 1.25dppx and higher with the existing tab size

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Initial implementation of Australis tabs (bug 738491)
User impact if declined: Users with >1dppx 
Testing completed (on m-c, etc.): Automated screenshot collection reviewed by dolske, myself, and others.
Risk to taking this patch (and alternatives if risky): Low risk image addition and copying some CSS from OS X to use the images with 1.25 dppx and higher.
String or IDL/UUID changes made by this patch: None
Attachment #8405754 - Flags: review?(mconley)
Attachment #8405754 - Flags: approval-mozilla-beta?
Attachment #8405754 - Flags: approval-mozilla-aurora?
Comment on attachment 8405754 [details] [diff] [review]
Approach 2 - v.1 Downscale @2x images for 1.25dppx and higher with the existing tab size

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

Looks fine to me. The placement of the block is a little odd - if you wanted to move it closer to the tabs.inc.css include, that might make more sense.

Thanks Matt!

::: browser/themes/windows/browser.css
@@ +2887,5 @@
>  }
>  
>  /* End private browsing indicators */
>  
> +@media (min-resolution: 1.25dppx) {

Is there a particular reason why this block is here? If not, it might make more sense to put this nearer to where tabs.inc.css is included.
Attachment #8405754 - Flags: review?(mconley) → review+
(In reply to Mike Conley (:mconley) from comment #44)
> Comment on attachment 8405754 [details] [diff] [review]
>
> Is there a particular reason why this block is here? If not, it might make
> more sense to put this nearer to where tabs.inc.css is included.

Nope, I think that was just like that from another patch and I didn't notice. Fixed.

https://hg.mozilla.org/integration/fx-team/rev/5c1bbf1f4820
Flags: in-testsuite-
Blocks: 995733
Attachment #8405754 - Flags: feedback?
Attachment #8405754 - Flags: approval-mozilla-beta?
Attachment #8405754 - Flags: approval-mozilla-beta+
Attachment #8405754 - Flags: approval-mozilla-aurora?
Attachment #8405754 - Flags: approval-mozilla-aurora+
https://hg.mozilla.org/mozilla-central/rev/5c1bbf1f4820
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 31
https://hg.mozilla.org/releases/mozilla-aurora/rev/965a0df971b9
status-firefox30: affected → fixed
status-firefox31: --- → fixed
https://hg.mozilla.org/releases/mozilla-beta/rev/975b76d0b1c0
status-firefox29: affected → fixed

Comment 49

3 years ago
Created attachment 8406126 [details]
Ugly curved tabs

Running from inbound which includes this patch and I still see ugly curved tabs.  My dpi is 120 (125%) on Windows 8.1.1.

Built from https://hg.mozilla.org/integration/mozilla-inbound/rev/2e83d29d7f6b

Mozilla/5.0 (Windows NT 6.3; WOW64; rv:31.0) Gecko/20100101 Firefox/31.0

Comment 50

3 years ago
btw... I've been using this snippet of css code to make the tabs flow nicely into the border:

#nav-bar {
    border-top: 2px solid  hsl(210, 6.1%, 71.1%) !important;
    background-clip: padding-box;}

Updated

3 years ago
Duplicate of this bug: 996386

Comment 52

3 years ago
The problem I think is the use of images, and this makes the tabs not scale well, can we not achieve the same results by replacing the images with something that will work better, such as a mix of CSS gradients, and a border radius?

Comment 53

3 years ago
(In reply to Matthew Bonner from comment #52)
> The problem I think is the use of images, and this makes the tabs not scale
> well, can we not achieve the same results by replacing the images with
> something that will work better, such as a mix of CSS gradients, and a
> border radius?

I would be surprised if this kind of curve were doable in pure CSS gradients + border radius without having to have many boxes. Even if it were, we did extensive performance testing on these (and still do automated performance testing on them) and how they affect the animation. I would be (pleasantly) surprised if there were a way we could avoid images and maintain performance.
(In reply to Gary [:streetwolf] from comment #49)
> Created attachment 8406126 [details]
> Ugly curved tabs

That's expected with this patch. Due to time constraints for shipping Firefox 29, we can't make it perfect for all situations. Bug 995733 tracks followup work.
Comment on attachment 8405754 [details] [diff] [review]
Approach 2 - v.1 Downscale @2x images for 1.25dppx and higher with the existing tab size

Big fingers. Removing the unwanted feedback flag.
Attachment #8405754 - Flags: feedback?
Flags: in-qa-testsuite?
Keywords: verifyme
Tested on a Microsoft Surface Pro 2 device running Windows 8.1 64bit using Firefox 30, build ID: 20140605174243 and Firefox 31 beta 5, build ID: 20140626181429.

Marking this issue verified as further work is tracked in bug 995733.
Status: RESOLVED → VERIFIED
status-firefox30: fixed → verified
status-firefox31: fixed → verified
Keywords: verifyme
You need to log in before you can comment on or make changes to this bug.