1073234 - show small swatch of current theme in the themes button

Status: VERIFIED FIXED

(Firefox :: Toolbars and Customization, defect)

Description

[Madhava Enros [:madhava]]

The mockup here: https://bug1007311.bugzilla.mozilla.org/attachment.cgi?id=8427694

shows a small version of the theme preview (or a scaled-down version of the square theme icon) in the Themes button. This helps to communicate what the button is about, and draws some attention to the button (which is otherwise a bit hidden amongst many others).

Can we add this here?

Comment 1

[Blake Winton (:bwinton) (:☕️)]

Some notes:

The button we want to change is defined at:
https://dxr.mozilla.org/mozilla-central/source/browser/components/customizableui/content/customizeMode.inc.xul#32

It looks like we'll need to grab the currentTheme like the code at:
https://dxr.mozilla.org/mozilla-central/source/browser/components/customizableui/CustomizeMode.jsm#1340

And we should probably set the image attribute of the button to the currentTheme's iconURL near:
https://dxr.mozilla.org/mozilla-central/source/browser/components/customizableui/CustomizeMode.jsm#293

Comment 2

[farhaan]

hi Blake ,
I am new to debugging , i want to take this as my first bug . can you help me fix it .
Thanks ,
Farhaan

Comment 3

[Blake Winton (:bwinton) (:☕️)]

I will certainly try, but another contributor (Lewis Cowper) might also be fixing this bug.
If he is, I'm sure we can find another bug you'll be interested in!

The first step to fixing bugs is to get a copy of Firefox building on your computer.  There is a reasonably simple set of steps at https://developer.mozilla.org/en-US/docs/Simple_Firefox_build  Would you give that a try, and please email me at [e-mail] if you run into any problems?

Thanks!
Blake.

Comment 4

[Lewis Cowper]

I've not started yet, feel free to take it if you'd like farhaan. :)

Comment 5

[farhaan]

Blake , 
I tried doing it, was actually trying it for past two days , with a little help from IRC i did that today.

Comment 6

[farhaan]

Lewis , 
I am really new to it, I really need help from you both , like you have to walk me through it at times , hope I am not bugging you
thanks a ton

Comment 7

[farhaan]

Lewis , 
I am really new to it, I really need help from you both , like you have to walk me through it at times , hope I am not bugging you
thanks a ton

Comment 8

[Blake Winton (:bwinton) (:☕️)]

Bugging us is why we're here.  :)

