Closed Bug 815131 Opened 7 years ago Closed 7 years ago

Downloads Panel footer CSS needs consolidating

Categories

(Firefox :: Downloads Panel, defect)

x86
All
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 20

People

(Reporter: mconley, Assigned: alexbio2008)

Details

Attachments

(1 file, 2 obsolete files)

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
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.
Attached patch patch.diff (obsolete) — Splinter Review
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.
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.
Attached patch patch2.diff (obsolete) — Splinter Review
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
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]
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
Closed: 7 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>
(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.
(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.
You need to log in before you can comment on or make changes to this bug.