274712 - New Options Dialog

Status: RESOLVED FIXED

(Firefox :: Settings UI, defect, P1)

Description

[Ben Goodger (use ben at mozilla dot org for email)]

Support instant apply prefs for MacOS X/GNOME, progress towards new options
reorg (see URL).

Comment 1

[Ben Goodger (use ben at mozilla dot org for email)]

Attachment: Day 1

progressmeter bindings, sample usage. Much to do remaining.

Comment 2

[Ben Goodger (use ben at mozilla dot org for email)]

Attachment: dynamic overlays

w00t. 

done in the quickest way possible. need to discuss with jst/hyatt/etc.

Comment 3

[Ben Goodger (use ben at mozilla dot org for email)]

Attachment: dynamic overlays

er, including the dom stuff too

Comment 4

[Ben Goodger (use ben at mozilla dot org for email)]

Attachment: dynamic overlays, 2

sends xul-overlay-merged notification to supplied observer when load and merge
is complete.

Comment 5

[Ben Goodger (use ben at mozilla dot org for email)]

Attachment: more

this time, showing the actual UI code.

Comment 6

[Steffen Wilberg]

Comment on attachment 168886 [details] [diff] [review]
more

>Index: browser/app/nsBrowserApp.cpp
> static const nsXREAppData kAppData = {
>   "Mozilla",
>-  "Firefox",
>+  "Firefox Debug",
really? :-)

Comment 7

[Ben Goodger (use ben at mozilla dot org for email)]

Attachment: patch

Fixes a notification issue with Forward Reference Resolution.

Comment 8

[Asaf Romano (gone)]

Comment on attachment 168946 [details] [diff] [review]
patch

crashes on OS X when trying to open the prefs window.

Comment 9

[Ben Goodger (use ben at mozilla dot org for email)]

Attachment: patch

content pane, rough cut.

Comment 10

[Ben Goodger (use ben at mozilla dot org for email)]

Attachment: patch

initial advanced pane work

Comment 11

[Ben Goodger (use ben at mozilla dot org for email)]

Attachment: patch

more...

Comment 12

[Ben Goodger (use ben at mozilla dot org for email)]

Attachment: patch

more...

Comment 13

[Ben Goodger (use ben at mozilla dot org for email)]

Attachment: patch

more...

Comment 14

[Ben Goodger (use ben at mozilla dot org for email)]

Attachment: patch

more...

Comment 15

[Ben Goodger (use ben at mozilla dot org for email)]

Attachment: patch

more...

Comment 16

[Ben Goodger (use ben at mozilla dot org for email)]

Attachment: patch

more...

Comment 17

[amano]

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

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Hi, Ben.

I fixed bug 95227.

Currently, default font option is separated by langGroup.
Be careful.

Comment 19

[Ben Goodger (use ben at mozilla dot org for email)]

Attachment: patch

implements <preferences> element, new preferences UI, browser sanitize,
viewconfig tweaks, etc.

Comment 20

[Asaf Romano (gone)]

Ben, your latest patch doesn't take care of comment 18 (serif/sans-serif changes).

Comment 21

[Grayson Mixon (ColdFusion650)]

how do u implement a .diff file?

Comment 22

[Mike Connor [:mconnor]]

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

[Markus S.]

How about adding "Sanitize after xy days" and/or "Clear saved form information
after xy days"?

Comment 24

[John Collins]

Just a suggestion/request, is having the option to choose between the new and
the current options dialogs a possibility?

Comment 25

[Jeff Walden [:Waldo]]

(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.)

Comment 26

[Mark Tyndall]

(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

[Hiro]

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

[Justin Wood (:Callek)]

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.

Comment 29

[Ben Goodger (use ben at mozilla dot org for email)]

Justin, please feel free to review the code located in
mozilla/browser/components/preferences and
mozilla/toolkit/content/widgets/preferences.xml

Comment 30

[Asaf Romano (gone)]

(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

[Peter van der Woude [:Peter6]]

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

[Dennis Jacobfeuerborn]

Attachment: screenshot of new options dialog

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

[Jungshik Shin]

This patch reverted the change made in bug 280007. Bug 283925 was filed on the
issue.

Comment 34

[Justin Wood (:Callek)]

Attachment: browser/components/preferences

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

[Justin Wood (:Callek)]

Attachment: Rest of the Patch

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=[e-mail] 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=[e-mail]
r=mconnor

mozilla/browser/base/content/browser.js
	1.393 -- Bug 278772 - Various Firefox code incorrectly constructs URIs.
patch by Gavin Sharp <[e-mail]> 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=[e-mail] r=mconnor
	1.390 -- Bug 237776 Reload should be disabled for blank tabs
p=[e-mail] 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

[Justin Wood (:Callek)]

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

Comment 37

[Ben Goodger (use ben at mozilla dot org for email)]

(In reply to comment #35)
> we should keep it on trunk through my review.  

Thank you Justin for giving me your permission!

Comment 38

[Ben Goodger (use ben at mozilla dot org for email)]

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

[Asaf Romano (gone)]

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

[Zibi Braniecki [:zbraniecki][:gandalf]]

Ben: You backed out part of my patch from toolkit/locales/jar.mn. See bug 279768
and bug 284183

Comment 41

[Boris Zbarsky [:bzbarsky]]

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

[Justin Wood (:Callek)]

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

[Dave Townsend [:mossop]]

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

[Peter van der Woude [:Peter6]]

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

[Asaf Romano (gone)]

Oh, my bad. I'll land a fix.

Comment 46

[Grey Nicholson (they/them)]

(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

[Peter van der Woude [:Peter6]]

(In reply to comment #46)

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

Comment 48

[Grey Nicholson (they/them)]

(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

[Dave Townsend [:mossop]]

(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

[Grey Nicholson (they/them)]

(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

[Jungshik Shin]

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. 
 

Comment 52

[Ben Goodger (use ben at mozilla dot org for email)]

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. 

Comment 53

[Jungshik Shin]

(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' (?).

Comment 54

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Does the default for selected langgroup depend on localization?

Comment 55

[Jungshik Shin]

(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

[Justin Wood (:Callek)]

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

[Boris Zbarsky [:bzbarsky]]

> 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

[Justin Wood (:Callek)]

ignore my !== aesthetic comments, I misunderstood the actual difference between
!= and !==.  Sorry for wasted time in reading those specific issues.

Comment 59

[(not currently active) Ted Mielczarek]

*** Bug 191524 has been marked as a duplicate of this bug. ***

Comment 60

[Adam Guthrie]

*** Bug 299510 has been marked as a duplicate of this bug. ***

Comment 61

[Grey Nicholson (they/them)]

*** Bug 217419 has been marked as a duplicate of this bug. ***

Comment 62

[Mike Connor [:mconnor]]

sorry for bugspam, long-overdue mass reassign of ancient QA contact bugs,
filter on "beltznerLovesGoats" to get rid of this mass change