344140 - "Recently closed tabs" entries not middle-clickable

Status: RESOLVED FIXED

(Firefox :: Menus, enhancement)

Description

[Oakwine]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1b1) Gecko/20060710 BonEcho/2.0b1
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1b1) Gecko/20060710 BonEcho/2.0b1

When I stop to think about it, I know that reopening a recently closed tab will create a new tab.  However, I usually middle-click any bookmark that I see, so my muscle-memory makes me middle-click the entries in the Recently Closed Tabs list.  This has no apparent effect.

Reproducible: Always

Steps to Reproduce:
1. Close a tab with content in it.
2. Mouse to History > Recently Closed Tabs >
3. Middle-click an entry.

Actual Results:  
Bon Echo closes the menu, but does nothing.

Expected Results:  
A new tab opens, "unclosing" that tab.

Comment 1

[Ria Klaassen (not reading all bugmail)]

Oh well, to get the right muscle-memory you should use the tab close buttons more often. ;)
Although it would make sense to me if the Recently Closed Tabs menu-item was middle-clickable.

Comment 2

[Simon Bünzli]

I'd rather educate our users to always click the correct button. But since that won't be possible and since this doesn't really hurt anybody, why not implement it anyway?

Comment 3

[Michael Ventnor]

Attachment: Correctly handles middle click

For some odd reason using checkForMiddleClick doesn't work. This was the best implementation I could think of.

Comment 4

[tommyjb]

The top-level menu item ('Recently Closed Tabs >') should be middle-clickable as well, given that it contains an 'Open In Tabs' sub-item.

Comment 5

[Asaf Romano (gone)]

Hrm, should middle click really do the same thing as left click? One option is to make middle click always append the tab to the end of the tabstrip.

Beltzner, any thoughts?

Comment 6

[Michael Ventnor]

Attachment: Patch with my proposed middle-click behaviour

Here's my idea, with a patch to go. When the user middle clicks on an item in the recently closed tabs menu, not only does the tab get put on the end but it is put in the background too. That way people who want to switch to it immediately will train their muscle-memory to left click, and people who want this kind of feature can have it through the middle click.

Comment 7

[Asaf Romano (gone)]

Comment on attachment 230298 [details] [diff] [review]
Patch with my proposed middle-click behaviour

get ui-review from beltzner first.

Comment 8

[Michael Ventnor]

Attachment: Patch with minor nitfixes

A few of my own nits fixed here. Changed the variable names to something more meaningful than "oldTab" and "newTab". We should also revert focus back to the current tab before moving the reopened tab, not after.

Comment 9

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 230306 [details] [diff] [review]
Patch with minor nitfixes


>+ * If middle-clicked, put the reopened tab on the end in the background.
>+ * @param aEvent

I understand the putting it at the end in a new tab - that's what middle-clicking a history entry in the history portion of the drop-down would do, too, and that makes sense to me. But why aren't we giving it focus? Now we've got middle-clicks in one part of the history menu appending-tabs-with-focus and middle-clicks in the other part of the history menu appending-tabs-without-focus.

Give 'em focus, and you can have my implicit uir+ :)

(thanks for fixing the no-middle-click support at all thing, though! nice catch!)

Comment 10

[Michael Ventnor]

Attachment: Patch with beltzner's suggestions

OK, the reopened tab will now get focus.

Comment 11

[Nickolay_Ponomarev]

Comment on attachment 230374 [details] [diff] [review]
Patch with beltzner's suggestions

>-    m.addEventListener("command", function(aEvent) { 
>-      undoCloseTab(aEvent.originalTarget.getAttribute("value"));
>-    }, false);
>+    m.addEventListener("click", function(aEvent) { closedTabsMenuClick(aEvent); }, false);

Does this mean activating the menu using the keyboard will no longer work?

Comment 12

[Nickolay_Ponomarev]

Also note that shortcuts like Shift+click (open in a new window) won't work. From the looks of it, fixing that isn't trivial with current session store API.

Comment 13

[Michael Ventnor]

Attachment: Patch addressing Nickolay's concern

Now that I think about it, this new way I have designed the patch no longer needs the click listener and can be put into the command listener again. Thanks for the catch, Nick.

Comment 14

[Nickolay_Ponomarev]

No, command event handler gets a nsIDOMXULCommandEvent event, which doesn't have the button property. And I don't think you need to ask beltzner for UI-review again if you do select the created tab.

Comment 15

[Michael Ventnor]

Comment on attachment 230384 [details] [diff] [review]
Patch addressing Nickolay's concern

OK, I'll try and fix all of this up when i get home.

Comment 16

[Michael Ventnor]

Attachment: Patch _really_ addressing Nickolay's concerns

Right, I think I've finally nailed it on the head with this one. It has restored keyboard activation and also has added Ctrl+Click support.

Comment 17

[Nickolay_Ponomarev]

Comment on attachment 230405 [details] [diff] [review]
Patch _really_ addressing Nickolay's concerns

>+    //Fix middle click
>+    m.addEventListener("click", function(aEvent) { 
>+      if (aEvent.button == 1)
>+        closedTabsMenuClick(aEvent);
>     }, false);

I didn't test the patch; does this close the popups on middle-clicking? I ask because the checkForMiddleClick() function, which you're copying here, has to close popup menus manually.

