723737 - Move the advanced preferences to in-content UI

Status: VERIFIED FIXED

(Firefox :: Settings UI, defect)

Comment 1

[Zuhao(Joe) Chen]

Attachment: 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] (UNAVAILABLE)]

(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] (UNAVAILABLE)]

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]

Attachment: 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)]

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]

Attachment: 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] (UNAVAILABLE)]

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

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]

Attachment: 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)]

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]

Attachment: advanced in-content pane

duno how that got in there :S but it is now removed

Comment 12

[Jon Rietveld]

Attachment: advanced in-content pane

fixed jar file

Comment 13

[Jon Rietveld]

Attachment: advanced in-content pane

added consistent title

Comment 14

[Jon Rietveld]

Attachment: advanced in-content pane

included search vbox's, and fixed title

Comment 15

[Jared Wein [:jaws] (please needinfo? me)]

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]

Attachment: Inner vboxs removed

vboxs inside the tabbox have been removed for purposes of pane switching

Comment 17

[Jared Wein [:jaws] (please needinfo? me)]

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]

Attachment: 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]

Attachment: Advanced with updated data-category

Comment 20

[Jon Rietveld]

Attachment: advanced in-content patch for new landing patch

Comment 21

[Jon Rietveld]

Attachment: advanced in-content patch for new landing patch

Comment 22

[Blair McBride [:Unfocused] (UNAVAILABLE)]

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]

Attachment: patch for testing

Comment 24

[Jon Rietveld]

Attachment: advanced in-content patch

updated openDialog to include params

Comment 25

[Jared Wein [:jaws] (please needinfo? me)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/68e7c139e580

Comment 26

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/68e7c139e580

Comment 27

[Alfredos-Panagiotis Damkalis [:fredy]]

I verify fixed this bug, tested at Mozilla/5.0 (X11; Linux x86_64; rv:15.0) Gecko/20120621 Firefox/15.0a2.