1030644 - Remove styling for .menulist-menupopup and .menulist-compact

Status: RESOLVED FIXED

(Toolkit :: Themes, defect)

Description

[Dão Gottwald [:dao]]

We have a bunch of CSS for "menulist-menupopup" and "menulist-compact" that are neither used on mozilla-central nor documented on developer.mozilla.org. Seems like we should get rid of this.

Comment 1

[Nootan Ghimire]

Want to work on it as simple introductory bugfix. (I did some one-or-two simple fixes some long time ago, and need to refresh myself again) :D . Can anyone help me do this? 

Thank you

Comment 2

[Dão Gottwald [:dao]]

I can help you.

Do you have the source code?

You'll find the relevant files here:

http://mxr.mozilla.org/mozilla-central/search?string=menulist-menupopup

and here:

http://mxr.mozilla.org/mozilla-central/search?string=menulist-compact

Comment 3

[Nootan Ghimire]

Unfortunately no, I do not have the source code. I remember pulling mercurial repo earlier, Can you help me locate where is it? Thanks!

Comment 4

[Dão Gottwald [:dao]]

Please see https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Source_Code/Mercurial

I'll be away for the rest of the weekend, will be back on Monday!

Comment 5

[Nootan Ghimire]

:dao : Can I start working by cloning only the last 100 revs? `hg clone --rev 100 https://hg.mozilla.org/mozilla-central firefox` 

I'm having a hard time cloning everything! :/ (Blame my internet connection and frequent power outages)

Comment 6

[Dão Gottwald [:dao]]

(In reply to nootan.ghimire from comment #5)
> :dao : Can I start working by cloning only the last 100 revs? `hg clone
> --rev 100 https://hg.mozilla.org/mozilla-central firefox` 
> 
> I'm having a hard time cloning everything! :/ (Blame my internet connection
> and frequent power outages)

You can use a bundle instead of hg clone:

https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Source_Code/Mercurial/Bundles

Comment 7

[Sudheesh Singanamalla (:ShellHacker)]

We are having a code sprint event to get new contributors started and I would like to assign this bug to Rahul who is willing to work on this at the event. Sorry to hijack the bug from you nootan.

Comment 8

[Rahul Jain]

Attachment: patch.v1

Removed the extra classes

Comment 9

[Stefan [:stefanh]]

Comment on attachment 8485437 [details] [diff] [review]
patch.v1

(This is just a drive-by comment and I'm not a reviewer)

-.menulist-menupopup > menuitem,
-menulist > menupopup > menuitem,
-.menulist-menupopup > menu,
-menulist > menupopup > menu

It looks like you remove too much here :-) You only want to remove the ".menulist-menupopup" (happens at several places).


-menulist:focus:not([open="true"]):not(.menulist-compact) > .menulist-label-box {
-  background-color: Highlight;
-  color: HighlightText;
-}

When the menulist is focused (but not opened),you still want the .menulist-label-box to be highlighted, but the ":not(.menulist-compact)" part can of course be removed (happens in at least 2 places).

Comment 10

[Rahul Jain]

Attachment: patch.v2

How about now?

Comment 11

[Stefan [:stefanh]]

(In reply to Rahul Jain from comment #10)
> Created attachment 8485466 [details] [diff] [review]
> patch.v2
> 
> How about now?

Yep, much better :-)

Comment 12

[Sudheesh Singanamalla (:ShellHacker)]

Comment on attachment 8485466 [details] [diff] [review]
patch.v2

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

Looks pretty good. Good work Rahul. Waiting for someone else to review this :)

Comment 13

[Dão Gottwald [:dao]]

Comment on attachment 8485466 [details] [diff] [review]
patch.v2

Please remove the /* ::::: compact menulists ::::: */ comment (appears four times in this patch). Looks good otherwise, thanks!

Comment 14

[Rahul Jain]

Attachment: Patch_1030644_v3.patch

Does this look good now ?

Comment 15

[Dão Gottwald [:dao]]

Comment on attachment 8487426 [details] [diff] [review]
Patch_1030644_v3.patch

perfect, thanks

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/bc10a5de560c

Comment 17

[[e-mail]]

(In reply to Dão Gottwald from comment #0)
> We have a bunch of CSS for "menulist-menupopup" and "menulist-compact" that
> are neither used on mozilla-central nor documented on developer.mozilla.org.
> Seems like we should get rid of this.

On the other hand they *are* used extensively in Thunderbird and SeaMonkey Mail... perhaps we should get comm-central merged into mozilla-central, then people would be able spot things like this...

Comment 18

[Kent James (:rkent)]

I really hate to be the bearer of bad news here, but this bug should not have been initiated in the first place without considering the impact on comm-central. If there is some reason why this change is really important to the Firefox world, therefore justifying having our tiny team of Thunderbird and Seamonkey developers try to develop workarounds, then I suppose we can do that. But so far the justification for this bug "neither used on mozilla-central nor documented on developer.mozilla.org" does not seem to justify those workarounds.

Could you please back this bug out?

I'm really sorry Raul that you got caught up in this. You did a great job here, it is we who have failed, not you.

Comment 19

[Dão Gottwald [:dao]]

I think you'll need to implement this on comm-central, as dead and undocumented code just isn't going to be maintained on mozilla-central and would just rot over time.

Comment 20

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/bc10a5de560c

Comment 21

[Jim Porter (:squib)]

(In reply to Dão Gottwald [:dao] from comment #19)
> I think you'll need to implement this on comm-central, as dead and
> undocumented code just isn't going to be maintained on mozilla-central and
> would just rot over time.

At the very least, when there's a bug on file to remove dead code, could everyone try to remember to check if comm-central uses it and give people a heads up if so? It's no fun having to react to bustage after the fact just because someone forgot you existed.