Closed Bug 1273240 Opened 4 years ago Closed 3 years ago

XUL 'text-link' should open tabs rather than windows; Middle mouse click on text-links does nothing (should open in new tab) [Port Bug 263433, Bug 1268943 and Bug 1274533]

Categories

(SeaMonkey :: General, defect)

All
Unspecified
defect
Not set

Tracking

(seamonkey2.42 wontfix, seamonkey2.43 wontfix, seamonkey2.44 wontfix, seamonkey2.45 wontfix, seamonkey2.46 fixed)

RESOLVED FIXED
seamonkey2.46
Tracking Status
seamonkey2.42 --- wontfix
seamonkey2.43 --- wontfix
seamonkey2.44 --- wontfix
seamonkey2.45 --- wontfix
seamonkey2.46 --- fixed

People

(Reporter: philip.chee, Assigned: frg)

Details

Attachments

(1 file, 3 obsolete files)

See:
Bug 263433 - 'text-link' XUL widget should open tabs rather than windows
Bug 1268943 - Middle mouse click on links in settings does nothing (should open in new tab)
Attached patch 1273240-middle_click.patch (obsolete) — Splinter Review
Patch as discussed on irc.

3 things:

- I do not found a testcase. Don't see any difference in behaviour.
- Should true used for the add observer to hold a weak reference?
- We do not have a shutdown routine and don't do the removeObserver. Right?
Assignee: nobody → frgrahl
Status: NEW → ASSIGNED
Attachment #8753775 - Flags: review?(philip.chee)
Attached patch 1273240-middle_click-V2.patch (obsolete) — Splinter Review
Bug 1274533 will land soon which will be change some functionality here.
Attachment #8753775 - Attachment is obsolete: true
Attachment #8753775 - Flags: review?(philip.chee)
Attachment #8755453 - Flags: review?(philip.chee)
Summary: XUL 'text-link' should open tabs rather than windows; Middle mouse click on text-links does nothing (should open in new tab) [Port Bug 263433 and Bug 1268943] → XUL 'text-link' should open tabs rather than windows; Middle mouse click on text-links does nothing (should open in new tab) [Port Bug 263433, Bug 1268943 and Bug 1274533]
Comment on attachment 8755453 [details] [diff] [review]
1273240-middle_click-V2.patch

> - I do not found a testcase. Don't see any difference in behaviour.
1. Load about:addons in a tab.
2. Pick a random extension and click on the [More] link.
3. Try {left|middle|right} click on the homepage link or the reviews link.

> - Should true used for the add observer to hold a weak reference?
Yes.

> - We do not have a shutdown routine and don't do the removeObserver.
Right

> +      case "handle-xul-text-link":
> +        let linkHandled = subject.QueryInterface(Ci.nsISupportsPRBool);
We don't have Ci so please use Components.interfaces instead.

> +        if (!linkHandled.data) {
> +          let win = this.getMostRecentBrowserWindow();
We don't have getMostRecentBrowserWindow(). Just use:
  Services.wm.getMostRecentWindow("navigator:browser");
instead like the rest of the file.

> +            data = JSON.parse(data);
> +            let where = win.whereToOpenLink(data);
Or just ... win.whereToOpenLink(JSON.parse(data));
Attachment #8755453 - Flags: review?(philip.chee) → review-
Attached patch 1273240-middle_click-V3.patch (obsolete) — Splinter Review
>> 3. Try {left|middle|right} click on the homepage link or the reviews link.

Thanks. V3 tested and found working. 

>> We don't have Ci so please use Components.interfaces instead.

Sorry. I know. Was sloppy and didn't catch it without a test case.

>> We don't have getMostRecentBrowserWindow(). Just use:

Would it make sense to port this in another bug?

>> Or just ... win.whereToOpenLink(JSON.parse(data));

Wouldn't work. The observer notification was changed to

>> .notifyObservers(linkHandled, "handle-xul-text-link", JSON.stringify(data));

and data in V2 patch was needed a second time a few lines below. I disliked overwriting the parameter and changed this to parse into a local variable.
Attachment #8755453 - Attachment is obsolete: true
Attachment #8757634 - Flags: review?(philip.chee)
Comment on attachment 8757634 [details] [diff] [review]
1273240-middle_click-V3.patch

r=me with the following issues fixed. Also a=me if APPROVAL is needed

> +            let aData = JSON.parse(data);
See this is what confused me because we usually only decorate the args but here data is one of the arguments. So please use something like dataObj on the left side of the assignment.

> +            let where = mostRecentBrowserWindow.whereToOpenLink(aData);
....  .whereToOpenLink(dataObj, false, true, true);

> +            // Preserve legacy behavior of non-modifier left-clicks
> +            // opening in a new selected tab.
> +            if (where == "current") {
> +              where = "tab";
.... where = "tabfocused";

> +            mostRecentBrowserWindow.openUILinkIn(aData.href, where);
> +            linkHandled.data = true;
Attachment #8757634 - Flags: review?(philip.chee) → review+
V4 with noted issues fixed. Review+ from Philip Chee carried over.
Attachment #8757634 - Attachment is obsolete: true
Attachment #8757676 - Flags: review+
Checked in. I think not important enough to go into the other branches.

https://hg.mozilla.org/comm-central/rev/5adad11ce60b
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → seamonkey2.46
You need to log in before you can comment on or make changes to this bug.