Closed
Bug 924050
Opened 11 years ago
Closed 11 years ago
Australis: Downloads button post-indicator-load has small icon in menupanel
Categories
(Firefox :: Theme, defect)
Firefox
Theme
Tracking
()
VERIFIED
FIXED
Firefox 29
People
(Reporter: Gijs, Assigned: mconley)
References
(Blocks 2 open bugs)
Details
(Whiteboard: [Australis:P3][Australis:M?])
Attachments
(2 files, 7 obsolete files)
14.43 KB,
application/zip
|
Details | |
25.95 KB,
patch
|
mconley
:
review+
|
Details | Diff | Splinter Review |
STR:
1. Open UX
2. Click the downloads panel
3. Open Customize Mode
4. Move the downloads button to the menupanel
ER:
Icon looks the same size as the other icons
AR:
Icon is too small
It probably also won't work well when a download is in progress while the button is in the menu panel. We should ensure that this situation does work well. A temporary fix would be always just showing the large icon, which would open the library, but a nicer fix would be ensuring that the progress bar and icon work when the button is in the menupanel, even if clicking will still open the library.
Reporter | ||
Comment 2•11 years ago
|
||
P2 at least until we have a rudimentary fix
Whiteboard: [Australis:P4][Australis:M?] → [Australis:P2][Australis:M?]
Reporter | ||
Comment 3•11 years ago
|
||
It's also not displaying correctly in the grid, but is shifted down. I've not been able to figure out what causes this - even after resetting different bits of the CSS of the stack to be identical to the image that is used in other buttons, I continue to see this difference, and I don't understand what causes it. :-\
OS: Mac OS X → All
Hardware: x86 → All
Reporter | ||
Comment 4•11 years ago
|
||
Note also that we'll need an icon that matches the panel size for the 'attention' state (ie, 'there are downloads you've not looked at yet').
Depends on: 923548
Comment 6•11 years ago
|
||
Hmm, sometimes I get a correctly-sized icon in the panel, sometimes I get the smaller icon.
Given that this is working but just visually glitchy, moving to P3.
Whiteboard: [Australis:P2][Australis:M?] → [Australis:P3][Australis:M?]
Comment 8•11 years ago
|
||
Adding blocking bug 939862, since I checked the deps of that before filing the now-dupe bug 960052 :-)
Blocks: australis-merge
Updated•11 years ago
|
Flags: needinfo?(mmaslaney)
Assignee | ||
Updated•11 years ago
|
Assignee: nobody → mconley
Assignee | ||
Comment 10•11 years ago
|
||
Beautiful! Thanks mmaslaney. Over to me...
Assignee | ||
Comment 11•11 years ago
|
||
Hey mmaslaney,
I noticed that Linux wasn't included in the assets that you provided. Do you think you could provide an arrow for that OS too?
I can produce the patch for the other OS's in the meantime.
Thanks!
-Mike
Flags: needinfo?(mmaslaney)
Comment 12•11 years ago
|
||
Also note that the arrow is green on Windows instead of blue.
Assignee | ||
Comment 14•11 years ago
|
||
Ok, this uses the current assets, and applies the rules for Windows, OS X and Linux.
Seems to do the job on OS X. Testing on Linux next. I see mmaslaney a few seats down, so I'm going to make sure he knows about the icon thing.
Comment 16•11 years ago
|
||
Updated Download-Glow menu panel icons.
Assignee | ||
Comment 17•11 years ago
|
||
Ok, this integrates mmaslaney's new icons. Works well on OS X - I need to test this on Windows and Linux though.
Attachment #8361011 -
Attachment is obsolete: true
Attachment #8361622 -
Attachment is obsolete: true
Attachment #8361694 -
Attachment is obsolete: true
Assignee | ||
Comment 18•11 years ago
|
||
Attachment #8363951 -
Attachment is obsolete: true
Assignee | ||
Comment 19•11 years ago
|
||
Comment on attachment 8363955 [details] [diff] [review]
Patch v1.2
Got some cycles to review this, mikedeboer?
Attachment #8363955 -
Flags: review?(mdeboer)
Assignee | ||
Comment 20•11 years ago
|
||
Tested on OS X, Windows (XP, 7, 8), Ubuntu.
Comment 21•11 years ago
|
||
Comment on attachment 8363955 [details] [diff] [review]
Patch v1.2
Review of attachment 8363955 [details] [diff] [review]:
-----------------------------------------------------------------
Have you tested this inside the overflow panel; does it already work as expected there?
If not, you can use `@inAnyPanel@`, like `#downloads-button@inAnyPanel@`.
Calling a file `download-glow-menuPanel-win8.png` is not really correct, because it'll also be used in 8.1 and perhaps even 9.
I'd like to propose `download-glow-menuPanel2.png`.
::: browser/themes/windows/downloads/indicator-aero.css
@@ +29,5 @@
> #downloads-indicator-counter {
> margin-bottom: -1px;
> }
> +
> +@media (-moz-os-version: windows-win8) {
This is not future-proof, please use:
preferred, in indicator.css:
```css
/* >= Win8 will be the default style */
#downloads-button@inAnyPanel@[attention] {
list-style-image: url("chrome://browser/skin/downloads/download-glow-menuPanel-win8.png");
}
%ifdef WINDOWS_AERO
@media (-moz-os-version: windows-vista),
(-moz-os-version: windows-win7) {
/* put styles for < Win8 here */
}
%endif
```
or alternatively, in indicator-aero.css:
```css
@media not all and (-moz-os-version: windows-vista) {
@media not all and (-moz-os-version: windows-win7) {
/* Put styles for Win 8 and higher here. */
}
}
```
Attachment #8363955 -
Flags: review?(mdeboer) → feedback+
Comment 22•11 years ago
|
||
Thanks for working on this, looks like we can it soon! :)
Assignee | ||
Comment 23•11 years ago
|
||
(In reply to Mike de Boer [:mikedeboer] from comment #21)
> Comment on attachment 8363955 [details] [diff] [review]
> Patch v1.2
>
> Review of attachment 8363955 [details] [diff] [review]:
> -----------------------------------------------------------------
>
> Have you tested this inside the overflow panel; does it already work as
> expected there?
>
> If not, you can use `@inAnyPanel@`, like `#downloads-button@inAnyPanel@`.
>
Good call - yeah, I didn't test it there. :)
> Calling a file `download-glow-menuPanel-win8.png` is not really correct,
> because it'll also be used in 8.1 and perhaps even 9.
> I'd like to propose `download-glow-menuPanel2.png`.
>
How about renaming the old one download-glow-menuPanel-XPVista7.png? Both are kinda ugly solutions, but this one at least makes it a little clearer what the difference is between the two images.
> ::: browser/themes/windows/downloads/indicator-aero.css
> @@ +29,5 @@
> > #downloads-indicator-counter {
> > margin-bottom: -1px;
> > }
> > +
> > +@media (-moz-os-version: windows-win8) {
>
> This is not future-proof, please use:
>
> preferred, in indicator.css:
> ```css
> /* >= Win8 will be the default style */
> #downloads-button@inAnyPanel@[attention] {
> list-style-image:
> url("chrome://browser/skin/downloads/download-glow-menuPanel-win8.png");
> }
>
> %ifdef WINDOWS_AERO
> @media (-moz-os-version: windows-vista),
> (-moz-os-version: windows-win7) {
> /* put styles for < Win8 here */
> }
> %endif
> ```
>
> or alternatively, in indicator-aero.css:
> ```css
> @media not all and (-moz-os-version: windows-vista) {
> @media not all and (-moz-os-version: windows-win7) {
> /* Put styles for Win 8 and higher here. */
> }
> }
> ```
Oh yeah, good call. :) Thanks!
Comment 24•11 years ago
|
||
(In reply to Mike Conley (:mconley) from comment #23)
> How about renaming the old one download-glow-menuPanel-XPVista7.png? Both
> are kinda ugly solutions, but this one at least makes it a little clearer
> what the difference is between the two images.
I agree, let's roll with that!
Assignee | ||
Comment 25•11 years ago
|
||
Attachment #8363955 -
Attachment is obsolete: true
Assignee | ||
Comment 26•11 years ago
|
||
Comment on attachment 8364479 [details] [diff] [review]
Patch v1.3
What do you think of this?
Attachment #8364479 -
Flags: review?(mdeboer)
Comment 27•11 years ago
|
||
Comment on attachment 8364479 [details] [diff] [review]
Patch v1.3
Review of attachment 8364479 [details] [diff] [review]:
-----------------------------------------------------------------
::: browser/themes/windows/downloads/indicator.css
@@ +37,5 @@
>
> +#downloads-button[cui-areatype="menu-panel"][attention] {
> + list-style-image: url("chrome://browser/skin/downloads/download-glow-menuPanel-XPVista7.png");
> + -moz-image-region: auto;
> +}
For supporting Windows 8 and beyond I listed two possible implementation options, one of which is preferred.
The reason it's the preferred method, is because it'll keep the default style sane, as it should represent the most up-to-date style, and will clearly denote styles for the (to be) legacy OS versions.
In other words, I'd like to see this method used in this patch.
Attachment #8364479 -
Flags: review?(mdeboer) → review-
Assignee | ||
Comment 28•11 years ago
|
||
(In reply to Mike de Boer [:mikedeboer] from comment #27)
> Comment on attachment 8364479 [details] [diff] [review]
> Patch v1.3
>
> Review of attachment 8364479 [details] [diff] [review]:
> -----------------------------------------------------------------
>
> ::: browser/themes/windows/downloads/indicator.css
> @@ +37,5 @@
> >
> > +#downloads-button[cui-areatype="menu-panel"][attention] {
> > + list-style-image: url("chrome://browser/skin/downloads/download-glow-menuPanel-XPVista7.png");
> > + -moz-image-region: auto;
> > +}
>
> For supporting Windows 8 and beyond I listed two possible implementation
> options, one of which is preferred.
>
> The reason it's the preferred method, is because it'll keep the default
> style sane, as it should represent the most up-to-date style, and will
> clearly denote styles for the (to be) legacy OS versions.
>
> In other words, I'd like to see this method used in this patch.
Oh, sorry. I didn't know one was preferred... *shrug*, I don't feel strongly about it. I'll go your way on this.
Comment 29•11 years ago
|
||
(In reply to Mike Conley (:mconley) from comment #28)
> (In reply to Mike de Boer [:mikedeboer] from comment #27)
> > In other words, I'd like to see this method used in this patch.
This sounded a bit... wrong, sorry about that!
> Oh, sorry. I didn't know one was preferred... *shrug*, I don't feel strongly
> about it. I'll go your way on this.
I'm just this nitpicky about it, because I've been working a lot on Windows 8 the past month... where this has been reason enough to r- a patch.
You also could've taken the 'easy' route, which is to defer the Win8 styling to bug 960517 and only update the OS versions we support at this time. As I suspect this patch will land before bug 960517, this will be the FIRST Win 8 specific icon in `browser/`, EVAH!
Assignee | ||
Comment 30•11 years ago
|
||
(In reply to Mike de Boer [:mikedeboer] from comment #29)
> (In reply to Mike Conley (:mconley) from comment #28)
> > (In reply to Mike de Boer [:mikedeboer] from comment #27)
> > > In other words, I'd like to see this method used in this patch.
>
> This sounded a bit... wrong, sorry about that!
Heh, no worries. :D
>
> > Oh, sorry. I didn't know one was preferred... *shrug*, I don't feel strongly
> > about it. I'll go your way on this.
>
> I'm just this nitpicky about it, because I've been working a lot on Windows
> 8 the past month... where this has been reason enough to r- a patch.
>
I hear ya - that's why I chose you as my reviewer - I think out of all of us, you're probably the best versed at the intricacies of Win8 stylings and the rules thereof. :)
> You also could've taken the 'easy' route, which is to defer the Win8 styling
> to bug 960517 and only update the OS versions we support at this time. As I
> suspect this patch will land before bug 960517, this will be the FIRST Win 8
> specific icon in `browser/`, EVAH!
\o/
Assignee | ||
Comment 31•11 years ago
|
||
Ok, switched to make the Windows 8 glyph the default, special-casing the older versions of Windows.
I've not yet tested this on Windows yet, so I'll request review once I have a chance to test it on Monday when I'm near my Windows box.
Attachment #8364479 -
Attachment is obsolete: true
Assignee | ||
Comment 32•11 years ago
|
||
Comment on attachment 8365629 [details] [diff] [review]
Patch v1.4
Review of attachment 8365629 [details] [diff] [review]:
-----------------------------------------------------------------
Ok, this seems to do the right thing on Windows XP/Vista/7/8.
Attachment #8365629 -
Flags: review?(mdeboer)
Comment 33•11 years ago
|
||
Comment on attachment 8365629 [details] [diff] [review]
Patch v1.4
Review of attachment 8365629 [details] [diff] [review]:
-----------------------------------------------------------------
::: browser/themes/windows/downloads/indicator.css
@@ +45,5 @@
> + (-moz-os-version: windows-win7) {
> + #downloads-button[cui-areatype="menu-panel"][attention] {
> + list-style-image: url("chrome://browser/skin/downloads/download-glow-menuPanel-XPVista7.png");
> + }
> +}
Since we have preprocessing flags for aero, the following is possible:
```css
%ifdef WINDOWS_AERO
@media (-moz-os-version: windows-vista),
(-moz-os-version: windows-win7) {
%endif
#downloads-button[cui-areatype="menu-panel"][attention] {
list-style-image: url("chrome://browser/skin/downloads/download-glow-menuPanel-XPVista7.png");
}
%ifdef WINDOWS_AERO
}
%endif
```
r=me with this change.
Attachment #8365629 -
Flags: review?(mdeboer) → review+
Assignee | ||
Comment 34•11 years ago
|
||
Thanks, done!
Attachment #8365629 -
Attachment is obsolete: true
Attachment #8365981 -
Flags: review+
Assignee | ||
Comment 35•11 years ago
|
||
Comment 36•11 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 29
Comment 37•11 years ago
|
||
The downloads icon has now the correct size in latest Nightly (build ID: 20140226030202) and latest Aurora (build ID: 20140227004002).
Status: RESOLVED → VERIFIED
You need to log in
before you can comment on or make changes to this bug.
Description
•