This patch creates the skeleton pre-processing file, landing page, and modifies the makefile(s) to add the in-content directory in the preferences folder. I did not modify the HTML5 history because of our discussion about using the search functionality to do the navigation.

Forgot to make one more correction for the applications patch to make sense while splitting them up.

Comment on attachment 603367 [details] [diff] [review] in-content landing page Review of attachment 603367 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for getting a patch up here Jon. ::: browser/components/preferences/in-content/Makefile.in @@ +1,3 @@ > +# -*- Mode: Java; tab-width: 4; indent-tabs-mode: nil; c-basic-offset: 4 -*- > +# ***** BEGIN LICENSE BLOCK ***** > +# Version: MPL 1.1/GPL 2.0/LGPL 2.1 New files should use the MPL 2.0. ::: browser/components/preferences/in-content/home.js @@ +3,5 @@ > + - v. 2.0. If a copy of the MPL was not distributed with this file, > + - You can obtain one at http://mozilla.org/MPL/2.0/. > + - > + - Contributor(s): > + - Jonathan Rietveld <jon.rietveld@gmail.com> The MPL2.0 doesn't have a contributors list. Can you copy the license from this file? http://mxr.mozilla.org/mozilla-central/source/b2g/components/b2g.idl#3 ::: browser/components/preferences/in-content/in-content.css @@ +34,5 @@ > + height: 200px; > +} > + > +prefpane { > +/* height: 400px;*/ This comment and the rule can be deleted. See the .options-box rule below for another comment that can be deleted. @@ +126,5 @@ > + background-repeat: no-repeat; > +} > + > +.heading { > + font-family: georgia,serif; These will need to be put in a theme CSS file. For now, let's just delete this line and use the default font-family. @@ +235,5 @@ > +} > + > +#encrytionPanel { > + > +} Let's remove the blank rules and only add them back if they are needed. ::: browser/components/preferences/in-content/preferences.xul @@ +90,5 @@ > + <toolbarbutton id="forward-btn" class="nav-button header-button" > + oncommand="cmd_forward()" tooltiptext="Go forward one page" disabled="true"/> > + <spacer flex="1"/> > + <!--<textbox id="header-search" type="search" searchbutton="true" > + placeholder="&search.placeholder;"/>--> Let's just delete this for now and add it as part of the search implementation bug. ::: browser/locales/en-US/chrome/browser/preferences/preferences.dtd @@ +17,4 @@ > > <!-- LOCALIZATION NOTE (paneSync.title): This should match syncBrand.shortName.label in ../syncBrand.dtd --> > <!ENTITY paneSync.title "Sync"> > +<!ENTITY landing.title "FireFox"> We shouldn't add this here. We should instead use brandShortName. See http://mxr.mozilla.org/mozilla-central/search?string=brandshortname&find=&findi=&filter=^[^\0]*%24&hitlimit=&tree=mozilla-central for examples on how brandShortName is used.

Comment on attachment 603375 [details] [diff] [review] *correct* in-content landing Review of attachment 603375 [details] [diff] [review]: ----------------------------------------------------------------- This is the right direction, but please address the feedback that was given above as well as Blair's previous comment (part of which affects this patch): https://bugzilla.mozilla.org/show_bug.cgi?id=723328#c17

I made all the correction from the feedback Jared gave me. I did not change any of the JavaScript since Owen is creating the backend search functionality to do the pane switching

Comment on attachment 605534 [details] [diff] [review] landing in-content pane Review of attachment 605534 [details] [diff] [review]: ----------------------------------------------------------------- This looks really good. Please fix the remaining issues and request review. I expect that Blair will have some feedback related to in-content.css, specifically why it is needed here and what was missing from inContent.css. It would be good if you could address what was lacking from inContent.css. General feedback: When uploading a new version of a patch, please mark old versions as obsolete. ::: browser/components/preferences/in-content/in-content.css @@ +6,5 @@ > + > +/* Landing Pane Styles */ > + > +#preferences-home { > + display: block; #preferences-home is an <xhtml:h1>. Headers are by default display:block, so we don't need the following line (http://www.w3.org/TR/xhtml1/dtds.html#dtdentry_xhtml1-strict.dtd_heading). nit: 2 spaces of indentation here and other places within this file. @@ +199,5 @@ > + right: 40px; > + margin: 0; > +} > + > +#header-search { Leave out the #header-search from this bug. We can move that element to a later bug when we introduce text searching.