So, Farhaan, I think the first step is to go to line 293 of browser/components/customizableui/CustomizeMode.jsm and add the line:
console.log(LightweightThemeManager.currentTheme);
then rebuild Firefox, and open the Browser Console with Ctrl-Shift-J (or Cmd-Shift-J if you're on a Mac), go into Customize Mode, and see what gets printed.

Comment 9

[farhaan]

I did that nothing reasonable i can see in the console!
How do i put screenshots here!

Comment 10

[:Gijs (he/him)]

(In reply to farhaan from comment #9)
> I did that nothing reasonable i can see in the console!

Did you rebuild with "./mach build" or "./mach build browser/components" ?

> How do i put screenshots here!

Click the "Add an attachment" link right under "Attachments", right above the comment box.

Comment 11

[farhaan]

Attachment: Screen Shot 2015-02-09 at 11.51.39 pm.png

Comment 12

[farhaan]

(In reply to :Gijs Kruitbosch from comment #10)
> (In reply to farhaan from comment #9)
> > I did that nothing reasonable i can see in the console!
> 
> Did you rebuild with "./mach build" or "./mach build browser/components" ?
> 
> > How do i put screenshots here!
> 
> Click the "Add an attachment" link right under "Attachments", right above
> the comment box.

i used "./mach build"

Comment 13

[Blake Winton (:bwinton) (:☕️)]

Hmm.  I also don't see anything in the console.  Can you paste the output of "hg diff" into http://pastebin.mozilla.org/ ?

And also, just to double-check, you are entering Customize mode after you open the Browser Console, right?

Comment 14

[farhaan]

I got it , it's showing null . I am attaching the screenshot

Comment 15

[farhaan]

Attachment: Screen Shot 2015-02-10 at 8.04.02 pm.png

Console showing null

Comment 16

[Blake Winton (:bwinton) (:☕️)]

Awesome!  Now try changing your theme, and re-entering Customize Mode, and see what it says then…  :)

Comment 17

[farhaan]

(In reply to Blake Winton (:bwinton) from comment #16)
> Awesome!  Now try changing your theme, and re-entering Customize Mode, and
> see what it says then…  :)

it is showing this!

Comment 18

[farhaan]

Attachment: Screen Shot 2015-02-10 at 11.30.30 pm.png

Comment 19

[Blake Winton (:bwinton) (:☕️)]

Great!  So, now you need to get the button (which has an id of "customization-lwtheme-button"), and set its "image" attribute to the currentTheme's iconURL.

I would add that code at line 293 of browser/components/customizableui/CustomizeMode.jsm.

(You'll also have to handle the case where the currentTheme is "null", perhaps by showing a default image of "chrome://browser/skin/theme-switcher-icon.png".)

Comment 20

[farhaan]

(In reply to Blake Winton (:bwinton) from comment #19)
> Great!  So, now you need to get the button (which has an id of
> "customization-lwtheme-button"), and set its "image" attribute to the
> currentTheme's iconURL.
> 
> I would add that code at line 293 of
> browser/components/customizableui/CustomizeMode.jsm.
> 
> (You'll also have to handle the case where the currentTheme is "null",
> perhaps by showing a default image of
> "chrome://browser/skin/theme-switcher-icon.png".)

http://pastebin.mozilla.org/8661634
this is what i tried to do! for a while i got the icon but then its not showing anything!

Comment 21

[Blake Winton (:bwinton) (:☕️)]

It looks close to working!  :)

I think there are two things you'll need to do…
1) Remove the quotes from around "LightweightThemeManager.currentTheme.iconURL", because we want to pass the value of that variable, not the name of the variable.
2) The iconURL might be null, in which case we should probably set it to the default icon.  I usually code up those tests like this:

let iconURL = "chrome://browser/skin/theme-switcher-icon.png";
if (LightweightThemeManager.currentTheme &&
    LightweightThemeManager.currentTheme.iconURL) {
  iconURL = LightweightThemeManager.currentTheme.iconURL;
}
document.getElementById("customization-lwtheme-button").setAttribute("image", iconURL);

Give something like that a try, and if you run into more errors, check the Browser Console, to see what the problem might be…

Comment 22

[farhaan]

WOW! i guess this is it, this patch works good



http://pastebin.mozilla.org/8662572

Comment 23

[Blake Winton (:bwinton) (:☕️)]

(In reply to farhaan from comment #22)
> WOW! i guess this is it, this patch works good
> http://pastebin.mozilla.org/8662572

Excellent!  The next step will be to post your patch for review, and see what the reviewer thinks…
There are instructions on how to do that at http://mozilla-version-control-tools.readthedocs.org/en/latest/devguide/contributing.html#submitting-patches-for-review

(I admit that is a new workflow for me, so I'll be interested in seeing how well it works!  :)

Comment 24

[farhaan]

(In reply to Blake Winton (:bwinton) from comment #23)
> (In reply to farhaan from comment #22)
> > WOW! i guess this is it, this patch works good
> > http://pastebin.mozilla.org/8662572
> 
> Excellent!  The next step will be to post your patch for review, and see
> what the reviewer thinks…
> There are instructions on how to do that at
> http://mozilla-version-control-tools.readthedocs.org/en/latest/devguide/
> contributing.html#submitting-patches-for-review
> 
> (I admit that is a new workflow for me, so I'll be interested in seeing how
> well it works!  :)

I am not able to figure out how to send it for review! can you please help me and elaborate how should I do it?
Only a single file i need to send right?

Comment 25

[Blake Winton (:bwinton) (:☕️)]

Well, I guess that part of the workflow needs a little more work.  :)

Can you try following the instructions at https://developer.mozilla.org/en-US/docs/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F and see if they make more sense?

(The final step will be something like "hg diff > bug1073234.diff" and then attaching bug1073234.diff to this bug, but there's a bit of setup that you'll need to do first.)

Comment 26

[farhaan]

Hi Blake,
  I will file the patch today but I have some problem with the patch , when I enter customize mode the swatch I see is perfect but when I change the theme the swatch doesn't change that instant, I have to esc and then again open customize mode and then swatch changes. Can we do something for this? :) That will make UI better , I feel! :)

Comment 27

[Blake Winton (:bwinton) (:☕️)]

That _would_ make it way better!  :)

I think we can do something for this.  Can you find out which function gets called when you click on a theme in the list?

Comment 28

[farhaan]

Let me see! I can try inspecting element but you know the code well , anyway if i get the popup function we can call a function there.

Comment 29

[farhaan]

gCustomizeMode.onLWThemesMenuHidden(event) this function is called on popuphidden i need to find this definition and call and put lines there!

Comment 30

[Blake Winton (:bwinton) (:☕️)]

Pro-tip: If you go to https://dxr.mozilla.org/mozilla-central/source/ and put "onLWThemesMenuHidden" in the Search mozilla-central box at the top, it should show you where the function is defined.  :)

Comment 31

[farhaan]

tip perceived :) but still finding a way to do it.

Comment 32

[Blake Winton (:bwinton) (:☕️)]

Cool.  Let me know how it goes!

Comment 33

[farhaan]

hi i tried getting my same code in the function called resetPreview() but its throwing error document not defined and when i put this.window.document still not working can you help me please!

Comment 34

[farhaan]

I did it :)

Comment 35

[Blake Winton (:bwinton) (:☕️)]

Wow!  I should go for a long lunch more often.  ;)

Did you want to post the changes you made to this bug, and I'll see if there's anything we need to clean up before you ask for review?

Comment 36

[farhaan]

yeah how should i do that?

Comment 37

[farhaan]

i generated a patch file! what should i do now?

Comment 38

[farhaan]

Attachment: swatch.diff

these are the changes i made

Comment 39

[Blake Winton (:bwinton) (:☕️)]

Comment on attachment 8564310 [details] [diff] [review]
swatch.diff

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

Awesome!  I've got a few changes I'ld like you to make so that the code fits in with the Mozilla style, but once those are fixed I think the patch is ready for review.  :)
(I know it seems like a lot of comments, but they're all really small changes.)

::: browser/components/customizableui/CustomizeMode.jsm
@@ +288,5 @@
>          this.visiblePalette.setAttribute("showing", "true");
>        }, 0);
>        this.paletteSpacer.hidden = true;
>        this._updateEmptyPaletteNotice();
> +         // Bug 1073234 show small swatch of current theme in the themes button

