827015 - Cleanup Clear Downloads button theming

Status: VERIFIED FIXED

(Firefox :: Downloads Panel, defect, P1)

Description

[Marco Bonardo [:mak]]

The library Clear Downloads button likely needs some theming love, an icon at least.

Comment 1

[Marco Bonardo [:mak]]

Mike, I assigned this to you but forgot to notify you about that, please let me know if you can't look at this.

Comment 2

[Mike Conley (:mconley) (:⚙️)]

(In reply to Marco Bonardo [:mak] from comment #1)
> Mike, I assigned this to you but forgot to notify you about that, please let
> me know if you can't look at this.

I should be OK to do this.

Comment 3

[Mike Conley (:mconley) (:⚙️)]

Attachment: OSX - Before patch

Comment 4

[Marco Bonardo [:mak]]

If possible, I'd like to have this in the tree by EOD, I'm starting asking approvals for uplifting patches, I think the most offending bugs will make inbound today, so we may re-enable the panel for everyone in the next days. The current theming on OSX is quite bad thus making this more important.

Comment 5

[Mike Conley (:mconley) (:⚙️)]

Attachment: WIP Patch 1

Checkpointing patch. Got it working on pinstripe - screenshot coming.

Comment 6

[Mike Conley (:mconley) (:⚙️)]

Attachment: OSX - After patch

Comment 7

[Mike Conley (:mconley) (:⚙️)]

Attachment: Ubuntu - Before patch

gnomestripe looks OK. I don't think we'll need to make changes there.

Comment 8

[Mike Conley (:mconley) (:⚙️)]

Attachment: Patch 1

Fixes for winstripe and gnomestripe (I needed to hide the icon).

Comment 9

[Mike Conley (:mconley) (:⚙️)]

Attachment: Windows 7 - Before patch

Comment 10

[Mike Conley (:mconley) (:⚙️)]

Attachment: Windows 7 - After patch

Comment 11

[Mike Conley (:mconley) (:⚙️)]

Attachment: Windows XP - Before patch

Comment 12

[Mike Conley (:mconley) (:⚙️)]

Attachment: Windows XP - After patch

Comment 13

[Mike Conley (:mconley) (:⚙️)]

Attachment: Ubuntu - After patch

I did make a subtle change to the Ubuntu button, so I'll refresh my screenshots there.

Comment 14

[Marco Bonardo [:mak]]

Comment on attachment 700046 [details] [diff] [review]
Patch 1

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

::: browser/themes/gnomestripe/places/organizer.css
@@ +93,5 @@
> +/**
> + * Downloads pane
> + */
> +
> +/* Clear Downloads button */

considered the id of the element (clearDownloadsButton), this additional comment doesn't seem to add anything, I'd remove it across all of the patch

@@ +95,5 @@
> + */
> +
> +/* Clear Downloads button */
> +
> +#placesToolbar > #clearDownloadsButton > .toolbarbutton-icon {

you should not need "#placesToolbar > ", #clearDownloadsButton is unique and already uses child selecton on .toolbarbutton-icon
Same across all of the patch.

::: browser/themes/winstripe/places/organizer.css
@@ +139,5 @@
> +}
> +
> +#clearDownloadsButton {
> +  -moz-padding-start: 8px;
> +  -moz-padding-end: 8px;

are these consistent with the other "buttons" in the toolbar? They seem to have a 4px padding, not 8px
http://mxr.mozilla.org/mozilla-central/source/browser/themes/winstripe/places/organizer.css#41
Though could be there is some additional padding I didn't see...

Comment 15

[Mike Conley (:mconley) (:⚙️)]

Attachment: Patch v2

(In reply to Marco Bonardo [:mak] from comment #14)
> Comment on attachment 700046 [details] [diff] [review]
> Patch 1
> 
> Review of attachment 700046 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/themes/gnomestripe/places/organizer.css
> @@ +93,5 @@
> > +/**
> > + * Downloads pane
> > + */
> > +
> > +/* Clear Downloads button */
> 
> considered the id of the element (clearDownloadsButton), this additional
> comment doesn't seem to add anything, I'd remove it across all of the patch
> 

Good point. Removed.

> @@ +95,5 @@
> > + */
> > +
> > +/* Clear Downloads button */
> > +
> > +#placesToolbar > #clearDownloadsButton > .toolbarbutton-icon {
> 
> you should not need "#placesToolbar > ", #clearDownloadsButton is unique and
> already uses child selecton on .toolbarbutton-icon
> Same across all of the patch.
> 

Alright, but I had to keep one #placesToolbar in for pinstripe to increase specificity in removing the list-style-image defaulting on #placesToolbar > toolbarbutton.

> ::: browser/themes/winstripe/places/organizer.css
> @@ +139,5 @@
> > +}
> > +
> > +#clearDownloadsButton {
> > +  -moz-padding-start: 8px;
> > +  -moz-padding-end: 8px;
> 
> are these consistent with the other "buttons" in the toolbar? They seem to
> have a 4px padding, not 8px
> http://mxr.mozilla.org/mozilla-central/source/browser/themes/winstripe/
> places/organizer.css#41
> Though could be there is some additional padding I didn't see...

