870309 - Clean up customizableui CSS files

Status: VERIFIED FIXED

(Firefox :: Toolbars and Customization, defect)

Description

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Our CSS files are a bit of a mess.... with most stuff defined in panelUIOverlay.css, which should be very minimal (it should only have styles for the button, as that's the only thing added in the overlay). Everything to do with the panel should be in panelUI.css.

Comment 1

[Mike Conley (:mconley) (:⚙️)]

Hey Blair - assigning this to you for M6. Let me know if it should be re-assigned.

Comment 2

[Mike Conley (:mconley) (:⚙️)]

Removing the items from M7 that do not block landing on m-c.

Comment 3

[Mike de Boer [:mikedeboer]]

Blair, I think this has come down to just renaming panelUIOverlay.(inc.)css to just panelUI.(inc.)css. Or do you see more areas for improvement?

Comment 4

[Mike de Boer [:mikedeboer]]

In any case, I would like to come up with a list of possible improvements so we may take this bug on in a structured manner.

Comment 5

[Blair McBride [:Unfocused] (UNAVAILABLE)]

(In reply to Mike de Boer [:mikedeboer] from comment #3)
> Blair, I think this has come down to just renaming panelUIOverlay.(inc.)css
> to just panelUI.(inc.)css. Or do you see more areas for improvement?

Yea, since we got rid of the overlay to add the button to in-content pages, that's enough. 

Though, you'll also want to remove the rules for #app-extension-point-end, which should have been removed when we removed the overlay.

Comment 6

[Manish Goregaokar [:manishearth]]

Assigning to Aayush as per in-person request at the IITKGP MozSetup event.

Comment 7

[Aayush]

Attachment: patch v1

Can you go through the changes I made?

Comment 8

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 8399083 [details] [diff] [review]
patch v1

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

Wrong patch :) This is the patch for bug 982735.

Comment 9

[Manish Goregaokar [:manishearth]]

Attachment: panel.patch

Uh oh, this is my fault. I was helping Aayush upload the patch from my laptop (I had scp'd it across from the build machine), and I guess I picked Sukanta's patch while uploading. Whoops.

Uploading it myself since I'm not sure if Aayush has the original patch with him. The metadata is associated with him, though.

Comment 10

[:Gijs (he/him)]

Comment on attachment 8399255 [details] [diff] [review]
panel.patch

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

This is the right direction, but:

1) this misses the OS X include change
2) this misses the removal of the app-extension point rules here: 
https://mxr.mozilla.org/mozilla-central/source/browser/themes/shared/customizableui/panelUIOverlay.inc.css#138

(all of the app-extension-point things are dead)

Comment 11

[Manish Goregaokar [:manishearth]]

Aayush, are you still working on this? Sorry about the delay in getting review, if necessary I can help you through putting up an improved patch.

Comment 12

[Nikhil Handa[:slayslot]]

Hello, I would like to help with this, if I can.

Comment 13

[[:Towkir] Ahmed]

(In reply to :Gijs Kruitbosch from comment #10)
> Comment on attachment 8399255 [details] [diff] [review]
> panel.patch
> 
> Review of attachment 8399255 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This is the right direction, but:
> 
> 1) this misses the OS X include change
> 2) this misses the removal of the app-extension point rules here: 
> https://mxr.mozilla.org/mozilla-central/source/browser/themes/shared/
> customizableui/panelUIOverlay.inc.css#138
> 
> (all of the app-extension-point things are dead)

Hi, I would like to work with this one, 
Looks like the extension point matter is already out of way, now what left is just to rename those files, If I am not wrong, may be I can submit a patch, and If I am wrong, what exactly should be done here ?

Comment 14

[[:Towkir] Ahmed]

beside, did you mention this line https://dxr.mozilla.org/mozilla-central/source/browser/themes/shared/customizableui/panelUIOverlay.inc.css#138 on comment 10 ? (as mxr is now broken).

Comment 15

[:Gijs (he/him)]

(In reply to [:Towkir] Ahmed from comment #14)
> beside, did you mention this line
> https://dxr.mozilla.org/mozilla-central/source/browser/themes/shared/
> customizableui/panelUIOverlay.inc.css#138 on comment 10 ? (as mxr is now
> broken).

No, the code that I referenced in comment #10 is already gone. We just want to rename the files, AIUI.

Comment 16

[[:Towkir] Ahmed]

Just to make sure before submitting patch,
https://dxr.mozilla.org/mozilla-central/search?q=panelUIOverlay&redirect=false
should be done according to comment 5 right ?

Comment 17

[:Gijs (he/him)]

(In reply to [:Towkir] Ahmed from comment #16)
> Just to make sure before submitting patch,
> https://dxr.mozilla.org/mozilla-central/
> search?q=panelUIOverlay&redirect=false
> should be done according to comment 5 right ?

Yes.

Comment 18

[[:Towkir] Ahmed]

Attachment: customizableuimodification.patch

Looks like the files in the "obj-x86_64-pc-linux-gnu" don't get added in the patch.
Please assign this to me if this looks good.
Thanks

Comment 19

[:Gijs (he/him)]

Comment on attachment 8772793 [details] [diff] [review]
customizableuimodification.patch

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

You should use "hg move" instead of copying and add/removing the file.

Comment 20

[[:Towkir] Ahmed]

Attachment: customizableUImodification.patch

Hope this looks good :)
And thanks for the hg rename tips, I was looking for something like this but discovered it a late, earlier google searches were of no use :p

Comment 21

[:Gijs (he/him)]

Comment on attachment 8772810 [details] [diff] [review]
customizableUImodification.patch

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

Please provide a better commit message that describes what the patch does.

You've also updated all the references to panelUIOverlay.inc.css to point to panelUI.inc.css but not renamed that file.

Comment 22

[[:Towkir] Ahmed]

Attachment: customizableUImodification.patch

Sorry about that, how could I miss this !!!

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/fx-team/rev/5f4846ed9f65
Shift to panelUI.css from panelUIOverlay.css in customizableui CSS files. r=gijs

Comment 24

[[:Towkir] Ahmed]

As per comment 23 ...

Comment 25

[:Gijs (he/him)]

(In reply to [:Towkir] Ahmed from comment #24)
> As per comment 23 ...

No, this is only fixed when it lands on central. It will (semi-)automatically get marked fixed at that point.

Comment 26

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/5f4846ed9f65

Comment 27

[Alfred Kayser]

It could also have been merged/included into browser.css?
That would save loading a separate style sheet on startup.

Comment 28

[:Gijs (he/him)]

(In reply to Alfred Kayser from comment #27)
> It could also have been merged/included into browser.css?
> That would save loading a separate style sheet on startup.

and it would have made browser.css even bigger / more of a dumping ground. If anything, more files rather than fewer seems the sensible approach here, to try and keep separate rules separate and have it obvious where particular bits belong. If there are perf gains in concatenating the files (seems doubtful as it's all chrome:// and preloaded anyway, and IIRC we tried this when looking for australis perf impact), by all means file a followup for concatenating at build-time - but it should probably wait for the resolution of bug 1288747. We're done in this bug.