The indentation on this line looks off.  Can you make sure you're only using spaces (and not tabs)?

@@ +289,5 @@
>        }, 0);
>        this.paletteSpacer.hidden = true;
>        this._updateEmptyPaletteNotice();
> +         // Bug 1073234 show small swatch of current theme in the themes button
> +      if(LightweightThemeManager.currentTheme == null){

We need a space before the ( and after the ).

@@ +290,5 @@
>        this.paletteSpacer.hidden = true;
>        this._updateEmptyPaletteNotice();
> +         // Bug 1073234 show small swatch of current theme in the themes button
> +      if(LightweightThemeManager.currentTheme == null){
> +        document.getElementById("customization-lwtheme-button").setAttribute("image","chrome://browser/skin/theme-switcher-icon.png");

This line is a little long.  We should add a newline (and two extra spaces of indent) before the ".setAttribute".  And an extra space after the ",", please!  :)

@@ +292,5 @@
> +         // Bug 1073234 show small swatch of current theme in the themes button
> +      if(LightweightThemeManager.currentTheme == null){
> +        document.getElementById("customization-lwtheme-button").setAttribute("image","chrome://browser/skin/theme-switcher-icon.png");
> +      }
> +      else{

The else should be on the same line as the }, and we should have a space before the {.

@@ +1406,5 @@
>    onLWThemesMenuHidden: function(aEvent) {
>      let doc = aEvent.target.ownerDocument;
>      let footer = doc.getElementById("customization-lwtheme-menu-footer");
>      let recommendedLabel = doc.getElementById("customization-lwtheme-menu-recommended");
> +    // Bug 1073234 instance show small swatch of current theme in the themes //button

This code block will need the same changes as the previous code block.

@@ +1420,5 @@
>          element.previousSibling.remove();
>        }
>      }
>      aEvent.target.removeAttribute("height");
> +

And we should probably remove this extra blank line.  But that's the last of the changes.  :)

Comment 40

[farhaan]

Attachment: swatch.diff

indentations and standards revised

Comment 41

[Blake Winton (:bwinton) (:☕️)]

Comment on attachment 8564415 [details] [diff] [review]
swatch.diff

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

Okay, two more space fixes, and then you should post the entire set of changes!

::: browser/components/customizableui/CustomizeMode.jsm
@@ -292,5 @@
>        // Bug 1073234 show small swatch of current theme in the themes button
>        if ( LightweightThemeManager.currentTheme == null ) {
>          document.getElementById("customization-lwtheme-button")
>            .setAttribute("image", "chrome://browser/skin/theme-switcher-icon.png");
>        }else {

No space after the ( or before the ), please.  And a space before the else…

(It should be like this:
  if (whatever) {
    ...
  } else {
    ...
  }
)

Comment 42

[farhaan]

Attachment: swatch.diff

Tried to fix the error and really sorry for the indentation errors .

Comment 43

[Blake Winton (:bwinton) (:☕️)]

Okay, the code looks great!  The last (well, almost last) thing to do is get a single patch file that contains all your changes, post that to this bug, and ask :jaws for review!

(To get all your changes, use "hg log" to find the revision before your first change (let's pretend it was "228834:715a00dcf5c0"), then use "hg diff -r 228834 > bug1073234.diff" to get the changes you made.  Uh, hopefully that makes sense, it's a little confusing to describe in words.)

Comment 44

[farhaan]

Attachment: bug1073234.diff

Comment 45

[farhaan]

see it its ok! and how do i ask :jaws to review it?

Comment 46

[Blake Winton (:bwinton) (:☕️)]

It looks like the spaces before the else are missing again, but once you fix that, when you attach the patch, there's a dropdown under the "Flags:" section called "review".  Set it to "?", and type ":jaws" in the field that appears.

Comment 47

[farhaan]

Attachment: bug1073234.diff

Comment 48

[farhaan]

Attachment: bug1073234.diff (previous one has extra space in if statement)

Comment 49

[farhaan]

hi Blake , 
  Till this bug gets reviewed can you point me to other bug ! Suggest me some and mentor me to solve them! :P

Comment 50

[Blake Winton (:bwinton) (:☕️)]

Yes, definitely!  Are you on Windows, Mac, or Linux?

Comment 51

[farhaan]

Mac :P

Comment 52

[Blake Winton (:bwinton) (:☕️)]

Rats.  That rules out a bunch of Windows bugs…

So, how about bug 995758?  It's a little more complicated, but I think you could investigate and fix it with some help…

Comment 53

[farhaan]

Ok I will take a look and try to reproduce it.

Comment 54

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8564660 [details] [diff] [review]
bug1073234.diff  (previous one has extra space in if statement)

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

There needs to be more margin between the icon and the text on the button. You should add a selector for #customization-lwtheme-button at http://hg.mozilla.org/mozilla-central/annotate/7f7e833005ea/browser/themes/shared/customizableui/customizeMode.inc.css#l157.

::: browser/components/customizableui/CustomizeMode.jsm
@@ +294,5 @@
> +        document.getElementById("customization-lwtheme-button")
> +          .setAttribute("image", "chrome://browser/skin/theme-switcher-icon.png");
> +      } else {
> +        document.getElementById("customization-lwtheme-button")
> +          .setAttribute("image", LightweightThemeManager.currentTheme.iconURL);

Can you pull this out to a separate function since it is duplicated in two places? Also, this might be cleaner:

let lwthemeButton = document.getElementById("customization-lwtheme-button");
let imageURL = LightweightThemeManager.currentTheme === null ?
                 "chrome://browser/skin/theme-switcher-icon.png" :
                 LightweightThemeManager.currentTheme.iconURL;
lwthemeButton.setAttribute("image", imageURL);

Comment 55

[farhaan]

Ok!  will try to do that , and also can you guide me on how do modify the css file please.

Comment 56

[farhaan]

and also tell me where in the file should I write the new function so that i don't mess with the structure of the file.

Comment 57

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to farhaan from comment #55)
(In reply to farhaan from comment #56)

I recommend experimenting with it a bit on your own to learn how it works. Feel free to ask here or on #fx-team or #introduction if something doesn't make sense to you.

Comment 58

[farhaan]

Thank you will try and get back to you may be tomorrow! :)

Comment 59

[farhaan]

Everytime I am trying to create a function it is calling for one call and not calling for the second one . I am not able to figure out the scope where I should define the function so that it works for both. Help needed

Comment 60

[farhaan]

Did it!

Comment 61

[farhaan]

Now going on to work on css thing!

Comment 62

[farhaan]

help required i cannot see any changes in the button i tried including the following code in css
https://pastebin.mozilla.org/8822480

help needed

Comment 63

[farhaan]

Attachment: Functions i made changes and its working properly. Css i made changes see if that is what you want and if not then guide me more on that

Please check it out

Comment 64

[Blake Winton (:bwinton) (:☕️)]

Hi Farhaan, I'm sorry I didn't get back to you sooner!

The structure of the css looks okay.  I don't _think_ you need the "-moz-image-region: rect(0, 24px, 24px, 0);" line, though…

Comment 65

[farhaan]

Hi blake thanks for getting back to me. I am making changes but I cannot see any change in the button

Comment 66

[farhaan]

Attachment: bug.diff

can you please checkout is it this what was required !

Comment 67

[Blake Winton (:bwinton) (:☕️)]

That looks like it should be good.  Let me try it on my computer and see what's up…

Comment 68

[Blake Winton (:bwinton) (:☕️)]

Ah, I see what's happening…  The selector you were using only controlled the padding around both the button and the text, not the padding in between the button and the text!

So after playing around a little, I think you need something more like this:
#customization-lwtheme-button  {
  padding: 2px 7px;
}

#customization-lwtheme-button > .box-inherit > .box-inherit > .button-icon {
  -moz-margin-end: 3px;
  height: 22px;
}

in the css.  :)

