In order to integrate light weight themes in customization mode, we want the ability for the user to set light weight themes in customization mode, idealy similar to what you can do on https://addons.mozilla.org/en-US/firefox/themes/

After the discussion with Phillip, we decided to change this into an Engineering bug. See the UX design of setting the LW theme in bug 1007311

Marco, I'm dropping this bug from the current iteration as I won't have time to get to it during the iteration, after picking bug 1022411. I prefer to leave it unassigned to allow someone else looking for bugs to pick it.

Thanks very much for the update Felipe. Dropping Bug 1007336 from Iteration 33.1

I'm pretty sure this isn't actionable until bug 1021736 gets fixed.

... and the result won't be very nice until bug 987586 gets fixed, either.

(In reply to :Gijs Kruitbosch from comment #4) > I'm pretty sure this isn't actionable until bug 1021736 gets fixed. We could just use some placeholder themes and then swap in the right ones once we have them. Or is there anything else I'm missing?

(In reply to Philipp Sackl [:phlsa] from comment #6) > (In reply to :Gijs Kruitbosch from comment #4) > > I'm pretty sure this isn't actionable until bug 1021736 gets fixed. > > We could just use some placeholder themes and then swap in the right ones > once we have them. Or is there anything else I'm missing? I would assume that we'd want to ship the preview images so that we don't hit the network for loading them which would be slow and a rather terrible UX and will break tests. It seems silly to ship random picks first, and then later update them to what we actually want. I also suspect there'll be copyright/licensing issues with including random images from pre-existing themes into the browser, and I wouldn't want that to be part of this engineering bug. So no, I don't think we can proceed with some plan of what we're shipping and how that works license-wise. :-(

(In reply to :Gijs Kruitbosch from comment #7) > to be part of this engineering bug. So no, I don't think we can proceed with > some plan of what we're shipping and how that works license-wise. :-( Sigh. *without* some plan etc.

Marco, can you please add this bug to the current iteration?

Added to Iteration 34.1

This patch installs the five default lightweight themes that were specified in the dependent bug, and adds the UI to customization mode for selecting and previewing the themes. Still need to move the images to be stored locally as well as the styling for the Themes menupopup.

We need to give credit for the images to the authors, but the names of the themes should probably be localized. Should we remove the updateURL from the themes? Should they still link to the theme hosted on AMO? Blair, what do you think?

Looping in Jorge for all this too. (In reply to Jared Wein [:jaws] (please needinfo? me) from comment #12) > We need to give credit for the images to the authors Yes, this is a hard requirement, as per the themes' licenses. Looping in Gerv, for any specifics/recommendations (see bug 1021736 comment 1 for the list of themes we want to use - they're all either CC-By 3.0 or CC-By-SA 3.0). > but the names of the themes should probably be localized. IMO, we have two options here: * Localize existing names * Pick new names that perhaps would describe the themes better to the widest possible audience (and localize them) > Should we remove the updateURL from the themes? I think this depends on a few things: * Do we trust the author/AMO reviews to not punk users that may assume this theme is from Mozilla? -- IMO, yes, that trust is already there - but worth asking this, as it's a different situation. * Is the original source in a high enough resolution? Or would we need to provide a modified version? -- IMO, I think there are cases were we would want to update the image. * Are we ok with taking the maintenance cost when the source theme is updated? ie, know when it's updated, and ensure the copy in the tree is also updated. This is more important if updateURL is defined. -- Dunno. * Having an updateURL defined means if the theme is applied, it will get periodically re-downloaded. Are we ok with putting that bandwidth hit on people? Or is just updating it via app-update good enough? -- IMO, no, we shouldn't put that on people if we're already shipping the image in-product. So, my preference is to remove updateURL and only update via app updates. > Should they still link to the theme hosted on AMO? I think this falls under "appropriate credit" for CC licensing. Gerv?

Looping in Amy Tsay, who is in charge of the theme community. I know there are prolific authors who have high-quality themes and we can trust. They would also be generally happy about it because of the extra attention their themes will get. I think theme updates are very rare.

Philipp, I think we should reconsider allowing hovering or focusing one of the menu items cause a preview of the theme. After spending some time using the attached patch, I've found that it is easy to think that a theme is permanently applied when in reality it is just being previewed. When a theme is previewed it reverts after 30 seconds, so it is easy to preview a theme, exit customization mode, and then have the theme disappear.

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #15) > Philipp, I think we should reconsider allowing hovering or focusing one of > the menu items cause a preview of the theme. After spending some time using > the attached patch, I've found that it is easy to think that a theme is > permanently applied when in reality it is just being previewed. Are you stopping the preview when you mouse out? If not, that would help.

This is really cool. Let me know if I can help with recommendations on the themes. Ideally we could pull from this collection: https://addons.mozilla.org/firefox/collections/mozilla/featured-personas/, because it's refreshed at least every two weeks with new content, and the quality is high. People definitely notice when featured content goes stale, especially if there are just 5.

(In reply to Amy Tsay [:amyt] from comment #17) > This is really cool. Let me know if I can help with recommendations on the > themes. Ideally we could pull from this collection: > https://addons.mozilla.org/firefox/collections/mozilla/featured-personas/, > because it's refreshed at least every two weeks with new content, and the > quality is high. People definitely notice when featured content goes stale, > especially if there are just 5. We've already decided on the 5 that we'll use for this first implementation (see bug 1021736). I'm not sure how often we'll rotate these, but the plan is to only update them on a release-schedule basis instead of every two weeks because we don't want to place the bandwidth costs on everyone who uses Firefox. (In reply to Matthew N. [:MattN] from comment #16) > (In reply to Jared Wein [:jaws] (please needinfo? me) from comment #15) > > Philipp, I think we should reconsider allowing hovering or focusing one of > > the menu items cause a preview of the theme. After spending some time using > > the attached patch, I've found that it is easy to think that a theme is > > permanently applied when in reality it is just being previewed. > > Are you stopping the preview when you mouse out? If not, that would help. Thanks :) I wasn't before, but I'm doing that now. It does help, I'm just not sure if it helps enough.

Stopping the preview when moving the mouse out should be sufficient. That's the same behavior as some of the previews on AMO. A few (minor) other issues after testing the patch: - The »Get more themes« link should lead to https://addons.mozilla.org/<locale>/firefox/themes - Since the popup is using a different styling now, let's give both »Manage« and »Get more themes« their own row and add a separator above them. - Could you reverse the order of the themes so that the currently selected theme is at the top of the list?

I don't know what the UX looks like here or what the constraints are, but reading CC-BY, I would suggest you want something like: <a href="[URL to addon on AMO]">[Theme Name]</a> is (C) [author(s)]. Available under <a href="[URL to CC-BY]">CC-BY</a>. No warranty. If you modify the theme, prefix it with "Based on ". Of course, for CC-BY-SA, update in the obvious way. Does that help? Gerv

(In reply to Gervase Markham [:gerv] from comment #20) > I don't know what the UX looks like here or what the constraints are, but > reading CC-BY, I would suggest you want something like: > > <a href="[URL to addon on AMO]">[Theme Name]</a> is (C) [author(s)]. > Available under <a href="[URL to CC-BY]">CC-BY</a>. No warranty. > > If you modify the theme, prefix it with "Based on ". Of course, for > CC-BY-SA, update in the obvious way. > > Does that help? > > Gerv Can we include this with the theme description in the addons manager, or does it need to be somewhere else in the UI?

The Add-ons Manager seems sufficient to me, but requesting needinfo from Gerv just to make sure. As I understand it, as long as we make the notice available and accessible from within the product then we are satisfying the requirements. We don't need to show the attribute in every place that the name appears.

This should cover all of the bases, although I'm not sure how to get the same sorting order that about:addons shows.

Add-On Manager is more than sufficient. Wherever you would give ownership or licensing information for the theme. Gerv

Comment on attachment 8468020 [details] [diff] [review] Patch Review of attachment 8468020 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/app/profile/firefox.js @@ +247,5 @@ > pref("xpinstall.whitelist.add", "addons.mozilla.org"); > pref("xpinstall.whitelist.add.180", "marketplace.firefox.com"); > > pref("lightweightThemes.update.enabled", true); > +pref("lightweightThemes.defaultThemes.340", "[{\"id\":\"1\",\"name\":\"A Web Browser Renaissance\",\"description\":\"A Web Browser Renaissance is (C) Sean.Martell. Available under CC-BY-SA. No warranty.\",\"headerURL\":\"resource:///chrome/browser/content/browser/defaultthemes/1.header.jpg\",\"footerURL\":\"resource:///chrome/browser/content/browser/defaultthemes/1.footer.jpg\",\"textcolor\":\"#000000\",\"accentcolor\":\"#f2d9b1\",\"iconURL\":\"resource:///chrome/browser/content/browser/defaultthemes/1.icon.jpg\",\"previewURL\":\"resource:///chrome/browser/content/browser/defaultthemes/1.preview.jpg\",\"author\":\"Sean.Martell\",\"version\":\"0\"},{\"id\":\"2\",\"name\":\"Space Fantasy\",\"description\":\"Space Fantasy is (C) fx5800p. Available under CC-BY-SA. No warranty.\",\"headerURL\":\"resource:///chrome/browser/content/browser/defaultthemes/2.header.jpg\",\"footerURL\":\"resource:///chrome/browser/content/browser/defaultthemes/2.footer.jpg\",\"textcolor\":\"#ffffff\",\"accentcolor\":\"#d9d9d9\",\"iconURL\":\"resource:///chrome/browser/content/browser/defaultthemes/2.icon.jpg\",\"previewURL\":\"resource:///chrome/browser/content/browser/defaultthemes/2.preview.jpg\",\"author\":\"fx5800p\",\"version\":\"1.0\"},{\"id\":\"3\",\"name\":\"Linen Light\",\"description\":\"Linen Light is (C) DVemer. Available under CC-BY-SA. No warranty.\",\"headerURL\":\"resource:///chrome/browser/content/browser/defaultthemes/3.header.png\",\"footerURL\":\"resource:///chrome/browser/content/browser/defaultthemes/3.footer.png\",\"textcolor\":\"#None\",\"accentcolor\":\"#ada8a8\",\"iconURL\":\"resource:///chrome/browser/content/browser/defaultthemes/3.icon.png\",\"previewURL\":\"resource:///chrome/browser/content/browser/defaultthemes/3.preview.png\",\"author\":\"DVemer\",\"version\":\"1.0\"},{\"id\":\"4\",\"name\":\"Pastel Gradient\",\"description\":\"Pastel Gradient is (C) darrinhenein. Available under CC-BY. No warranty.\",\"headerURL\":\"resource:///chrome/browser/content/browser/defaultthemes/4.header.png\",\"footerURL\":\"resource:///chrome/browser/content/browser/defaultthemes/4.footer.png\",\"textcolor\":\"#000000\",\"accentcolor\":\"#000000\",\"iconURL\":\"resource:///chrome/browser/content/browser/defaultthemes/4.icon.png\",\"previewURL\":\"resource:///chrome/browser/content/browser/defaultthemes/4.preview.png\",\"author\":\"darrinhenein\",\"version\":\"1.0\"},{\"id\":\"5\",\"name\":\"Carbon Light\",\"headerURL\":\"resource:///chrome/browser/content/browser/defaultthemes/5.header.png\",\"footerURL\":\"resource:///chrome/browser/content/browser/defaultthemes/5.footer.png\",\"textcolor\":\"#3b3b3b\",\"accentcolor\":\"#2e2e2e\",\"iconURL\":\"resource:///chrome/browser/content/browser/defaultthemes/5.icon.jpg\",\"previewURL\":\"resource:///chrome/browser/content/browser/defaultthemes/5.preview.jpg\",\"author\":\"Jaxivo\",\"description\":\"Carbon Light is (C) Jaxivo. Available under CC-BY-SA. No warranty.\",\"version\":\"1.0\"}]"); Alas, putting the descriptions here means they can't be localized. Also, I think we should be providing a way for locales to specify which themes are available - like we do for search engines. Finally, I don't think we should be installing these themes automatically like this, without human interaction. It's one thing to do this for new profiles, but IMO it feels intrusive if we're installing these into existing profiles. I had always thought of these as recommended themes. So they'd show up in the menu (possibly under a "recommended" heading?), but not install until you selected one. The LightweightThemeManager API supports all this - you can pass just any object with the right properties into previewTheme(), and as soon as it's set as the current theme it becomes installed. ::: browser/components/customizableui/CustomizeMode.jsm @@ +286,5 @@ > delete this._enableOutlinesTimeout; > }, 0); > > + // Don't block customizationready with populating the themes menu. > + AddonManager.getAddonsByTypes(["theme"], this._buildLWThemesMenu.bind(this)); At the moment, "theme" includes both lightweight themes AND old-school heavyweight themes. Bug 520124 would change this so lightweight themes become type "lwtheme", but until that mythical day you need to filter based on the "skinnable" property (which lightweight themes don't have). This is important for a couple of reasons: * Heavyweight themes require restart to apply. * ATM, lightweight themes can only be used when using the default theme. If you're not using the default theme, a restart is required. * Will get super complex when we fix 520124, which makes lightweight themes be able to be applied to any heavyweight theme. This also means that if either a lightweight theme or the default theme is not currently enabled, I think we should just hide this UI. That lets us cover the 99% without the huge complexity of supporting heavyweight themes (until bug 520124 lands). @@ +1224,5 @@ > this.areas.splice(index, 1); > } > }, > > + _buildLWThemesMenu: function(aThemes) { This menu feels quite out of place in about:customizing - fiddly little controls with unrecognizable icons, next to all the larger controls elsewhere. The original mockup had the menuitems much larger, and recognizable thumbnails. Also, I think there needs to be a visual cue for the currently applied theme. ::: browser/components/customizableui/content/customizeMode.inc.xul @@ +34,5 @@ > + <menuseparator id="customization-lwtheme-menuseparator"/> > + <menuitem id="customization-lwtheme-menu-manage" label="&customizeMode.lwthemes.menuManage;" > + oncommand="BrowserOpenAddonsMgr('addons://list/theme');"/> > + <menuitem id="customization-lwtheme-menu-getmore" label="&customizeMode.lwthemes.menuGetMore;" > + oncommand="openUILinkIn('https://addons.mozilla.org/firefox/themes', 'tab');"/> This URL should be in prefs, with the locale being substituted (UI locale and locales in accepted-languages can be different - we want the one closest to the UI locale). ::: browser/locales/en-US/chrome/browser/browser.dtd @@ +698,5 @@ > <!ENTITY customizeMode.toolbars "Show / Hide Toolbars"> > <!ENTITY customizeMode.titlebar "Title Bar"> > +<!ENTITY customizeMode.lwthemes "Themes"> > +<!ENTITY customizeMode.lwthemes.menuManage "Manage"> > +<!ENTITY customizeMode.lwthemes.menuGetMore "Get More Themes"> Access keys?

(In reply to Blair McBride [:Unfocused] from comment #25) > Also, I think we should be providing a way for locales to specify which > themes are available - like we do for search engines. Er, incomplete thought. I don't think this is required for this bug - but something we should keep in mind, and see if it's easy to do in this bug.

Oh, and: I think we should guarantee that the default theme/option to not have a lightweight theme is guaranteed in a predictable place.

(In reply to Blair McBride [:Unfocused] from comment #27) > Oh, and: I think we should guarantee that the default theme/option to not > have a lightweight theme is guaranteed in a predictable place. This can be covered by Restore Default, which this patch doesn't do.

(In reply to Blair McBride [:Unfocused] from comment #25) > Alas, putting the descriptions here means they can't be localized. These descriptions are just licensing details. license.html (about:license) is not localized, so I don't see why these need to be.

This patch matches the styling of the spec, but in doing so I had to switch from a menupopup to a panel. I can't get keyboard access to work in the panel, and the DOMMenuActive/DOMMenuInactive events don't fire either. When I tried styling the menupopup, I couldn't figure out how to get rid of the shadow that the popup has on Windows. Obviously accessibility will trump styling here, but do you know how I could style <menupopup> to match this style?

Comment on attachment 8470226 [details] [diff] [review] Patch v2 Review of attachment 8470226 [details] [diff] [review]: ----------------------------------------------------------------- > Obviously accessibility will trump styling here, but do you know how I could > style <menupopup> to match this style? I think we can just get keyboard accessibility working for <panel>, by following a similar approach to what Gijs took in bug 881937: * Convert <menuitem> elements to <toolbarbutton> (or <button> or whatever) * Set tabindex="0" on each thing we want keyboard navigatable * Manually focus the first focusable element in the popup when it opens * For preview/reset, you'll need focus/blur events for keyboard, and mouseover/mouseout for mouse ::: browser/components/customizableui/CustomizeMode.jsm @@ +286,5 @@ > delete this._enableOutlinesTimeout; > }, 0); > > + // Don't block customizationready with populating the themes menu. > + AddonManager.getAddonsByTypes(["theme"], this._buildLWThemesMenu.bind(this)); This should be able to be done lazily (ie, onLWThemesMenuShowing) without any noticeable lag. I'm wary about adding anything to the initialization here, if it's avoidable. @@ +1270,5 @@ > + let lwthemePrefs = Services.prefs.getBranch("lightweightThemes."); > + let recommendedThemes = lwthemePrefs.getComplexValue("recommendedThemes", > + Ci.nsISupportsString).data; > + recommendedThemes = JSON.parse(recommendedThemes); > + for (let theme of recommendedThemes) { Open question: Should we allow a theme to be in both lists? If not, which list should it show in?

(In reply to Blair McBride [:Unfocused] from comment #31) > @@ +1270,5 @@ > > + let lwthemePrefs = Services.prefs.getBranch("lightweightThemes."); > > + let recommendedThemes = lwthemePrefs.getComplexValue("recommendedThemes", > > + Ci.nsISupportsString).data; > > + recommendedThemes = JSON.parse(recommendedThemes); > > + for (let theme of recommendedThemes) { > > Open question: Should we allow a theme to be in both lists? If not, which > list should it show in? This patch doesn't allow a theme to show in both lists. It begins in the "recommended" list, but then when it is installed it shows up in the "my themes" list. When it is removed through the Add-ons Manager, it will no longer show up in either list. I find this behavior to be the best, as someone may not want to see one of the themes anymore and thus they have a built-in way to remove them. It also doesn't make much sense to me to show themes twice in the same panel. I think the only downside is if someone uninstalls the theme and wants to get it back a few weeks later, they don't have a simple way to do so (minus going to about:config and resetting the recommendedThemes pref).

Now keyboard accessible and built during popupshowing instead of init.

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #32) > > Open question: Should we allow a theme to be in both lists? If not, which > > list should it show in? > > This patch doesn't allow a theme to show in both lists. It begins in the > "recommended" list, but then when it is installed it shows up in the "my > themes" list. When it is removed through the Add-ons Manager, it will no > longer show up in either list. Oh, sorry, I meant the situation where a theme is *aleady* installed - not via this menu. In that case, such themes do show up twice. Given your reasoning, I think they should be auto-removed from the recommended list if they're already found to be installed.

Comment on attachment 8471036 [details] [diff] [review] Patch v2.1 Review of attachment 8471036 [details] [diff] [review]: ----------------------------------------------------------------- I can haz tests? ::: browser/app/profile/firefox.js @@ +248,5 @@ > pref("xpinstall.whitelist.add.180", "marketplace.firefox.com"); > > pref("lightweightThemes.update.enabled", true); > +pref("lightweightThemes.getMoreURL", "https://addons.mozilla.org/%LOCALE%/firefox/themes"); > +pref("lightweightThemes.recommendedThemes", "[{\"id\":\"1\",\"name\":\"A Web Browser Renaissance\",\"description\":\"A Web Browser Renaissance is (C) Sean.Martell. Available under CC-BY-SA. No warranty.\",\"headerURL\":\"resource:///chrome/browser/content/browser/defaultthemes/1.header.jpg\",\"footerURL\":\"resource:///chrome/browser/content/browser/defaultthemes/1.footer.jpg\",\"textcolor\":\"#000000\",\"accentcolor\":\"#f2d9b1\",\"iconURL\":\"resource:///chrome/browser/content/browser/defaultthemes/1.icon.jpg\",\"previewURL\":\"resource:///chrome/browser/content/browser/defaultthemes/1.preview.jpg\",\"author\":\"Sean.Martell\",\"version\":\"0\"},{\"id\":\"2\",\"name\":\"Space Fantasy\",\"description\":\"Space Fantasy is (C) fx5800p. Available under CC-BY-SA. No warranty.\",\"headerURL\":\"resource:///chrome/browser/content/browser/defaultthemes/2.header.jpg\",\"footerURL\":\"resource:///chrome/browser/content/browser/defaultthemes/2.footer.jpg\",\"textcolor\":\"#ffffff\",\"accentcolor\":\"#d9d9d9\",\"iconURL\":\"resource:///chrome/browser/content/browser/defaultthemes/2.icon.jpg\",\"previewURL\":\"resource:///chrome/browser/content/browser/defaultthemes/2.preview.jpg\",\"author\":\"fx5800p\",\"version\":\"1.0\"},{\"id\":\"3\",\"name\":\"Linen Light\",\"description\":\"Linen Light is (C) DVemer. Available under CC-BY-SA. No warranty.\",\"headerURL\":\"resource:///chrome/browser/content/browser/defaultthemes/3.header.png\",\"footerURL\":\"resource:///chrome/browser/content/browser/defaultthemes/3.footer.png\",\"textcolor\":\"#None\",\"accentcolor\":\"#ada8a8\",\"iconURL\":\"resource:///chrome/browser/content/browser/defaultthemes/3.icon.png\",\"previewURL\":\"resource:///chrome/browser/content/browser/defaultthemes/3.preview.png\",\"author\":\"DVemer\",\"version\":\"1.0\"},{\"id\":\"4\",\"name\":\"Pastel Gradient\",\"description\":\"Pastel Gradient is (C) darrinhenein. Available under CC-BY. No warranty.\",\"headerURL\":\"resource:///chrome/browser/content/browser/defaultthemes/4.header.png\",\"footerURL\":\"resource:///chrome/browser/content/browser/defaultthemes/4.footer.png\",\"textcolor\":\"#000000\",\"accentcolor\":\"#000000\",\"iconURL\":\"resource:///chrome/browser/content/browser/defaultthemes/4.icon.png\",\"previewURL\":\"resource:///chrome/browser/content/browser/defaultthemes/4.preview.png\",\"author\":\"darrinhenein\",\"version\":\"1.0\"},{\"id\":\"5\",\"name\":\"Carbon Light\",\"headerURL\":\"resource:///chrome/browser/content/browser/defaultthemes/5.header.png\",\"footerURL\":\"resource:///chrome/browser/content/browser/defaultthemes/5.footer.png\",\"textcolor\":\"#3b3b3b\",\"accentcolor\":\"#2e2e2e\",\"iconURL\":\"resource:///chrome/browser/content/browser/defaultthemes/5.icon.jpg\",\"previewURL\":\"resource:///chrome/browser/content/browser/defaultthemes/5.preview.jpg\",\"author\":\"Jaxivo\",\"description\":\"Carbon Light is (C) Jaxivo. Available under CC-BY-SA. No warranty.\",\"version\":\"1.0\"}]"); Hmm. How are we going to update this list in the future? ATM, it's not possible to do so (without a bunch of extra engineering). This is something we need to figure out now. And in terms of the descriptions, IMO these aren't equivalent to about:license, as that's just for licenses and any viewing of that is from intentionally looking for the license info. These descriptions are exposed in Customize Mode and in the Add-ons Manager. (Though bug 624602 would probably be the ideal fix, TBH) Also: There's potential ID collision here, by using numericals as the ID. IDs coming from AMO for lightweight themes are numerical, so we can easily fix this by making these IDs something like "recommended-1" etc. And finally: For the CC licensing, "appropriate credit" includes linking back to the original work. Can add that by adding a homepageURL property to each theme. ::: browser/components/customizableui/CustomizeMode.jsm @@ +1222,5 @@ > } > }, > > + getMoreThemes: function(aEvent) { > + let win = aEvent.target.ownerDocument.defaultView; this.window ? @@ +1244,5 @@ > + tbb.setAttribute("tooltiptext", aTheme.description); > + tbb.setAttribute("tabindex", "0"); > + tbb.classList.add("customization-lwtheme-menu-theme"); > + if (aTheme.isActive) { > + tbb.setAttribute("active", "true"); This needs to be indicated to screen readers somehow. @@ +1245,5 @@ > + tbb.setAttribute("tabindex", "0"); > + tbb.classList.add("customization-lwtheme-menu-theme"); > + if (aTheme.isActive) { > + tbb.setAttribute("active", "true"); > + tbb.focus(); Focusing here doesn't work - you need to wait until the element is inserted into the DOM, and for popupshown to fire. @@ +1268,5 @@ > + > + let panel = footer.parentNode; > + for (let theme of aThemes) { > + // Only allow the "default" full theme to be shown in this list. > + if (theme.skinnable && You need to check if this property *exists*. The value can be either true of false for heavyweight themes. @@ +1269,5 @@ > + let panel = footer.parentNode; > + for (let theme of aThemes) { > + // Only allow the "default" full theme to be shown in this list. > + if (theme.skinnable && > + theme.id != "{972ce4c6-7e08-4474-a285-3208198ce6fd}") { I'm worried the item for the default theme isn't obvious/useful enough: * It's not guaranteed to be at the top of the list (we're seeing it there by mere coincidence) * The icon isn't descriptive @@ +1289,5 @@ > + for (let theme of recommendedThemes) { > + let tbb = buildToolbarButton(doc, theme); > + tbb.addEventListener("command", function() { > + LightweightThemeManager.setLocalTheme(this.theme); > + recommendedThemes = recommendedThemes.filter(function(aTheme) aTheme != this.theme); |this| is undefined here - you want a fat-arrow function. Also, as a reminder: This form of function is deprecated anyway (see bug 995610, which was mentioned on firefox-dev about 2 months ago). @@ +1301,5 @@ > + panel.insertBefore(tbb, footer); > + } > + let hideRecommendedLabel = (footer.previousSibling == recommendedLabel); > + let recommendedLabel = doc.getElementById("customization-lwtheme-menu-recommended"); > + recommendedLabel.hidden = hideRecommendedLabel; Think we should also hide the "My Themes" section if the only theme there is the default theme AND the recommended section is empty. ::: browser/themes/shared/customizableui/customizeMode.inc.css @@ +328,5 @@ > + border-top: 1px solid hsla(210,4%,10%,.05); > + border-bottom: 1px solid hsla(210,4%,10%,.05); > +} > + > +#customization-lwtheme-menu-footer { This should have some whitespace between it and the last theme item.

(In reply to Blair McBride [:Unfocused] from comment #35) > > Hmm. How are we going to update this list in the future? ATM, it's not > possible to do so (without a bunch of extra engineering). This is something > we need to figure out now. The idea is that updates would use a new pref name (recommendedThemes.350 for example), and then this code could just reference the "newest" pref name. > And in terms of the descriptions, IMO these aren't equivalent to > about:license, as that's just for licenses and any viewing of that is from > intentionally looking for the license info. These descriptions are exposed > in Customize Mode and in the Add-ons Manager. (Though bug 624602 would > probably be the ideal fix, TBH) Should bug 624602 be a hard blocker? > * The icon isn't descriptive I'm not sure what icon could be. We don't have a "default" icon elsewhere.

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #36) > The idea is that updates would use a new pref name (recommendedThemes.350 > for example), and then this code could just reference the "newest" pref name. Ok. > Should bug 624602 be a hard blocker? Yea, why not. I'll pick that up - shouldn't take too long.

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #36) > > * The icon isn't descriptive > > I'm not sure what icon could be. We don't have a "default" icon elsewhere. Maybe Stephen has an idea.

This needs tests and some minor tweaks to the styling of the panel.

Comment on attachment 8474074 [details] [diff] [review] Patch v3 (missing tests still) Can you review this and then I'll upload tests for your review later?

Adding Gijs as a potential second reviewer since I heard that Unfocused might be on PTO.

Try server push, https://tbpl.mozilla.org/?tree=Try&rev=9835f84e1188

Comment on attachment 8475386 [details] [diff] [review] Patch v3 (with test) Review of attachment 8475386 [details] [diff] [review]: ----------------------------------------------------------------- f+ because missing test, aria stuff, and potential leaks. :-( ::: browser/app/profile/firefox.js @@ +248,5 @@ > pref("xpinstall.whitelist.add.180", "marketplace.firefox.com"); > > pref("lightweightThemes.update.enabled", true); > +pref("lightweightThemes.getMoreURL", "https://addons.mozilla.org/%LOCALE%/firefox/themes"); > +pref("lightweightThemes.recommendedThemes", "[{\"id\":\"recommended-1\",\"homepageURL\":\"https://addons.mozilla.org/firefox/addon/a-web-browser-renaissance/\",\"headerURL\":\"resource:///chrome/browser/content/browser/defaultthemes/1.header.jpg\",\"footerURL\":\"resource:///chrome/browser/content/browser/defaultthemes/1.footer.jpg\",\"textcolor\":\"#000000\",\"accentcolor\":\"#f2d9b1\",\"iconURL\":\"resource:///chrome/browser/content/browser/defaultthemes/1.icon.jpg\",\"previewURL\":\"resource:///chrome/browser/content/browser/defaultthemes/1.preview.jpg\",\"author\":\"Sean.Martell\",\"version\":\"0\"},{\"id\":\"recommended-2\",\"homepageURL\":\"https://addons.mozilla.org/firefox/addon/space-fantasy/\",\"headerURL\":\"resource:///chrome/browser/content/browser/defaultthemes/2.header.jpg\",\"footerURL\":\"resource:///chrome/browser/content/browser/defaultthemes/2.footer.jpg\",\"textcolor\":\"#ffffff\",\"accentcolor\":\"#d9d9d9\",\"iconURL\":\"resource:///chrome/browser/content/browser/defaultthemes/2.icon.jpg\",\"previewURL\":\"resource:///chrome/browser/content/browser/defaultthemes/2.preview.jpg\",\"author\":\"fx5800p\",\"version\":\"1.0\"},{\"id\":\"recommended-3\",\"homepageURL\":\"https://addons.mozilla.org/firefox/addon/linen-light/\",\"headerURL\":\"resource:///chrome/browser/content/browser/defaultthemes/3.header.png\",\"footerURL\":\"resource:///chrome/browser/content/browser/defaultthemes/3.footer.png\",\"textcolor\":\"#None\",\"accentcolor\":\"#ada8a8\",\"iconURL\":\"resource:///chrome/browser/content/browser/defaultthemes/3.icon.png\",\"previewURL\":\"resource:///chrome/browser/content/browser/defaultthemes/3.preview.png\",\"author\":\"DVemer\",\"version\":\"1.0\"},{\"id\":\"recommended-4\",\"homepageURL\":\"https://addons.mozilla.org/firefox/addon/pastel-gradient/\",\"headerURL\":\"resource:///chrome/browser/content/browser/defaultthemes/4.header.png\",\"footerURL\":\"resource:///chrome/browser/content/browser/defaultthemes/4.footer.png\",\"textcolor\":\"#000000\",\"accentcolor\":\"#000000\",\"iconURL\":\"resource:///chrome/browser/content/browser/defaultthemes/4.icon.png\",\"previewURL\":\"resource:///chrome/browser/content/browser/defaultthemes/4.preview.png\",\"author\":\"darrinhenein\",\"version\":\"1.0\"},{\"id\":\"recommended-5\",\"homepageURL\":\"https://addons.mozilla.org/firefox/addon/carbon-light/\",\"headerURL\":\"resource:///chrome/browser/content/browser/defaultthemes/5.header.png\",\"footerURL\":\"resource:///chrome/browser/content/browser/defaultthemes/5.footer.png\",\"textcolor\":\"#3b3b3b\",\"accentcolor\":\"#2e2e2e\",\"iconURL\":\"resource:///chrome/browser/content/browser/defaultthemes/5.icon.jpg\",\"previewURL\":\"resource:///chrome/browser/content/browser/defaultthemes/5.preview.jpg\",\"author\":\"Jaxivo\",\"version\":\"1.0\"}]"); Warbl at this massive pref. Don't really know a better way of dealing with it, though (short of asking you to stick it in a separate json file...) ::: browser/components/customizableui/CustomizeMode.jsm @@ +1242,5 @@ > + tbb.setAttribute("image", aTheme.iconURL); > + tbb.setAttribute("tooltiptext", aTheme.description); > + tbb.setAttribute("tabindex", "0"); > + tbb.classList.add("customization-lwtheme-menu-theme"); > + tbb.setAttribute("aria-selected", aTheme.isActive); I think this is wrong and should be aria-checked. See http://www.w3.org/TR/wai-aria/states_and_properties#aria-selected vs. #aria-checked , esp. the bits about focus. If we're doing this, it'd be wise to get an a11y review. I *think* that for the aria-selected/checked to work well with a11y tools, you should specify a role attribute as well, possibly on the container, too. Jamie or Marco would be good to loop in here. @@ +1249,5 @@ > + } > + tbb.addEventListener("focus", previewTheme); > + tbb.addEventListener("mouseover", previewTheme); > + tbb.addEventListener("blur", resetPreview); > + tbb.addEventListener("mouseout", resetPreview); The handlers explicitly have a ref to |doc|, and implicitly to |this|. I expect this will leak because you never remove either the listeners or the elements (until the menu is reopened, but then you add new ones, so...). Please include a test next time you ask for review, and run a debug try run. To be safe, I would just do the menu clearing on popuphiding instead of in here; that'd also fix the potential leaks. ::: browser/components/customizableui/test/browser.ini @@ +114,5 @@ > [browser_995164_registerArea_during_customize_mode.js] > [browser_996364_registerArea_different_properties.js] > [browser_996635_remove_non_widgets.js] > [browser_1003588_no_specials_in_panel.js] > +[browser_1007336_lwthemes_in_customize_mode.js] You didn't hg add this file so it's missing from this patch. ::: browser/components/customizableui/test/head.js @@ +230,5 @@ > +function promiseObserverNotified(aTopic) { > + let deferred = Promise.defer(); > + Services.obs.addObserver(function onNotification(aSubject, aTopic, aData) { > + Services.obs.removeObserver(onNotification, aTopic); > + deferred.resolve(); Maybe resolve with aSubject and aData passed? Seems like people may want that data. ::: browser/themes/shared/customizableui/customizeMode.inc.css @@ +276,5 @@ > margin-right: 0; > } > > +#customization-lwtheme-menu > .panel-arrowcontainer > .panel-arrowcontent { > + display: -moz-box; Is this necessary?

Sorry about the previous patch not included the test file. The trypush above was corrected to have the test file "hg add-ed", but I forgot to update the attached patch. I've made the changes per your review feedback. Try push here, https://tbpl.mozilla.org/?tree=Try&rev=acdecced89ce

Comment on attachment 8476025 [details] [diff] [review] Patch v3.1 Review of attachment 8476025 [details] [diff] [review]: ----------------------------------------------------------------- Code-wise, this looks fine... appearance-wise (on mac), not so much: http://i.imgur.com/MJbRQ0h.png It looks like there is too much margin around items, and too little inbetween the icons and the descriptions. ::: browser/components/customizableui/test/browser_1007336_lwthemes_in_customize_mode.js @@ +15,5 @@ > + let themesButton = document.getElementById("customization-lwtheme-button"); > + let popup = document.getElementById("customization-lwtheme-menu"); > + > + let popupShownPromise = popupShown(popup); > + EventUtils.synthesizeMouseAtCenter(themesButton, {}); Nit: themesButton.click(); should work, I believe. @@ +22,5 @@ > + > + let header = document.getElementById("customization-lwtheme-menu-header"); > + let recommendedHeader = document.getElementById("customization-lwtheme-menu-recommended"); > + > + is(header.nextSibling.theme.id, DEFAULT_THEME_ID, "The only theme should be the Default theme"); Nit: move this below the next assert and just make it say "That theme should be the default theme". @@ +34,5 @@ > + info("Clicked on first theme"); > + yield themeChangedPromise; > + > + popupShownPromise = popupShown(popup); > + EventUtils.synthesizeMouseAtCenter(themesButton, {}); .click() as above

The test hangs with .click(), which is why I used sythesizeMouse. I tried it again after reading your comment and it still hangs, so I've kept it using synthesizeMouse. I have fixed the styling issues.

Comment on attachment 8476771 [details] [diff] [review] Patch v3.2 Review of attachment 8476771 [details] [diff] [review]: ----------------------------------------------------------------- Assuming you fix: - the border on the selected item - the change in size on "get more themes" when comparing hover/non-hover - the manage button not being flush with the left border of the panel - the manage button causing popuphidden not to be called (!?) and therefore duplicating everything in the menu (probably by factoring out its action into a method on gCustomizeMode like the "get more themes" item is already, and calling hidePopup() from there) ... r=me! :-)

Addressed visual nits, will file a follow-up for the color of the arrow not matching on the popup. https://hg.mozilla.org/integration/fx-team/rev/12ecc1bfb2ab

Release Note Request (optional, but appreciated) [Why is this notable]: New feature for customization mode, with built-in recommended themes for people to try [Suggested wording]: Now easier to install themes for the browser by using the Customize feature of Firefox. Firefox will now come bundled with 5 recommended themes. [Links (documentation, blog post, etc)]: I will write a blog post for this and update this bug with a link to the blog post.

Jared, your proposed wording is a bit too long. Instead, I used "Improved theme installation and customization". Don't hesitate to propose a better wording.

Something like "Theme installation in customize mode"? To clarify how it improved (accessible from the customize mode).

Don't use "improved" or "installation". If anything, this new panel is more like a stripped version of the "Appearance" page in the Add-on Manager ("stripped" because it doesn't allow theme removal). Suggestion: Use "theme switching" or something else of that nature. "Theme/persona switching is now easier" "Theme/persona switching is now easier in Customization mode" "Switching themes/personas is now easier/more intuitive in Customization mode" "Easily switch themes/personas directly in the Customizing mode"

OK. thanks. I used "Easily switch themes/personas directly in the Customizing mode".

The lightweight themes can now be used/changed in customize mode on Nightly 34.0a1, buil ID: 20140831030206. By default, 5 themes are displayed (mentioned in bug 1021736). After a theme is chosen, it is moved from the Recommended themes section to My themes. Tested on Windows 7 64 bit, Windows 8.1 64bit (MS Pro 2 device), Ubuntu 12.04 32bit and Mac OS X 10.8.5. User Agents: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:34.0) Gecko/20100101 Firefox/34.0 Mozilla/5.0 (Windows NT 6.3; WOW64; rv:34.0) Gecko/20100101 Firefox/34.0 Mozilla/5.0 (X11; Linux i686; rv:34.0) Gecko/20100101 Firefox/34.0 Mozilla/5.0 (Macintosh; Intel Mac OS X 10.8; rv:34.0) Gecko/20100101 Firefox/34.0