Note: There are a few cases of duplicates in user autocompletion which are being worked on.

Downloads Panel footer CSS needs consolidating

RESOLVED FIXED in Firefox 20

Status

()

Firefox
Downloads Panel
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: mconley, Assigned: alex)

Tracking

Trunk
Firefox 20
x86
All
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

The footer in the Downloads Panel contains two things - the Downloads Summary, and the "Show All Downloads" button.

There are one or two places where we apply a style rule to both the summary and the button, when we could just apply it to the footer (the background color comes to mind).

We should audit the CSS rules for the footer and its children, and then generalize the rules that could apply to the footer alone.
Alex contacted me about fixing this one - I'll be mentoring him.
Assignee: nobody → alexbio2008
Alex:

So basically, in the new downloads panel, we have a footer vbox, and it has some childnodes that could share a few rules.

For example, see:

https://mxr.mozilla.org/mozilla-central/source/browser/themes/gnomestripe/downloads/downloads.css#27

That could probably be changed to:

#downloadsPanel[hasdownloads] #downloadsFooter {
   border-top: 1px solid ThreeDShadow;
   background-image: -moz-linear-gradient(hsla(0,0%,0%,.15), hsla(0,0%,0%,.08) 6px);
}

There's a similar rule in pinstripe:

https://mxr.mozilla.org/mozilla-central/source/browser/themes/pinstripe/downloads/downloads.css#34

and winstripe:

https://mxr.mozilla.org/mozilla-central/source/browser/themes/winstripe/downloads/downloads.css#32

https://mxr.mozilla.org/mozilla-central/source/browser/themes/winstripe/downloads/downloads-aero.css#10

-Mike
(Assignee)

Comment 3

5 years ago
I changed in all these css files #downloadsHistory to #downloadsFooter.
Now what should I do next?
(In reply to alex from comment #3)
> I changed in all these css files #downloadsHistory to #downloadsFooter.
> Now what should I do next?

I'd like to see and comment on your changes.

Generate a diff with this command:

hg diff -p -U 8 > patch.diff

Then attach that patch.diff file to this bug.
(Assignee)

Comment 5

5 years ago
Created attachment 686132 [details] [diff] [review]
patch.diff
Comment on attachment 686132 [details] [diff] [review]
patch.diff

Thanks Alex!

Because we're using #downloadsFooter, we can now remove the #downloadsSummary, for each of these as well.
(Assignee)

Comment 7

5 years ago
you welcome.
should I remove the #downlaodsSummary?
Yes - so, for example, in here:

https://bugzilla.mozilla.org/attachment.cgi?id=686132&action=diff

On line 27 of browser/themes/gnomestripe/downloads/downloads.css, you can remove #downloadsSummary. Same goes for pin and winstripe.
(Assignee)

Comment 9

5 years ago
Created attachment 686149 [details] [diff] [review]
patch2.diff
Attachment #686132 - Attachment is obsolete: true
Comment on attachment 686149 [details] [diff] [review]
patch2.diff

Adding to my review queue.
Attachment #686149 - Flags: review?(mconley)
This looks good on Windows - testing Ubuntu and OSX next.
Looks good on Ubuntu! OSX is next - might take me an hour or so for my build to finish.
Comment on attachment 686149 [details] [diff] [review]
patch2.diff

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

This looks good across the board. Thanks Alex!
Attachment #686149 - Flags: review?(mconley) → review+
Keywords: checkin-needed
(Assignee)

Comment 14

5 years ago
You are welcome :).
Landed on mozilla-inbound as https://hg.mozilla.org/integration/mozilla-inbound/rev/fe8d7ec04fe1
Keywords: checkin-needed
Whiteboard: [good first bug][mentor=mconley][lang=css]
Created attachment 686759 [details] [diff] [review]
Patch landed on mozilla-inbound

