1138630 - Replace the »update arrow« unicode stuff with a proper image

Status: RESOLVED FIXED

(Firefox :: Toolbars and Customization, defect)

Description

[:Gijs (he/him)]

... because it still doesn't really look good on Windows.

The way to do this would be to set the badge to " " and style the badge element to have a background image in addition to the background color we already set on it.

Stephen, can you provide an image?

John, would you be interested in taking this bug? :-)

Comment 1

[John Tungul]

Sure I can do this. Once Stephen supplies the image, I can get the patch created and submitted on here for review.

Comment 2

[Tim Nguyen :ntim]

Attachment: update-arrow.svg

This SVG seems to render sharp in Firefox (on Windows, at 1x size, in the browser area). But I'm not sure how that looks on the update badge.

Comment 3

[Tim Nguyen :ntim]

Attachment: update-arrow-alt.svg

An alternative with a higher arrow triangle

Comment 4

[Tim Nguyen :ntim]

Attachment: Screenshots

Comment 5

[:Gijs (he/him)]

Comment on attachment 8577645 [details]
update-arrow.svg

Stephen should look at this, not me.

Comment 6

[Tim Nguyen :ntim]

Comment on attachment 8577645 [details]
update-arrow.svg

Just talked to Stephen on IRC, and he likes the alt version better.

Comment 7

[Tim Nguyen :ntim]

Attachment: Patch

Comment 8

[:Gijs (he/him)]

Comment on attachment 8586419 [details] [diff] [review]
Patch

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

You just got bitrotted by Dão's refactor of the windows aero stuff.

::: browser/themes/shared/customizableui/panelUIOverlay.inc.css
@@ +106,5 @@
>    background-position: 100% 0, calc(100% - 1px) 0, calc(100% - 2px) 0;
>  }
>  
> +#PanelUI-menu-button[update-status="succeeded"] .toolbarbutton-badge::after {
> +  content: "";

Why can't you use the SVG file as content here? (you can use an image as content, and it'll nip a layout warning + inefficiency in the bud)

@@ +108,5 @@
>  
> +#PanelUI-menu-button[update-status="succeeded"] .toolbarbutton-badge::after {
> +  content: "";
> +  background-image: url(chrome://browser/skin/update-badge.svg);
> +  background-size: 10px 10px;

The SVG file has a size, why do we need to re-specify it here?

@@ +112,5 @@
> +  background-size: 10px 10px;
> +  background-position: center center;
> +  background-repeat: no-repeat;
> +  background-color: #74BF43;
> +  display: inline-block;

Why inline-block and not just block?

::: browser/themes/shared/update-badge.svg
@@ +1,2 @@
> +<svg xmlns="http://www.w3.org/2000/svg" width="10px" height="10px">
> +	<polygon points="4,9 4,5 2,5 5,1 8,5 6,5 6,9" fill="#fff"/>

Nit: as Gavin so eloquently phrased it earlier in #fx-team, KILL THE TABS

(2 spaces please ;-) )

Comment 9

[Tim Nguyen :ntim]

(In reply to :Gijs Kruitbosch from comment #8)
> Comment on attachment 8586419 [details] [diff] [review]
> Patch
> 
> Review of attachment 8586419 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> You just got bitrotted by Dão's refactor of the windows aero stuff.
No more aero section in jar.mn \o/

> ::: browser/themes/shared/customizableui/panelUIOverlay.inc.css
> @@ +106,5 @@
> >    background-position: 100% 0, calc(100% - 1px) 0, calc(100% - 2px) 0;
> >  }
> >  
> > +#PanelUI-menu-button[update-status="succeeded"] .toolbarbutton-badge::after {
> > +  content: "";
> 
> Why can't you use the SVG file as content here? (you can use an image as
> content, and it'll nip a layout warning + inefficiency in the bud)
Fixed

> @@ +108,5 @@
> >  
> > +#PanelUI-menu-button[update-status="succeeded"] .toolbarbutton-badge::after {
> > +  content: "";
> > +  background-image: url(chrome://browser/skin/update-badge.svg);
> > +  background-size: 10px 10px;
> 
> The SVG file has a size, why do we need to re-specify it here?
Removed
 
> @@ +112,5 @@
> > +  background-size: 10px 10px;
> > +  background-position: center center;
> > +  background-repeat: no-repeat;
> > +  background-color: #74BF43;
> > +  display: inline-block;
> 
> Why inline-block and not just block?
Removed alltogether.
 
> ::: browser/themes/shared/update-badge.svg
> @@ +1,2 @@
> > +<svg xmlns="http://www.w3.org/2000/svg" width="10px" height="10px">
> > +	<polygon points="4,9 4,5 2,5 5,1 8,5 6,5 6,9" fill="#fff"/>
> 
> Nit: as Gavin so eloquently phrased it earlier in #fx-team, KILL THE TABS
> 
> (2 spaces please ;-) )
Done :)

Comment 10

[Tim Nguyen :ntim]

Attachment: Patch

Comment 11

[Tim Nguyen :ntim]

Attachment: Patch

Comment 12

[:Gijs (he/him)]

Comment on attachment 8587471 [details] [diff] [review]
Patch

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

This still needed the height to be sized appropriately, I take it? If not, please get rid of that, too.

Comment 13

[Tim Nguyen :ntim]

(In reply to :Gijs Kruitbosch from comment #12)
> Comment on attachment 8587471 [details] [diff] [review]
> Patch
> 
> Review of attachment 8587471 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This still needed the height to be sized appropriately, I take it? If not,
> please get rid of that, too.

Yeah, it need the height to be set, otherwise it was too big.

Comment 14

[Matthew N. [:MattN]]

https://hg.mozilla.org/integration/fx-team/rev/335575e02706

Comment 15

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/335575e02706

Comment 16

[Tim Nguyen :ntim]

Comment on attachment 8587471 [details] [diff] [review]
Patch

Approval Request Comment
[Feature/regressing bug #]: bug 1117722
[User impact if declined]: update badge icon is low res on Windows, and looks like a stick
[Describe test coverage new/current, TreeHerder]: landed on m-c
[Risks and why]: low, mostly a css change
[String/UUID change made/needed]: None

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/3e1d92b65592

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/33496825c683