Last Comment Bug 733473 - Implement initial prerequisites for in-content preferences, and landing page
: Implement initial prerequisites for in-content preferences, and landing page
Status: RESOLVED FIXED
:
Product: Firefox
Classification: Client Software
Component: Preferences (show other bugs)
: Trunk
: All All
: -- normal with 1 vote (vote)
: Firefox 15
Assigned To: Jon Rietveld
:
Mentors:
Depends on:
Blocks: 718011
  Show dependency treegraph
 
Reported: 2012-03-06 11:09 PST by Jon Rietveld
Modified: 2012-05-09 07:41 PDT (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
in-content landing page (18.30 KB, patch)
2012-03-06 11:14 PST, Jon Rietveld
jaws: feedback+
Details | Diff | Splinter Review
*correct* in-content landing (18.17 KB, patch)
2012-03-06 11:23 PST, Jon Rietveld
jaws: feedback+
Details | Diff | Splinter Review
landing in-content pane (15.05 KB, patch)
2012-03-13 14:10 PDT, Jon Rietveld
jaws: feedback+
Details | Diff | Splinter Review
landing in-content pane (14.73 KB, patch)
2012-03-13 16:25 PDT, Jon Rietveld
jaws: feedback+
Details | Diff | Splinter Review
landing in-content pane (14.63 KB, patch)
2012-03-16 07:38 PDT, Jon Rietveld
no flags Details | Diff | Splinter Review
in-content landing pane (22.10 KB, patch)
2012-03-18 21:39 PDT, Jon Rietveld
jaws: feedback-
Details | Diff | Splinter Review
in-content landing pane (17.95 KB, patch)
2012-03-19 14:14 PDT, Jon Rietveld
no flags Details | Diff | Splinter Review
in-content landing pane (24.39 KB, patch)
2012-03-20 18:05 PDT, Jon Rietveld
bmcbride: feedback+
Details | Diff | Splinter Review
in-content landing pane (22.39 KB, patch)
2012-03-22 13:10 PDT, Jon Rietveld
jaws: feedback+
Details | Diff | Splinter Review
in-content landing pane (22.41 KB, patch)
2012-03-22 13:56 PDT, Jon Rietveld
jaws: feedback+
Details | Diff | Splinter Review
landing in-content pane (22.45 KB, patch)
2012-03-22 16:18 PDT, Jon Rietveld
no flags Details | Diff | Splinter Review
landing in-content pane (23.46 KB, patch)
2012-03-24 20:08 PDT, Jon Rietveld
no flags Details | Diff | Splinter Review
landing in-content pane (24.81 KB, patch)
2012-03-25 23:05 PDT, Jon Rietveld
no flags Details | Diff | Splinter Review
landing in-content pane (24.81 KB, patch)
2012-03-26 17:44 PDT, Jon Rietveld
jaws: feedback+
Details | Diff | Splinter Review
landing in-content patch (25.22 KB, patch)
2012-03-28 18:34 PDT, Jon Rietveld
jaws: feedback+
Details | Diff | Splinter Review
landing in-content patch (25.22 KB, patch)
2012-03-28 19:42 PDT, Jon Rietveld
no flags Details | Diff | Splinter Review
landing in-content patch (25.22 KB, patch)
2012-03-28 20:00 PDT, Jon Rietveld
jaws: feedback+
bmcbride: feedback+
Details | Diff | Splinter Review
landing in-content patch (26.85 KB, patch)
2012-04-08 22:45 PDT, Jon Rietveld
jaws: feedback+
Details | Diff | Splinter Review
Landing with updated data-category (26.93 KB, patch)
2012-04-10 11:08 PDT, MSU Capstone Team
no flags Details | Diff | Splinter Review
landing in-content patch w/ Initialized dispatcher (27.02 KB, patch)
2012-04-12 12:35 PDT, Jon Rietveld
no flags Details | Diff | Splinter Review
landing in-content patch w/ Initialized dispatcher (27.02 KB, patch)
2012-04-12 13:17 PDT, Jon Rietveld
jaws: feedback+
bmcbride: feedback+
Details | Diff | Splinter Review
patch for testing (26.73 KB, patch)
2012-04-18 18:11 PDT, Jon Rietveld
no flags Details | Diff | Splinter Review
screenshot of new style for linux (124.80 KB, image/png)
2012-04-18 22:27 PDT, Jon Rietveld
no flags Details
landing in-content patch (26.73 KB, patch)
2012-04-20 09:22 PDT, Jon Rietveld
jaws: feedback+
Details | Diff | Splinter Review
landing in-content patch (26.78 KB, patch)
2012-04-28 10:37 PDT, Jon Rietveld
jaws: review+
bmcbride: review-
Details | Diff | Splinter Review
landing page screenshot (577.79 KB, image/png)
2012-05-07 09:21 PDT, Jon Rietveld
no flags Details
landing in-content patch (26.94 KB, patch)
2012-05-07 09:24 PDT, Jon Rietveld
no flags Details | Diff | Splinter Review
landing in-content patch (26.88 KB, patch)
2012-05-07 16:35 PDT, Jon Rietveld
bmcbride: review+
Details | Diff | Splinter Review

Description Jon Rietveld 2012-03-06 11:09:58 PST

    
Comment 1 Jon Rietveld 2012-03-06 11:14:19 PST
Created attachment 603367 [details] [diff] [review]
in-content landing page

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.
Comment 2 Jon Rietveld 2012-03-06 11:23:41 PST
Created attachment 603375 [details] [diff] [review]
*correct* in-content landing

Forgot to make one more correction for the applications patch to make sense while splitting them up.
Comment 3 Jared Wein [:jaws] (please needinfo? me) 2012-03-06 11:55:17 PST
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 4 Jared Wein [:jaws] (please needinfo? me) 2012-03-08 00:02:57 PST
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
Comment 5 Jon Rietveld 2012-03-13 14:10:40 PDT
Created attachment 605534 [details] [diff] [review]
landing in-content pane

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 6 Jared Wein [:jaws] (please needinfo? me) 2012-03-13 14:30:55 PDT
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.
Comment 7 Jon Rietveld 2012-03-13 16:25:58 PDT
Created attachment 605588 [details] [diff] [review]
landing in-content pane

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 8 Jared Wein [:jaws] (please needinfo? me) 2012-03-13 16:32:09 PDT
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 9 Jared Wein [:jaws] (please needinfo? me) 2012-03-13 20:06:28 PDT
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;|?
Comment 10 Blair McBride [:Unfocused] (mostly unavailable, needinfo open, reviews not) 2012-03-15 22:57:10 PDT
(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.
Comment 11 Jon Rietveld 2012-03-16 07:38:07 PDT
Created attachment 606574 [details] [diff] [review]
landing in-content pane

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 12 Jared Wein [:jaws] (please needinfo? me) 2012-03-16 13:05:11 PDT
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.
Comment 13 Jon Rietveld 2012-03-18 21:39:47 PDT
Created attachment 607063 [details] [diff] [review]
in-content landing pane

made changes for feedback in comment 10
Comment 14 Jared Wein [:jaws] (please needinfo? me) 2012-03-19 11:04:08 PDT
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.
Comment 15 Jon Rietveld 2012-03-19 14:14:07 PDT
Created attachment 607315 [details] [diff] [review]
in-content landing pane

added in-content/preferences.css files in gnomestripe, pinstripe, and winstripe. Modified the jar file to add these files as well.
Comment 16 Jon Rietveld 2012-03-20 18:05:07 PDT
Created attachment 607806 [details] [diff] [review]
in-content landing pane

had to fix some css styles that didn't get copied over for some reason...
Comment 17 Blair McBride [:Unfocused] (mostly unavailable, needinfo open, reviews not) 2012-03-21 19:05:43 PDT
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.
Comment 18 Blair McBride [:Unfocused] (mostly unavailable, needinfo open, reviews not) 2012-03-22 01:26:45 PDT
(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 19 Jared Wein [:jaws] (please needinfo? me) 2012-03-22 11:38:20 PDT
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 :)
Comment 20 Jon Rietveld 2012-03-22 13:10:06 PDT
Created attachment 608437 [details] [diff] [review]
in-content landing pane

I have made it through the novel and hopefully all is good :)
Comment 21 Jared Wein [:jaws] (please needinfo? me) 2012-03-22 13:21:35 PDT
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.
Comment 22 Jon Rietveld 2012-03-22 13:56:54 PDT
Created attachment 608459 [details] [diff] [review]
in-content landing pane

fixed toolbarbutton indentation
Comment 23 Jared Wein [:jaws] (please needinfo? me) 2012-03-22 16:02:23 PDT
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.
Comment 24 Jon Rietveld 2012-03-22 16:18:26 PDT
Created attachment 608527 [details] [diff] [review]
landing in-content pane

fixed indentation in landing.xul
Comment 25 Jon Rietveld 2012-03-24 20:08:04 PDT
Created attachment 609062 [details] [diff] [review]
landing in-content pane

added styles for consistent title, also fixed https://bugzilla.mozilla.org/show_bug.cgi?id=738796 comment #1
Comment 26 Jon Rietveld 2012-03-25 23:05:24 PDT
Created attachment 609223 [details] [diff] [review]
landing in-content pane

added feedback from other patches, included search vbox
Comment 27 Jon Rietveld 2012-03-26 16:49:26 PDT
Patch Order:
Landing,
inContent Whitelist,
General,
Tabs,
Privacy,
Advanced,
Applications,
Content,
Sync,
Security,
Search,
Switch Pref
Comment 28 Blair McBride [:Unfocused] (mostly unavailable, needinfo open, reviews not) 2012-03-26 17:36:38 PDT
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?
Comment 29 Jared Wein [:jaws] (please needinfo? me) 2012-03-26 17:39:23 PDT
(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).
Comment 30 Jon Rietveld 2012-03-26 17:44:45 PDT
Created attachment 609560 [details] [diff] [review]
landing in-content pane

hopefully this one is correct, otherwise I will have to do some investigating...
Comment 31 Jared Wein [:jaws] (please needinfo? me) 2012-03-26 17:51:46 PDT
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?
Comment 32 Blair McBride [:Unfocused] (mostly unavailable, needinfo open, reviews not) 2012-03-26 22:03:45 PDT
(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.
Comment 33 Jon Rietveld 2012-03-28 18:34:55 PDT
Created attachment 610387 [details] [diff] [review]
landing in-content patch

contains style for privacy link fix and feedback fixed
Comment 34 Jared Wein [:jaws] (please needinfo? me) 2012-03-28 19:35:18 PDT
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.
Comment 35 Jon Rietveld 2012-03-28 19:42:37 PDT
Created attachment 610399 [details] [diff] [review]
landing in-content patch

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
Comment 36 Jon Rietveld 2012-03-28 20:00:22 PDT
Created attachment 610400 [details] [diff] [review]
landing in-content patch

This should be the correct patch :)
Comment 37 Blair McBride [:Unfocused] (mostly unavailable, needinfo open, reviews not) 2012-04-02 07:54:40 PDT
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.
Comment 38 Jon Rietveld 2012-04-08 22:45:50 PDT
Created attachment 613226 [details] [diff] [review]
landing in-content patch

removed vbox around heading and fixed blairs feedback
Comment 39 MSU Capstone Team 2012-04-10 11:08:25 PDT
Created attachment 613681 [details] [diff] [review]
Landing with updated data-category
Comment 40 Jon Rietveld 2012-04-12 12:35:29 PDT
Created attachment 614497 [details] [diff] [review]
landing in-content patch w/ Initialized dispatcher

added dispatcher for event Initialized to this patch at the end of init_all
Comment 41 Jon Rietveld 2012-04-12 13:17:25 PDT
Created attachment 614529 [details] [diff] [review]
landing in-content patch w/ Initialized dispatcher

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 42 Blair McBride [:Unfocused] (mostly unavailable, needinfo open, reviews not) 2012-04-15 20:24:23 PDT
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.
Comment 43 Blair McBride [:Unfocused] (mostly unavailable, needinfo open, reviews not) 2012-04-15 20:46:41 PDT
(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 44 Blair McBride [:Unfocused] (mostly unavailable, needinfo open, reviews not) 2012-04-15 21:04:56 PDT
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 45 Blair McBride [:Unfocused] (mostly unavailable, needinfo open, reviews not) 2012-04-15 22:43:23 PDT
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.
Comment 46 Jon Rietveld 2012-04-18 18:11:35 PDT
Created attachment 616396 [details] [diff] [review]
patch for testing
Comment 47 Jon Rietveld 2012-04-18 22:27:20 PDT
Created attachment 616460 [details]
screenshot of new style for linux
Comment 48 Jon Rietveld 2012-04-20 09:22:43 PDT
Created attachment 616995 [details] [diff] [review]
landing in-content patch

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 49 Jared Wein [:jaws] (please needinfo? me) 2012-04-26 10:14:38 PDT
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.
Comment 50 Jon Rietveld 2012-04-28 10:37:32 PDT
Created attachment 619326 [details] [diff] [review]
landing in-content patch

sets instantApply to true at the beginning of the init_all function
Comment 51 Blair McBride [:Unfocused] (mostly unavailable, needinfo open, reviews not) 2012-05-06 21:49:54 PDT
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 52 Blair McBride [:Unfocused] (mostly unavailable, needinfo open, reviews not) 2012-05-06 23:34:10 PDT
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;
}
Comment 53 Jon Rietveld 2012-05-07 09:19:29 PDT
(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)
Comment 54 Jon Rietveld 2012-05-07 09:21:06 PDT
Created attachment 621627 [details]
landing page screenshot

shows what I see with removing border: none, box-shadow: none, background: none styles in windows
Comment 55 Jon Rietveld 2012-05-07 09:24:49 PDT
Created attachment 621630 [details] [diff] [review]
landing in-content patch

added style prefpane > .content-box { overflow:auto }
Comment 56 Jon Rietveld 2012-05-07 16:35:05 PDT
Created attachment 621783 [details] [diff] [review]
landing in-content patch

removed nits from comment 53 from gnomestripe
Comment 57 Blair McBride [:Unfocused] (mostly unavailable, needinfo open, reviews not) 2012-05-07 21:24:07 PDT
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)
Comment 58 Jared Wein [:jaws] (please needinfo? me) 2012-05-08 22:37:07 PDT
\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
Comment 59 Ed Morley [:emorley] 2012-05-09 07:41:43 PDT
https://hg.mozilla.org/mozilla-central/rev/f1148c7cd5e3

Note You need to log in before you can comment on or make changes to this bug.