Comment 69

[farhaan]

Thanks a lot blake it worked fine . Tell me how did you land up doing it , I tried too much but it didn't happen. And should i give this patch for review again?

Comment 70

[farhaan]

Attachment: bug1073234.diff

Comment 71

[Blake Winton (:bwinton) (:☕️)]

Yeah, let's ask Jaws for review again!

As for how I did it, I mostly used the Browser Toolbox (https://developer.mozilla.org/en-US/docs/Tools/Browser_Toolbox) to inspect the buttons, and then I poked around with the styles until it looked good to me.  :)

(You should take some time to play around with the Browser Toolbox, it's pretty amazing!)

Comment 72

[farhaan]

Attachment: bug1073234.diff

patch for review

Comment 73

[Blake Winton (:bwinton) (:☕️)]

Hey Farhaan, it looks like the patch you posted doesn't cleanly apply to the mozilla-central source code.  Can you re-generate it using the instructions in comment 43 and re-attach it?  Thanks!

Comment 74

[farhaan]

Attachment: bug1073234.diff

Comment 75

[farhaan]

Hi Blake, check this one out and do let me know how it works. And browser toolbox is pretty kickass

Comment 76

[Blake Winton (:bwinton) (:☕️)]

Yeah, it's still not quite right.

You see the part on the left here:
https://www.dropbox.com/s/5juuqkqdq5b1p97/Screenshot%202015-02-23%2014.12.23.png?dl=0
that code was never in Firefox, so it makes no sense to remove it…

If you send me the list of commands you're running to get the diff, I'll try to help you get a better diff.  :)

Comment 77

[farhaan]

oh! right that was the first patch i submitted: my steps are :
1.hg log to get the the last update then
2.hg diff -r 22879 > bug1073234.diff

Comment 78

[Blake Winton (:bwinton) (:☕️)]

Hmmm.  Could you put the first page or so of output of "hg log" into pastebin for me, so that I can double-check which revision you're using?

Comment 79

[farhaan]

Hey Blake , I guess i got the problem can you please check now if the diff is fine?

Comment 80

[farhaan]

Attachment: bug1073234.diff

Comment 81

[Blake Winton (:bwinton) (:☕️)]

Comment on attachment 8568515 [details] [diff] [review]
bug1073234.diff

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

Looks good to me!  Here are the things I think Jaws will ask for, but let's ask him and see.  :)

Thanks,
Blake.

::: browser/components/customizableui/CustomizeMode.jsm
@@ -277,5 @@
>        CustomizableUI.addListener(this);
>        window.PanelUI.endBatchUpdate();
>        this._customizing = true;
>        this._transitioning = false;
> -

Why are we removing this line?  Let's just leave it…

@@ +1406,5 @@
>    onLWThemesMenuHidden: function(aEvent) {
>      let doc = aEvent.target.ownerDocument;
>      let footer = doc.getElementById("customization-lwtheme-menu-footer");
>      let recommendedLabel = doc.getElementById("customization-lwtheme-menu-recommended");
> +    // Bug 1073234 instance show small swatch of current theme in the themes //button

We already have this comment in the function, so let's remove it from here.

::: browser/themes/shared/customizableui/customizeMode.inc.css
@@ +153,5 @@
>    -moz-image-region: rect(0, 24px, 24px, 0);
>    padding: 2px 7px;
>  }
>  
> +##customization-lwtheme-button  {

I think we only want one "#" at the start of this line.

Comment 82

[farhaan]

Attachment: bug1073234.diff

Comment 83

[farhaan]

Hi Blake , I made amends please check them out. I asked jaws for review . Thanks a ton for the help.

Comment 84

[Blake Winton (:bwinton) (:☕️)]

Looks good to me!  Let's see what Jaws says.  :)

Comment 85

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8568559 [details] [diff] [review]
bug1073234.diff

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

::: browser/components/customizableui/CustomizeMode.jsm
@@ +127,5 @@
>      } else {
>        this.enter();
>      }
>    },
> +  // Bug 1073234 show small swatch of current theme in the themes button

