Closed Bug 201177 Opened 19 years ago Closed 19 years ago

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

Categories

(SeaMonkey :: Preferences, enhancement)

enhancement
Not set
normal

Tracking

(Not tracked)

VERIFIED FIXED
mozilla1.4beta

People

(Reporter: iann_bugzilla, Assigned: iann_bugzilla)

References

(Depends on 1 open bug)

Details

Attachments

(5 files, 15 obsolete files)

48.36 KB, image/jpeg
Details
46.20 KB, image/jpeg
Details
45.03 KB, image/jpeg
Details
5.69 KB, patch
jag+mozilla
: review+
jag+mozilla
: superreview+
Details | Diff | Splinter Review
2.86 KB, patch
neil
: review+
jag+mozilla
: superreview+
sspitzer
: approval1.4b+
Details | Diff | Splinter Review
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.
Taking bug but keeping Ben in CC list
Assignee: ben → ian
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.
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".
This is the tabbed browsing pref UI part of the patch for bug 197671 which I'm
now spinning off into this bug.
Attached patch Patch v1.0a (obsolete) — Splinter Review
Corrects small typo spotted by Jason:

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

>Is this correct?  Shouldn't the accesskey="&startupTabRadio.accesskey"?
Attachment #119820 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attached image Screenshot Option 1 (obsolete) —
Screenshot of option one version of the proposed new UI
Attached image Screenshot Option 2 (obsolete) —
Screenshot of option two version of the proposed new UI
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?
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.)
i prefer option 1, since group box titles are usually section names, never parts
of sentences (iirc).
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
Attached image Screenshot Option 2a (obsolete) —
Screenshot of the ammended option two version of the proposed new UI
This patch alters the prefs UI as per screenshot 1
This patch alters the prefs UI as per screenshot 2
Attachment #119822 - Attachment is obsolete: true
This patch alters the prefs UI as per screenshot 2a
Attached image Alternate UI. (obsolete) —
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".
The sentence would even makes sense in French in that changed order.  (I'm not
sure about other languages though.)
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.
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]"
Attached image Screenshot Option 3 (obsolete) —
Screenshot of option 3 - mock up of pref UI proposed by jag
Patch comming soon
Attached patch Patch v1.1 Option 3 layout (obsolete) — Splinter Review
Patch featuring the option 3 layout - any comments/feedback?
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.
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.)
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!
Ian: Obsolete all but the Options 3 attachments?  (I'll obsolete mine if you'll
obsolete yours. <grin>)
Attachment #119873 - Attachment is obsolete: true
Attachment #119874 - Attachment is obsolete: true
Attachment #119900 - Attachment is obsolete: true
Attachment #119994 - Attachment description: Screen Option 3 with expanded drop list → Screenshot Option 3 with expanded drop list
Attachment #119913 - Attachment is obsolete: true
Attachment #119906 - Attachment is obsolete: true
Attachment #119907 - Attachment is obsolete: true
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.
Attachment #119908 - Attachment is obsolete: true
Adding target of 1.4b and setting Hardware -> All
Hardware: PC → All
Target Milestone: --- → mozilla1.4beta
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.
Attachment #119953 - Attachment is obsolete: true
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?)
I think that is just the way jpeg turned out, on the original there is no spot
in the middle.
Attached patch Patch v1.2 (obsolete) — Splinter Review
Updated patch that works in conjunction with patch v1.6 for bug 197671
Attachment #119956 - Attachment is obsolete: true
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?
Attachment #120120 - Flags: superreview?(jaggernaut)
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.
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.

Attachment #120120 - Flags: superreview?(jaggernaut)
Attached patch UI Patch v1.3 (obsolete) — Splinter Review
UI patch v1.3 with disabled button taken out and Navigator startup page button
moved to the bottom of the list
Attachment #120120 - Attachment is obsolete: true
Attachment #120648 - Flags: superreview?(jaggernaut)
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).
Attachment #120648 - Flags: superreview?(jaggernaut) → superreview+
> Add sizetopopup="never" to this

Never mind that. There seems to be a bug in the css for the Mac classic menulist.
Attached patch Patch v1.3a (obsolete) — Splinter Review
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.
Attachment #120648 - Attachment is obsolete: true
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
Attachment #121226 - Flags: superreview+
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.
Attachment #121226 - Flags: superreview+ → superreview-
Attached patch Patch v1.3b (obsolete) — Splinter Review
Fix my nits, fix accesskeys.
Attachment #121226 - Attachment is obsolete: true
Comment on attachment 121401 [details] [diff] [review]
Patch v1.3b

sr=me,sr=hewitt on my changes
Attachment #121401 - Flags: superreview+
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 :-/
Attachment #121401 - Flags: review+
Flags: blocking1.4b?
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).
Is this now ready to ask for driver approval?
Oh... With childNode caching you mean something like:

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

?
Attached patch Patch v1.3cSplinter Review
Addresses Neil's comments.
Attachment #121401 - Attachment is obsolete: true
Comment on attachment 121521 [details] [diff] [review]
Patch v1.3c

sr=jag/hewitt
Attachment #121521 - Flags: superreview+
Comment on attachment 121521 [details] [diff] [review]
Patch v1.3c

Neil said r=Neil.
Attachment #121521 - Flags: review+
Attachment #121521 - Flags: approval1.4b?
Flags: blocking1.4b? → blocking1.4b-
Comment on attachment 121521 [details] [diff] [review]
Patch v1.3c

a=asa (on behalf of drivers) for checkin to 1.4b.
Attachment #121521 - Flags: approval1.4b? → approval1.4b+
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.
Status: ASSIGNED → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED
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.
Status: RESOLVED → VERIFIED
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).
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)
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. :)
Status: VERIFIED → REOPENED
Resolution: FIXED → ---
Attachment #122157 - Flags: superreview?(jaggernaut)
Attachment #122157 - Flags: review?(jaggernaut)
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
nominating for 1.4final.

robin, i think online help might need updating for the navigator pref panel.
Flags: blocking1.4?
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?
Attachment #122157 - Flags: superreview?(jaggernaut)
Attachment #122157 - Flags: superreview+
Attachment #122157 - Flags: review?(neil)
Attachment #122157 - Flags: review?(jaggernaut)
Attachment #122157 - Flags: review?(neil) → review+
Checked and it does stay set at -1
Comment on attachment 122157 [details] [diff] [review]
UI Patch fix v1.0

Requesting drivers approval
Attachment #122157 - Flags: approval1.4b?
Comment on attachment 122157 [details] [diff] [review]
UI Patch fix v1.0

a=sspitzer
Attachment #122157 - Flags: approval1.4b? → approval1.4b+
Checked in, marking fixed again.
Status: REOPENED → RESOLVED
Closed: 19 years ago19 years ago
Resolution: --- → FIXED
Flags: blocking1.4?
vrfy'd fixed with 2003.05.07 comm builds.

see bug 204157 for remaining issue with opening browser windows from non-browser
apps.
Status: RESOLVED → VERIFIED
*** Bug 204913 has been marked as a duplicate of this bug. ***
Blocks: 205261
*** Bug 184673 has been marked as a duplicate of this bug. ***
Product: Browser → Seamonkey
You need to log in before you can comment on or make changes to this bug.