>+function closedTabsMenuClick(aEvent) {
>+  var where = whereToOpenLink(aEvent);
>+  undoCloseTab(aEvent.originalTarget.getAttribute("value"));
>+  if (aEvent.button == 1 || where == "tab") {

Why do you need the aEvent.button check here? whereToOpenLink is supposed to take care of that.

Comment 18

[Michael Ventnor]

(In reply to comment #17)
> (From update of attachment 230405 [details] [diff] [review] [edit])
> >+    //Fix middle click
> >+    m.addEventListener("click", function(aEvent) { 
> >+      if (aEvent.button == 1)
> >+        closedTabsMenuClick(aEvent);
> >     }, false);
> 
> I didn't test the patch; does this close the popups on middle-clicking? I ask
> because the checkForMiddleClick() function, which you're copying here, has to
> close popup menus manually.
> 
> >+function closedTabsMenuClick(aEvent) {
> >+  var where = whereToOpenLink(aEvent);
> >+  undoCloseTab(aEvent.originalTarget.getAttribute("value"));
> >+  if (aEvent.button == 1 || where == "tab") {
> 
> Why do you need the aEvent.button check here? whereToOpenLink is supposed to
> take care of that.
> 

1. Yes.
2. If the person has the "middle click a link should open tabs" setting off, middle clicking this will be the same as normal click (tab not moved to the end). Of course if this is wanted I can erase this check.

Comment 19

[Michael Ventnor]

(In reply to comment #18)
> (In reply to comment #17)
> > (From update of attachment 230405 [details] [diff] [review] [edit] [edit])
> > >+    //Fix middle click
> > >+    m.addEventListener("click", function(aEvent) { 
> > >+      if (aEvent.button == 1)
> > >+        closedTabsMenuClick(aEvent);
> > >     }, false);
> > 
> > I didn't test the patch; does this close the popups on middle-clicking? I ask
> > because the checkForMiddleClick() function, which you're copying here, has to
> > close popup menus manually.
> > 
> > >+function closedTabsMenuClick(aEvent) {
> > >+  var where = whereToOpenLink(aEvent);
> > >+  undoCloseTab(aEvent.originalTarget.getAttribute("value"));
> > >+  if (aEvent.button == 1 || where == "tab") {
> > 
> > Why do you need the aEvent.button check here? whereToOpenLink is supposed to
> > take care of that.
> > 
> 
> 1. Yes.
> 2. If the person has the "middle click a link should open tabs" setting off,
> middle clicking this will be the same as normal click (tab not moved to the
> end). Of course if this is wanted I can erase this check.
> 

PS. I tried the checkForMiddleClick function twice and it didn't work.

Comment 20

[Michael Ventnor]

Comment on attachment 230405 [details] [diff] [review]
Patch _really_ addressing Nickolay's concerns

Add Beltzner's ui-r, waiting on Gavin's review and a checkin.

Comment 21

[Michael Ventnor]

Attachment: Patch for trunk changes

There have been significant changes to the trunk since this patch was made. I updated the patch to reflect these changes.

Comment 22

[Michael Ventnor]

Attachment: Code shift

This moves the button check to within the called function, making the code a bit cleaner.

Comment 23

[Asaf Romano (gone)]

Comment on attachment 242848 [details] [diff] [review]
Code shift

>Index: browser/base/content/browser.js
>===================================================================

>+/**
>+*  Re-open a closed tab and put it to the end of the tab strip. Used for a middle click.

nite: 80 characters for a line please

>+ * @param aEvent
>+ *        The event when the user clicks the menu item

nit: prefix this with two spaces.

>+ */
>+function undoCloseMiddleClick(aEvent) {
>+  if (aEvent.button != 1)
>+    return;
>+
>+  undoCloseTab(aEvent.originalTarget.getAttribute("value"));

nit: use the value property instead

>+  var tabbrowser = getBrowser();
>+  var reopenedTab = tabbrowser.mCurrentTab;
>+  tabbrowser.moveTabTo(reopenedTab, tabbrowser.mTabContainer.childNodes.length - 1);

tabbrrowser has a handy moveTabToEnd method

Comment 24

[Michael Ventnor]

Attachment: Nits fixed

You sure love your nits, Asaf. :P

Comment 25

[Asaf Romano (gone)]

Comment on attachment 242852 [details] [diff] [review]
Nits fixed

I sure don't like the way you're interpreting the sense behind code reviews (hint: bug 345257), but anyway:

>Index: browser/base/content/browser.js
>===================================================================

>+/**
>+ *  Re-open a closed tab and put it to the end of the tab strip. 
>+ *  Used for a middle click.
>+ *  @param aEvent
>+ *        The event when the user clicks the menu item

nit: missing space ;)

>+ */
>+function undoCloseMiddleClick(aEvent) {
>+  if (aEvent.button != 1)
>+    return;
>+
>+  undoCloseTab(aEvent.originalTarget.value);
>+  var tabbrowser = getBrowser();
>+  tabbrowser.moveTabToEnd();

make this |getBrowser().moveTabToEnd();|


r=mano otherwise.

Comment 26

[Michael Ventnor]

Attachment: Nits fixed (again)

Sorry, it's just that sometimes the suggestions seem so vague. By prefixing the @param comment did you mean to put the space before or after the asterisk? If I put it before, it made the alignment different to the comments on the actual undoCloseTab function, so I didn't think you meant that.

Nonetheless, I think this patch is good to go now (really). :P

Comment 27

[Asaf Romano (gone)]

hrm, i get
patching file browser/base/content/browser.js
patch: **** malformed patch at line 53:

when trying to apply your patch, please diff again.

Comment 28

[Michael Ventnor]

Attachment: Bitrot fixed

Try again with this patch.

Comment 29

[Asaf Romano (gone)]

Checking in browser/base/content/browser.js;
/cvsroot/mozilla/browser/base/content/browser.js,v  <--  browser.js
new revision: 1.723; previous revision: 1.722
done