Closed
Bug 724686
Opened 13 years ago
Closed 13 years ago
General pane for in-content Preferences
Categories
(Firefox :: Settings UI, defect)
Firefox
Settings UI
Tracking
()
VERIFIED
FIXED
Firefox 15
People
(Reporter: owenccarpenter, Assigned: jon.rietveld)
References
Details
(Whiteboard: [testday-20120622])
Attachments
(1 file, 12 obsolete files)
27.19 KB,
patch
|
Unfocused
:
review+
jaws
:
feedback+
|
Details | Diff | Splinter Review |
User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_7_2) AppleWebKit/535.7 (KHTML, like Gecko) Chrome/16.0.912.77 Safari/535.7
Expected results:
In-content Preferences pane - General tab
Updated•13 years ago
|
Status: UNCONFIRMED → NEW
Component: Untriaged → Preferences
Ever confirmed: true
QA Contact: untriaged → preferences
Updated•13 years ago
|
Assignee: nobody → saylesd1
Blocks: 718011
Status: NEW → ASSIGNED
OS: Mac OS X → All
Hardware: x86 → All
Comment 1•13 years ago
|
||
This is the first patch I've submitted myself, so hopefully it's done correctly!
I'm having trouble in a few areas: first, there is a button whose label is not getting populated, and I'm pretty sure I traced it back to the fact that gMainPane.init() (gMainPane is created in general.xul, the function defined in main.js) is somehow not getting called, so then the updateUseCurrentContentButton function call within the init is also not made. I tried putting a dump statement in the init function and got nothing, and I cannot figure out why init is being called.
Also, I don't think General has the right fonts, though it's using inContentUI.css.
Any other feedback is appreciated. Thanks!
Attachment #595182 -
Flags: feedback?(jwein)
Attachment #595182 -
Flags: feedback?(bmcbride)
Comment 2•13 years ago
|
||
Comment on attachment 595182 [details] [diff] [review]
first patch for General pane of in-content prefs
Review of attachment 595182 [details] [diff] [review]:
-----------------------------------------------------------------
(In reply to Devan Sayles from comment #1)
> Created attachment 595182 [details] [diff] [review]
> first patch for General pane of in-content prefs
>
> This is the first patch I've submitted myself, so hopefully it's done
> correctly!
Yep, you've done it correctly and the about:general page looks nice so far. Thanks!
> I'm having trouble in a few areas: first, there is a button whose label is
> not getting populated, and I'm pretty sure I traced it back to the fact that
> gMainPane.init() (gMainPane is created in general.xul, the function defined
> in main.js) is somehow not getting called, so then the
> updateUseCurrentContentButton function call within the init is also not
> made. I tried putting a dump statement in the init function and got nothing,
> and I cannot figure out why init is being called.
I don't think the event handler for onpaneload is being called. This might happen because the file isn't being included through the use of a <prefpane src="" />. Can you try using onload="gMainPane.init()" here and see if that makes a difference?
> Also, I don't think General has the right fonts, though it's using
> inContentUI.css.
about:general looks like it has the correct fonts to me. Are you specifically talking about the font-size of the captions in the fieldset? I do see that general.css sets the size of that font to 20px.
> Any other feedback is appreciated. Thanks!
General feedback: Take a read through https://developer.mozilla.org/en/Writing_Efficient_CSS and https://wiki.mozilla.org/DevTools/CSSTips for tips on writing the CSS and what will be accepted as CSS selectors.
Thanks for getting the ball rolling on the General preferences part.
Attachment #595182 -
Flags: feedback?(jwein) → feedback+
Comment 3•13 years ago
|
||
(In reply to Jared Wein [:jaws] (Vacation Feb. 8 - Feb. 10) from comment #2)
> Comment on attachment 595182 [details] [diff] [review]
> first patch for General pane of in-content prefs
>
> Review of attachment 595182 [details] [diff] [review]:
> -----------------------------------------------------------------
>
>
> > I'm having trouble in a few areas: first, there is a button whose label is
> > not getting populated, and I'm pretty sure I traced it back to the fact that
> > gMainPane.init() (gMainPane is created in general.xul, the function defined
> > in main.js) is somehow not getting called, so then the
> > updateUseCurrentContentButton function call within the init is also not
> > made. I tried putting a dump statement in the init function and got nothing,
> > and I cannot figure out why init is being called.
>
> I don't think the event handler for onpaneload is being called. This might
> happen because the file isn't being included through the use of a <prefpane
> src="" />. Can you try using onload="gMainPane.init()" here and see if that
> makes a difference?
>
I'm not sure I understand what you mean by that. I tried adding a src attribute to the prefpane (with the value being the location of the javascript file), and the patch already had an onpaneload="gMainPane.init();" attribute in the prefpane, but neither of these seem to make a difference. I'm not sure how to test if the event handler for onpaneload is being called (besides seeing if the button in question gets a label), or what else I should be experimenting with to try and fix the problem.
I did find some documentation here (http://www.openxul.com/references/ref_prefpane.html) that talks about using the src attribute in connection with an overlay file, and I took the overlay out of general.xul, so might that be related to the issue at all?
Comment 4•13 years ago
|
||
(In reply to Devan Sayles from comment #3)
> (In reply to Jared Wein [:jaws] (Vacation Feb. 8 - Feb. 10) from comment #2)
> > Comment on attachment 595182 [details] [diff] [review]
> > first patch for General pane of in-content prefs
> >
> > Review of attachment 595182 [details] [diff] [review]:
> > -----------------------------------------------------------------
> >
> >
> > > I'm having trouble in a few areas: first, there is a button whose label is
> > > not getting populated, and I'm pretty sure I traced it back to the fact that
> > > gMainPane.init() (gMainPane is created in general.xul, the function defined
> > > in main.js) is somehow not getting called
> > I don't think the event handler for onpaneload is being called. This might
> > happen because the file isn't being included through the use of a <prefpane
> > src="" />. Can you try using onload="gMainPane.init()" here and see if that
> > makes a difference?
> >
> I'm not sure I understand what you mean by that.
The function gMainPane.init() is only called when the onpaneload event is fired. It could be that this event isn't being fired, and that's why your code isn't running.
The way that you are testing if the code is running is perfectly sound.
Can you try replacing the attribute name of onpaneload with onload? For example, how it currently says <prefpane onpaneload="gMainPane.init()" ... >, it would now say <prefpane onload="gMainPane.init()" ... >.
If that still doesn't work, can you try moving the <script src="main.js"/> tag down to the end of the XUL file and adding some inline script after it to call gMainPane.init()? For example:
> <script src="main.js" />
> <script>
> gMainPane.init();
> </script>
Comment 5•13 years ago
|
||
(In reply to Jared Wein [:jaws] (Vacation Feb. 8 - Feb. 10) from comment #4)
> (In reply to Devan Sayles from comment #3)
> > (In reply to Jared Wein [:jaws] (Vacation Feb. 8 - Feb. 10) from comment #2)
> > > Comment on attachment 595182 [details] [diff] [review]
> > > first patch for General pane of in-content prefs
> > >
> > > Review of attachment 595182 [details] [diff] [review]:
> > > -----------------------------------------------------------------
> > >
> > >
Changing onpaneload to onload didn't seem to have an effect, but moving the script src tag to the end of the file and adding an inline call to init finally populated the button label! But I was wondering why it is necessary to explicitly make that call to init, when it is made first in the prefpane tag?
Also, upon further testing of the General pane, I have noticed that a number of the button events are not being properly triggered when the buttons are pressed - currently, only the "Choose" button actually appears to react properly when pressed. Do you have advice for troubleshooting this issue?
Comment 6•13 years ago
|
||
(In reply to Devan Sayles from comment #5)
> Do you have advice for troubleshooting this issue?
The Error Console shows (most) uncaught exceptions, as well as various warnings and info messages: https://developer.mozilla.org/en/Error_Console
Pressing the "Manage Add-ons" button, for instance, will result in the following exception:
Error: openUILinkIn is not defined
Source file: chrome://browser/content/preferences/main.js
Line: 449
Another handy tool is MXR, which is a searchable index of the source code: https://mxr.mozilla.org/mozilla-central/
Searching for an identifier (function) named "openUILinkIn":
https://mxr.mozilla.org/mozilla-central/ident?i=openUILinkIn
... shows that it's defined in utilityOverlay.js, which is loaded by the original preferences dialog.
Comment 7•13 years ago
|
||
(In reply to Jared Wein [:jaws] (Vacation Feb. 8 - Feb. 10) from comment #4)
> Can you try replacing the attribute name of onpaneload with onload? For
> example, how it currently says <prefpane onpaneload="gMainPane.init()" ...
> >, it would now say <prefpane onload="gMainPane.init()" ... >.
...
(In reply to Devan Sayles from comment #5)
> Changing onpaneload to onload didn't seem to have an effect
Should be adding onload="gMainPane.init();" to the <page> element, not the <prefpane> element. Just tested that, and it works fine.
Comment 8•13 years ago
|
||
Comment on attachment 595182 [details] [diff] [review]
first patch for General pane of in-content prefs
Review of attachment 595182 [details] [diff] [review]:
-----------------------------------------------------------------
::: browser/components/preferences/general.xul
@@ +40,5 @@
> +#
> +# ***** END LICENSE BLOCK *****
> +
> +<?xml-stylesheet href="chrome://global/skin/global.css" type="text/css"?>
> +<?xml-stylesheet href="chrome://global/skin/inContentUI.css" type="text/css"?>
inContentUI.css should be loaded via @import in a platform-specific (ie, not /content/) stylesheet (see bug 719717 comment 17 for reasoning).
@@ +43,5 @@
> +<?xml-stylesheet href="chrome://global/skin/global.css" type="text/css"?>
> +<?xml-stylesheet href="chrome://global/skin/inContentUI.css" type="text/css"?>
> +<?xml-stylesheet href="chrome://browser/content/preferences/general.css"?>
> +
> +<!DOCTYPE overlay [
The document element is a <page>, so the DOCTYPE should be for page (not overlay).
@@ +46,5 @@
> +
> +<!DOCTYPE overlay [
> + <!ENTITY % brandDTD SYSTEM "chrome://branding/locale/brand.dtd">
> + <!ENTITY % mainDTD SYSTEM "chrome://browser/locale/preferences/main.dtd">
> + <!ENTITY % aboutHomeDTD SYSTEM "chrome://browser/locale/aboutHome.dtd">
Why is aboutHome.dtd here?
@@ +63,5 @@
> +
> + <script type="application/javascript" src="chrome://browser/content/preferences/main.js"/>
> +
> + <preferences id="mainPreferences">
> + <!-- XXX Button preferences -->
While you're touching this code, this comment can be removed (XXX typically denotes a todo item, which this clearly isn't... and it doesn't seem related to anything).
@@ +126,5 @@
> + <menulist id="browserStartupPage" preference="browser.startup.page">
> + <menupopup>
> + <menuitem label="&startupHomePage.label;" value="1" id="browserStartupHomePage"/>
> + <menuitem label="&startupBlankPage.label;" value="0" id="browserStartupBlank"/>
> + <menuitem label="&startupLastSession.label;" value="3" id="browserStartupLastSession"/>
This would be a good time to clean up inconsistent indenting like this.
Attachment #595182 -
Flags: feedback?(bmcbride) → feedback+
Assignee | ||
Comment 9•13 years ago
|
||
general patch split off form huge privacy bug patch.
*Note: I did create an init method to add all the panes init methods in.
Attachment #603386 -
Flags: feedback?(jwein)
Attachment #603386 -
Flags: feedback?(bmcbride)
Updated•13 years ago
|
Attachment #595182 -
Attachment is obsolete: true
Updated•13 years ago
|
Attachment #603386 -
Attachment is patch: true
Comment 10•13 years ago
|
||
Comment on attachment 603386 [details] [diff] [review]
in-content general pane
Review of attachment 603386 [details] [diff] [review]:
-----------------------------------------------------------------
::: browser/components/preferences/in-content/main.xul
@@ +87,5 @@
> + type="int"/>
> +
> + </preferences>
> +
> + <vbox id="general-content" hidden="true">
These shouldn't have a containing vbox. There should be a vbox containing each preference with an attribute designating which category it belongs to.
Attachment #603386 -
Flags: feedback?(jwein) → feedback+
Assignee | ||
Comment 11•13 years ago
|
||
Fixed all issues addressed in Jared's latest feedback.
Attachment #605538 -
Flags: feedback?(jwein)
Attachment #605538 -
Flags: feedback?(bmcbride)
Assignee | ||
Comment 12•13 years ago
|
||
I have not worked on the JavaScript to fix the use current page button, but plan to do that tonight
Assignee | ||
Comment 13•13 years ago
|
||
Fixed javascript for Use Current Page Button. Modified javascript is in the function _getTabsForHomePage.
Attachment #603386 -
Attachment is obsolete: true
Attachment #605538 -
Attachment is obsolete: true
Attachment #603386 -
Flags: feedback?(bmcbride)
Attachment #605538 -
Flags: feedback?(jwein)
Attachment #605538 -
Flags: feedback?(bmcbride)
Attachment #606082 -
Flags: feedback?(jwein)
Attachment #606082 -
Flags: feedback?(bmcbride)
Assignee | ||
Comment 14•13 years ago
|
||
Fixed javascript for Use Current Page Button. Modified javascript is in the function _getTabsForHomePage + changed licenses.
Attachment #606082 -
Attachment is obsolete: true
Attachment #606082 -
Flags: feedback?(jwein)
Attachment #606082 -
Flags: feedback?(bmcbride)
Attachment #606091 -
Flags: feedback?(jwein)
Attachment #606091 -
Flags: feedback?(bmcbride)
Updated•13 years ago
|
Assignee: saylesd1 → jon.rietveld
Comment 15•13 years ago
|
||
Comment on attachment 606091 [details] [diff] [review]
general in-content pane
Review of attachment 606091 [details] [diff] [review]:
-----------------------------------------------------------------
::: browser/components/preferences/in-content/main.js
@@ +1,3 @@
> +/*-- 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/. */
Super nitpicky, but this should be:
/* 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/. */
See https://www.mozilla.org/MPL/headers/ for more examples.
@@ +158,5 @@
> + // We should only include visible & non-pinned tabs
> +
> + tabs = win.gBrowser.visibleTabs.slice(win.gBrowser._numPinnedTabs);
> +
> + for (var i=0; i<tabs.length; i++) {
nit: spaces around = and < operators.
@@ +159,5 @@
> +
> + tabs = win.gBrowser.visibleTabs.slice(win.gBrowser._numPinnedTabs);
> +
> + for (var i=0; i<tabs.length; i++) {
> + if(tabs[i].linkedBrowser.currentURI.spec.toString() == "about:preferences") {
nit: space after if.
@@ +160,5 @@
> + tabs = win.gBrowser.visibleTabs.slice(win.gBrowser._numPinnedTabs);
> +
> + for (var i=0; i<tabs.length; i++) {
> + if(tabs[i].linkedBrowser.currentURI.spec.toString() == "about:preferences") {
> + tabs.splice(i,1);
Does |i| need to be decremented here? What happens if you have two about:preference tabs opened next to each other? To make this code simpler and less error-prone, you should use Array.filter here (https://developer.mozilla.org/en/JavaScript/Reference/Global_Objects/Array/filter).
@@ +451,5 @@
> + * Displays the Add-ons Manager.
> + */
> + showAddonsMgr: function ()
> + {
> + openUILinkIn("about:addons", "window");
This won't need to open about:addons in a window since we're now in a tab. Can you change this to open about:addons in a tab (instead of a window) now?
Attachment #606091 -
Flags: feedback?(jwein) → feedback+
Assignee | ||
Comment 16•13 years ago
|
||
update javascript to use filter and update about:addon to open in new tab
Attachment #606091 -
Attachment is obsolete: true
Attachment #606091 -
Flags: feedback?(bmcbride)
Attachment #608456 -
Flags: feedback?(jwein)
Attachment #608456 -
Flags: feedback?(bmcbride)
Comment 17•13 years ago
|
||
Comment on attachment 608456 [details] [diff] [review]
in-content general pane
Review of attachment 608456 [details] [diff] [review]:
-----------------------------------------------------------------
::: browser/components/preferences/in-content/main.js
@@ +170,5 @@
> + * Check to see if a tab is not about:preferences
> + */
> + isAboutPreferences: function (element, index, array)
> + {
> + return (!(element.linkedBrowser.currentURI.spec.toString() == "about:preferences"));
could this just be:
> return element.linkedBrowser.currentURI.spec.toString() != "about:preferences";
Attachment #608456 -
Flags: feedback?(jwein) → feedback+
Assignee | ||
Comment 18•13 years ago
|
||
yup, that makes much more sense haha
Attachment #608456 -
Attachment is obsolete: true
Attachment #608456 -
Flags: feedback?(bmcbride)
Attachment #608528 -
Flags: feedback?(jwein)
Attachment #608528 -
Flags: feedback?(bmcbride)
Assignee | ||
Comment 19•13 years ago
|
||
added consistent title
Attachment #608528 -
Attachment is obsolete: true
Attachment #608528 -
Flags: feedback?(jwein)
Attachment #608528 -
Flags: feedback?(bmcbride)
Attachment #609061 -
Flags: feedback?(jwein)
Attachment #609061 -
Flags: feedback?(bmcbride)
Assignee | ||
Comment 20•13 years ago
|
||
included search vbox's, and fixed title
Attachment #609061 -
Attachment is obsolete: true
Attachment #609061 -
Flags: feedback?(jwein)
Attachment #609061 -
Flags: feedback?(bmcbride)
Attachment #609222 -
Flags: feedback?(jwein)
Attachment #609222 -
Flags: feedback?(bmcbride)
Updated•13 years ago
|
Attachment #609222 -
Flags: feedback?(jwein) → feedback+
Comment 21•13 years ago
|
||
Comment on attachment 609222 [details] [diff] [review]
general in-content pane
Review of attachment 609222 [details] [diff] [review]:
-----------------------------------------------------------------
::: browser/components/preferences/in-content/jar.mn
@@ +2,5 @@
> * content/browser/preferences/in-content/preferences.js
> * content/browser/preferences/in-content/landing.xul
> * content/browser/preferences/in-content/preferences.xul
> +* content/browser/preferences/in-content/main.xul
> +* content/browser/preferences/in-content/main.js
main.js doesn't need run through the preprocessor, remove the *
::: browser/components/preferences/in-content/main.js
@@ +109,5 @@
> + setHomePageToBookmark: function ()
> + {
> + var rv = { urls: null, names: null };
> + openDialog("chrome://browser/content/preferences/selectBookmark.xul",
> + "Select Bookmark", "resizable=yes, modal=yes", rv);
Fix indentation here (probably not lining up because there's a tab there instead of space characters).
@@ +159,5 @@
> + // We should only include visible & non-pinned tabs
> +
> + tabs = win.gBrowser.visibleTabs.slice(win.gBrowser._numPinnedTabs);
> +
> + filteredTabs = tabs.filter(this.isAboutPreferences);
No need to keep around multiple arrays for this, just re-use 'tabs'.
@@ +168,5 @@
> +
> + /**
> + * Check to see if a tab is not about:preferences
> + */
> + isAboutPreferences: function (element, index, array)
Names of arguments should be prefixed with 'a'. ie, aElement, aIndex, etc.
See https://developer.mozilla.org/En/Mozilla_Coding_Style_Guide#Prefixes
@@ +170,5 @@
> + * Check to see if a tab is not about:preferences
> + */
> + isAboutPreferences: function (element, index, array)
> + {
> + return (element.linkedBrowser.currentURI.spec.toString() != "about:preferences");
Don't think you should need the .toString() call here, .spec is already a string.
::: browser/components/preferences/in-content/main.xul
@@ +57,5 @@
> +
> +<vbox data-category="general" hidden="true">
> + <hbox class="heading">
> + <image class="preference-icon" type="general"/>
> + <html:h1 class="spacing">&paneGeneral.title;</html:h1>
Remove the 'spacing' class (I saw the privacy pane patch has it removed, so it's not used, right?)
@@ +69,5 @@
> +
> + <hbox align="center">
> + <label value="&startupPage.label;"
> + accesskey="&startupPage.accesskey;"
> + control="browserStartupPage"/>
Indentation is messed up for wrapped lines from here onwards.
Attachment #609222 -
Flags: feedback?(bmcbride) → feedback+
Assignee | ||
Comment 22•13 years ago
|
||
removed vbox from heading, fixed blairs feedback, removed vbox around 1st child elements and added data-category and hidden to 1st child element instead
Attachment #609222 -
Attachment is obsolete: true
Attachment #613227 -
Flags: feedback?(jwein)
Attachment #613227 -
Flags: feedback?(bmcbride)
Reporter | ||
Comment 23•13 years ago
|
||
Comment 24•13 years ago
|
||
Comment on attachment 613227 [details] [diff] [review]
general in-content patch
Review of attachment 613227 [details] [diff] [review]:
-----------------------------------------------------------------
::: browser/components/preferences/in-content/main.js
@@ +147,5 @@
> + var win;
> + var tabs = [];
> +
> + const Cc = Components.classes, Ci = Components.interfaces;
> + // If we're in instant-apply mode, use the most recent browser window
This comment doesn't apply anymore, since instantApply will always be true with in-content preferences.
Attachment #613227 -
Flags: feedback?(jwein) → feedback+
Assignee | ||
Comment 25•13 years ago
|
||
took in account jareds feedback and made adjustments for new landing patch
Attachment #613227 -
Attachment is obsolete: true
Attachment #613227 -
Flags: feedback?(bmcbride)
Attachment #614498 -
Flags: feedback?(jwein)
Attachment #614498 -
Flags: feedback?(bmcbride)
Assignee | ||
Comment 26•13 years ago
|
||
Attachment #614498 -
Attachment is obsolete: true
Attachment #614498 -
Flags: feedback?(jwein)
Attachment #614498 -
Flags: feedback?(bmcbride)
Attachment #614530 -
Flags: feedback?(jwein)
Attachment #614530 -
Flags: feedback?(bmcbride)
Updated•13 years ago
|
Attachment #614530 -
Flags: feedback?(jwein) → feedback+
Updated•13 years ago
|
Attachment #613677 -
Attachment is obsolete: true
Updated•13 years ago
|
Attachment #614530 -
Flags: feedback?(bmcbride) → review+
Comment 27•13 years ago
|
||
Target Milestone: --- → Firefox 15
Comment 28•13 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Comment 29•12 years ago
|
||
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]
You need to log in
before you can comment on or make changes to this bug.
Description
•