So, this is my fault for writing this patch in a hurry while I was running out the door; it looks like the padding is actually 9px on Windows;

padding-end is 1px from http://mxr.mozilla.org/mozilla-central/source/browser/themes/winstripe/places/organizer.css#42

+ padding-end for the label, from:

http://mxr.mozilla.org/mozilla-central/source/browser/themes/winstripe/places/organizer.css#70

So, with symmetry, that's 9px padding on either end to match the appearance of the menu buttons.

Comment 16

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 700425 [details] [diff] [review]
Patch v2

Follow-up to my last comment - for winstripe, we *could* match the padding of the button with the padding on the back/forth buttons, but I have to say, it looks a bit strange for the Clear Downloads to be more "squeezed in" than the menu-button directly to its left. That's why I opted to match the menu-button's padding.

Comment 17

[Marco Bonardo [:mak]]

Comment on attachment 700425 [details] [diff] [review]
Patch v2

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

Thanks for verifying the padding

::: browser/themes/pinstripe/places/organizer.css
@@ +291,5 @@
> +/**
> + * Downloads pane
> + */
> +
> +#placesToolbar > #clearDownloadsButton {

crazy css precedences, this is likely expected cause the original rule has id+element and wins over id... oh well, the original rule should have used a class probably... ok.

Comment 18

[Mike Conley (:mconley) (:⚙️)]

Landed on mozilla-inbound as https://hg.mozilla.org/integration/mozilla-inbound/rev/386676422536

Comment 19

[Dão Gottwald [:dao]]

(In reply to Marco Bonardo [:mak] from comment #17)
> > +#placesToolbar > #clearDownloadsButton {
> 
> crazy css precedences, this is likely expected cause the original rule has
> id+element and wins over id... oh well, the original rule should have used a
> class probably... ok.

Please file a bug on fixing this. Adding redundant bloat to selectors isn't very maintainable. See also bug 813802 comment 53.

Comment 20

[Marco Bonardo [:mak]]

(In reply to Dão Gottwald [:dao] from comment #19)
> Please file a bug on fixing this. Adding redundant bloat to selectors isn't
> very maintainable. See also bug 813802 comment 53.

Yes, I think we need some cleanup of the Library styling and contents (this crazy stuff with menus and toolbars on different platforms...). I will file a bug to cleanup Library styling.

Comment 21

[Marco Bonardo [:mak]]

filed bug 829067. If we want to fix this specific case with !important fine by me, I don't think it's that important for now, but if you have strong feelings about that...

Comment 22

[Marco Bonardo [:mak]]

Comment on attachment 700425 [details] [diff] [review]
Patch v2

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Downloads panel feature
User impact if declined: incomplete ui
Testing completed (on m-c, etc.): m-i (merge pending)
Risk to taking this patch (and alternatives if risky): limited to the feature
String or UUID changes made by this patch: none

Comment 23

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/386676422536

Comment 24

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/386676422536

Comment 25

[Marco Bonardo [:mak]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/70e9e1d7f6fc

Comment 26

[Simona Badau, Desktop QA]

Verified as fixed on the latest Nightly and Aurora - Clear Downloads button looks like in Comments 6,10,12 and 13.

Verified on Windows 7, Windows XP, Ubuntu 12.04 and Mac OS X 10.8:

Mozilla/5.0 (Windows NT 6.1; rv:21.0) Gecko/20130128 Firefox/21.0 Build ID:20130128030943
Mozilla/5.0 (Windows NT 6.1; rv:20.0) Gecko/20130128 Firefox/20.0 Build ID:20130128042018	


Mozilla/5.0 (Windows NT 5.1; rv:21.0) Gecko/20130129 Firefox/21.0 Build ID:20130129030851
Mozilla/5.0 (Windows NT 5.1; rv:20.0) Gecko/20130128 Firefox/20.0 Build ID:20130128042018

Mozilla/5.0 (X11; Linux i686; rv:21.0) Gecko/20130128 Firefox/21.0 Build ID:20130128030943
Mozilla/5.0 (X11; Linux i686; rv:20.0) Gecko/20130123 Firefox/20.0 Build ID:20130128042018

Mozilla/5.0 (Macintosh; Intel Mac OS X 10.8; rv:21.0) Gecko/20130128 Firefox/21.0	Build ID:20130128030943
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.8; rv:20.0) Gecko/20130128 Firefox/20.0	Build ID:20130128042018