We can use `hg annotate` to see why changes were made, so this comment is unnecessary.

@@ +128,5 @@
>        this.enter();
>      }
>    },
> +  // Bug 1073234 show small swatch of current theme in the themes button
> +  swatchForTheme: function(whereApplied){

Please rename `whereApplied` to `aDocument`. Using `aDocument` or `doc` as the argument name will make it easier for someone reading the code to get an idea of what kind of object they are dealing with and what capabilities come with that object.

::: browser/themes/shared/customizableui/customizeMode.inc.css
@@ +154,5 @@
>    padding: 2px 7px;
>  }
>  
> +#customization-lwtheme-button  {
> +  padding: 2px 7px;

It would be nice to move the `padding: 2px 7px;` from #customization-titlebar-visibility-button down to here, and add #customization-titlebar-visibility-button to the selector list here so that it is not duplicated. In essence, you should end up with:

#customization-titlebar-visibility-button {
  list-style-image: url("chrome://browser/skin/customizableui/customize-titleBar-toggle.png");
  -moz-image-region: rect(0, 24px, 24px, 0);
}

#customization-titlebar-visibility-button,
#customization-lwtheme-button {
  padding: 2px 7px;
}

@@ +158,5 @@
> +  padding: 2px 7px;
> +}
> +
> +#customization-lwtheme-button > .box-inherit > .box-inherit > .button-icon {
> +  -moz-margin-end: 3px;

