The current tabs preferences pane is overcrowded and in some themes is clipped at the bottom. We can make it less congested by moving the link behaviour preferences to a separate pane.

>diff --git a/suite/common/pref/preferences.xul b/suite/common/pref/preferences.xul >+ <treeitem id="linksItem" >+ label="&links.label;" >+ prefpane="links_pane" >+ helpTopic="navigator_pref_tabbed_browsing" Note to self: file a followup help bug.

Comment on attachment 450131 [details] [diff] [review] Patch v1.0 links pane. In principle I am happy with this but I'd like some better accesskeys in the two resultant pref panes now there is more choice.

> Ian Neal 2010-06-15 13:43:14 PDT > (From update of attachment 450131 [details] [diff] [review]) > In principle I am happy with this but I'd like some better accesskeys in the > two resultant pref panes now there is more choice. I changed about two access keys; the rest of the keys are pretty much optimized. Tabbed Browsing->Open tabs instead of windows. These access keys come from platformCommunicatorOverlay.dtd and I don't want to change anything in case I break something. Link Behaviour. The first group uses the first available consonant in (current|tab|window). The second group uses the first available vowel in (current|tab|window).

Comment on attachment 451589 [details] [diff] [review] Patch v1.1 Access Keys >+++ b/suite/locales/en-US/chrome/common/pref/pref-links.dtd >@@ -0,0 +1,17 @@ >+<!ENTITY linksHeader.label "Link Behaviour"> >+<!ENTITY newWindow.label "Link open behavior"> >+<!ENTITY newWindowDescription.label "Open links meant to open a new window in"> >+<!ENTITY external.label "Links from other applications"> >+<!ENTITY externalDescription.label "Open links passed from other applications in"> >+ >+<!ENTITY openCurrent.label "The current tab/window"> >+<!ENTITY newWindowGroupCurrent.accesskey "c"> >+<!ENTITY externalGroupCurrent.accesskey "u"> >+ >+<!ENTITY openTab.label "A new tab in the current window"> >+<!ENTITY newWindowGroupTab.accesskey "t"> >+<!ENTITY externalGroupTab.accesskey "a"> Shouldn't this be "A"? >+ >+<!ENTITY openWindow.label "A new window"> >+<!ENTITY newWindowGroupWindow.accesskey "w"> >+<!ENTITY externalGroupWindow.accesskey "o"> What about using "n" for one of these? r=me with those addressed.

Comment on attachment 451589 [details] [diff] [review] Patch v1.1 Access Keys I'm not really fond of this, as I think that we actually should and could win space by merging the UI prefs for internal and external links, which I believe we will need to do when we make all tabs in all components the same - but then, we are not there yet...

Comment on attachment 451589 [details] [diff] [review] Patch v1.1 Access Keys > >+<!ENTITY openTab.label "A new tab in the current window"> > >+<!ENTITY newWindowGroupTab.accesskey "t"> > >+<!ENTITY externalGroupTab.accesskey "a"> > Shouldn't this be "A"? > >+<!ENTITY openWindow.label "A new window"> > >+<!ENTITY newWindowGroupWindow.accesskey "w"> > >+<!ENTITY externalGroupWindow.accesskey "o"> > What about using "n" for one of these? > r=me with those addressed. See Comment 3. Normally this would be optimal. However the way I reverse engineered the current access keys is that the key words are (current|tab|window) so the first group uses the first available consonant from these words. And the second group selects the first available vowel. > What about using "n" for one of these? The "n" ends up on _n_ew and not wi_n_dow.

Why not just change the radiobuttons into a menulist? +- Link open behaviour -----------------------------+ | Open links meant to open a new window in | | [ A new window | V ] | | Open links passed from other applications in | | [ A new tab in the current window | V ] | +---------------------------------------------------+

Hmm. Good Idea. This means I don't have to split the pref-tabs and I can get rid of all those pesky access keys.

> Why not just change the radiobuttons into a menulist? Actually this doesn't seem to save any significant vertical space.

>> Why not just change the radiobuttons into a menulist? > Actually this doesn't seem to save any significant vertical space. I think moving the links to a separate prefpane as in patch v1.1 is still a good idea as this frees up some space for newer tabbrowser prefs e.g. those features Misak is working on. Neil? IanN?

