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?

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

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

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 bwinton@mozilla.com if you run into any problems? Thanks! Blake.

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

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

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

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

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.

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

(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.

(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"

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?

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

Console showing null

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

(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!

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".)

(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!

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…

WOW! i guess this is it, this patch works good http://pastebin.mozilla.org/8662572

(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! :)

(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?

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.)

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! :)

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?

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.

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

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. :)

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

Cool. Let me know how it goes!

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!

I did it :)

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?

yeah how should i do that?

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

these are the changes i made

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. :)

indentations and standards revised

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 { ... } )

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

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.)

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

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.

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

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

Mac :P

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…

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

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);

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

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.

(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.

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

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

Did it!

Now going on to work on css thing!

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

Please check it out

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…

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

can you please checkout is it this what was required !

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

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. :)

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?

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!)

patch for review

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!

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

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. :)

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

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?

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

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.

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

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

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; }

Tried making changes you recommended.

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.

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.

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

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

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

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! :)

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.

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

(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.

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

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

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

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; } ?

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; }

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

check this out Blake

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 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 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

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

(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/

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.