This is the wrong change. In comment #54, I pointed to:

#customization-titlebar-visibility-button > .button-box > .button-text {
  /* Sadly, button.css thinks its margins are perfect for everyone. */
  -moz-margin-start: 6px !important;
}

Where you should add a selector for #customization-lwtheme-button. In essence, you should end up with:

#customization-lwtheme-button > .button-box > .box-inherit > .button-text,
#customization-titlebar-visibility-button > .button-box > .button-text {
  /* Sadly, button.css thinks its margins are perfect for everyone. */
  -moz-margin-start: 6px !important;
}

@@ +159,5 @@
> +}
> +
> +#customization-lwtheme-button > .box-inherit > .box-inherit > .button-icon {
> +  -moz-margin-end: 3px;
> +  height: 22px;

We shouldn't need to change the height to 22px here. We use 24px for the titlebar-visibility button, and the icon here will pick up the 24px size due to the CSS rule from:

.customizationmode-button > .box-inherit > .box-inherit > .button-icon,
.customizationmode-button > .button-box > .button-icon {
  height: 24px;
}

Comment 86

[farhaan]

Attachment: bug1073234.diff

Tried making changes you recommended.

Comment 87

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8569466 [details] [diff] [review]
bug1073234.diff

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

This patch looks the same as the previous one.

