201177 - Need to add pref UI to control new tab/window behaviour

Status: VERIFIED FIXED

(SeaMonkey :: Preferences, enhancement)

Description

[Ian Neal]

As a result of bug 197671 and bug 200956 there will be two new preferences added
to control the behaviour of what happens when (a) a new tab and (b) a new window
is created. After bug 197671 is checked in there will be 4 choices for each of
these preferences:
Use Navigator Startup preference
Blank Page
Home Page
Last Page visited
and if and when bug 200956 is checked in there will be one additional choice for
each preference of:
Custom Page

Bug 197671 currently adds some pref UI to the tabbed browsing section for tab
behaviour but doesn't add anything to control new window behaviour.

This bug is to discuss the best place for this frontend pref UI to go. The
immediate options I can see are:
(a) Leave it as is with only the UI introduced by bug 197671
(b) Add the necessary UI to support the custom page for tabbed browsing.
(c) Do (a) or (b) and create a new sub menu under Navigator for Window Browsing
and the necessary  UI to support the new window behaviour (with or without the
custom page option)
(d) Move the UI introduced by bug 197671 into a new sub menu under Navigator and
add the necessary UI to support the new window behaviour (with or without the
custom page option)

I'm not saying these are the only options, just the ones I've thought of (or had
suggested to me) so far.

Comment 1

[Ian Neal]

Taking bug but keeping Ben in CC list

Comment 2

[Jason Bassford]

I think that the UI can be simplified (so as not to require a sub-panel) by
replacing the current "When Navigator starts up, display" with a simple dropdown
box from which each of the categories can be selected:

 -[When Navigator starts up, display]-     V
  [When new Tab is created, display]
  [When new Window is created, display]

When going into the Navigator pref panel, it would always default to showing the
1st item.

Comment 3

[Jason Bassford]

Of course, selecting "New Tab" or "New Window" (or "Navigator startup" after
having selected one of those) would have to dynamically change the rest of the
UI to show/hide the radio button for "Same as Navigator startup".

I'll also mention that we don't necessarily need to have a textarea box directly
in the UI for the entry of any "custom page", if that gets implemented.  We
could use a button that calls a dialogue.  (Such as Ctrl-Shift-L.)  This is
assuming that we can't find space for a textarea control and such a button isn't
considered "bad UI practice".

Comment 4

[Ian Neal]

Attachment: Initial patch to tabbed browsing pref UI

This is the tabbed browsing pref UI part of the patch for bug 197671 which I'm
now spinning off into this bug.

Comment 5

[Ian Neal]

Attachment: Patch v1.0a

Corrects small typo spotted by Jason:

>> +	 <radio value="0" label="&startupTabRadio.label;"
>	 accesskey="&blankTabRadio.accesskey;"/>

>Is this correct?  Shouldn't the accesskey="&startupTabRadio.accesskey"?

Comment 6

[Ian Neal]

Attachment: Screenshot Option 1

Screenshot of option one version of the proposed new UI

Comment 7

[Ian Neal]

Attachment: Screenshot Option 2

Screenshot of option two version of the proposed new UI

Comment 8

[Ian Neal]

I did look at putting the drop down in the caption (like the fonts pref UI) but
it makes the default browser pref box in windows appear slightly offset and, if
it has "Display when" before it, the right hand side of the default browser pref
box is pushed off the edge of the window.
Any comments/preferences on which option?

Comment 9

[Will Martin]

Option two looks cleaner to me.  It's a bit strangely phrased though.  Assuming
you're configuring startup to show about:blank, it says "Display when Navigator
starts up Blank page."  Perhaps if you reversed the order of the drop box and
the radio button values?  Like this:

+--------------------------+
| Show |a blank| page when |
|                          |
| [X] Navigator starts.    |
| [ ] A new tab opens.     |
| [ ] A new window opens.  |
+--------------------------+

This the drop box would have the values of "a blank", "the home", "the last
visited".  

In the event of later additions to the pref, for example a custom page, you
could add "a custom" to the drop-down box without having to reconfigure the rest
of the UI.  (Selecting a custom page could bring up a separate input box asking
for the URI.)

