Update icon in hamburger menu looks grainy on low-res displays

RESOLVED FIXED in Firefox 55

Status

()

defect
P2
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: ato, Assigned: dthayer)

Tracking

unspecified
mozilla55
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox55 fixed)

Details

Attachments

(3 attachments)

Reporter

Description

2 years ago
Posted image lowresgrainy.png
The application update icon that appears when a new Firefox version is available, looks crisp and beautiful on hi-res screens, but grainy on low-res 1:1 displays.

See the attached screenshots.
Reporter

Comment 1

2 years ago
Posted image highrescrisp.png
Bram, can you help with creating an image to fix this?
Blocks: 893505
Flags: needinfo?(bram)
The low-res (@1x) image is here:
https://cl.ly/3Z2A1O2p3R0i

But perhaps we should use an SVG that can be freely scaled?
https://cl.ly/0c0d353y160M
Flags: needinfo?(bram)
Assignee

Comment 4

2 years ago
I think the grainyness mentioned is just due to the 1px border rasterization. Bram, would you mind reviewing this image where I'm using a 2px border instead? (This would only be for 1:1 displays.)

https://d2ppvlu71ri8gs.cloudfront.net/items/0x0I072m2V2B1h282J1W/newbadge.PNG
Flags: needinfo?(bram)
(In reply to Doug Thayer [:dthayer] from comment #4)
> I think the grainyness mentioned is just due to the 1px border
> rasterization. Bram, would you mind reviewing this image where I'm using a
> 2px border instead? (This would only be for 1:1 displays.)
> 
> https://d2ppvlu71ri8gs.cloudfront.net/items/0x0I072m2V2B1h282J1W/newbadge.PNG

Thanks for posting the example, Doug. It looks good to me!
Flags: needinfo?(bram)
Comment hidden (mozreview-request)

Comment 7

2 years ago
mozreview-review
Comment on attachment 8866192 [details]
Bug 1359881 - Increase border width of update badge

https://reviewboard.mozilla.org/r/137832/#review141058

::: browser/themes/shared/customizableui/panelUI.inc.css:119
(Diff revision 1)
> -  border: 1px solid -moz-dialog;
> +  border: 2px solid -moz-dialog;
>    /* "!important" is necessary to override the rule in toolbarbutton.css */
> -  margin: -9px 0 0 !important;
> +  margin: -8px 0 0 !important;

So, I'm a little confused. I don't have hardware to test this off-hand so I'm just going to comment and leave the r? for now, and hopefully you can clarify and/or next week when I'm home I can look at it myself.

Basically, you're increasing the border, and decreasing the negative top margin. As I understand it, that moves the top of the border (and therefore the image) *down* by 1px, and because the border is bigger, the green bit of the image moves down 2px. I kind of assume this is my morning sleepy brain missing something, but that doesn't seem right?

The other thing I'm confused about is that this is XUL, and so the border "eats into" the width/height, which is staying the same, which will cause the image we're using to be warped, which will not make things better on lo-res devices... right? I guess I'm missing something else here, maybe? :-)
Assignee

Comment 8

2 years ago
mozreview-review-reply
Comment on attachment 8866192 [details]
Bug 1359881 - Increase border width of update badge

https://reviewboard.mozilla.org/r/137832/#review141058

> So, I'm a little confused. I don't have hardware to test this off-hand so I'm just going to comment and leave the r? for now, and hopefully you can clarify and/or next week when I'm home I can look at it myself.
> 
> Basically, you're increasing the border, and decreasing the negative top margin. As I understand it, that moves the top of the border (and therefore the image) *down* by 1px, and because the border is bigger, the green bit of the image moves down 2px. I kind of assume this is my morning sleepy brain missing something, but that doesn't seem right?
> 
> The other thing I'm confused about is that this is XUL, and so the border "eats into" the width/height, which is staying the same, which will cause the image we're using to be warped, which will not make things better on lo-res devices... right? I guess I'm missing something else here, maybe? :-)

Thanks for the quick feedback, Gijs! :)

First, I included an image in the bug, which I am realizing I should link here in case you haven't seen it: https://d2ppvlu71ri8gs.cloudfront.net/items/0x0I072m2V2B1h282J1W/newbadge.PNG I asked Bram to review it for some of the reasons you mentioned and he gave a thumbs up.

The added border width will in fact eat into the badge's space rather than increasing it, but it does not cause the image to change size. I'm not sure if there are conditions under which images _would_ change size, but this is not one of them. The reason I am increasing the border without increasing the min-width or min-height is that doing so makes the badge too big over all, and it ends up either eating into the second pancake of the menu icon, or sitting too high and looking off-balance.

Regarding why I moved the badge down a pixel - the added space makes the badge appear set out a bit more from the pancake menu icon. Moving it down a bit makes it feel more nestled into it again. It's a bit subtle and maybe I'm crazy but it felt like the right direction to go.

Comment 9

2 years ago
mozreview-review
Comment on attachment 8866192 [details]
Bug 1359881 - Increase border width of update badge

https://reviewboard.mozilla.org/r/137832/#review141338

Thanks for the clarifications! In that case, this is good to go.

FWIW, I *think* we force-set the width and height of (all) badges elsewhere... I wonder if we can make the badge itself look better on lodpi if we adjust the image for this (IIRC it's some bizarre size like 12x13px - but maybe there's other code that's already working around that) ?

Anyway, that doesn't need to block this bug.
Attachment #8866192 - Flags: review?(gijskruitbosch+bugs) → review+
Assignee

Updated

2 years ago
Assignee: nobody → dothayer
Assignee

Updated

2 years ago
Keywords: checkin-needed

Comment 11

2 years ago
Pushed by rstrong@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/ddcfea98fb34
Increase border width of update badge r=Gijs
https://hg.mozilla.org/mozilla-central/rev/ddcfea98fb34
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.