Comment 88

[Jared Wein [:jaws] (please needinfo? me)]

Perhaps you need to run `hg qrefresh`?

If you made the changes to the files but forgot to hit "Save", then you should make sure that you rebuild the browser with your changes and test that it works as desired.

Comment 89

[farhaan]

Attachment: bug1073234.diff

Hey , sorry i forgot to write them in the file. These are changes and yes i test them every time.

Comment 90

[farhaan]

Sorry, I didn't get what you want to say?

Comment 91

[farhaan]

I got a mail saying my patch got approved now what to do? :)

Comment 92

[Blake Winton (:bwinton) (:☕️)]

The next step will be to take some screenshots of the button with various themes selected, post them here, and ask Madhava for ui-review!  (I'll probably do the Mac ones, since they need to be posted really sooquickly, if that's okay with you.  Of course, if you want to post some before I get to it, feel free!  :)

Comment 93

[Blake Winton (:bwinton) (:☕️)]

Mac screenshots:
  Default theme - https://www.dropbox.com/s/og74y6n4nm5hhq6/Screenshot%202015-02-26%2011.12.52.png?dl=0
  Space Fantasy - https://www.dropbox.com/s/7zu5lp9z97aytfk/Screenshot%202015-02-26%2011.13.07.png?dl=0
  Renaissance - https://www.dropbox.com/s/4kbsjt116a6z2mr/Screenshot%202015-02-26%2011.13.22.png?dl=0

Windows screenshots coming soon.

Comment 94

[farhaan]

i have mac though!! but you can do it so any work for me left?? and will it be merge?

Comment 95

[Blake Winton (:bwinton) (:☕️)]