Of course, this makes sense in English.  Arranging the UI like that might not
make any sense at all in other languages.  Even in English, it's still a bit
clumsy for UI purposes.  If you didn't feel like reversing the values of the
drop box and the radio buttons, you could leave the radio buttons there for
blank/home/last-visited, and change the top part to read "New |Navigators|
show", with the values of the drop box being "Navigators", "Windows", "Tabs".  

Either way, I don't think we'll be able to make the UI clear without at least a
couple of words surrounding the drop box.  (Which is do-able, see the abortive
UI design I proposed in bug 197671.)

Comment 10

[sairuh (rarely reading bugmail)]

i prefer option 1, since group box titles are usually section names, never parts
of sentences (iirc).

Comment 11

[Ian Neal]

Re: Comment #9
Suggestion 1: I could put a few "a"s and "the"s in so it radio buttons would be:
"the Navigator startup page"
"a blank page"
"the home page"
"the last page visited"
and the menu items would be:
"the Navigator starts up"
"a new Window is created"
"a new Tab is created"

Unfortunately option 1, when you add "a" and "the" in, causes the Default
Browser caption box to overflow past the edge of the Preferences window,
especially if "When" and ", display" are put either side of the menu list
instead of "Display when" before it.

I'll attach a mock up of the ammended option 2 for comment.

Suggestion 2: I don't think it would be possible to setup the UI in that way.
Radio buttons wouldn't work in that setup which would leave checkboxes. In that
case you would have to disable all other checkboxes for that row once you had
checked one box - the user would then have to go through each type of page to
find the one that was already checked before they could check another box.

Suggestion 3: I did look at your suggested UI when designing this one and looked
at doing it exactly that way but I couldn't find an equivalent to tabs and
windows for the third option which didn't sound slightly clumsy. 

Re: Comment #10
There seems to be a lot of captions that are parts of sentences, to name just a few:
Under Navigator category - When Navigator starts up, display
Under Tabbed Browsing subcategory - Open tabs instead of windows for
Under Downloads subcategory - When starting a download

Comment 12

[Ian Neal]

Attachment: Screenshot Option 2a

Screenshot of the ammended option two version of the proposed new UI

Comment 13

[Ian Neal]

Attachment: Patch v1.1o1 using layout option 1

This patch alters the prefs UI as per screenshot 1

Comment 14

[Ian Neal]

Attachment: Patch v1.1o2 using layout option 2

This patch alters the prefs UI as per screenshot 2

Comment 15

[Ian Neal]

Attachment: Patch v1.1o2a using layout option 2a

This patch alters the prefs UI as per screenshot 2a

Comment 16

[Jason Bassford]

Attachment: Alternate UI.

Here's a different take on the UI that has the benefit of resulting in a
grammatically correct sentence.

The options for the dropdown box would be "Navigator start up", "New tabs", and
"New windows".

Comment 17

[Jason Bassford]

The sentence would even makes sense in French in that changed order.  (I'm not
sure about other languages though.)

Comment 18

[Ian Neal]

I'm not 100% sure that reversing it makes sense, in that you usually have a
statement and then options, not options and then a statement.

Comment 19

[jag (Peter Annema)]

I still like the idea of having a dropdown in the caption the best (see comment 2). Could you use this for the caption?"Display on [Navigator Startup]""Display on [New Window]""Display on [New Tab]"

Comment 20

[Ian Neal]

Attachment: Screenshot Option 3

Screenshot of option 3 - mock up of pref UI proposed by jag
Patch comming soon

Comment 21

[Ian Neal]

Attachment: Patch v1.1 Option 3 layout

Patch featuring the option 3 layout - any comments/feedback?

Comment 22

[Will Martin]

The screenshot of option 3 looks like the best of the lot to me.  Having the
drop-down box in the caption is a little atypical, but it's clear and clean. 
I'll vote for that one.

Comment 23

[Jason Bassford]

I'll also agree that Option 3 looks the best.  (Along with my own submission,
but if you don't want statement/options reversed, then Option 3 seems the most
natural to me.)

Comment 24

[sairuh (rarely reading bugmail)]

i like option #3 as well. quick question, would it be too much trouble to attach
a screenshot of the expanded droplist (for "Display on...")? thanks!

Comment 25

[Jason Bassford]

Ian: Obsolete all but the Options 3 attachments?  (I'll obsolete mine if you'll
obsolete yours. <grin>)

