Closed Bug 827015 Opened 7 years ago Closed 7 years ago

Cleanup Clear Downloads button theming

Categories

(Firefox :: Downloads Panel, defect, P1)

defect

Tracking

()

VERIFIED FIXED
Firefox 21
Tracking Status
firefox20 --- verified

People

(Reporter: mak, Assigned: mconley)

References

Details

Attachments

(8 files, 3 obsolete files)

The library Clear Downloads button likely needs some theming love, an icon at least.
Depends on: 822572
Flags: needinfo?(shorlander)
Priority: -- → P2
Assignee: nobody → mconley
Mike, I assigned this to you but forgot to notify you about that, please let me know if you can't look at this.
Flags: needinfo?(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.
Flags: needinfo?(mconley)
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.
Priority: P2 → P1
Attached patch WIP Patch 1 (obsolete) — Splinter Review
Checkpointing patch. Got it working on pinstripe - screenshot coming.
Attached image Ubuntu - Before patch (obsolete) —
gnomestripe looks OK. I don't think we'll need to make changes there.
Attached patch Patch 1 (obsolete) — Splinter Review
Fixes for winstripe and gnomestripe (I needed to hide the icon).
Attachment #700037 - Attachment is obsolete: true
Attached image Ubuntu - After patch
I did make a subtle change to the Ubuntu button, so I'll refresh my screenshots there.
Attachment #700040 - Attachment is obsolete: true
Attachment #700046 - Attachment description: WIP Patch 2 → Patch 1
Attachment #700046 - Attachment filename: 827015-wip-2.diff → 827015-v1.diff
Attachment #700046 - Flags: review?(mak77)
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...
Attachment #700046 - Flags: review?(mak77)
Attached patch Patch v2Splinter Review
(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.
Attachment #700046 - Attachment is obsolete: true
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.
Attachment #700425 - Flags: review?(mak77)
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.
Attachment #700425 - Flags: review?(mak77) → review+
(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.
(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.
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 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
Attachment #700425 - Flags: approval-mozilla-aurora?
Flags: needinfo?(shorlander)
https://hg.mozilla.org/mozilla-central/rev/386676422536
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 21
Attachment #700425 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Keywords: icon
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
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.