Fixed the css issues. I am not sure about the display:block feedback since that style is being applied to the hbox, not the header. If I do remove it then the icons don't wrap to a new line properly...

Comment on attachment 605588 [details] [diff] [review] landing in-content pane Review of attachment 605588 [details] [diff] [review]: ----------------------------------------------------------------- Sorry about the #preferences-home, I misread that file. ::: browser/components/preferences/in-content/in-content.css @@ +46,5 @@ > + background-image: url("chrome://browser/skin/preferences/Options.png"); > + background-repeat: no-repeat; > +} > + > +#general-img { Is this being used? I see the class of |.general-img| is used, but no ID of the same name.

Comment on attachment 605588 [details] [diff] [review] landing in-content pane Review of attachment 605588 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/components/preferences/in-content/in-content.css @@ +13,5 @@ > +#header { > + margin-bottom: 18px; > +} > + > +a.inline-link:hover { Are there any .inline-link instances that are not an <a> tag? We should either remove the tag selector from this (so it would be .inline-link:hover) or add a different class so we don't share the same class between <a> and non-<a> elements. @@ +14,5 @@ > + margin-bottom: 18px; > +} > + > +a.inline-link:hover { > + cursor: pointer; Why is this rule necessary? Doesn't the default style of a:hover have |cursor:pointer;|?

