Last Comment Bug 815131 - Downloads Panel footer CSS needs consolidating
: Downloads Panel footer CSS needs consolidating
Status: RESOLVED FIXED
:
Product: Firefox
Classification: Client Software
Component: Downloads Panel (show other bugs)
: Trunk
: x86 All
: -- normal (vote)
: Firefox 20
Assigned To: alex
:
: :Paolo Amadini
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-11-26 06:58 PST by Mike Conley (:mconley)
Modified: 2012-12-03 16:21 PST (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch.diff (2.84 KB, patch)
2012-11-28 08:53 PST, alex
no flags Details | Diff | Splinter Review
patch2.diff (2.88 KB, patch)
2012-11-28 09:17 PST, alex
mconley: review+
Details | Diff | Splinter Review
Patch landed on mozilla-inbound (2.82 KB, patch)
2012-11-29 12:58 PST, Mike Conley (:mconley)
no flags Details | Diff | Splinter Review

Description Mike Conley (:mconley) 2012-11-26 06:58:16 PST
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.
Comment 1 Mike Conley (:mconley) 2012-11-28 07:27:04 PST
Alex contacted me about fixing this one - I'll be mentoring him.
Comment 2 Mike Conley (:mconley) 2012-11-28 08:18:06 PST
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
Comment 3 alex 2012-11-28 08:43:32 PST
I changed in all these css files #downloadsHistory to #downloadsFooter.
Now what should I do next?
Comment 4 Mike Conley (:mconley) 2012-11-28 08:47:35 PST
(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.
Comment 5 alex 2012-11-28 08:53:06 PST
Created attachment 686132 [details] [diff] [review]
patch.diff
Comment 6 Mike Conley (:mconley) 2012-11-28 08:55:10 PST
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.
Comment 7 alex 2012-11-28 09:01:00 PST
you welcome.
should I remove the #downlaodsSummary?
Comment 8 Mike Conley (:mconley) 2012-11-28 09:02:24 PST
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.
Comment 9 alex 2012-11-28 09:17:50 PST
Created attachment 686149 [details] [diff] [review]
patch2.diff
Comment 10 Mike Conley (:mconley) 2012-11-28 10:42:07 PST
Comment on attachment 686149 [details] [diff] [review]
patch2.diff

Adding to my review queue.
Comment 11 Mike Conley (:mconley) 2012-11-28 11:49:07 PST
This looks good on Windows - testing Ubuntu and OSX next.
Comment 12 Mike Conley (:mconley) 2012-11-28 11:55:24 PST
Looks good on Ubuntu! OSX is next - might take me an hour or so for my build to finish.
Comment 13 Mike Conley (:mconley) 2012-11-28 13:47:15 PST
Comment on attachment 686149 [details] [diff] [review]
patch2.diff

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

This looks good across the board. Thanks Alex!
Comment 14 alex 2012-11-29 04:48:56 PST
You are welcome :).
Comment 15 Mike Conley (:mconley) 2012-11-29 12:55:50 PST
Landed on mozilla-inbound as https://hg.mozilla.org/integration/mozilla-inbound/rev/fe8d7ec04fe1
Comment 16 Mike Conley (:mconley) 2012-11-29 12:58:43 PST
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.
Comment 17 Ed Morley [:emorley] 2012-11-30 02:14:45 PST
https://hg.mozilla.org/mozilla-central/rev/fe8d7ec04fe1
Comment 18 Dão Gottwald [:dao] 2012-12-01 08:35:45 PST
(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
Comment 19 Marco Bonardo [::mak] 2012-12-01 10:09:38 PST
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)
Comment 20 Marco Bonardo [::mak] 2012-12-01 10:11:38 PST
Ah downloadsFooter may indeed use the child selector here, it's a direct child of the panel. I thought this was about #downloadsHistory :/
Comment 21 Dão Gottwald [:dao] 2012-12-01 13:07:35 PST
(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.
Comment 22 Marco Bonardo [::mak] 2012-12-03 01:48:56 PST
(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.
Comment 23 Mike Conley (:mconley) 2012-12-03 06:45:24 PST
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.
Comment 24 Dão Gottwald [:dao] 2012-12-03 06:47:52 PST
(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>
Comment 25 Mike Conley (:mconley) 2012-12-03 07:00:40 PST
Follow-up landed on mozilla-inbound as https://hg.mozilla.org/integration/mozilla-inbound/rev/69de9132d9e0
Comment 26 Dão Gottwald [:dao] 2012-12-03 07:01:47 PST
(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.
Comment 27 Dão Gottwald [:dao] 2012-12-03 07:02:59 PST
(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?
Comment 28 Mike Conley (:mconley) 2012-12-03 07:04:44 PST
(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.
Comment 29 Mike Conley (:mconley) 2012-12-03 07:21:12 PST
Filed bug 817584
Comment 30 Marco Bonardo [::mak] 2012-12-03 07:58:23 PST
(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.
Comment 31 Ryan VanderMeulen [:RyanVM] 2012-12-03 16:21:44 PST
https://hg.mozilla.org/mozilla-central/rev/69de9132d9e0

Note You need to log in before you can comment on or make changes to this bug.