Comment 26

[Ian Neal]

Attachment: Screenshot Option 3 with expanded drop list

Comment 27

[Ian Neal]

Attachment: Screenshot Option 3 with 4 radio buttons showing

Comment 28

[Jason Bassford]

How about adding the 4th radio button (and text) to the "Navigator Startup"
section, but having it greyed out and unselectable.  It might make things appear
more consistent, if it didn't appear and disappear but were simply enabled and
disabled.

Comment 29

[Ian Neal]

Adding target of 1.4b and setting Hardware -> All

Comment 30

[Ian Neal]

Attachment: Screenshot Option 3 with one disabled radio button

Screenshot of Option 3 with 4 buttons (including one disabled) showing in
Navigator Startup choice.
This will be in v1.2 of the patch which is tied in with v1.6 of the patch for
bug 197671. Just waiting for some feedback from jag before finalising that
version of the patch.

Comment 31

[Jason Bassford]

I like it.  (Odd though.  It looks like there's a dot in it - although I can
barely make it out.  Is that how disabled radio buttons always look?)

Comment 32

[Ian Neal]

I think that is just the way jpeg turned out, on the original there is no spot
in the middle.

Comment 33

[Ian Neal]

Attachment: Patch v1.2

Updated patch that works in conjunction with patch v1.6 for bug 197671

Comment 34

[Ian Neal]

Now that bug 197671 has had it's sr= I'm looking to get this
reviewed/superreviewed so both patches can be checked in at the same time. Any
takers?

Comment 35

[jag (Peter Annema)]

I'll have to play with this patch. I don't like the greyed out radio button. The
radios moving around might not be too bad, alternatively you could move the
additional option (navigator startup page) to the bottom of the list. Let me get
back to you on that later.

Comment 36

[Paul Klapperich]

How about, instead of a 4th radio button, place a check box below the radio
buttons to eliminate the drop down box, making all options work the same?

That would prevent a user from setting navigator and then linking one of the
other options to it with the 4th radio but leaving the 3rd option on its own,
but then I really don't see justification for the 4th radio button in that
scenerio anyway when the user could very easily click the same radio button on
one of the other options that they had clicked on the navigator startup page.
With the radio, you still have to go through the settings groups, where as a
checkbox would actually save time if one wanted them all to be the same.

Comment 37

[Ian Neal]

Attachment: UI Patch v1.3

UI patch v1.3 with disabled button taken out and Navigator startup page button
moved to the bottom of the list

Comment 38

[jag (Peter Annema)]

Comment on attachment 120648 [details] [diff] [review]
UI Patch v1.3