(In reply to Jared Wein [:jaws] from comment #6) > I expect that Blair will have some feedback related to in-content.css, > specifically why it is needed here and what was missing from inContent.css. > It would be good if you could address what was lacking from inContent.css. I don't see anything I'd put in inContent.css (minus what bug 660726 will do). The in-content.css file here should really just be called preferences.css, and moved to the different themes.

fixed css styles, as far as the entire css file goes should I just leave it until we start focusing more on styling or should I move all the styles to the respective themes in preferences.css?

Comment on attachment 606574 [details] [diff] [review] landing in-content pane Review of attachment 606574 [details] [diff] [review]: ----------------------------------------------------------------- Jon: Please make the changes requested in comment #10.

made changes for feedback in comment 10

Comment on attachment 607063 [details] [diff] [review] in-content landing pane Review of attachment 607063 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/themes/gnomestripe/preferences/preferences.css @@ +40,1 @@ > */ These styles should be located in a preferences.css file at this folder path: browser/themes/gnomestripe/preferences/in-content/preferences.css and again for winstripe/pinstripe.

added in-content/preferences.css files in gnomestripe, pinstripe, and winstripe. Modified the jar file to add these files as well.

had to fix some css styles that didn't get copied over for some reason...

Comment on attachment 607806 [details] [diff] [review] in-content landing pane Review of attachment 607806 [details] [diff] [review]: ----------------------------------------------------------------- Still waiting for various (most?) things to be fixed up from my review at bug 723328 comment 17. Also, could you fix the indentation of, uh, everything? It sounds trivial but it makes it harder to read, and no patch will land without that fixed (and it's frustrating commenting on it every time). For example, landing.xul uses 4-space indent instead of 2-space; landing.xul and preferences.xul wrap lines by the second line doesn't line up (attribute should line up with the first attribute on the first line); landing.xul has 2 whitespace lines instead of 1. ::: browser/components/preferences/in-content/home.js @@ +1,1 @@ > +/* - This Source Code Form is subject to the terms of the Mozilla Public This file should be named preferences.js, to match preferences.xul. @@ +1,5 @@ > +/* - This Source Code Form is subject to the terms of the Mozilla Public > + - License, v. 2.0. If a copy of the MPL was not distributed with this file, > + - You can obtain one at http://mozilla.org/MPL/2.0/. */ > + > +const Ci = Components.interfaces; This isn't used here, but it'll be used by other JS files for the various preferences. Which no doubt will also use Cc (Components.classes), Cu (Components.utils), and Cr (Components.results) - so you should define those here too (so they're not needed in the other files). Also, before this line, add the following line: "use strict": That will opt-in to strict mode. We're trying to do that for all new JS files. Don't worry about doing that in the other bugs though, as it will most likely require a bunch of other changes in those files. @@ +2,5 @@ > + - License, v. 2.0. If a copy of the MPL was not distributed with this file, > + - You can obtain one at http://mozilla.org/MPL/2.0/. */ > + > +const Ci = Components.interfaces; > +var currentPageState; Convention is to have a blank line between constants and global variables. @@ +6,5 @@ > +var currentPageState; > + > +function gotoPref(page) { > + document.getElementById("preferences-home").hidden = true; > + window.history.pushState(page,"Preferences"); Use window.title instead of "Preferences" (as a bonus, it's already localized for you). Same with other uses of that string in this file. Style nit: Always separate arguments by a space. ie: window.history.pushState(page, "Preferences"); ::: browser/components/preferences/in-content/jar.mn @@ +1,4 @@ > +browser.jar: > +* content/browser/preferences/in-content/home.js > +* content/browser/preferences/in-content/landing.xul > +* content/browser/preferences/in-content/preferences.xul \ No newline at end of file \ No newline at end of file None of these files use the text preprocessor - remove the * (See https://developer.mozilla.org/en/JAR_Manifests for details on what symbols like * do in jar.mn files) ::: browser/components/preferences/in-content/landing.xul @@ +5,5 @@ > +<html:h1>&brandShortName;</html:h1> > + > +<hbox id="preferences-home" flex="1"> > + > + <button label="&paneGeneral.title;" orient="vertical" class="options-box" Rather than use the orient="vertical" attribute, put it in the theme CSS file - this allows other themes to override that style, if they want the text beside the icon. @@ +7,5 @@ > +<hbox id="preferences-home" flex="1"> > + > + <button label="&paneGeneral.title;" orient="vertical" class="options-box" > + oncommand="gotoPref('general-content');"> > + <image class="options-icon general-img" id="paneGeneral"/> The general-img class and paneGeneral ID are redundant - use one or the other (. @@ +8,5 @@ > + > + <button label="&paneGeneral.title;" orient="vertical" class="options-box" > + oncommand="gotoPref('general-content');"> > + <image class="options-icon general-img" id="paneGeneral"/> > + <p>&paneGeneral.title;</p> <p> is HTML, not XUL. If you want HTML, use <html:p>; if you want XUL, use <label> or <description>. @@ +44,5 @@ > + > + > + <button label="&paneSync.title;" orient="vertical" class="options-box" > + oncommand="gotoPref('sync-content');"> > + <image class="sync-img" id="paneSecurity"/> This is missing the options-icon class. ::: browser/components/preferences/in-content/preferences.xul @@ +15,5 @@ > + - the Preferences dialog in a browsing session, so we work around the problem > + - by putting it here instead. > + --> > +<?xml-stylesheet href="chrome://browser/content/preferences/in-content/handlers.css"?> > +<?xml-stylesheet href="chrome://browser/skin/preferences/applications.css"?> This should be part of the patch in bug 733469. @@ +79,5 @@ > + </stringbundleset> > + > + <hbox id="header"> > + <toolbarbutton id="back-btn" class="nav-button header-button" > + oncommand="cmd_back()" tooltiptext="Go back one page" disabled="true"/> The tooltiptext string needs to be localized (put it in a .dtd file). @@ +81,5 @@ > + <hbox id="header"> > + <toolbarbutton id="back-btn" class="nav-button header-button" > + oncommand="cmd_back()" tooltiptext="Go back one page" disabled="true"/> > + <toolbarbutton id="forward-btn" class="nav-button header-button" > + oncommand="cmd_forward()" tooltiptext="Go forward one page" disabled="true"/> Ditto. ::: browser/themes/gnomestripe/preferences/in-content/preferences.css @@ +13,5 @@ > +#header { > + margin-bottom: 18px; > +} > + > +.options-text { This appears to be unused. @@ +18,5 @@ > + margin: 0; > + padding: 0; > +} > + > +.options-box:hover { This should go after .options-box, not before it. @@ +22,5 @@ > +.options-box:hover { > + cursor: pointer; > +} > + > +.options-box { .options-box is a bit too generic, and doesn't describe what it's for (it's the preferences - *everything* is an option). Strawman naming proposal: .landingbutton Similarly for the related classes: .landingbutton-icon and .landingbutton-text @@ +30,5 @@ > + box-shadow: none; > + width: 32px; > +} > + > +.options-box p { This should just use a class added to the label (is that what .options-text was meant for?). @@ +84,5 @@ > + background-image: url("chrome://browser/skin/preferences/Options-sync.png"); > + background-repeat: no-repeat; > +} > + > +/* General Pane Styles */ The styles/comments about specific panes should be in there associated patches. Although, there are some generic styles here (eg, caption, .indent) that seem very generic - they should stay here, but not be under a comment saying they're only for a specific pane. @@ +110,5 @@ > + > +/* Privacy Pane Styles */ > +.heading { > + height: 65px; > + background-color: rgba(192,199,210,0.70); 0.7, not 0.70 @@ +115,5 @@ > + font-size: 2.75em; > + font-weight: bold; > + -moz-padding-start: 25px; > + padding-top: 10px; > + border-radius: 5px 5px 0px 0px; Whenever there's a 0px, use 0 instead. @@ +137,5 @@ > + width: 48px; > +} > + > +/* Miscellaneous */ > +#addon-home { This doesn't appear to be used. @@ +141,5 @@ > +#addon-home { > + padding: 20px; > +} > + > +.nav-button { Add a comment here stating that these styles can be removed once bug 660726 lands. ::: browser/themes/pinstripe/preferences/in-content/preferences.css @@ +95,5 @@ > +.indent { > + -moz-margin-start: 50px; > +} > + > +/* Applications Pane Styles */ Styles for individual panes should be in their relevant patches. ::: browser/themes/winstripe/jar.mn @@ +87,4 @@ > #endif > skin/classic/browser/preferences/saveFile.png (preferences/saveFile.png) > * skin/classic/browser/preferences/preferences.css (preferences/preferences.css) > +* skin/classic/browser/preferences/in-content/preferences.css (preferences/in-content/preferences.css) Ditto with the other jar.mn file - no need for the * @@ +257,4 @@ > #endif > skin/classic/aero/browser/preferences/saveFile.png (preferences/saveFile-aero.png) > * skin/classic/aero/browser/preferences/preferences.css (preferences/preferences.css) > +* skin/classic/aero/browser/preferences/in-content/preferences.css (preferences/in-content/preferences.css) And again.

(In reply to Blair McBride (:Unfocused) from comment #17) > ::: browser/components/preferences/in-content/jar.mn > @@ +1,4 @@ > > +browser.jar: > > +* content/browser/preferences/in-content/home.js > > +* content/browser/preferences/in-content/landing.xul > > +* content/browser/preferences/in-content/preferences.xul > \ No newline at end of file > > \ No newline at end of file > > None of these files use the text preprocessor - remove the * Er, sorry, preferences.xul does indeed use the preprocessor - you need the * for that file. Also, that file needs a newline at the end (in case that wasn't obvious).

Comment on attachment 607806 [details] [diff] [review] in-content landing pane Review of attachment 607806 [details] [diff] [review]: ----------------------------------------------------------------- I'll clear my feedback request since Blair has provided a pretty thorough feedback pass already :)

I have made it through the novel and hopefully all is good :)

Comment on attachment 608437 [details] [diff] [review] in-content landing pane Review of attachment 608437 [details] [diff] [review]: ----------------------------------------------------------------- Dao: Can you do a feedback pass on the CSS here? ::: browser/components/preferences/in-content/preferences.xul @@ +86,5 @@ > + </stringbundleset> > + > + <hbox id="header"> > + <toolbarbutton id="back-btn" class="nav-button header-button" > + oncommand="cmd_back()" tooltiptext="&buttonBack.tooltip;" nit: these attributes should line up with the first attribute on the line above them.

fixed toolbarbutton indentation

Comment on attachment 608459 [details] [diff] [review] in-content landing pane Review of attachment 608459 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/components/preferences/in-content/landing.xul @@ +6,5 @@ > + > +<hbox id="preferences-home" flex="1"> > + > + <button label="&paneGeneral.title;" class="landingButton" > + oncommand="gotoPref('general-content');"> these need extra indentation too. sorry for missing these earlier.

fixed indentation in landing.xul

added styles for consistent title, also fixed https://bugzilla.mozilla.org/show_bug.cgi?id=738796 comment #1

added feedback from other patches, included search vbox

Patch Order: Landing, inContent Whitelist, General, Tabs, Privacy, Advanced, Applications, Content, Sync, Security, Search, Switch Pref

This seems to be an old version of the patch - the other patches assume home.js has been renamed to preferences.jsm, for instance. Could you attach a newer version?

(In reply to Blair McBride (:Unfocused) from comment #28) > This seems to be an old version of the patch - the other patches assume > home.js has been renamed to preferences.jsm, for instance. Could you attach > a newer version? Er, I think you mean preferences.js (not .jsm).

hopefully this one is correct, otherwise I will have to do some investigating...

Comment on attachment 609560 [details] [diff] [review] landing in-content pane Review of attachment 609560 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/components/preferences/in-content/jar.mn @@ +1,3 @@ > +browser.jar: > +* content/browser/preferences/in-content/preferences.js > +* content/browser/preferences/in-content/landing.xul Does landing.xul require the preprocessor? It looks like it doesn't. ::: browser/components/preferences/in-content/landing.xul @@ +42,5 @@ > + <image class="preference-icon" type="security"/> > + <label class="landingButton-label">&paneSecurity.title;</label> > + </button> > + > + nit: Can you remove the extra blank line where there are two blank lines and any whitespace characters that are on their own line?

(In reply to Jared Wein [:jaws] from comment #9) > > +a.inline-link:hover { > > + cursor: pointer; > > Why is this rule necessary? Doesn't the default style of a:hover have > |cursor:pointer;|? I have no idea why the default style (:-moz-any-link) isn't applying here, to give the pointer cursor. Unless someone else has an idea, we can just leave in that style (but don't use the .inline-link class). In which case, you should file a separate bug to fix that properly - and mention that in a comment in the CSS file.

contains style for privacy link fix and feedback fixed

Comment on attachment 610387 [details] [diff] [review] landing in-content patch Review of attachment 610387 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/components/preferences/in-content/landing.xul @@ +2,5 @@ > + - License, v. 2.0. If a copy of the MPL was not distributed with this file, > + - You can obtain one at http://mozilla.org/MPL/2.0/. --> > + > +<vbox data-category="landing" hidden="false"> > + <html:h1 class="indent-small">Firefox</html:h1> This should be changed back to &brandShortName; ::: browser/themes/gnomestripe/preferences/in-content/preferences.css @@ +93,5 @@ > + -moz-box-align: center; > +} > + > +/* This style is for bug 740213 and should be removed once that > + bug has a solution */ nit: period at the end of the sentence for this and the other theme files. @@ +95,5 @@ > + > +/* This style is for bug 740213 and should be removed once that > + bug has a solution */ > +.mouse-pointer { > + cursor: pointer; nit: two space indentation for this and the other theme files.

sorry forgot I changed that to firefox for taking screenshots, also changed IDEs and forgot to change the spacing to 2 spaces but it is all fixed

This should be the correct patch :)

Comment on attachment 610400 [details] [diff] [review] landing in-content patch Review of attachment 610400 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/components/preferences/in-content/landing.xul @@ +14,5 @@ > + </button> > + > + <button label="&paneTabs.title;" class="landingButton" > + oncommand="gotoPref('tabs');"> > + <image class="preference-icon" type="tabs"/> Class should be landingButton-icon, to match the other classes. ::: browser/components/preferences/in-content/preferences.js @@ +10,5 @@ > + > +Components.utils.import("resource://gre/modules/XPCOMUtils.jsm"); > + > +function init_all() { > + window.history.replaceState("landing",document.title); Space after the comma. @@ +16,5 @@ > + updateCommands(); > +} > + > +function gotoPref(page) { > + window.history.pushState(page,document.title); Space after the comma. @@ +32,5 @@ > +function onStatePopped(aEvent) { > + updateCommands(); > + // TODO To ensure we can't go forward again we put an additional entry > + // for the current state into the history. Ideally we would just strip > + // the history but there doesn't seem to be a way to do that. Bug 590661 Remove this comment, it doesn't relate to anything here. @@ +33,5 @@ > + updateCommands(); > + // TODO To ensure we can't go forward again we put an additional entry > + // for the current state into the history. Ideally we would just strip > + // the history but there doesn't seem to be a way to do that. Bug 590661 > + search(aEvent.state, 'data-category'); Double quotes ("). @@ +40,5 @@ > +function updateCommands() { > + if(canGoBack()) { > + document.getElementById("back-btn").disabled = false; > + } else { > + document.getElementById("back-btn").disabled = true; document.getElementById("back-btn").disabled = !canGoBack(); @@ +46,5 @@ > + > + if(canGoForward()) { > + document.getElementById("forward-btn").disabled = false; > + } else { > + document.getElementById("forward-btn").disabled = true; Similar to above. ::: browser/components/preferences/in-content/preferences.xul @@ +13,5 @@ > + > +<!-- XXX This should be in applications.xul, but bug 393953 means putting it > + - there causes the Applications pane not to work the first time you open > + - the Preferences dialog in a browsing session, so we work around the problem > + - by putting it here instead. Remove this comment - it's not relevant anymore, since you're not using overlays here. @@ +16,5 @@ > + - the Preferences dialog in a browsing session, so we work around the problem > + - by putting it here instead. > + --> > +<?xml-stylesheet > + href="chrome://browser/content/preferences/in-content/handlers.css"?> This doesn't appear to be added to the /in-content/ folder in any of the patches (ie, it should just be pointing to chrome://browser/content/preferences/handlers.css) @@ +60,5 @@ > +#define USE_WIN_TITLE_STYLE > +#endif > + > +<page > + onload="init_all();" No newline between "<page" and "onload=" @@ +64,5 @@ > + onload="init_all();" > + xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul" > + xmlns:html="http://www.w3.org/1999/xhtml" > +#ifdef USE_WIN_TITLE_STYLE > + title="&prefWindow.titleWin;"> Fix the indenting - should be in line with the other attributes. @@ +66,5 @@ > + xmlns:html="http://www.w3.org/1999/xhtml" > +#ifdef USE_WIN_TITLE_STYLE > + title="&prefWindow.titleWin;"> > +#else > + title="&prefWindow.titleGNOME;"> Ditto. @@ +81,5 @@ > + src="chrome://browser/locale/preferences/preferences.properties"/> > + > + <stringbundleset id="appManagerBundleset"> > + <stringbundle id="appManagerBundle" > + src="chrome://browser/locale/preferences/applicationManager.properties"/> This <stringbundle> needs indented by an additional 2 spaces. @@ +86,5 @@ > + </stringbundleset> > + > + <hbox id="header"> > + <toolbarbutton > + id="back-btn" class="nav-button header-button" No newline between "<toolbarbutton" and "id=" @@ +90,5 @@ > + id="back-btn" class="nav-button header-button" > + oncommand="cmd_back()" tooltiptext="&buttonBack.tooltip;" > + disabled="true"/> > + <toolbarbutton > + id="forward-btn" class="nav-button header-button" Ditto. ::: browser/themes/gnomestripe/jar.mn @@ +70,4 @@ > skin/classic/browser/preferences/Options-sync.png (preferences/Options-sync.png) > #endif > * skin/classic/browser/preferences/preferences.css (preferences/preferences.css) > + skin/classic/browser/preferences/in-content/preferences.css (preferences/in-content/preferences.css) Just use one space between "preferences.css" and "(" ::: browser/themes/gnomestripe/preferences/in-content/preferences.css @@ +16,5 @@ > + -moz-box-align: center; > + -moz-box-orient: vertical; > + border: none; > + background: none; > + box-shadow: none; You don't need to set the border, background, and box-shadow properties to none in gnomestripe (they don't have any affect in this case, because buttons are styled based on the system theme by default). @@ +77,5 @@ > + -moz-margin-start: 50px; > +} > + > +.spacing { > + margin: 0 25px; As far as I can tell, this class is only used by bug 724686, and that usage should be removed. So this should be removed too. Applies to the other themes too. @@ +92,5 @@ > + margin-bottom: 15px; > + -moz-box-align: center; > +} > + > +/* This style is for bug 740213 and should be removed once that Add "XXX" to the start of this comment - it's a Mozilla convention used to make comments describing todo items easier to find. Applies to the other themes too. @@ +94,5 @@ > +} > + > +/* This style is for bug 740213 and should be removed once that > + bug has a solution. */ > +.mouse-pointer { To make sure this works for all links, you should select based on the tag (instead of adding a class): html:a { } And you'll need to add the following to the top of the CSS file: @namespace html "http://www.w3.org/1999/xhtml"; (Applies to all themes.) @@ +99,5 @@ > + cursor: pointer; > +} > + > + > +/* Styles Below can be removed once bug 660726 lands */ Same with this comment. Also, these styles are copied from the wrong theme (winstripe, rather than gnomestripe). gnomestripe (the Linux theme) uses system icons, and doesn't alter the appearance of the buttons. You'll need to use the styles from: https://mxr.mozilla.org/mozilla-central/source/toolkit/themes/gnomestripe/mozapps/extensions/extensions.css#273 And: https://mxr.mozilla.org/mozilla-central/source/toolkit/themes/gnomestripe/mozapps/extensions/extensions.css#940 ::: browser/themes/pinstripe/jar.mn @@ +96,4 @@ > #endif > skin/classic/browser/preferences/saveFile.png (preferences/saveFile.png) > * skin/classic/browser/preferences/preferences.css (preferences/preferences.css) > + skin/classic/browser/preferences/in-content/preferences.css (preferences/in-content/preferences.css) Just use one space between "preferences.css" and "(" ::: browser/themes/pinstripe/preferences/in-content/preferences.css @@ +96,5 @@ > +} > + > +/* Styles Below can be removed once bug 660726 lands */ > +.nav-button { > + list-style-image: url(chrome://mozapps/skin/extensions/navigation.png); Again, these styles seem to be copied from winstripe, not pinstripe. See https://mxr.mozilla.org/mozilla-central/source/toolkit/themes/pinstripe/mozapps/extensions/extensions.css Note: Part of the style there requires it be run through the text preprocessor, so you'll need to add a '*' to the start of the line for this file in browser/themes/pinstripe/jar.mn ::: browser/themes/winstripe/jar.mn @@ +89,4 @@ > #endif > skin/classic/browser/preferences/saveFile.png (preferences/saveFile.png) > * skin/classic/browser/preferences/preferences.css (preferences/preferences.css) > + skin/classic/browser/preferences/in-content/preferences.css (preferences/in-content/preferences.css) The "(" should line up with the "(" on the previous line. @@ +263,4 @@ > #endif > skin/classic/aero/browser/preferences/saveFile.png (preferences/saveFile-aero.png) > * skin/classic/aero/browser/preferences/preferences.css (preferences/preferences.css) > + skin/classic/aero/browser/preferences/in-content/preferences.css (preferences/in-content/preferences.css) The "(" should line up with the "(" on the previous line. ::: browser/themes/winstripe/preferences/in-content/preferences.css @@ +121,5 @@ > +.header-button[disabled="true"] { > + opacity: 0.8; > +} > + > +.header-button { Not all of the relevant styles were copied, so the pressed (active) state is missing, and the disabled state doesn't look very disabled. See https://mxr.mozilla.org/mozilla-central/source/toolkit/themes/winstripe/mozapps/extensions/extensions.css#1176 and onwards.

removed vbox around heading and fixed blairs feedback

added dispatcher for event Initialized to this patch at the end of init_all

moved Initialized dispatcher here, and later saw feedback in test patch about moving all the events code to the bottom of the init_all function

Comment on attachment 614529 [details] [diff] [review] landing in-content patch w/ Initialized dispatcher Review of attachment 614529 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/components/preferences/in-content/jar.mn @@ +1,2 @@ > +browser.jar: > +* content/browser/preferences/in-content/preferences.js preferences.js doesn't need run through the prepocessor, remove the *. ::: browser/components/preferences/in-content/landing.xul @@ +1,5 @@ > +<!-- This Source Code Form is subject to the terms of the Mozilla Public > + - License, v. 2.0. If a copy of the MPL was not distributed with this file, > + - You can obtain one at http://mozilla.org/MPL/2.0/. --> > + > +<vbox data-category="landing" hidden="false"> Remove hidden="false" - it doesn't do anything. ::: browser/components/preferences/in-content/preferences.js @@ +8,5 @@ > +const Ci = Components.interfaces; > +const Cu = Components.utils; > +const Cr = Components.results; > + > +Components.utils.import("resource://gre/modules/XPCOMUtils.jsm"); Since you're making a shorthand alias for Components.utils above, just use Cu.import() here. @@ +9,5 @@ > +const Cu = Components.utils; > +const Cr = Components.results; > + > +Components.utils.import("resource://gre/modules/XPCOMUtils.jsm"); > +Components.utils.import("resource://services-sync/service.js"); service.js should be imported in sync.js (bug 735091), as it's specific to Sync code. It's Services.jsm that should be imported here, since it's a generic module. @@ +15,5 @@ > +function init_all() { > + window.history.replaceState("landing", document.title); > + window.addEventListener("popstate", onStatePopped, true); > + updateCommands(); > + var initFinished = document.createEvent("Event"); Nit: break up this code with a blank line after updateCommands(). ::: browser/themes/gnomestripe/preferences/in-content/preferences.css @@ +34,5 @@ > + background-image: url("chrome://browser/skin/preferences/Options.png"); > + background-repeat: no-repeat; > +} > + > +.preference-icon { The only thing that differs between this style and .landingButton-icon is the margin - merge the common properties into one style. Same applies to pinstripe and winstripe.

(In reply to Blair McBride (:Unfocused) from comment #42) > @@ +15,5 @@ > > +function init_all() { > > + window.history.replaceState("landing", document.title); > > + window.addEventListener("popstate", onStatePopped, true); > > + updateCommands(); > > + var initFinished = document.createEvent("Event"); > > Nit: break up this code with a blank line after updateCommands(). Ugh, but that would bitrot all the other patches... feel free to ignore it.

Comment on attachment 614529 [details] [diff] [review] landing in-content patch w/ Initialized dispatcher Review of attachment 614529 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/themes/gnomestripe/preferences/in-content/preferences.css @@ +106,5 @@ > + > +/* XXX This style is for bug 740213 and should be removed once that > + bug has a solution. */ > +description > html:a { > + cursor: pointer; Did you check this worked? :) It should be html|a

Comment on attachment 614529 [details] [diff] [review] landing in-content patch w/ Initialized dispatcher Review of attachment 614529 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/themes/gnomestripe/preferences/in-content/preferences.css @@ +88,5 @@ > + font-size: 20px; > +} > + > +.indent { > + -moz-margin-start: 50px; I keep forgetting to ask: Why did you give this a value of 50px? It's overriding an existing style that sets it to 23px.

Notes: 1. Preferences.js does need to be run through the preprocessor because of the tabs init function (not added till tabs patch is added ontop of landing). 2. removed indent and indents-small from preferences.css 3. Does not include new gnomestripe styling

Comment on attachment 616995 [details] [diff] [review] landing in-content patch Review of attachment 616995 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/components/preferences/in-content/preferences.js @@ +11,5 @@ > + > +Cu.import("resource://gre/modules/XPCOMUtils.jsm"); > +Cu.import("resource://gre/modules/Services.jsm"); > + > +function init_all() { We should set document.documentElement.instantApply = true here (or as an attribute on the <page> element if that would work) so that the in-content preferences aren't dependent on the about:config pref being true.

sets instantApply to true at the beginning of the init_all function

Comment on attachment 619326 [details] [diff] [review] landing in-content patch Review of attachment 619326 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/themes/gnomestripe/preferences/in-content/preferences.css @@ +18,5 @@ > + -moz-box-align: center; > + -moz-box-orient: vertical; > + border: none; > + background: none; > + box-shadow: none; Nit: border, background, and box-shadow should be removed, as they have no affect here (I mentioned this in comment 37). Only applies to gnomestripe.

Comment on attachment 619326 [details] [diff] [review] landing in-content patch Review of attachment 619326 [details] [diff] [review]: ----------------------------------------------------------------- Just realised how badly this works on a small screen :( Really need to make the <prefpane> scroll. The following CSS should do the job: prefpane > .content-box { overflow: auto; }

(In reply to Blair McBride (:Unfocused) from comment #51) > Comment on attachment 619326 [details] [diff] [review] > landing in-content patch > > Review of attachment 619326 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: browser/themes/gnomestripe/preferences/in-content/preferences.css > @@ +18,5 @@ > > + -moz-box-align: center; > > + -moz-box-orient: vertical; > > + border: none; > > + background: none; > > + box-shadow: none; > > Nit: border, background, and box-shadow should be removed, as they have no > affect here (I mentioned this in comment 37). Only applies to gnomestripe. If I don't include those styles I get a bunch of buttons (I attached an image of what I see)

shows what I see with removing border: none, box-shadow: none, background: none styles in windows

added style prefpane > .content-box { overflow:auto }

removed nits from comment 53 from gnomestripe

Comment on attachment 616460 [details] screenshot of new style for linux Jon: Could you file a new bug (blocking bug 738796) for the landing page improvements on Linux, and attach this screenshot there? (And a patch if you have one, even unfinished)

\o/ Pushed to mozilla-inbound. These patches should be merged to mozilla-central within the next day or two. Thanks! https://hg.mozilla.org/integration/mozilla-inbound/rev/f1148c7cd5e3