(In reply to comment #10) > I think moving the links to a separate prefpane as in patch v1.1 is still a > good idea as this frees up some space for newer tabbrowser prefs e.g. those > features Misak is working on. Neil? IanN? Hmm, there are a number of tab-related prefs in browser-prefs.js which we don't expose yet. It would also give us a chance to add the diversion restriction.

(In reply to comment #11) > It would also give us a chance to add the diversion restriction. xref Bug 378213

(In reply to comment #11) > (In reply to comment #10) > > I think moving the links to a separate prefpane as in patch v1.1 is still a > > good idea as this frees up some space for newer tabbrowser prefs e.g. those > > features Misak is working on. Neil? IanN? > Hmm, there are a number of tab-related prefs in browser-prefs.js which we don't > expose yet. It would also give us a chance to add the diversion restriction. Indeed, is it worth rolling up into this patch?

Comment on attachment 452223 [details] Links Prefs as menulists. I prefer a separate pref panel

Comment on attachment 451589 [details] [diff] [review] Patch v1.1 Access Keys Having had another look at the wording, I think with a few tweaks could simplify things. "other applications" being repeated for example.

> Having had another look at the wording, I think with a few tweaks could > simplify things. "other applications" being repeated for example. Suggestions for wording? +<!ENTITY newWindow.label "Link open behavior"> +<!ENTITY newWindowDescription.label "Open links meant to open a new window in"> +<!ENTITY external.label "Links from other applications"> +<!ENTITY externalDescription.label "Open links passed from other applications in"> How about: "Open external links in" ?

(In reply to comment #16) > +<!ENTITY newWindow.label "Link open behavior"> > (...) > +<!ENTITY external.label "Links from other applications"> > > How about: "Open external links in" ? Or to make it more in line with the other box: "External link open behavior"?

>> How about: "Open external links in" ? > Or to make it more in line with the other box: "External link open behavior"? I'm talking about this entity: +<!ENTITY externalDescription.label "Open links passed from other applications in">

(In reply to comment #18) > >> How about: "Open external links in" ? > > Or to make it more in line with the other box: "External link open behavior"? > > I'm talking about this entity: > +<!ENTITY externalDescription.label "Open links passed from other applications > in"> Ah, I see. Ian's comment left it open (on purpose?) whether to change the label or description. I'm still unsure which is better to be changed: "external links" is a bit too vague for my taste but as long as there is a reference to "other applications" in either the label or description I guess it's OK. BTW: You have 2x "Behaviour" (BE) in your patch, and in this bug's summary. ;-)

> Having had another look at the wording, I think with a few tweaks could > simplify things. "other applications" being repeated for example. --------------------------------------- From: http://kb.mozillazine.org/Browser.link.open_external 1 Open links from external programs in the current window. 2 Open links from external programs in a new window. (Default in SeaMonkey) 3 Open links from external programs in new tabs in an existing window. (Default in Firefox) --------------------------------------- From: Firefox 3.0 mozilla/browser/components/preferences/tabs.js * browser.link.open_external * - determines where pages opened by external applications are opened: * 0 opens such links in the default window, * 1 opens such links in the most recent window or tab, * 2 opens such links in a new window, * 3 opens such links in a new tab <!ENTITY newPagesIn.label "New pages should be opened in:"> --------------------------------------- In the attached screenshot, I changed the description in the second group from: Open links passed from other applications in to New Pages sent from other programs should open in

Comment on attachment 451589 [details] [diff] [review] Patch v1.1 Access Keys I've just been thinking ahead to if/when bug 378213 gets fixed and what is the best layout to allow for that. For the moment I am happy to go with the current patch. What do you think about adding all four options or would that be too confusing?

Comment on attachment 453028 [details] Prefpane Screenshot: Tweak description. I was thinking of changing the groupbox caption. For example for the external links one, to "When links are passed from other applications open in" and removing the description. Though, as I said, if we're looking at how the layout will change when other prefs are added, it might be better to leave those as they are.

Comment on attachment 451589 [details] [diff] [review] Patch v1.1 Access Keys > I've just been thinking ahead to if/when bug 378213 gets fixed and what is the > best layout to allow for that. For the moment I am happy to go with the current > patch. Hmm. > // 0: no restrictions - divert everything > // 1: don't divert window.open at all > // 2: don't divert window.open with features > pref("browser.link.open_newwindow.restriction", 0); Looking at: http://mxr.mozilla.org/mozilla-central/source/xpfe/appshell/src/nsContentTreeOwner.cpp#877 > What do you think about adding all four options or would that be too > confusing? I think this needs some thought as to the UI. So I prefer that the restriction preference be done in a separate patch. Lets get the UI in this patch into the tree first.

Comment on attachment 451589 [details] [diff] [review] Patch v1.1 Access Keys >- >+ Please try to avoid random whitespace changes. This one is OK though. >- </prefpane> >+ </prefpane> As is this. >+ helpTopic="navigator_pref_tabbed_browsing" File a bug on a new help topic? >new file mode 100644 This isn't a new file, it's a copy of pref-tabs.dtd with some lines deleted. (pref-links.xul probably looks too different to pref-tabs.xul though.) >+<!ENTITY links.label "Link Behaviour"> Needs to be US spelling, unfortunately :-(

(In reply to comment #20) > From: Firefox 3.0 > > mozilla/browser/components/preferences/tabs.js > > * browser.link.open_external > * - determines where pages opened by external applications are opened: > * 0 opens such links in the default window, > * 1 opens such links in the most recent window or tab, > * 2 opens such links in a new window, > * 3 opens such links in a new tab Actually 0 makes zero sense here, as this is the pref used when the API is passed a zero value. 1 is the default default ;-)

> (From update of attachment 451589 [details] [diff] [review]) > >- > >+ > Please try to avoid random whitespace changes. This one is OK though. Fixed. > >- </prefpane> > >+ </prefpane> > As is this. Fixed. > >+ helpTopic="navigator_pref_tabbed_browsing" > File a bug on a new help topic? Will do. > >new file mode 100644 > This isn't a new file, it's a copy of pref-tabs.dtd with some lines deleted. > (pref-links.xul probably looks too different to pref-tabs.xul though.) Now using hg cp > >+<!ENTITY links.label "Link Behaviour"> > Needs to be US spelling, unfortunately :-( Fixed.

Pushed to comm-central http://hg.mozilla.org/comm-central/rev/9fb2b78003f3

Help changes are needed: here or in another bug?