Consider adding UI for pref browser.link.open_newwindow.restriction

RESOLVED FIXED in seamonkey2.1b2

Status

enhancement
RESOLVED FIXED
9 years ago
9 years ago

People

(Reporter: InvisibleSmiley, Assigned: neil)

Tracking

Trunk
seamonkey2.1b2
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

7.17 KB, patch
iann_bugzilla
: review+
Details | Diff | Splinter Review
Bug 505311 changed the default of browser.link.open_newwindow.restriction from 0 to 2. Maybe this deserves its own preferences UI, e.g. on the new Link Behavior pane.

Cf. http://kb.mozillazine.org/Browser.link.open_newwindow.restriction
Browser -> Link Behaviour -> "Link open behaviour" group box. Possibly a menulist at the bottom of this groupbox with three choices corresponding to browser.link.open_newwindow.restriction
Whiteboard: [good first bug]
Looking at the possible values, we might actually want to use two widgets:
1. A checkbox at the top of the groupbox that disables the whole contents of the groupbox (checked: pref = 1, unchecked: pref != 1).
2. A checkbox at the bottom of the groupbox that enables the exception for windows that specify how they should be displayed (checked: pref = 2, unchecked: pref = 0 unless first checkbox is checked)

That's a bit more complicated wrt. the implementation but possibly cleaner in the UI than a drop-down that might come naturally from a programmer's POV but actually be harder for someone to come up with precise descriptions for the individual options.
Unnecessary. We can use onsynctopref to trigger enabling/disabling the checkboxes in the groupbox. We do the same in some of our other panels. The menulist could be on top though.
Posted patch Possible patch (obsolete) — Splinter Review
Let the bikeshedding begin!
(In reply to comment #4)
> Created attachment 500552 [details] [diff] [review]
> Possible patch

So you disable open_newwindow.restriction UI when open_newwindow is 2. Did you come to this logic through code inspection? The MZ KB suggests that in fact open_newwindow should be disabled if open_newwindow.restriction is 1.

Pro Tip: Use attachment flags to get things reviewed or feedback. :-P

P.S. I wonder if the radiogroups should be indented (more). At least on Linux it looks all mixed now (only the bold headings are really distinguishable from the other text on the pane). Cf. the Downloads pane for what I'd recommend (although it's a checkbox instead of a description there).
Comment on attachment 500552 [details] [diff] [review]
Possible patch

Requesting review on Neil's behalf (which I'm only doing as he didn't assign himself either).
Attachment #500552 - Flags: review?(iann_bugzilla)
Assignee: nobody → neil
Status: NEW → ASSIGNED
Comment on attachment 500552 [details] [diff] [review]
Possible patch

if browser.link.open_newwindow = 2 on opening the links pref pane it does not disable the restrictionGroup only if you switch to 2 whilst the pane is open.
r-
Attachment #500552 - Flags: review?(iann_bugzilla) → review-
(In reply to comment #5)
> So you disable open_newwindow.restriction UI when open_newwindow is 2. Did you
> come to this logic through code inspection? The MZ KB suggests that in fact
> open_newwindow should be disabled if open_newwindow.restriction is 1.
There are three things that can be diverted into tabs:
1. <a target>
2. window.open without features
3. window.open with features
Gecko versions 1.8.1 through 1.9.2 did not distinguish between cases 1. and 2.
As a result, setting open_newwindow to 2 and setting restriction to 1 both had the same effect of turning off the diverts. However, prior to 1.8.1, and newly restored in Gecko 2, setting restriction to 1 does not affect <a target> which still diverts according to the open_newwindow preference.

(In reply to comment #7)
> if browser.link.open_newwindow = 2 on opening the links pref pane it does not
> disable the restrictionGroup only if you switch to 2 whilst the pane is open.
As Ratty pointed out I should have used onsynctopreference but for the purposes of this draft I was too lazy to add the necessary script file.
Posted patch Revised patchSplinter Review
OK, let's see how you like this version.

I changed my mind about onsynctopreference; we seem to use Startup and onchange except when we want to manipulate the returned value.
Attachment #500552 - Attachment is obsolete: true
Attachment #508772 - Flags: review?(iann_bugzilla)
Attachment #508772 - Flags: review?(iann_bugzilla) → review+
Pushed changeset 2d936fdce609 to comm-central.
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Whiteboard: [good first bug]
Target Milestone: --- → seamonkey2.1b2
Blocks: 636788
You need to log in before you can comment on or make changes to this bug.