Closed Bug 274712 Opened 20 years ago Closed 19 years ago

New Options Dialog

Categories

(Firefox :: Settings UI, defect, P1)

defect

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).
Attached patch Day 1 (obsolete) — Splinter Review
progressmeter bindings, sample usage. Much to do remaining.
Attached patch dynamic overlays (obsolete) — Splinter Review
w00t. 

done in the quickest way possible. need to discuss with jst/hyatt/etc.
Attached patch dynamic overlays (obsolete) — Splinter Review
er, including the dom stuff too
Attachment #168838 - Attachment is obsolete: true
Attached patch dynamic overlays, 2 (obsolete) — Splinter Review
sends xul-overlay-merged notification to supplied observer when load and merge
is complete.
Attachment #168839 - Attachment is obsolete: true
Blocks: 206651
Attached patch more (obsolete) — Splinter Review
this time, showing the actual UI code.
Blocks: 266487
Comment on attachment 168886 [details] [diff] [review]
more

>Index: browser/app/nsBrowserApp.cpp
> static const nsXREAppData kAppData = {
>   "Mozilla",
>-  "Firefox",
>+  "Firefox Debug",
really? :-)
Attached patch patch (obsolete) — Splinter Review
Fixes a notification issue with Forward Reference Resolution.
Attachment #168886 - Attachment is obsolete: true
Comment on attachment 168946 [details] [diff] [review]
patch

crashes on OS X when trying to open the prefs window.
Attached patch patch (obsolete) — Splinter Review
content pane, rough cut.
Attachment #168946 - Attachment is obsolete: true
Attached patch patch (obsolete) — Splinter Review
initial advanced pane work
Attachment #169159 - Attachment is obsolete: true
Attached patch patch (obsolete) — Splinter Review
more...
Attachment #169166 - Attachment is obsolete: true
Attached patch patch (obsolete) — Splinter Review
more...
Attachment #169264 - Attachment is obsolete: true
Attached patch patch (obsolete) — Splinter Review
more...
Attachment #169352 - Attachment is obsolete: true
Attached patch patch (obsolete) — Splinter Review
more...
Attachment #169412 - Attachment is obsolete: true
Attached patch patch (obsolete) — Splinter Review
more...
Attachment #169420 - Attachment is obsolete: true
Attached patch patch (obsolete) — Splinter Review
more...
Attachment #169421 - Attachment is obsolete: true
Blocks: 75119
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. 
Hi, Ben.

I fixed bug 95227.

Currently, default font option is separated by langGroup.
Be careful.
Attached file patch
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
Status: NEW → ASSIGNED
Flags: blocking-aviary1.1+
Priority: -- → P1
Target Milestone: --- → Firefox1.1
No longer blocks: 264201, 266487
Ben, your latest patch doesn't take care of comment 18 (serif/sans-serif changes).
how do u implement a .diff file?
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.
How about adding "Sanitize after xy days" and/or "Clear saved form information
after xy days"?
Just a suggestion/request, is having the option to choose between the new and
the current options dialogs a possibility?
(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.)
Status: ASSIGNED → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED
(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">
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+


Blocks: 283600
Blocks: 283660
No longer blocks: 283600
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.
Justin, please feel free to review the code located in
mozilla/browser/components/preferences and
mozilla/toolkit/content/widgets/preferences.xml
Blocks: 283678
Blocks: 224243
(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).
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
Blocks: 283713
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
No longer blocks: 283772
Blocks: 283738
Blocks: 283869
Blocks: 283945
This patch reverted the change made in bug 280007. Bug 283925 was filed on the
issue.
Blocks: 283925
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
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 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
(In reply to comment #35)
> we should keep it on trunk through my review.  

Thank you Justin for giving me your permission!
Also, 

> I will consider an r+ from "General Community" on the UI itself

LOL - you're new to Firefox aren't you!

:D
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)
Ben: You backed out part of my patch from toolkit/locales/jar.mn. See bug 279768
and bug 284183
Blocks: 284437
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 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.
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.
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
Oh, my bad. I'll land a fix.
(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.
(In reply to comment #46)

> It's optional anyway.
No, it does  not work w/o the ; (in contrast to javascript).

(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
(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.
(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.
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. 
 
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
Depends on: 283697
(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?
(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 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.
> bz did have an assumption that they are not

Make that "bz will bet money, based on code inspection, that they do absolutely
nothing".
ignore my !== aesthetic comments, I misunderstood the actual difference between
!= and !==.  Sorry for wasted time in reading those specific issues.
Blocks: 286197
Blocks: 286133
*** Bug 191524 has been marked as a duplicate of this bug. ***
*** Bug 299510 has been marked as a duplicate of this bug. ***
*** Bug 217419 has been marked as a duplicate of this bug. ***
Blocks: 308754
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
Depends on: 498553
You need to log in before you can comment on or make changes to this bug.