Closed Bug 723737 Opened 12 years ago Closed 12 years ago

Move the advanced preferences to in-content UI

Categories

(Firefox :: Settings UI, defect)

defect
Not set
normal

Tracking

()

VERIFIED FIXED
Firefox 15

People

(Reporter: jaws, Assigned: owenccarpenter)

References

Details

(Whiteboard: [testday-20120622])

Attachments

(1 file, 14 obsolete files)

44.95 KB, patch
jaws
: review+
Unfocused
: review+
Details | Diff | Splinter Review
      No description provided.
Attached patch in-content "advanced" tab (obsolete) — Splinter Review
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?
Attachment #596169 - Flags: feedback?(jwein)
Attachment #596169 - Flags: feedback?(bmcbride)
(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 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.
Attachment #596169 - Flags: feedback?(bmcbride) → feedback+
Attachment #596169 - Flags: feedback?(jwein) → feedback+
Attached patch in-content advanced pane (obsolete) — Splinter Review
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.
Attachment #603384 - Flags: feedback?(jwein)
Attachment #603384 - Flags: feedback?(bmcbride)
Attachment #596169 - Attachment is obsolete: true
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?
Attachment #603384 - Flags: feedback?(jwein) → feedback+
Attached patch advanced in-content pane (obsolete) — Splinter Review
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
Attachment #605540 - Flags: feedback?(jwein)
Attachment #605540 - Flags: feedback?(bmcbride)
Attachment #603384 - Attachment is obsolete: true
Attachment #603384 - Flags: feedback?(bmcbride)
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.
Attachment #605540 - Flags: feedback?(bmcbride) → feedback+
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 :)
Attachment #605540 - Flags: feedback?(jwein)
Attached patch advanced in-content pane (obsolete) — Splinter Review
all feedback from Blair has been fixed, I'm starting to hate the tab button... ;)
Attachment #605540 - Attachment is obsolete: true
Attachment #608491 - Flags: feedback?(jwein)
Attachment #608491 - Flags: feedback?(bmcbride)
Assignee: joejoevictor → jon.rietveld
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?
Attachment #608491 - Flags: feedback?(jwein) → feedback+
Attached patch advanced in-content pane (obsolete) — Splinter Review
duno how that got in there :S but it is now removed
Attachment #608491 - Attachment is obsolete: true
Attachment #608491 - Flags: feedback?(bmcbride)
Attachment #608502 - Flags: feedback?(jwein)
Attachment #608502 - Flags: feedback?(bmcbride)
Attached patch advanced in-content pane (obsolete) — Splinter Review
fixed jar file
Attachment #608502 - Attachment is obsolete: true
Attachment #608502 - Flags: feedback?(jwein)
Attachment #608502 - Flags: feedback?(bmcbride)
Attachment #608509 - Flags: feedback?(jwein)
Attachment #608509 - Flags: feedback?(bmcbride)
Attachment #608509 - Flags: feedback?(jwein) → feedback+
Attached patch advanced in-content pane (obsolete) — Splinter Review
added consistent title
Attachment #608509 - Attachment is obsolete: true
Attachment #608509 - Flags: feedback?(bmcbride)
Attachment #609060 - Flags: feedback?(jwein)
Attachment #609060 - Flags: feedback?(bmcbride)
Attached patch advanced in-content pane (obsolete) — Splinter Review
included search vbox's, and fixed title
Attachment #609060 - Attachment is obsolete: true
Attachment #609060 - Flags: feedback?(jwein)
Attachment #609060 - Flags: feedback?(bmcbride)
Attachment #609221 - Flags: feedback?(jwein)
Attachment #609221 - Flags: feedback?(bmcbride)
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.
Attachment #609221 - Flags: feedback?(jwein) → feedback+
Attached patch Inner vboxs removed (obsolete) — Splinter Review
vboxs inside the tabbox have been removed for purposes of pane switching
Attachment #609965 - Flags: feedback?(jwein)
Attachment #609965 - Flags: feedback?(bmcbride)
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.
Attachment #609965 - Flags: feedback?(jwein) → feedback+
Attachment #609221 - Attachment is obsolete: true
Attachment #609221 - Flags: feedback?(bmcbride)
Assignee: jon.rietveld → owenccarpenter
Attached patch advanced in-content patch (obsolete) — Splinter Review
removed vbox from heading, fixed jareds feedback, removed vbox around 1st child elements and added data-category and hidden to 1st child element instead
Attachment #613230 - Flags: feedback?(jwein)
Attachment #613230 - Flags: feedback?(bmcbride)
Attachment #609965 - Attachment is obsolete: true
Attachment #609965 - Flags: feedback?(bmcbride)
Attachment #613230 - Flags: feedback?(jwein) → feedback+
Attachment #613230 - Attachment is obsolete: true
Attachment #613230 - Flags: feedback?(bmcbride)
Attachment #614501 - Flags: feedback?(jwein)
Attachment #614501 - Flags: feedback?(bmcbride)
Attachment #614501 - Attachment is obsolete: true
Attachment #614501 - Flags: feedback?(jwein)
Attachment #614501 - Flags: feedback?(bmcbride)
Attachment #614533 - Flags: feedback?(jwein)
Attachment #614533 - Flags: feedback?(bmcbride)
Attachment #614533 - Flags: feedback?(jwein) → feedback+
Attachment #613675 - Attachment is obsolete: true
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
Attachment #614533 - Flags: feedback?(bmcbride) → feedback+
Attached patch patch for testing (obsolete) — Splinter Review
updated openDialog to include params
Attachment #614533 - Attachment is obsolete: true
Attachment #616399 - Attachment is obsolete: true
Attachment #616997 - Flags: feedback?(bmcbride)
Attachment #616997 - Flags: review+
Attachment #616997 - Flags: feedback?(bmcbride) → review+
https://hg.mozilla.org/mozilla-central/rev/68e7c139e580
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
I verify fixed this bug, tested at Mozilla/5.0 (X11; Linux x86_64; rv:15.0) Gecko/20120621 Firefox/15.0a2.
Status: RESOLVED → VERIFIED
Whiteboard: [testday-20120622]
Depends on: 949589
You need to log in before you can comment on or make changes to this bug.