Closed
Bug 274712
Opened 20 years ago
Closed 19 years ago
New Options Dialog
Categories
(Firefox :: Settings UI, defect, P1)
Firefox
Settings UI
Tracking
()
RESOLVED
FIXED
Future
People
(Reporter: bugs, Assigned: bugs)
References
(Depends on 1 open bug, Blocks 1 open bug, )
Details
Attachments
(4 files, 14 obsolete files)
142.31 KB,
application/zip
|
Details | |
43.55 KB,
image/png
|
Details | |
386.61 KB,
patch
|
Details | Diff | Splinter Review | |
174.23 KB,
patch
|
Details | Diff | Splinter Review |
Support instant apply prefs for MacOS X/GNOME, progress towards new options reorg (see URL).
Assignee | ||
Comment 1•20 years ago
|
||
progressmeter bindings, sample usage. Much to do remaining.
Assignee | ||
Comment 2•20 years ago
|
||
w00t. done in the quickest way possible. need to discuss with jst/hyatt/etc.
Assignee | ||
Comment 3•20 years ago
|
||
er, including the dom stuff too
Attachment #168838 -
Attachment is obsolete: true
Assignee | ||
Comment 4•20 years ago
|
||
sends xul-overlay-merged notification to supplied observer when load and merge is complete.
Attachment #168839 -
Attachment is obsolete: true
Assignee | ||
Comment 5•20 years ago
|
||
this time, showing the actual UI code.
Comment 6•20 years ago
|
||
Comment on attachment 168886 [details] [diff] [review] more >Index: browser/app/nsBrowserApp.cpp > static const nsXREAppData kAppData = { > "Mozilla", >- "Firefox", >+ "Firefox Debug", really? :-)
Assignee | ||
Comment 7•20 years ago
|
||
Fixes a notification issue with Forward Reference Resolution.
Attachment #168886 -
Attachment is obsolete: true
Comment 8•20 years ago
|
||
Comment on attachment 168946 [details] [diff] [review] patch crashes on OS X when trying to open the prefs window.
Assignee | ||
Comment 9•20 years ago
|
||
content pane, rough cut.
Attachment #168946 -
Attachment is obsolete: true
Assignee | ||
Comment 10•20 years ago
|
||
initial advanced pane work
Attachment #169159 -
Attachment is obsolete: true
Assignee | ||
Comment 11•20 years ago
|
||
more...
Assignee | ||
Updated•20 years ago
|
Attachment #169166 -
Attachment is obsolete: true
Comment 17•20 years ago
|
||
When re-designing the options dialog, maybe the possibility to turn on/off to send the referrer could be added. This is covered by bug 216596.
Comment 18•19 years ago
|
||
Hi, Ben. I fixed bug 95227. Currently, default font option is separated by langGroup. Be careful.
Assignee | ||
Comment 19•19 years ago
|
||
implements <preferences> element, new preferences UI, browser sanitize, viewconfig tweaks, etc.
Attachment #168767 -
Attachment is obsolete: true
Attachment #168849 -
Attachment is obsolete: true
Attachment #169426 -
Attachment is obsolete: true
Assignee | ||
Updated•19 years ago
|
Status: NEW → ASSIGNED
Flags: blocking-aviary1.1+
Priority: -- → P1
Target Milestone: --- → Firefox1.1
Assignee | ||
Updated•19 years ago
|
Comment 20•19 years ago
|
||
Ben, your latest patch doesn't take care of comment 18 (serif/sans-serif changes).
Comment 21•19 years ago
|
||
how do u implement a .diff file?
Comment 22•19 years ago
|
||
we'll have to fix the fonts thing after this hits trunk, it needs to go on sooner rather than later. Please file a bug as appropriate.
Comment 23•19 years ago
|
||
How about adding "Sanitize after xy days" and/or "Clear saved form information after xy days"?
Comment 24•19 years ago
|
||
Just a suggestion/request, is having the option to choose between the new and the current options dialogs a possibility?
Comment 25•19 years ago
|
||
(In reply to comment #24) > Just a suggestion/request, is having the option to choose between the new and > the current options dialogs a possibility? No. (Also, to keep this bug report readable and so as not to spam everyone watching it, please don't comment unless you're helping to fix this.)
Assignee | ||
Updated•19 years ago
|
Status: ASSIGNED → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED
Comment 26•19 years ago
|
||
(From attachment 174200 [details])
in
browser/locales/en-US/chrome/browser/preferences/sanitize.dtd
line 3
<!ENTITY items.label "Santize Items">
probably wants to be
<!ENTITY items.label "Sanitize Items">
Comment 27•19 years ago
|
||
In OS X version, a new OPtion window cannot be shut. (A shutting red button at the left of the title bar of Window cannot be selected. ) And the effect when the tab in the option window is switched might be a little heavy in OSX. Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; ja-JP; rv:1.8b2) Gecko/20050225 Firefox/1.0+
Comment 28•19 years ago
|
||
Ben, would it have feasable to get r= on this large patch? I know not the actions/rules of a "personal branch" merge, but from your checkin it sounds like this is actually being built right now, and I did not notice any r= for your branch comits... I was watching them in passing, and do not think your work is bad (In fact I like the overall design of the new options) though it is my personal belief that all code at least once should be reviewed.
Assignee | ||
Comment 29•19 years ago
|
||
Justin, please feel free to review the code located in mozilla/browser/components/preferences and mozilla/toolkit/content/widgets/preferences.xml
Comment 30•19 years ago
|
||
(In reply to comment #27) > In OS X version, a new OPtion window cannot be shut. > (A shutting red button at the left of the title bar of Window cannot be selected. ) filed bug 283660 on this (and the collapse toolbar button issue).
Comment 31•19 years ago
|
||
FYI, I keep a list of regressions observed after the new Options Window landed http://forums.mozillazine.org/viewforum.php?f=23 See "The latest Win32 build .... not yet out" threads
Comment 32•19 years ago
|
||
The new dialog looks extremely funny over here. It took me about five minutes to make this screenshot because when I click on the desktop or in other windows most (but not all) of the window disappears. Build is: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8b2) Gecko/20050223 Firefox/1.0+ with a new profile
Comment 33•19 years ago
|
||
This patch reverted the change made in bug 280007. Bug 283925 was filed on the issue.
Comment 34•19 years ago
|
||
This is the diff from current trunk of browser/components/preferences to prior to this bug's landing. This is to facilitate a better review from me, as per Ben's Request. Other Landing Changes forthcoming. For this patches Bonsai changes, See: http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=MozillaTinderboxAll&branch=HEAD&branchtype=match&dir=mozilla%2Fbrowser%2Fcomponents%2Fpreferences&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=1998-03-27+18%3A13%3A00&maxdate=2005-02-28+04%3A52&cvsroot=%2Fcvsroot
Comment 35•19 years ago
|
||
I think this is including every file which this Bug dealt with...it does cover the entire initial comit (not counting binary files) Comits to these files between my current version and dated version which may not apply (but will be included in my review for ease): mozilla/toolkit/components/viewconfig/content/config.js = Bug 281414 - global s/nsIPrefBranchInternal/nsIPrefBranch2/ rs=darin (did not change backwards-compatible code in extensions/irc extensions/venkman or extensions/inspector) mozilla/browser/base/content/browser-sets.inc 1.39 -- Bug 284140 - (regression from 237776) escape no longer freezes animated gifs. r=mconnor 1.38 -- Bug 237776 Reload should be disabled for blank tabs p=ryanvm@gmail.com r=mconnor 1.37 -- 283600 - reintroduce blake's nasty hack to allow findbar to work. (missing id) mozilla/browser/base/content/browser-menubar.inc Bug 237776 Reload should be disabled for blank tabs p=ryanvm@gmail.com r=mconnor mozilla/browser/base/content/browser.js 1.393 -- Bug 278772 - Various Firefox code incorrectly constructs URIs. patch by Gavin Sharp <gavin.sharp@gmail.com> r=mconnor sr=darin 1.392 -- 283944 - edit options... button on xpinstall info bar does not work 1.391 -- Bug 283262 "event is not defined" when pressing alt+enter in the url bar, (BrowserLoadURL) p=gavin.sharp@gmail.com r=mconnor 1.390 -- Bug 237776 Reload should be disabled for blank tabs p=ryanvm@gmail.com r=mconnor 1.389 -- attempt to fix leaks 1.388 -- trying to narrow down leaks 1.387 -- Bug 281414 - global s/nsIPrefBranchInternal/nsIPrefBranch2/ rs=darin (did not change backwards-compatible code in extensions/irc extensions/venkman or extensions/inspector) 1.386 -- New Options Dialoge mozilla/browser/base/content/sanitize.js 1.3 -- 283601 - cache button remains enabled after clicking. (make it reenable after 10 seconds). also, remove some spurious dumps Review of entire set forthcoming. As a note, Ben I do feel a review should have happend before initial landing, though since it has landed and has had more code changes since then, we should keep it on trunk through my review. Though if you feel overall that all code should get a look at before it is live, feel free to back this patch and the prior one out (as if I am right, nothing will break from a backout, but I cannot guarantee) Furthermore, I am not qualified to r+ or r- on general UI design, and as this was listed on the wiki for some time, I will consider an r+ from "General Community" on the UI itself, my review will be based on code alone.
Comment 36•19 years ago
|
||
Comment on attachment 175898 [details] [diff] [review] Rest of the Patch By the way, command to create this was: cvs diff -u8pdN -D 2005-02-25 mozilla/browser/locales/en-US/chrome/browser/browser.dtd mozilla/browser/locales/jar.mn mozilla/browser/locales/en-US/chrome/browser/preferences/ mozilla/browser/themes/pinstripe/browser/jar.mn mozilla/browser/themes/pinstripe/browser/preferences/preferences.css mozilla/browser/themes/winstripe/browser/jar.mn mozilla/browser/themes/winstripe/browser/preferences/preferences.css mozilla/toolkit/components/passwordmgr/resources/content/passwordManager.xul mozilla/toolkit/components/viewconfig/content/config.js mozilla/toolkit/components/viewconfig/content/config.xul mozilla/toolkit/content/globalOverlay.js mozilla/toolkit/content/jar.mn mozilla/toolkit/content/xul.css mozilla/toolkit/content/widgets/checkbox.xml mozilla/toolkit/content/widgets/colorpicker.xml mozilla/toolkit/content/widgets/listbox.xml mozilla/toolkit/content/widgets/menulist.xml mozilla/toolkit/content/widgets/preferences.xml mozilla/toolkit/content/widgets/stringbundle.xml mozilla/toolkit/content/widgets/tabbox.xml mozilla/toolkit/content/widgets/toolbar.xml mozilla/toolkit/locales/jar.mn mozilla/toolkit/locales/en-US/chrome/mozapps/downloads/unknownContentType.prope rties mozilla/toolkit/locales/en-US/chrome/global/config.dtd mozilla/toolkit/locales/en-US/chrome/global/preferences.dtd mozilla/toolkit/themes/pinstripe/global/config.css mozilla/toolkit/themes/pinstripe/global/global.css mozilla/toolkit/themes/winstripe/global/config.css mozilla/toolkit/themes/winstripe/global/global.css mozilla/toolkit/themes/winstripe/global/jar.mn mozilla/toolkit/themes/winstripe/global/menulist.css mozilla/toolkit/themes/winstripe/global/preferences.css mozilla/toolkit/themes/winstripe/global/textbox.css mozilla/browser/app/profile/firefox.js mozilla/browser/base/jar.mn mozilla/browser/base/content/browser-menubar.inc mozilla/browser/base/content/browser-scripts.inc mozilla/browser/base/content/browser-sets.inc mozilla/browser/base/content/browser.js mozilla/browser/base/content/credits.xhtml mozilla/browser/base/content/sanitize.js mozilla/browser/base/content/sanitize.xul mozilla/browser/components/Makefile.in
Assignee | ||
Comment 37•19 years ago
|
||
(In reply to comment #35) > we should keep it on trunk through my review. Thank you Justin for giving me your permission!
Assignee | ||
Comment 38•19 years ago
|
||
Also,
> I will consider an r+ from "General Community" on the UI itself
LOL - you're new to Firefox aren't you!
:D
Comment 39•19 years ago
|
||
FWIW, the following are the current mac-specific issues: * Bug 283660 (have patch) - the new preferences window can't be closed on OS X (and doesn't have a collapse toolbar button) * Bug 283713 - preferences dialog gets progressively shorter when rapidly switching panels * [no bug] - the fade is too slow (i.e. behavior is like like shift+click-on-pane)
Comment 40•19 years ago
|
||
Ben: You backed out part of my patch from toolkit/locales/jar.mn. See bug 279768 and bug 284183
Comment 41•19 years ago
|
||
So.. the simplest way I can explain the diff pointed to in bug 279768 comment 59 is that the files for this patch were simply copied on top of whatever was tip at the time, instead of cvs merging. If that's what happened then we probably need to look at every single file touched by this patch to make sure there weren't any unintended changes. Ben, please let me know if I'm guessing wrong, and there was in fact CVS merging involved....
Comment 42•19 years ago
|
||
Comment on attachment 175898 [details] [diff] [review] Rest of the Patch I am not done my review of this yet, but just a few things for starters... >Index: mozilla/toolkit/content/widgets/preferences.xml >=================================================================== >+ <binding id="preference"> >+ <implementation> >+ <method name="setElementValue"> >+ <parameter name="aElement"/> >+ <body> >+ <![CDATA[ ... >+ try { >+ var event = document.createEvent("Events"); >+ event.initEvent("preferenceread", false, true); >+ var f = new Function ("event", >+ aElement.getAttribute("onpreferenceread")); >+ rv = f(event); >+ } >+ catch (e) { >+ dump(e); >+ } >+ } Please remove, comment out, or enhance with an actual error message, this dump. >+ <method name="getElementValue"> >+ </method> Same here >+ <binding id="prefwindow" >+ <constructor> >+ </constructor> + <destructor>if (this._timer) this._timer.cancel();this._timer = null;</destructor> to save a small bit of perf incase the timer is running? >+ <method name="addPane"> >+ <parameter name="aPaneElement"/> >+ <body> >+ <![CDATA[ >+ this.appendChild(aPaneElement); >+ >+ // Set up pane button >+ this._makePaneButton(aPaneElement); >+ ]]> >+ </body> >+ </method> wrong, should be this._panedeck.appendChild(aPaneElement); >+ <binding id="prefpane"> >+ <handler event="paneload"> >+ <![CDATA[ >+ // Initialize all values from preferences. >+ var elements = this.preferenceElements; >+ for (var i = 0; i < elements.length; ++i) { >+ try { >+ var preference = this.preferenceForElement(elements[i]); >+ preference.setElementValue(elements[i]); >+ } >+ catch (e) { >+ dump("*** No preference found for " + elements[i].getAttribute("preference") + "\n"); >+ } >+ } >+ ]]> >+ </handler> This Dump too. Might be good to remove or comment out, if you do the same for the others. ... More complete review to come.
Comment 43•19 years ago
|
||
One oddity I have come across is to do with the lastSelected and currentPane properties which hold the id of the selected pane and the pane element respectively. These are both updated in the _selectPane method, unless the prefwindow type is "child" in which case they are never updated. This means that child prefwindows do not persist their selected pane, and finding the current pane programmatically is a little more complex than would otherwise be necessary. Of course I have no documentation on the prefwindow to look at so it may be that this is how these properties should work.
Comment 44•19 years ago
|
||
Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.8b2) Gecko/20050305 Firefox/1.0+ in chrome://browser/skin/preferences/preferences.css #changeActionContent { padding-top: 8px; padding-bottom: 10px ----------------------^ the ; is missing
Comment 45•19 years ago
|
||
Oh, my bad. I'll land a fix.
Comment 46•19 years ago
|
||
(In reply to comment #44) > Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.8b2) Gecko/20050305 > Firefox/1.0+ > > in > chrome://browser/skin/preferences/preferences.css > > #changeActionContent { > padding-top: 8px; > padding-bottom: 10px > ----------------------^ > > the ; is missing It's optional anyway.
Comment 47•19 years ago
|
||
(In reply to comment #46) > It's optional anyway. No, it does not work w/o the ; (in contrast to javascript).
Comment 48•19 years ago
|
||
(In reply to comment #47) > (In reply to comment #46) > > > It's optional anyway. > No, it does not work w/o the ; (in contrast to javascript). > But it should, shouldn't it? - http://www.w3.org/TR/CSS1#basic-concepts
Comment 49•19 years ago
|
||
(In reply to comment #48) > > > It's optional anyway. > > No, it does not work w/o the ; (in contrast to javascript). > > > But it should, shouldn't it? - http://www.w3.org/TR/CSS1#basic-concepts The semi-colon is optional if it is the last thing in the declaration block. In this case there are further style declarations following the missing semi-colon which will be ignored.
Comment 50•19 years ago
|
||
(In reply to comment #49) > (In reply to comment #48) > > > > It's optional anyway. > > > No, it does not work w/o the ; (in contrast to javascript). > > > > > But it should, shouldn't it? - http://www.w3.org/TR/CSS1#basic-concepts > > The semi-colon is optional if it is the last thing in the declaration block. In > this case there are further style declarations following the missing semi-colon > which will be ignored. Ah, sorry - I read a } where there wasn't one.
Comment 51•19 years ago
|
||
Ben, can you explain what the default font is for? AFAIK, there's no backend change to intrdouce 'the (overall) default font' in Gfx. As far as the backend is concerned, the font selection is per 'langGroup'. Now without any change in the backend, 'the default font' is introduced in the new preference and I wonder how it's related to our Gfx and layout code.
Assignee | ||
Comment 52•19 years ago
|
||
The "Default Font" settings are just a convenience that maps to the default font settings in the selected langgroup in the font dialog. It's an attempt at simplification for the most common situations/users.
Target Milestone: Firefox1.1 → Future
Comment 53•19 years ago
|
||
(In reply to comment #52) > The "Default Font" settings are just a convenience that maps to the default font > settings in the selected langgroup in the font dialog. It's an attempt at > simplification for the most common situations/users. That could be confusing because 'Default Font' can be interpreted the way I interpreted it (comment #51). Apparently, 'the selected langgroup' is the last langgroup whose font config was changed in 'Advanced'. The connection between two is not very clear. Probably most of users wouldn't touch more than one langgroups, in which case it might be all right. Others who sometimes change fonts for more than one groups (e.g. those who can speak -at least read - English and their native languages) could have a surprise. I'd not say the previous font pref. UI is clear and easy to use, but I'm afraid the new font pref. UI sacrifices even more clarity at the cost of 'simplicity' (?).
Does the default for selected langgroup depend on localization?
Comment 55•19 years ago
|
||
(In reply to comment #54) > Does the default for selected langgroup depend on localization? Indirectly. 'The selected langgroup' is controlled by 'font.language.group' the _default_ value of which is localizable( 'http://lxr.mozilla.org/seamonkey/search?string=font.language.group'). However, as I wrote before, 'font.language.group' is changed to the last langGroup whose font setting is changed. (see bug 85747)
Comment 56•19 years ago
|
||
Comment on attachment 175898 [details] [diff] [review] Rest of the Patch >Index: mozilla/toolkit/content/widgets/preferences.xml >=================================================================== >+ <binding id="preferences"> >+ <method name="observe"> >+ var preference = this.childNodes[i]; Severity, minor: childNodes are not guaranteed to be bound to "preference" Perhaps set children includes preference to force-out non preference elements? (or just assume callers do it right) >+ <binding id="preference"> >+ <method name="setElementValue"> >+ var val = rv !== undefined ? rv : (this.instantApply ? this.valueFromPreferences : this.value); Severity aesthetic: != rather than !== Severity Medium: if we are locked, This method does not disable labels which are controls of the elements which specify this preference, should it? >+ <method name="getElementValue"> >+ if (rv !== undefined) Same aesthetic nit with !== >+ <binding id="prefwindow" >+ <content xul:dlgbuttons="accept,cancel" xul:persist="lastSelected screenX screenY" I am unsure if these xul:attribute's are correct, bz did have an assumption that they are not, and I am not able to determine if they are, as of yet. I will re-comment if they are in-fact not correct; >+ <property name="lastSelected" >+ document.persist(this.id, "lastSelected"); Severity aesthetic: If the xul:persist works, this should not be necessary >+ <method name="_selectPane"> >+ var oldPane = this.lastSelected ? document.getElementById(this.lastSelected) : this.preferencePanes[0]; >+ oldPane.selected = !(aPaneElement.selected = true); >+ if (!this._initialized || oldPane.id != aPaneElement.id) { Severity moderate: oldPane.id may == aPaneElement.id, and in which case there will be no .selected which is valued true. Suggest, set the true after the false. |aPaneElement.selected = !(oldPane.selected = false);| >+ <method name="animate"> >+ this._multiplier = aNewPane.contentHeight > aOldPane.contentHeight ? 1 : -1; Severity major: If we are already animating, the _multiplier will be set wrong. see Bug 283713 >+ <method name="addPane"> >+ this.appendChild(aPaneElement); Severity major: I already commented on this, this._panedeck.appendChild >+ <method name="openSubDialog"> >+ return openDialog(aURL, "", "modal,centerscreen" + (aFeatures != "" ? ("," + aFeatures) : ""), >+ <method name="openWindow"> >+ win = parentWindow.openDialog(aURL, "_blank", features, aParams); Severity aesthetic: "" or "_blank", consistency? >+ <binding id="prefpane"> >+ <property name="contentHeight"> >+ var targetHeight = parseInt(window.getComputedStyle(this._content, "").height); Does padding apply here?, if so, it needs to be added in. ===== I will not make any more claims to dates/times of further reviews on this package, but I will try at least the remaining toolkit bits sooner than later, the browser/ bits shortly following.
Comment 57•19 years ago
|
||
> bz did have an assumption that they are not
Make that "bz will bet money, based on code inspection, that they do absolutely
nothing".
Comment 58•19 years ago
|
||
ignore my !== aesthetic comments, I misunderstood the actual difference between != and !==. Sorry for wasted time in reading those specific issues.
Comment 59•19 years ago
|
||
*** Bug 191524 has been marked as a duplicate of this bug. ***
Comment 60•19 years ago
|
||
*** Bug 299510 has been marked as a duplicate of this bug. ***
Comment 61•19 years ago
|
||
*** Bug 217419 has been marked as a duplicate of this bug. ***
Comment 62•18 years ago
|
||
sorry for bugspam, long-overdue mass reassign of ancient QA contact bugs, filter on "beltznerLovesGoats" to get rid of this mass change
QA Contact: mconnor → preferences
You need to log in
before you can comment on or make changes to this bug.
Description
•