Last Comment Bug 723737 - Move the advanced preferences to in-content UI
: Move the advanced preferences to in-content UI
Status: VERIFIED FIXED
[testday-20120622]
:
Product: Firefox
Classification: Client Software
Component: Preferences (show other bugs)
: Trunk
: All All
: -- normal (vote)
: Firefox 15
Assigned To: MSU Capstone Team
:
Mentors:
Depends on: 949589
Blocks: 718011
  Show dependency treegraph
 
Reported: 2012-02-02 15:16 PST by Jared Wein [:jaws] (please needinfo? me)
Modified: 2013-12-12 11:06 PST (History)
9 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
in-content "advanced" tab (25.41 KB, patch)
2012-02-10 14:26 PST, Zuhao(Joe) Chen
jaws: feedback+
bmcbride: feedback+
Details | Diff | Review
in-content advanced pane (46.84 KB, patch)
2012-03-06 11:34 PST, Jon Rietveld
jaws: feedback+
Details | Diff | Review
advanced in-content pane (46.52 KB, patch)
2012-03-13 14:19 PDT, Jon Rietveld
bmcbride: feedback+
Details | Diff | Review
advanced in-content pane (45.09 KB, patch)
2012-03-22 14:57 PDT, Jon Rietveld
jaws: feedback+
Details | Diff | Review
advanced in-content pane (44.60 KB, patch)
2012-03-22 15:32 PDT, Jon Rietveld
no flags Details | Diff | Review
advanced in-content pane (44.60 KB, patch)
2012-03-22 15:42 PDT, Jon Rietveld
jaws: feedback+
Details | Diff | Review
advanced in-content pane (44.76 KB, patch)
2012-03-24 20:06 PDT, Jon Rietveld
no flags Details | Diff | Review
advanced in-content pane (47.25 KB, patch)
2012-03-25 23:03 PDT, Jon Rietveld
jaws: feedback+
Details | Diff | Review
Inner vboxs removed (45.82 KB, patch)
2012-03-27 17:34 PDT, MSU Capstone Team
jaws: feedback+
Details | Diff | Review
advanced in-content patch (44.83 KB, patch)
2012-04-08 22:50 PDT, Jon Rietveld
jaws: feedback+
Details | Diff | Review
Advanced with updated data-category (44.84 KB, patch)
2012-04-10 11:06 PDT, MSU Capstone Team
no flags Details | Diff | Review
advanced in-content patch for new landing patch (44.85 KB, patch)
2012-04-12 12:38 PDT, Jon Rietveld
no flags Details | Diff | Review
advanced in-content patch for new landing patch (44.95 KB, patch)
2012-04-12 13:19 PDT, Jon Rietveld
jaws: feedback+
bmcbride: feedback+
Details | Diff | Review
patch for testing (44.95 KB, patch)
2012-04-18 18:12 PDT, Jon Rietveld
no flags Details | Diff | Review
advanced in-content patch (44.95 KB, patch)
2012-04-20 09:25 PDT, Jon Rietveld
jaws: review+
bmcbride: review+
Details | Diff | Review

Description Jared Wein [:jaws] (please needinfo? me) 2012-02-02 15:16:40 PST

    
Comment 1 Zuhao(Joe) Chen 2012-02-10 14:26:08 PST
Created attachment 596169 [details] [diff] [review]
in-content "advanced" tab

In-content "Advanced" Tab for feedback. Not much styling or relayout done but will add in asap. Buttons in "Network" sub-tab are not working. Please advise. In the mockup we've seen, the whole tab is kinda divided into several pages. For now, should i go ahead and divide "advanced" page into sub pages accordingly?
Comment 2 Blair McBride [:Unfocused] (mostly unavailable, needinfo open, reviews not) 2012-02-12 20:55:47 PST
(In reply to Zuhao(Joe) Chen from comment #1)
> Buttons in "Network" sub-tab are not working. Please advise.

See bug 724686 comment 6 for some debugging tips.

Looks like you're missing a string bungle with an ID of "bundlePreferences", which is in the original preferences.

For some background info:
https://developer.mozilla.org/en/XUL_Tutorial/Property_Files


> In the mockup we've seen, the whole tab is kinda divided into
> several pages. For now, should i go ahead and divide "advanced" page into
> sub pages accordingly?

Yes, although they're not truely "pages" - the contents of those tabs will be in one list, and the subpages are just filters on that list (see bug 719717 comment 21 and bug 719717 comment 25). The main Advanced page should be just what's currently the "General" tab - but with some extra buttons to change the view to what is currently either the "Network", "Update", or "Encryption" tabs. And that will have to work as filters like what the main sections are built on.
Comment 3 Blair McBride [:Unfocused] (mostly unavailable, needinfo open, reviews not) 2012-02-12 21:39:29 PST
Comment on attachment 596169 [details] [diff] [review]
in-content "advanced" tab

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

::: browser/components/about/AboutRedirector.cpp
@@ +107,5 @@
>      nsIAboutModule::ALLOW_SCRIPT },
>    { "permissions", "chrome://browser/content/preferences/aboutPermissions.xul",
>      nsIAboutModule::ALLOW_SCRIPT },
> +  { "advanced", "chrome://browser/content/preferences/in-content-advanced.xul",
> +    nsIAboutModule::ALLOW_SCRIPT },    

