Closed Bug 1138630 Opened 9 years ago Closed 9 years ago

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

Categories

(Firefox :: Toolbars and Customization, defect)

All
Windows
defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 40
Iteration:
40.1 - 13 Apr
Tracking Status
firefox37 --- unaffected
firefox38 --- fixed
firefox39 --- fixed
firefox40 --- fixed

People

(Reporter: Gijs, Assigned: ntim, Mentored)

References

Details

Attachments

(3 files, 3 obsolete files)

... 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? :-)
Flags: needinfo?(shorlander)
Flags: needinfo?(jtungul53)
Flags: in-testsuite-
Flags: firefox-backlog+
Sure I can do this. Once Stephen supplies the image, I can get the patch created and submitted on here for review.
Flags: needinfo?(jtungul53)
Attached image update-arrow.svg (obsolete) —
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.
Attachment #8577645 - Flags: feedback?(shorlander)
Attachment #8577645 - Flags: feedback?(gijskruitbosch+bugs)
Attached image update-arrow-alt.svg
An alternative with a higher arrow triangle
Attached image Screenshots
Comment on attachment 8577645 [details]
update-arrow.svg

Stephen should look at this, not me.
Attachment #8577645 - Flags: feedback?(gijskruitbosch+bugs)
Assignee: nobody → ntim.bugs
Flags: needinfo?(shorlander) → needinfo?
OS: Windows 8.1 → Windows
Hardware: x86_64 → All
Flags: needinfo?
Comment on attachment 8577645 [details]
update-arrow.svg

Just talked to Stephen on IRC, and he likes the alt version better.
Attachment #8577645 - Flags: feedback?(shorlander)
Status: NEW → ASSIGNED
Attached patch Patch (obsolete) — Splinter Review
Attachment #8577645 - Attachment is obsolete: true
Attachment #8586419 - Flags: review?(gijskruitbosch+bugs)
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 ;-) )
Attachment #8586419 - Flags: review?(gijskruitbosch+bugs) → feedback+
(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 :)
Attached patch Patch (obsolete) — Splinter Review
Attachment #8586419 - Attachment is obsolete: true
Attachment #8587445 - Flags: review?(gijskruitbosch+bugs)
Attached patch PatchSplinter Review
Attachment #8587445 - Attachment is obsolete: true
Attachment #8587445 - Flags: review?(gijskruitbosch+bugs)
Attachment #8587471 - Flags: review?(gijskruitbosch+bugs)
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.
Attachment #8587471 - Flags: review?(gijskruitbosch+bugs) → review+
(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.
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/335575e02706
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 40
Iteration: --- → 40.1 - 13 Apr
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
Attachment #8587471 - Flags: approval-mozilla-beta?
Attachment #8587471 - Flags: approval-mozilla-aurora?
Attachment #8587471 - Flags: approval-mozilla-beta?
Attachment #8587471 - Flags: approval-mozilla-beta+
Attachment #8587471 - Flags: approval-mozilla-aurora?
Attachment #8587471 - Flags: approval-mozilla-aurora+
Flags: qe-verify-
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: