Move the advanced preferences to in-content UI

VERIFIED FIXED in Firefox 15

Status

()

Firefox
Preferences
VERIFIED FIXED
6 years ago
4 years ago

People

(Reporter: jaws, Assigned: MSU Capstone Team)

Tracking

(Blocks: 1 bug)

Trunk
Firefox 15
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [testday-20120622])

Attachments

(1 attachment, 14 obsolete attachments)

44.95 KB, patch
jaws
: review+
Unfocused
: review+
Details | Diff | Splinter Review
Comment hidden (empty)

Comment 1

6 years ago
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?
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+

Comment 4

6 years ago
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.
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+

Comment 6

6 years ago
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
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)

Comment 9

6 years ago
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... ;)
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+

Comment 11

6 years ago
Created attachment 608502 [details] [diff] [review]
advanced in-content pane

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)

Comment 12

6 years ago
Created attachment 608509 [details] [diff] [review]
advanced in-content pane

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+

Comment 13

6 years ago
Created attachment 609060 [details] [diff] [review]
advanced in-content pane

added consistent title
Attachment #608509 - Attachment is obsolete: true
Attachment #608509 - Flags: feedback?(bmcbride)
Attachment #609060 - Flags: feedback?(jwein)
Attachment #609060 - Flags: feedback?(bmcbride)

Comment 14

6 years ago
Created attachment 609221 [details] [diff] [review]
advanced in-content pane

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+
(Assignee)

Comment 16

6 years ago
Created attachment 609965 [details] [diff] [review]
Inner vboxs removed

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

Comment 18

5 years ago
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
Attachment #613230 - Flags: feedback?(jwein)
Attachment #613230 - Flags: feedback?(bmcbride)
(Assignee)

Comment 19

5 years ago
Created attachment 613675 [details] [diff] [review]
Advanced with updated data-category
Attachment #609965 - Attachment is obsolete: true
Attachment #609965 - Flags: feedback?(bmcbride)
Attachment #613230 - Flags: feedback?(jwein) → feedback+

Comment 20

5 years ago
Created attachment 614501 [details] [diff] [review]
advanced in-content patch for new landing patch
Attachment #613230 - Attachment is obsolete: true
Attachment #613230 - Flags: feedback?(bmcbride)
Attachment #614501 - Flags: feedback?(jwein)
Attachment #614501 - Flags: feedback?(bmcbride)

Comment 21

5 years ago
Created attachment 614533 [details] [diff] [review]
advanced in-content patch for new landing patch
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+

Comment 23

5 years ago
Created attachment 616399 [details] [diff] [review]
patch for testing

Comment 24

5 years ago
Created attachment 616997 [details] [diff] [review]
advanced in-content patch

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/integration/mozilla-inbound/rev/68e7c139e580
Target Milestone: --- → Firefox 15
https://hg.mozilla.org/mozilla-central/rev/68e7c139e580
Status: ASSIGNED → RESOLVED
Last Resolved: 5 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]

Updated

4 years ago
Depends on: 949589
You need to log in before you can comment on or make changes to this bug.