Don't leave any trailing whitespace.

::: browser/components/preferences/advanced.js
@@ +199,5 @@
>     */
>    showConnections: function ()
>    {
> +    openDialog("chrome://browser/content/preferences/connection.xul",
> +                                           "Connection Settings", "model=yes", null);

Second argument here should be "mozilla:connectionmanager" or something similar. Though, saying that, you should be able to eventually make that just another sub-section, rather than a dialog (same with most, but probably not all, of these dialogs).

Style nit: The second line should be indented to where the first argument of the previous line starts.

::: browser/components/preferences/in-content-advanced.css
@@ +1,5 @@
> +@import url("chrome://global/skin/inContentUI.css");
> +
> +#page-root{
> +  -moz-box-align:center;
> +  -webkit-box-align:center;

One of the great things about the Firefox UI is that you don't need to develop for other browsers ;)

Some general CSS code style guidelines:
- Space before {
- Space after :

Also, eventually, most CSS will need moved into theme-specific files (winstripe, pinstripe, gnomestripe).

::: browser/components/preferences/in-content-advanced.xul
@@ +2,5 @@
> +
> +# In-Content "Advanced" tab
> +# Created by Michigan State University Capstone Team
> +
> +<!DOCTYPE overlay [

The document element is <page>, so this DOCTYPE should be for page.

@@ +17,5 @@
> +<?xml-stylesheet href="chrome://global/skin/" type="text/css"?>
> +
> +<page id="page-root" xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul">
> +
> +  <prefpane id="paneAdvanced" onpaneload="gAdvancedPane.init();" class="main-content">

I haven't explicitly tested, but calling gAdvancedPane.init() here probably isn't working - see bug 724686 comment 7.

@@ +24,5 @@
> +      <preference id="browser.preferences.advanced.selectedTabIndex"
> +                  name="browser.preferences.advanced.selectedTabIndex"
> +                  type="int"/>
> +
> +      <!--XXX button prefs -->

Same with the other patches, remove this comment. XXX typically denotes a todo item (or alerting you to a hack), which this isn't.

@@ +27,5 @@
> +
> +      <!--XXX button prefs -->
> +
> +      <!-- General tab -->
> +      <preference id="accessibility.browsewithcaret"   name="accessibility.browsewithcaret"   type="bool"/>

There's a lot of inconsistent line-wrapping/indenting style with these <preference> elements, this is a good time to clean that up.
Comment 4 Jon Rietveld 2012-03-06 11:34:53 PST
Created attachment 603384 [details] [diff] [review]
in-content advanced pane

Advanced patch split from huge privacy bug patch. Can't think of any major changes I made here...did comment out some JavaScript that was causing an error. I know we had talked about the code that was causing the issue but I was unsure if Joe already created a solution or not so just commented it out for now.
Comment 5 Jared Wein [:jaws] (please needinfo? me) 2012-03-08 21:52:45 PST
Comment on attachment 603384 [details] [diff] [review]
in-content advanced pane

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

::: browser/components/preferences/in-content/advanced.js
@@ +54,5 @@
> +
> +//    var extraArgs = window.arguments[1];
> +//    if (extraArgs && extraArgs["advancedTab"]){
> +//      advancedPrefs.selectedTab = document.getElementById(extraArgs["advancedTab"]);
> +//    } else {

These commented out lines should either be deleted or uncommented.

::: browser/components/preferences/in-content/advanced.xul
@@ +2,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/.
> +   -
> +   - Contributor(s):

Please remove the contributor list on MPL 2.0. Your name will be in the hg log :)

@@ +91,5 @@
> +    <stringbundle id="bundleBrand" src="chrome://branding/locale/brand.properties"/>
> +#endif
> +
> +<!-- id=advancedPrefs-->
> +<tabbox id="advanced-content" flex="1" hidden="true"

Hmmm.... I'm not sure how we want to handle <tabbox> with our search implementation. Thoughts?
Comment 6 Jon Rietveld 2012-03-13 14:19:52 PDT
Created attachment 605540 [details] [diff] [review]
advanced in-content pane

Fixed all issues addressed in Jared's latest feedback. I am going to address the tabset issue tonight. Going to make those into buttons and have them function more like the landing pane, but will always show the advanced - general "tab"/pane when you first go to the advanced pane
Comment 7 Blair McBride [:Unfocused] (mostly unavailable, needinfo open, reviews not) 2012-03-22 06:07:24 PDT
Comment on attachment 605540 [details] [diff] [review]
advanced in-content pane

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

::: browser/components/preferences/in-content/advanced.xul
@@ +2,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/.
> +   -
> +   - ***** END LICENSE BLOCK ***** -->

MPL2 doesn't use the BEGIN/END LICENSE BLOCK bits. See https://www.mozilla.org/MPL/headers/ for exact text to use.

@@ +12,5 @@
> +
> +  <!-- General tab -->
> +  <preference id="accessibility.browsewithcaret"   name="accessibility.browsewithcaret"   type="bool"/>
> +  <preference id="accessibility.typeaheadfind"     name="accessibility.typeaheadfind"     type="bool"/>
> +  <preference id="accessibility.blockautorefresh"  name="accessibility.blockautorefresh"  type="bool"/>

There's a mix of formatting/indentation styles for all these <preferences> - make them all like the very first one for consistency.

@@ +79,5 @@
> +          name="security.OCSP.disable_button.managecrl"
> +          type="bool"/>
> +<preference id="security.disable_button.openDeviceManager"
> +          name="security.disable_button.openDeviceManager"
> +          type="bool"/>

These should be indented, as they're children of <preferences>. Also, you broke the indentation of the wrapped lines.

@@ +87,5 @@
> +    <stringbundle id="bundleShell" src="chrome://browser/locale/shellservice.properties"/>
> +    <stringbundle id="bundleBrand" src="chrome://branding/locale/brand.properties"/>
> +#endif
> +
> +<!-- id=advancedPrefs-->

Remove this.

::: browser/components/preferences/in-content/preferences.xul
@@ +74,5 @@
>        src="chrome://browser/content/preferences/in-content/privacy.js"/>
>    <script type="application/javascript"
>       src="chrome://browser/content/preferences/in-content/tabs.js"/>
> +  <script type="application/javascript"
> +     src="chrome://browser/content/preferences/in-content/advanced.js"/>

Load this in advanced.xul instead.
Comment 8 Jared Wein [:jaws] (please needinfo? me) 2012-03-22 11:17:06 PDT
Comment on attachment 605540 [details] [diff] [review]
advanced in-content pane

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

I'll clear my feedback request since Blair has provided a pretty thorough feedback pass already :)
Comment 9 Jon Rietveld 2012-03-22 14:57:07 PDT
Created attachment 608491 [details] [diff] [review]
advanced in-content pane

all feedback from Blair has been fixed, I'm starting to hate the tab button... ;)
Comment 10 Jared Wein [:jaws] (please needinfo? me) 2012-03-22 15:07:05 PDT
Comment on attachment 608491 [details] [diff] [review]
advanced in-content pane

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

::: browser/components/preferences/in-content/landing.xul
@@ +4,4 @@
>  
>  <html:h1>&brandShortName;</html:h1>
>  
> +<hbox id="preferences-home" flex="1" hidden="false">

hidden="false" should be the default value. is this change needed for something to work correctly?
Comment 11 Jon Rietveld 2012-03-22 15:32:26 PDT
Created attachment 608502 [details] [diff] [review]
advanced in-content pane

duno how that got in there :S but it is now removed
Comment 12 Jon Rietveld 2012-03-22 15:42:36 PDT
Created attachment 608509 [details] [diff] [review]
advanced in-content pane

fixed jar file
Comment 13 Jon Rietveld 2012-03-24 20:06:24 PDT
Created attachment 609060 [details] [diff] [review]
advanced in-content pane

added consistent title
Comment 14 Jon Rietveld 2012-03-25 23:03:51 PDT
Created attachment 609221 [details] [diff] [review]
advanced in-content pane

included search vbox's, and fixed title
Comment 15 Jared Wein [:jaws] (please needinfo? me) 2012-03-27 11:42:20 PDT
Comment on attachment 609221 [details] [diff] [review]
advanced in-content pane

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

::: browser/components/preferences/in-content/advanced.xul
@@ +148,5 @@
> +      <vbox data-category="advanced" hidden="true">
> +        <tabpanel id="generalPanel" orient="vertical">
> +
> +          <!-- Accessibility -->
> +          <vbox data-category="advanced" hidden="true">

I don't know think we'll need these <vbox>s within the tabbox. I think we'll need to find a different widget to use other than a tabbox for freetext searching, so for now just put the <vbox> around the tabbox and remove the <vbox>s inside of the tabbox.
Comment 16 MSU Capstone Team 2012-03-27 17:34:20 PDT
Created attachment 609965 [details] [diff] [review]
Inner vboxs removed

vboxs inside the tabbox have been removed for purposes of pane switching
Comment 17 Jared Wein [:jaws] (please needinfo? me) 2012-03-27 17:41:40 PDT
Comment on attachment 609965 [details] [diff] [review]
Inner vboxs removed

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

::: browser/components/preferences/in-content/advanced.xul
@@ +144,5 @@
> +
> +    <tabpanels flex="1">
> +
> +      <!-- General -->
> +        <tabpanel id="generalPanel" orient="vertical">

These tabpanels are now indented two spaces too much. When the <vbox>s got removed, all the nested code should have had two spaces removed.
Comment 18 Jon Rietveld 2012-04-08 22:50:40 PDT
Created attachment 613230 [details] [diff] [review]
advanced in-content patch

removed vbox from heading, fixed jareds feedback, removed vbox around 1st child elements and added data-category and hidden to 1st child element instead
Comment 19 MSU Capstone Team 2012-04-10 11:06:09 PDT
Created attachment 613675 [details] [diff] [review]
Advanced with updated data-category
Comment 20 Jon Rietveld 2012-04-12 12:38:29 PDT
Created attachment 614501 [details] [diff] [review]
advanced in-content patch for new landing patch
Comment 21 Jon Rietveld 2012-04-12 13:19:07 PDT
Created attachment 614533 [details] [diff] [review]
advanced in-content patch for new landing patch
Comment 22 Blair McBride [:Unfocused] (mostly unavailable, needinfo open, reviews not) 2012-04-16 00:22:56 PDT
Comment on attachment 614533 [details] [diff] [review]
advanced in-content patch for new landing patch

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

::: browser/components/preferences/in-content/advanced.js
@@ +278,5 @@
> +                   permissionType   : "offline-app",
> +                   manageCapability : Components.interfaces.nsIPermissionManager.DENY_ACTION,
> +                   windowTitle      : bundlePreferences.getString("offlinepermissionstitle"),
> +                   introText        : bundlePreferences.getString("offlinepermissionstext") };
> +    openDialog("chrome://browser/content/preferences/permissions.xul",

In changing this to use openDialog instead of openWindow, it's not longer passing the 'params' argument (set directly above).

Which inevitably results in the following exception being (rightfully) thrown:

Error: aParams is null
Source file: chrome://browser/content/preferences/permissions.js
Line: 151
Comment 23 Jon Rietveld 2012-04-18 18:12:34 PDT
Created attachment 616399 [details] [diff] [review]
patch for testing
Comment 24 Jon Rietveld 2012-04-20 09:25:01 PDT
Created attachment 616997 [details] [diff] [review]
advanced in-content patch

updated openDialog to include params
Comment 25 Jared Wein [:jaws] (please needinfo? me) 2012-05-08 22:39:45 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/68e7c139e580
Comment 26 Ed Morley [:emorley] 2012-05-09 07:42:51 PDT
https://hg.mozilla.org/mozilla-central/rev/68e7c139e580
Comment 27 Alfredos-Panagiotis Damkalis [:fredy] 2012-06-22 04:35:20 PDT
I verify fixed this bug, tested at Mozilla/5.0 (X11; Linux x86_64; rv:15.0) Gecko/20120621 Firefox/15.0a2.

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