Open Bug 877697 Opened 8 years ago Updated 5 years ago

panelUIOverlay.inc.css uses far too many descendant selectors

Categories

(Firefox :: Theme, defect)

x86
All
defect
Not set
normal

Tracking

()

People

(Reporter: mconley, Unassigned)

References

(Blocks 1 open bug)

Details

(Whiteboard: [Australis:P5])

I'm concerned about the high number of descendant selectors we have in browser/themes/shared/panelUIOverlay.inc.css.

https://developer.mozilla.org/en/Writing_Efficient_CSS tells us that descendant selectors are very expensive, and we should try writing something more specific.
Alternatively, could we use a scoped stylesheet here? That should make the impact of descendent selectors negligible.
Not taking for Australis:M7.
Whiteboard: [Australis:M?] → [Australis:M?][Australis:P5]
Whiteboard: [Australis:M?][Australis:P5] → [Australis:M?][Australis:P5] [feature] p=0
No longer blocks: fxdesktopbacklog
Whiteboard: [Australis:M?][Australis:P5] [feature] p=0 → [Australis:M?][Australis:P5]
Whiteboard: [Australis:M?][Australis:P5] → [Australis:P5]
Mentor: manishearth
Whiteboard: [Australis:P5] → [Australis:P5][good first bug]
Whiteboard: [Australis:P5][good first bug] → [Australis:P5][good first bug][lang=css]
Manish I'm new and trying to get involved. Can I start working on this bug?
hi! I'm new here , Please assign this bug to me!
(In reply to Abhirav Kariya from comment #4)
> hi! I'm new here , Please assign this bug to me!

I think the new policy is to start work on this bug before we mark it as assigned. Also, Sean might be working on it (sorry Sean, I missed your comment). If so, you might want to pick a new bug.

Anyway, if you start working on this and put up a simple patch, I can mark it as assigned. Thanks for your interest!

Let me know if you need help.
I wanted to work on it, yes. I was waiting for a reply before I jumped in and worked on it. I didn't want to step on toes.

Thanks
(In reply to Sean Zaferopolos from comment #6)
> I wanted to work on it, yes. I was waiting for a reply before I jumped in
> and worked on it. I didn't want to step on toes.

Sorry about that, sure, start working on it :)

Abhrav, you might want to work on this bug: https://bugzilla.mozilla.org/show_bug.cgi?id=908954
panelUIOverlay.inc.css. location? the location in description is wrong.
help me patch this bug as im new to all this!!
(In reply to Abhirav Kariya from comment #10)
> help me patch this bug as im new to all this!!

Currently Sean is working on this, maybe pick a different bug? Bugsahoy might help: www.joshmatthews.net/bugsahoy/
ok
hello Manish
I am new at this.
I would like to work on this bug.
Manish, 
This has been on my to-do for a while and keeps getting pushed back. Someone else can do it. I will try again some other time.
Assignee: nobody → gopalmeena94
I have looked through the file.
I don't know where we have to change.
I need some examples.
http://mxr.mozilla.org/mozilla-central/source/browser/themes/shared/customizableui/panelUIOverlay.inc.css

https://developer.mozilla.org/en-US/docs/Web/Guide/CSS/Writing_efficient_CSS#Avoid_the_descendant_selector.21

Firstly, notice that we have a lot of descendent selectors to .panelUI-grid. This is for all the toolbar buttons in the dropdown panel. Perhaps moving those selectors to a scoped stylesheet there (<style scoped>) would be better?

Also, we have cases like this: http://mxr.mozilla.org/mozilla-central/source/browser/themes/shared/customizableui/panelUIOverlay.inc.css#376 where there are a lot of descendent selectors using ids.

Enabling the Browser Toolbox in your build and poking around should help, since you can tweak the browser-level CSS.
In cases like these[1] it might make sense to give the .toolbar-icon in question its own class, and just use `.panelUI-grid .badge-toolbaricon`.

Most of the descendent selectors here can be fixed by splitting the file into three; one for the palette, one for the panel, and one for everything else, and putting the palette/panel ones as scoped stylesheets in their respective locations.


 [1]: http://mxr.mozilla.org/mozilla-central/source/browser/themes/shared/customizableui/panelUIOverlay.inc.css#430
@Gopal do you still have any questions? Let me know if that's the case. If you need help you can ask me or :jaws in IRC. Thanks!
@ Manish  , sir i have exams now. i will do it after 6-7 days
Gopal, are you still working on this bug?
Flags: needinfo?(gopalmeena94)
No response, resetting.
Assignee: gopalmeena94 → nobody
Hey, I would like to work on this bug......
(In reply to Akilan Elango [:falcon] from comment #22)
> Hey, I would like to work on this bug......

Awesome! Do you have your environment set up? If not, see https://developer.mozilla.org/en/docs/Simple_Firefox_build

The file you need to update is http://mxr.mozilla.org/mozilla-central/source/browser/themes/shared/customizableui/panelUIOverlay.inc.css, according to the instructions in comment 0 and comment 16. Please let me know if you have any further questions!
Flags: needinfo?(gopalmeena94)
Assignee: nobody → nanonikhil
Can you please post the links with updated line numbers because I think that the line numbers you were referring to previously have changed.
Also, in which files should scoped stylesheet be included? Can you include an example?
Flags: needinfo?(manishearth)
Flags: needinfo?(jaws)
Stuff like http://mxr.mozilla.org/mozilla-central/source/browser/themes/shared/customizableui/panelUIOverlay.inc.css#412 . It's all over the file. `>` is a descendent selector.

The file handles the code for the panel shown by the hamburger dropdown, IIRC.
Flags: needinfo?(manishearth)
Flags: needinfo?(jaws) → needinfo?
(In reply to Manish Goregaokar [:manishearth] from comment #26)
> Stuff like
> http://mxr.mozilla.org/mozilla-central/source/browser/themes/shared/
> customizableui/panelUIOverlay.inc.css#412 . It's all over the file. `>` is a
> descendent selector.

Whoa, slow down. No, ">" is not a descendant selector.

"#foo #bar"

is using a descendant selector ("match if the element matches #bar, and any of its ancestors matches #foo")

"#foo > #bar"

is using a child selector ("match if the element matches #bar, and its parent matches #foo")
Flags: needinfo?
Oh, sorry, got confused there for a bit :s Thanks for the save.
Sorry but I'm still a bit confused. Can you please provide a small patch to demonstrate a change so that I can follow it and proceed accordingly? Thanks.
(In reply to Nikhil Gupta from comment #29)
> Sorry but I'm still a bit confused. Can you please provide a small patch to
> demonstrate a change so that I can follow it and proceed accordingly? Thanks.

Looking at this file again, I think this shouldn't be a good first bug. Different instances of problematic selectors will require different solutions here, and that will take some knowledge of CSS and the ability to look at the selectors on a case-by-case basis, by yourself. It isn't possible for me to tell you "fix this selector this way" and then for you to fix all the other selectors the same way.

Would you consider working on bug 1178023 instead? There is already a suggestion there for what needs to change, you would just need to create an actual patch there and flag me for review. I'll needinfo you on that bug instead of here.
Assignee: nanonikhil → nobody
Whiteboard: [Australis:P5][good first bug][lang=css] → [Australis:P5][lang=css]
Hi, I am new to fixing bugs and I would like to work on the bug please.
After some discussion, this is probably not as easy as we thought it was initially.

Sorry about that.
Mentor: manishearth
Whiteboard: [Australis:P5][lang=css] → [Australis:P5]
You need to log in before you can comment on or make changes to this bug.