Closed Bug 570970 Opened 10 years ago Closed 9 years ago

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

Categories

(SeaMonkey :: Preferences, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
seamonkey2.1a3

People

(Reporter: philip.chee, Assigned: philip.chee)

References

Details

Attachments

(3 files, 2 obsolete files)

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.
Attached patch Patch v1.0 links pane. (obsolete) — Splinter Review
>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.
Attachment #450131 - Flags: review?(iann_bugzilla)
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.
Attachment #450131 - Flags: review?(iann_bugzilla)
Attached patch Patch v1.1 Access Keys (obsolete) — Splinter Review
> 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).
Attachment #450131 - Attachment is obsolete: true
Attachment #451589 - Flags: review?(iann_bugzilla)
Attachment #451589 - Flags: feedback?(kairo)
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.
Attachment #451589 - Flags: review?(iann_bugzilla) → review+
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...
Attachment #451589 - Flags: feedback?(kairo)
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.
Attachment #451589 - Flags: superreview?(neil)
Attachment #451589 - Flags: feedback?(iann_bugzilla)
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?
Attachment #452223 - Flags: feedback?(neil)
Attachment #452223 - Flags: feedback?(iann_bugzilla)
(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
Attachment #452223 - Flags: feedback?(iann_bugzilla) → feedback-
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.
Attachment #451589 - Flags: feedback?(iann_bugzilla) → feedback-
> 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. ;-)
Attachment #452223 - Flags: feedback?(neil)
Attachment #451589 - Flags: superreview?(neil)
> 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
Attachment #453028 - Flags: review?(iann_bugzilla)
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?
Attachment #451589 - Flags: feedback- → feedback+
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.
Attachment #453028 - Flags: review?(iann_bugzilla) → review-
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.
Attachment #451589 - Flags: superreview?(neil)
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 :-(
Attachment #451589 - Flags: superreview?(neil) → superreview+
(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.
Attachment #451589 - Attachment is obsolete: true
Attachment #453962 - Flags: superreview+
Attachment #453962 - Flags: review+
Pushed to comm-central
http://hg.mozilla.org/comm-central/rev/9fb2b78003f3
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Help changes are needed: here or in another bug?
Depends on: 574609
Target Milestone: --- → seamonkey2.1a3
You need to log in before you can comment on or make changes to this bug.