570970 - Move the Link Behaviour preferences from the tabs pane to a separate pane.

Status: RESOLVED FIXED

(SeaMonkey :: Preferences, defect)

Description

[Philip Chee]

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.

Comment 1

[Philip Chee]

Attachment: Patch v1.0 links 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 2

[Ian Neal]

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.

Comment 3

[Philip Chee]

Attachment: Patch v1.1 Access Keys

> 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 4

[Ian Neal]

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 5

[Robert Kaiser]

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 6

[Philip Chee]

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.

Comment 7

[[e-mail]]

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 ]      |
+---------------------------------------------------+

Comment 8

[Philip Chee]

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.

Comment 9

[Philip Chee]

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

Comment 10

[Philip Chee]

Attachment: Links Prefs as menulists.

>> 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?

Comment 11

[[e-mail]]

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

Comment 12

[Jens Hatlak (:InvisibleSmiley)]

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

xref Bug 378213

Comment 13

[Ian Neal]

(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 14

[Ian Neal]

Comment on attachment 452223 [details]
Links Prefs as menulists.

I prefer a separate pref panel

Comment 15

[Ian Neal]

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.

Comment 16

[Philip Chee]

> 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" ?

Comment 17

[Jens Hatlak (:InvisibleSmiley)]

(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"?

Comment 18

[Philip Chee]

>> 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">

Comment 19

[Jens Hatlak (:InvisibleSmiley)]

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

Comment 20

[Philip Chee]

Attachment: Prefpane Screenshot: Tweak description.

> 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 21

[Ian Neal]

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 22

[Ian Neal]

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 23

[Philip Chee]

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 24

[[e-mail]]

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 :-(

Comment 25

[[e-mail]]

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

Comment 26

[Philip Chee]

Attachment: [for checkin] Patch v1.2 r=IanN sr=Neil

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

Comment 27

[Philip Chee]

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

Comment 28

[Giacomo Magnini]

Help changes are needed: here or in another bug?