Closed Bug 924050 Opened 11 years ago Closed 10 years ago

Australis: Downloads button post-indicator-load has small icon in menupanel

Categories

(Firefox :: Theme, defect)

defect
Not set
normal

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)

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.
P2 at least until we have a rudimentary fix
Whiteboard: [Australis:P4][Australis:M?] → [Australis:P2][Australis:M?]
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
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
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?]
Adding blocking bug 939862, since I checked the deps of that before filing the now-dupe bug 960052 :-)
Flags: needinfo?(mmaslaney)
Blocks: 960258
Assignee: nobody → mconley
Attached file downloadFinished-menuPanel.zip (obsolete) —
Assets attached.
Flags: needinfo?(mmaslaney)
Beautiful! Thanks mmaslaney. Over to me...
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)
Also note that the arrow is green on Windows instead of blue.
That's a good call. Thanks ge3k0s!
Status: NEW → ASSIGNED
Attached patch Patch v1 (obsolete) — Splinter Review
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.
Attached image downloadFinished-menuPanel.png (obsolete) —
Linux download button
Flags: needinfo?(mmaslaney)
Updated Download-Glow menu panel icons.
Attached patch Patch v1.1 (obsolete) — Splinter Review
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
Attached patch Patch v1.2 (obsolete) — Splinter Review
Attachment #8363951 - Attachment is obsolete: true
Comment on attachment 8363955 [details] [diff] [review]
Patch v1.2

Got some cycles to review this, mikedeboer?
Attachment #8363955 - Flags: review?(mdeboer)
Tested on OS X, Windows (XP, 7, 8), Ubuntu.
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+
Thanks for working on this, looks like we can it soon! :)
(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!
(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!
Attached patch Patch v1.3 (obsolete) — Splinter Review
Attachment #8363955 - Attachment is obsolete: true
Comment on attachment 8364479 [details] [diff] [review]
Patch v1.3

What do you think of this?
Attachment #8364479 - Flags: review?(mdeboer)
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-
(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.
(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!
(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/
Attached patch Patch v1.4 (obsolete) — Splinter Review
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
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 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+
Thanks, done!
Attachment #8365629 - Attachment is obsolete: true
Attachment #8365981 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/e82db1c2c4c9
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 29
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.