>Index: content/pref-navigator.xul
>===================================================================
>@@ -37,21 +37,46 @@
>
>   <script type="application/x-javascript">
>   <![CDATA[
>-    var _elementIDs = ["startupPage", "bookmarksButton",
>-                       "goButton", "homeButton",
>-                       "printButton", "searchButton" ];
>+var _elementIDs = ["startupPage", "newWinPage", "newTabPage",
>+                   "bookmarksButton", "goButton", "homeButton",
>+                   "printButton", "searchButton" ];

Indentation

>+        <menulist id="selectDisplayOn" oncommand="switchPage(this);">

Add sizetopopup="never" to this

>Index: content/pref-navigator.js
>===================================================================
>RCS file: /cvsroot/mozilla/xpfe/components/prefwindow/resources/content/pref-navigator.js,v
>retrieving revision 1.12
>diff -u -r1.12 pref-navigator.js
>--- content/pref-navigator.js   4 Oct 2002 00:28:03 -0000       1.12
>+++ content/pref-navigator.js   15 Apr 2003 23:53:29 -0000
>@@ -293,3 +293,8 @@
>   prefWindow.setPref("int", countPref, URIs.length);
> }
>
>+function switchPage( aElement )

No space after/before '(' and ')' like rest of file

>+{
>+  var deck = document.getElementById( "behaviourDeck" );
>+  deck.setAttribute( "selectedIndex", aElement.selectedItem.value );

Set focus to the radiogroup that now gets shown, otherwise tab behaviour will
be a little weird (e.g. last group was shown, first group is now shown, [tab]
will take you to the next widget; if however first group was shown, and second
group is now shown, [tab] will take you to the second radiogroup.)

sr=jag with those changes (test please).

Comment 39

[jag (Peter Annema)]

> Add sizetopopup="never" to this

Never mind that. There seems to be a bug in the css for the Mac classic menulist.

Comment 40

[Ian Neal]

Attachment: Patch v1.3a

Updated patch with indentation fixed, extra spaces removed and focus set for
selected radiogroup. Need someone on a Mac to test last part as problem doesn't
appear to show up on other platforms.

Comment 41

[jag (Peter Annema)]

Comment on attachment 121226 [details] [diff] [review]
Patch v1.3a

>Index: content/pref-navigator.xul
>===================================================================
>RCS file: /cvsroot/mozilla/xpfe/components/prefwindow/resources/content/pref-navigator.xul,v
>retrieving revision 1.69
>diff -u -r1.69 pref-navigator.xul
>--- content/pref-navigator.xul	4 Oct 2002 00:28:04 -0000	1.69
>+++ content/pref-navigator.xul	21 Apr 2003 23:30:34 -0000
>@@ -46,12 +46,37 @@
>+      <caption>
>+        <hbox align="center">
>+          <label value="&navRadio;" control="selectDisplayOn"/>
>+        </hbox>

<caption align="center">
  <label value="&navRadio;" control="selectDisplayOn"/>
  ...

That should have the desired effect.

>Index: content/pref-navigator.js
>===================================================================
>+function switchPage(aElement)
>+{
>+  var deck = document.getElementById("behaviourDeck");
>+  deck.setAttribute("selectedIndex", aElement.selectedItem.value);
>+  deck.childNodes[deck.selectedIndex].focus();
>+}

deck.selectedIndex = aElement.selectedIndex;
deck.selectedPanel.focus();

Except leave out the last line. It's a problem with Mac focus in the classic
skin that we'll need to fix differently.

Oh, and because you're using aElement.selectedIndex you don't need the value=
on the menulist items.

sr=jag

Comment 42

[jag (Peter Annema)]

Comment on attachment 121226 [details] [diff] [review]
Patch v1.3a

Actually, I just realized that the accesskeys won't work like this (though they
should, imo, but that's gonna be a lot of work to fix). The problem is that if
two elements have the same accesskey, only the one "lower" in the file will get
the accesskey registered. So for the first two radiogroups, accesskeys won't
work. What we really should do is allow multiple elements with the same
accesskey and find the first one visible (though "visible" is differently
defined for <deck>s, so ... hrm).

For now, I'll add/remove accesskeys in switchPage(). Patch coming up.

Comment 43

[jag (Peter Annema)]

Attachment: Patch v1.3b

Fix my nits, fix accesskeys.

Comment 44

[jag (Peter Annema)]

Comment on attachment 121401 [details] [diff] [review]
Patch v1.3b

sr=me,sr=hewitt on my changes

Comment 45

[[e-mail]]

Comment on attachment 121401 [details] [diff] [review]
Patch v1.3b

>setPageAccessKeys(document.getElementById("behaviourDeck").firstChild);
Why not document.getElementById("startupPage")? Or set the access keys
directly? Although it turns out that access keys on <radio>s don't work
properly because the set focus to the <radio> which is wrong :-(

>+  var node = group.firstChild;
>+  while (node) {
>+    node.setAttribute("accesskey", node.getAttribute("ak"));
>+    node = node.nextSibling;
>+  }
Nit: peterv told me once that childNode caching is better.

>+<!ENTITY  startPageRadio.label      "Navigator startup page">
>+<!ENTITY  startPageRadio.accesskey  "s">
> <!ENTITY  blankPageRadio.label      "Blank page">
> <!ENTITY  blankPageRadio.accesskey  "n">
These confused me, since because radio/checkbox access keys don't display I
tried to use Alt+B for blank page and Alt+N for Navigator startup page :-/

Comment 46

[jag (Peter Annema)]

I seem to recall that "b" was already taken. And yeah, we don't propertly set
focus to the radiogroup when setting focus to the radio, I noticed this
yesterday (didn't get around to looking for / filing a bug). What's this about
nodeChild caching?

Oh, so the reason I don't get it directly is because what I really wanted to do
is deck.selectedPanel, but apparently the xbl isn't ready yet at that point, so
the next closest was getting the first child (so that if the menulist ever
changes, that code doesn't have to change).

Comment 47

[Ian Neal]

Is this now ready to ask for driver approval?

Comment 48

[jag (Peter Annema)]

Oh... With childNode caching you mean something like:

var children = element.childNodes;
for (var i = 0; i < children.length; ++i)
  children[i].removeAttribute("acccesskey");

?

Comment 49

[jag (Peter Annema)]

Attachment: Patch v1.3c

Addresses Neil's comments.

Comment 50

[jag (Peter Annema)]

Comment on attachment 121521 [details] [diff] [review]
Patch v1.3c

sr=jag/hewitt

Comment 51

[jag (Peter Annema)]

Comment on attachment 121521 [details] [diff] [review]
Patch v1.3c

Neil said r=Neil.

Comment 52

[Asa Dotzler [:asa]]

Comment on attachment 121521 [details] [diff] [review]
Patch v1.3c

a=asa (on behalf of drivers) for checkin to 1.4b.

Comment 53

[jag (Peter Annema)]

Checked in. Sorry Ian, forgot to mention you as author of the patch in the
checkin comment. I added a dummy checkin (adding you to the authors list) to
rectify that.

Comment 54

[sairuh (rarely reading bugmail)]

looks good, so verifying fixed. tested with 2003.04.28 comm builds.

the only issue i see is with the commercial builds: the Navigator pref panel is
now clipped a little bit at the bottom. (nscp builds have a lot more checkboxes
in that panel.) i've spun off http://bugscape.mcom.com/post_bug.cgi to cover that.

Comment 55

[jag (Peter Annema)]

Ian: feedback I've gotten on this is that the "Navigator Startup Page"
radiobutton is confusing ("how do I set what page that is?", apparently they
didn't seem to get that it would load whatever you had set the startup page pref
to), and the small benefit (not having to explicitely synch up new window/tab
with navigator startup) doesn't outweigh that confusion and additional complexity.

I suggest we remove the UI, and change all.js "browser.windows.loadOnNewWindow"
to 1. The only "problem" left then is that for people who've chosen -1 for new
window/tab, the UI would show no radio button selected, which I think will be
okay (people upgrading from 1.4a won't see this, just people running nightlies,
and that's a risk they take with nightlies).

Comment 56

[Ian Neal]

Attachment: UI Patch fix v1.0

Patch that removes 4th option from new tab/window lists. It sets the default
behaviour for new windows to be home page (1) rather than same as browser
startup behaviour (-1)

Comment 57

[sairuh (rarely reading bugmail)]

reopening since there still seems to be issues.

in fact, with ian's latest change will make
http://bugscape.mcom.com/show_bug.cgi?id=23599 obsolete. :)

Comment 58

[Ian Neal]

I suppose an alternative would be to rename that particular option to something
more understandable though finding something short enough to fit in the space
available might be difficult - 'Same page as Navigator Startup Pref' is a bit
long :-S

Comment 59

[sairuh (rarely reading bugmail)]

nominating for 1.4final.

robin, i think online help might need updating for the navigator pref panel.

Comment 60

[jag (Peter Annema)]

Comment on attachment 122157 [details] [diff] [review]
UI Patch fix v1.0

r+sr=jag

Note to QA: people can still manually set these prefs to -1 in their preference
file, and we'll honour that.

Ian: I believe this should work, but, if you haven't already, could you make
sure that if the tab pref is currently set to -1 and we load that pref panel
and then hit OK, that it gets written back as -1?

Comment 61

[Ian Neal]

Checked and it does stay set at -1

Comment 62

[Ian Neal]

Comment on attachment 122157 [details] [diff] [review]
UI Patch fix v1.0

Requesting drivers approval

Comment 63

[(not reading, please use [e-mail] instead)]

Comment on attachment 122157 [details] [diff] [review]
UI Patch fix v1.0

a=sspitzer

Comment 64

[jag (Peter Annema)]

Checked in, marking fixed again.

Comment 65

[sairuh (rarely reading bugmail)]

vrfy'd fixed with 2003.05.07 comm builds.

see bug 204157 for remaining issue with opening browser windows from non-browser
apps.

Comment 66

[Jason Bassford]

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

Comment 67

[Felix Miata]

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