(Yeah, I know you have a mac, but I was in the middle of it anyways, so I figured I'ld save you the work of taking the screenshots.  ;)

I think we're probably almost done, but we should see if Madhava wants any changes before it gets merged.

Comment 96

[Blake Winton (:bwinton) (:☕️)]

(You could set ui-review? :madhava on the patch, to ask him for a ui-review.)

Comment 97

[farhaan]

Thanks blake , I did that ! Lets hope he likes it

Comment 98

[Blake Winton (:bwinton) (:☕️)]

Windows screenshots:
  Default theme - https://www.dropbox.com/s/odc1cqo4dnaoney/Screenshot%202015-02-26%2015.34.12.png?dl=0
  Space Fantasy - https://www.dropbox.com/s/u7nf5s3jn5fctye/Screenshot%202015-02-26%2015.35.15.png?dl=0
  Renaissance - https://www.dropbox.com/s/xbj1pzk6nryolz5/Screenshot%202015-02-26%2015.35.00.png?dl=0

Comment 99

[Blake Winton (:bwinton) (:☕️)]

Hmm.  The padding looks off on those Windows screenshots…  I wonder…  Should the lines:
#customization-lwtheme-button > .box-inherit > .box-inherit > .button-icon,
#customization-titlebar-visibility-button > .button-box > .button-text {
  /* Sadly, button.css thinks its margins are perfect for everyone. */
  -moz-margin-start: 6px !important;
}

actually be:
#customization-lwtheme-button > .box-inherit > .box-inherit > .button-text,
#customization-titlebar-visibility-button > .button-box > .button-text {
  /* Sadly, button.css thinks its margins are perfect for everyone. */
  -moz-margin-start: 6px !important;
}

?

Comment 100

[Blake Winton (:bwinton) (:☕️)]

Or perhaps 
#customization-lwtheme-button > .box-inherit > .box-inherit > .button-icon,
#customization-titlebar-visibility-button > .button-box > .button-icon {
  /* Sadly, button.css thinks its margins are perfect for everyone. */
  -moz-margin-end: 6px !important;
}

Comment 101

[farhaan]

Hey blake the second one is better i am sending this patch just check it out hope it works on windows too

Comment 102

[farhaan]

Attachment: bug1073234.diff

check this out Blake

Comment 103

[Jared Wein [:jaws] (please needinfo? me)]

It should be

#customization-lwtheme-button > .box-inherit > .box-inherit > .button-text,
#customization-titlebar-visibility-button > .button-box > .button-text {
  /* Sadly, button.css thinks its margins are perfect for everyone. */
  -moz-margin-start: 6px !important;
}

I missed this in the review.

Comment 104

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8570577 [details] [diff] [review]
bug1073234.diff

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

::: browser/themes/shared/customizableui/customizeMode.inc.css
@@ +158,5 @@
>    padding: 2px 7px;
>  }
>  
> +#customization-lwtheme-button > .box-inherit > .box-inherit > .button-icon,
> +#customization-titlebar-visibility-button > .button-box > .button-icon {

Please change this back to applying to the .button-text and using -moz-margin-start

Comment 105

[farhaan]

Attachment: bug1073234.diff

Comment 106

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8570798 [details] [diff] [review]
bug1073234.diff

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

Looks good. I fixed a couple spacing nits in your patch, ran the test suite locally, and pushed your changes to the fx-team repository.

In about a day or two they should get merged to mozilla-central, and then they will appear in Firefox Nightly the following day. Thanks!

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

Comment 107

[farhaan]

Hey jaws thanks a ton , now what should i do next?

Comment 108

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to farhaan from comment #107)
> Hey jaws thanks a ton , now what should i do next?

Hey Farhaan, nothing more for you to do on this bug. You can look for another bug to work on at http://www.joshmatthews.net/bugsahoy/

Comment 109

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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

Comment 110

[Cornel Ionce [:noni] [Hubs QA]]

Mozilla/5.0 (Windows NT 6.1; WOW64; rv:39.0) Gecko/20100101 Firefox/39.0
Mozilla/5.0 (X11; Linux i686; rv:39.0) Gecko/20100101 Firefox/39.0
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.9; rv:39.0) Gecko/20100101 Firefox/39.0

Verified fixed on latest Nightly, build ID: 20150305072141.
A square icon (with the in-use theme) is displayed in the themes button.