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)
Tracking
()
Tracking | Status | |
---|---|---|
firefox37 | --- | unaffected |
firefox38 | --- | fixed |
firefox39 | --- | fixed |
firefox40 | --- | fixed |
People
(Reporter: Gijs, Assigned: ntim, Mentored)
References
Details
Attachments
(3 files, 3 obsolete files)
137 bytes,
image/svg+xml
|
Details | |
8.61 KB,
image/png
|
Details | |
7.71 KB,
patch
|
Gijs
:
review+
Sylvestre
:
approval-mozilla-aurora+
Sylvestre
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
... 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+
Comment 1•9 years ago
|
||
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)
Assignee | ||
Comment 2•9 years ago
|
||
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)
Assignee | ||
Comment 3•9 years ago
|
||
An alternative with a higher arrow triangle
Assignee | ||
Comment 4•9 years ago
|
||
Reporter | ||
Comment 5•9 years ago
|
||
Comment on attachment 8577645 [details]
update-arrow.svg
Stephen should look at this, not me.
Attachment #8577645 -
Flags: feedback?(gijskruitbosch+bugs)
Assignee | ||
Updated•9 years ago
|
Assignee: nobody → ntim.bugs
Flags: needinfo?(shorlander) → needinfo?
OS: Windows 8.1 → Windows
Hardware: x86_64 → All
Assignee | ||
Updated•9 years ago
|
Flags: needinfo?
Assignee | ||
Comment 6•9 years ago
|
||
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)
Assignee | ||
Updated•9 years ago
|
Status: NEW → ASSIGNED
Assignee | ||
Updated•9 years ago
|
Assignee | ||
Updated•9 years ago
|
status-firefox37:
--- → unaffected
Assignee | ||
Comment 7•9 years ago
|
||
Attachment #8577645 -
Attachment is obsolete: true
Attachment #8586419 -
Flags: review?(gijskruitbosch+bugs)
Reporter | ||
Comment 8•9 years ago
|
||
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+
Assignee | ||
Comment 9•9 years ago
|
||
(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 :)
Assignee | ||
Comment 10•9 years ago
|
||
Attachment #8586419 -
Attachment is obsolete: true
Attachment #8587445 -
Flags: review?(gijskruitbosch+bugs)
Assignee | ||
Comment 11•9 years ago
|
||
Attachment #8587445 -
Attachment is obsolete: true
Attachment #8587445 -
Flags: review?(gijskruitbosch+bugs)
Attachment #8587471 -
Flags: review?(gijskruitbosch+bugs)
Reporter | ||
Comment 12•9 years ago
|
||
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+
Assignee | ||
Comment 13•9 years ago
|
||
(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.
Assignee | ||
Updated•9 years ago
|
Keywords: checkin-needed
Comment 14•9 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/335575e02706
Keywords: checkin-needed
Whiteboard: [fixed-in-fx-team]
Comment 15•9 years ago
|
||
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
Updated•9 years ago
|
Iteration: --- → 40.1 - 13 Apr
Assignee | ||
Comment 16•9 years ago
|
||
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?
Updated•9 years ago
|
Attachment #8587471 -
Flags: approval-mozilla-beta?
Attachment #8587471 -
Flags: approval-mozilla-beta+
Attachment #8587471 -
Flags: approval-mozilla-aurora?
Attachment #8587471 -
Flags: approval-mozilla-aurora+
Updated•9 years ago
|
Flags: qe-verify-
You need to log in
before you can comment on or make changes to this bug.
Description
•