I made one small correction to a rule before landing on inbound - just adding this here for bookkeeping.
Attachment #686149 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/fe8d7ec04fe1
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 20
(In reply to Mike Conley (:mconley) from comment #16)
> Created attachment 686759 [details] [diff] [review]
> Patch landed on mozilla-inbound
> 
> I made one small correction to a rule before landing on inbound

You replaced a child selector with an descendent selector. How exactly does this make sense? You should be using child selectors across the board. https://developer.mozilla.org/en/Writing_Efficient_CSS#Avoid_the_descendant_selector
These items are uniquely identified, can be only in one position, and have a single uniquely identified ancestor, the descendant selector should have basically the same performance as the child selector.  Did we decide to ban descendant selectors regardless their need? (this seems indeed the policy from the doc)
Ah downloadsFooter may indeed use the child selector here, it's a direct child of the panel. I thought this was about #downloadsHistory :/
(In reply to Marco Bonardo [:mak] from comment #19)
> the descendant selector should have
> basically the same performance as the child selector.

No. If #downloadsFooter's parent node (i.e. #downloadsPanel) doesn't have the hasdownloads attribute, the descendent selector will keep cycling through the grandparent nodes up to the root node and look for a match, whereas the child selector will always stop at #downloadsFooter's parent node.
(In reply to Dão Gottwald [:dao] from comment #21)
> No. If #downloadsFooter's parent node (i.e. #downloadsPanel) doesn't have
> the hasdownloads attribute, the descendent selector will keep cycling
> through the grandparent nodes up to the root node and look for a match,

That's true for tags and classes, since it can't predict when they will be matched. In this specific case, when the iterative process matches the id, since it's unique in the document, it stops.
I can see why it's safer to specify the rule as a global suggestion though, so if we want to be really explicit, nothing against that.
To be completely honest, I'm looking at the change I made and wondering why in the world I made it. Not very encouraging. :/

I'll push a follow-up patch to put the child selector back.
(In reply to Marco Bonardo [:mak] from comment #22)
> (In reply to Dão Gottwald [:dao] from comment #21)
> > No. If #downloadsFooter's parent node (i.e. #downloadsPanel) doesn't have
> > the hasdownloads attribute, the descendent selector will keep cycling
> > through the grandparent nodes up to the root node and look for a match,
> 
> That's true for tags and classes, since it can't predict when they will be
> matched. In this specific case, when the iterative process matches the id,
> since it's unique in the document, it stops.

nope:

data:text/html,<style>%23a[foo] span { color:red }</style><body><span id="a" foo><span id="a"><span>2</span></span></span>
Follow-up landed on mozilla-inbound as https://hg.mozilla.org/integration/mozilla-inbound/rev/69de9132d9e0
(In reply to Dão Gottwald [:dao] from comment #24)
> (In reply to Marco Bonardo [:mak] from comment #22)
> > (In reply to Dão Gottwald [:dao] from comment #21)
> > > No. If #downloadsFooter's parent node (i.e. #downloadsPanel) doesn't have
> > > the hasdownloads attribute, the descendent selector will keep cycling
> > > through the grandparent nodes up to the root node and look for a match,
> > 
> > That's true for tags and classes, since it can't predict when they will be
> > matched. In this specific case, when the iterative process matches the id,
> > since it's unique in the document, it stops.
> 
> nope:
> 
> data:text/html,<style>%23a[foo] span { color:red }</style><body><span id="a"
> foo><span id="a"><span>2</span></span></span>

Well, this test case didn't really proof much. Here's a better one:

data:text/html,<style>%23a[foo] %23b { color:red }</style><body><span id="a" foo><div id="a"><span id="b">2</span></span></span>

Basically, ids work just like classes as far as CSS is concerned.
(In reply to Mike Conley (:mconley) from comment #25)
> Follow-up landed on mozilla-inbound as
> https://hg.mozilla.org/integration/mozilla-inbound/rev/69de9132d9e0

Are you going to fix browser/themes/winstripe/downloads/downloads.css, browser/themes/pinstripe/downloads/downloads.css and browser/themes/gnomestripe/downloads/downloads.css as well?
(In reply to Dão Gottwald [:dao] from comment #27)
> (In reply to Mike Conley (:mconley) from comment #25)
> > Follow-up landed on mozilla-inbound as
> > https://hg.mozilla.org/integration/mozilla-inbound/rev/69de9132d9e0
> 
> Are you going to fix browser/themes/winstripe/downloads/downloads.css,
> browser/themes/pinstripe/downloads/downloads.css and
> browser/themes/gnomestripe/downloads/downloads.css as well?

Good lord, did I use descendent selectors there as well?

I'm going to file a new bug.
Filed bug 817584
(In reply to Dão Gottwald [:dao] from comment #26)
> data:text/html,<style>%23a[foo] %23b { color:red }</style><body><span id="a"
> foo><div id="a"><span id="b">2</span></span></span>
> 
> Basically, ids work just like classes as far as CSS is concerned.

ugh, that's unfortunate perf-wise, I wonder if it's done for compatibility mode, since would be a really trivial optimization to do. Thanks for the enlightening test-case btw.
https://hg.mozilla.org/mozilla-central/rev/69de9132d9e0
You need to log in before you can comment on or make changes to this bug.