263942 - Reload button should have middle click support (open same URL in new tab, clone tab)

Status: RESOLVED FIXED

(Firefox :: General, enhancement)

Description

[Ido Beeri]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; rv:1.7.3) Gecko/20040913 Firefox/0.10.1
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; rv:1.7.3) Gecko/20040913 Firefox/0.10.1

When middle-clicking the Back button, you go back one page in a new tab.
Why not making the same for the Reload button?

When middle-clicking the Reload button, the same page should be reopened in a
new tab.

Reproducible: Always
Steps to Reproduce:

Comment 1

[Ian Macfarlane]

This is also a good way to help people duplicate the current tab without adding
options for that.

Comment 2

[Phil Ringnalda (:philor)]

*** Bug 278131 has been marked as a duplicate of this bug. ***

Comment 3

[Ryan VanderMeulen [:RyanVM]]

Attachment: Disable reload command on blank pages/tabs

Comment 4

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 171189 [details] [diff] [review]
Disable reload command on blank pages/tabs

Sorry, submitted this to the wrong bug somehow.

Comment 5

[Jaime Mitchell (use [e-mail] for email)]

*** Bug 251207 has been marked as a duplicate of this bug. ***

Comment 6

[John Mellor (Jomel)]

Before this bug can be fixed, we must decide what Shift middle-clicking (same as
Ctrl-Shift-clicking) the button should do:
Currently, Shift-clicking causes the reload to bypass the cache (consistent with
Ctrl-Shift-R keyboard shortcut and the general use of Shift as a modifier).
However Shift middle-clicking (and Ctrl-Shift-clicking) things normally opens
the target in a new *background* tab (e.g. back and forward buttons, see also
comment for whereToOpenLink in
http://lxr.mozilla.org/mozilla/source/browser/base/content/utilityOverlay.js#127),
so it would make sense for Shift middle-clicking to reload the page in a new
background tab.
Personally I think the latter would make things more consistent. Either way we
should probably think up a modifier combination for reload tab in a new
background tab bypassing cache... (and possibly change the modifier for bypass
cache when leftclicking on the reload button, or at least make both work).

Note: I use the terms Ctrl and Shift loosely - the actual keyboard shortcuts may
vary a little, this is just what they are on windows.

Comment 7

[Ian Macfarlane]

What about double clicking it to reload from network? This could even be
combined with the various ctrl and shift options to open in new tabs etc. Plus
it feels right for some reason (at least to me).

Comment 8

[Jesse Ruderman]

*** Bug 324373 has been marked as a duplicate of this bug. ***

Comment 9

[John Mellor (Jomel)]

I talked to timeless about this on IRC and he said it'd make much more sense to add the functionality to the url proxy icon (aka the favicon to the left of the location bar) instead of the reload button, so I've opened bug 332498 for that approach. It is perhaps slightly less intuitive (at least with the current UI), but it would certainly clear up any issues with conflicting modifier keys, etc.

Comment 11

[Vincent Lefevre]

Shouldn't this bug block the tracking bug 70501?

Comment 12

[Klaas Heidstra]

Attachment: Patch - Make link modifiers work with the reload button

Before Firefox 3 you could clone the current tab by middle-clicking the url-bar go-button. In Firefox 3 however the go-button doesn't appear until you edit the url, which makes it impossible to clone the current tab in a single middle-click (because of that this could be called a regression compared to Firefox 2). This can be solved by making middle-click possible on the reload-button.

What this patch does is:

Shift+Reload-button             reload page in current tab/window bypassing the
Alt+Reload-button               save current page as... cache (just as before)
Ctrl+Reload-button              current page in new tab, selected*
Ctrl+Shift+Reload-button        current page in new tab, in background*
Middle on Reload-button         same as Ctrl+Reload-button*
Shift+Middle on Reload-button   same as Ctrl+Shift+Reload-button*

(*) This is reversed when you set the pref to load bookmark tabs in the background.

Comment 13

[Dão Gottwald [:dao]]

Comment on attachment 325797 [details] [diff] [review]
Patch - Make link modifiers work with the reload button

>-                <menuitem label="&reloadCmd.label;" accesskey="&reloadCmd.accesskey;" command="Browser:Reload" key="key_reload"/>
>+                <menuitem label="&reloadCmd.label;" accesskey="&reloadCmd.accesskey;" key="key_reload"
>+                          oncommand="if (event.shiftKey) BrowserReloadSkipCache(event, true); else BrowserReload(event, true)"
>+                          onclick="checkForMiddleClick(this, event);">
>+                  <observes element="Browser:Reload" attribute="disabled" />
>+                </menuitem>

Gnomestripe uses menuitem[command="Browser:Reload"], and you're braking this here.
You're also putting too much logic in the oncommand attribute, IMHO. I think you should simplify this by always ignoring Alt.

Please request ui review from beltzner.

Comment 14

[Klaas Heidstra]

> Gnomestripe uses menuitem[command="Browser:Reload"], and you're braking this
> here.
> You're also putting too much logic in the oncommand attribute, IMHO. I think
> you should simplify this by always ignoring Alt.
> 
> Please request ui review from beltzner.

When you set it to always ignore alt it is not possible to save the current page with Alt+Click on the reload button (although I think that is not a very big problem). Is it a problem to use menuitem[key="key_reload"] in browser.css for gnomestripe? 

Comment 15

[Dão Gottwald [:dao]]

(In reply to comment #14)
> When you set it to always ignore alt it is not possible to save the current
> page with Alt+Click on the reload button (although I think that is not a very
> big problem).

Exactly, saving a page by clicking reload is probably the wrong thing anyway. It seems unintuitive to say the least.

> Is it a problem to use menuitem[key="key_reload"] in browser.css
> for gnomestripe?

no

Comment 16

[Klaas Heidstra]

Attachment: patch v2 - adresses review comments

With this patch it is not possible anymore to use Alt with the reload button. It also fixes the problem with gnomestripe.

Comment 17

[Dão Gottwald [:dao]]

Comment on attachment 325819 [details] [diff] [review]
patch v2 - adresses review comments

>-                <menuitem label="&reloadCmd.label;" accesskey="&reloadCmd.accesskey;" command="Browser:Reload" key="key_reload"/>
>+                <menuitem label="&reloadCmd.label;" accesskey="&reloadCmd.accesskey;" key="key_reload"
>+                          oncommand="if (event.shiftKey) BrowserReloadSkipCache(event); else BrowserReload(event)"
>+                          onclick="checkForMiddleClick(this, event);">
>+                  <observes element="Browser:Reload" attribute="disabled" />
>+                </menuitem>
...
>       <toolbarbutton id="reload-button" class="toolbarbutton-1 chromeclass-toolbar-additional"
>                      label="&reloadCmd.label;"
>-                     command="Browser:Reload"
>-                     tooltiptext="&reloadButton.tooltip;"/>
>+                     oncommand="if (event.shiftKey) BrowserReloadSkipCache(event); else BrowserReload(event)"
>+                     onclick="checkForMiddleClick(this, event);"
>+                     tooltiptext="&reloadButton.tooltip;">
>+        <observes element="Browser:Reload" attribute="disabled"/>
>+      </toolbarbutton>

I think we should stick with command="Browser:Reload", pass the event in browser-sets.inc:

-<command id="Browser:Reload" oncommand="if (event.shiftKey) BrowserReloadSkipCache(); else BrowserReload()" disabled="true"/>
+<command id="Browser:Reload" oncommand="if (event.shiftKey) BrowserReloadSkipCache(event); else BrowserReload(event)" disabled="true"/>

... and fix checkForMiddleClick to work with the command attribute as well as the oncommand attribute. E.g:

-    var fn = new Function("event", node.getAttribute("oncommand"));
-    fn.call(node, event);
+    var target = node.hasAttribute("oncommand") ? node :
+                 node.ownerDocument.getElementById(node.getAttribute("command"));
+    var fn = new Function("event", target.getAttribute("oncommand"));
+    fn.call(target, event);

What do you think?

Comment 18

[Klaas Heidstra]

(In reply to comment #17)
> (From update of attachment 325819 [details] [diff] [review])
> I think we should stick with command="Browser:Reload", pass the event in
> browser-sets.inc:
> 
> -<command id="Browser:Reload" oncommand="if (event.shiftKey)
> BrowserReloadSkipCache(); else BrowserReload()" disabled="true"/>
> +<command id="Browser:Reload" oncommand="if (event.shiftKey)
> BrowserReloadSkipCache(event); else BrowserReload(event)" disabled="true"/>
> 
> ... and fix checkForMiddleClick to work with the command attribute as well as
> the oncommand attribute. E.g:
> 
> -    var fn = new Function("event", node.getAttribute("oncommand"));
> -    fn.call(node, event);
> +    var target = node.hasAttribute("oncommand") ? node :
> +                
> node.ownerDocument.getElementById(node.getAttribute("command"));
> +    var fn = new Function("event", target.getAttribute("oncommand"));
> +    fn.call(target, event);
> 
> What do you think?

The reason why I used the oncommand attribute is because it's done in the same way for other elements with middle-click support (like the back-button). I've experimented with your approach above, but it causes all sorts of problems with elements who don't need middle click support. Some of those problems are:
- F5 doesn't work anymore when Shift or Alt is pressed
- Ctrl/ctrl+Shift+reload on the context menu opens a new tab
I think these problems are also the reason why the back-button is using oncommand, so it would be best to do it in the same way for the reload button.

Comment 19

[Klaas Heidstra]

(In reply to comment #18)
> - F5 doesn't work anymore when Shift or Alt is pressed
Sorry for this one I see now that this is intended behavior, but the other point still stands.

Comment 20

[Dão Gottwald [:dao]]

(In reply to comment #18)
> - Ctrl/ctrl+Shift+reload on the context menu opens a new tab

That would be consistent, right? Why do you think it's bad?

Comment 21

[John Mellor (Jomel)]

(In reply to comment #20)
> (In reply to comment #18)
> > - Ctrl/ctrl+Shift+reload on the context menu opens a new tab
> 
> That would be consistent, right? Why do you think it's bad?
> 

Indeed, Ctrl-click on the back/forward dropdown menu opens new tabs after all.

Comment 22

[John Mellor (Jomel)]

(In reply to comment #20)
> (In reply to comment #18)
> > - Ctrl/ctrl+Shift+reload on the context menu opens a new tab
> 
> That would be consistent, right? Why do you think it's bad?
> 

Indeed, Ctrl-click on the back/forward dropdown menu opens new tabs after all.

Comment 23

[John Mellor (Jomel)]

Oops, wrong button. Sorry for bugspam.

Comment 24

[Klaas Heidstra]

(In reply to comment #22)
> (In reply to comment #20)
> > (In reply to comment #18)
> > > - Ctrl/ctrl+Shift+reload on the context menu opens a new tab
> > 
> > That would be consistent, right? Why do you think it's bad?
> > 
> 
> Indeed, Ctrl-click on the back/forward dropdown menu opens new tabs after all.

When you right click on page and you Ctrl+click the back menuitem it doesn't open a new tab. So if we do it in a way that it also works for the context/menu, then it should also be fixed for the back/forward menuitems.

Comment 25

[Dão Gottwald [:dao]]

(In reply to comment #24)
> > Indeed, Ctrl-click on the back/forward dropdown menu opens new tabs after all.
> 
> When you right click on page and you Ctrl+click the back menuitem it doesn't
> open a new tab.

He probably meant the back/forward dropdown in the nav toolbar.

> So if we do it in a way that it also works for the
> context/menu, then it should also be fixed for the back/forward menuitems.

Yes, that could be a followup bug. I don't see a downside of allowing modifier keys in context menus.

Comment 26

[Klaas Heidstra]

Attachment: patch v3 - using command instead of oncommand

This patch makes checkForMiddleClick() also work for the command attribute.

Comment 27

[Klaas Heidstra]

Should I file a followup bug for using command in the other places as well (and making middle-click possible on back/forward and view background image context menuitems)?

Comment 28

[Dão Gottwald [:dao]]

Comment on attachment 325965 [details] [diff] [review]
patch v3 - using command instead of oncommand

>       <menuitem id="context-reload"
>                 label="&reloadCmd.label;"
>                 accesskey="&reloadCmd.accesskey;"
>-                command="Browser:Reload"/>
>+                command="Browser:Reload"
>+                onclick="checkForMiddleClick(this, event);">
>+        <observes element="Browser:Reload" attribute="disabled" />
>+      </menuitem>

The <observes> element isn't needed anymore, right?

>+function BrowserReload(aEvent)
>+{
>+  if (!aEvent)
>+    aEvent = {};

>+function BrowserReloadSkipCache(aEvent)
>+{
>+  if (!aEvent)
>+    aEvent = {};

whereToOpenLink already handles the aEvent==undefined case.

Comment 29

[Dão Gottwald [:dao]]

(In reply to comment #27)
> Should I file a followup bug for using command in the other places as well (and
> making middle-click possible on back/forward and view background image context
> menuitems)?

Yep.

Comment 30

[Dão Gottwald [:dao]]

(In reply to comment #28)
> >+function BrowserReload(aEvent)
> >+{
> >+  if (!aEvent)
> >+    aEvent = {};

Oh, and while you're at it, please move the bracket to the function head's line:

function BrowserReload(aEvent) {

Comment 31

[Klaas Heidstra]

(In reply to comment #28)
> The <observes> element isn't needed anymore, right?

I think so but it's used for the back/forward buttons as well so maybe another bug?

> whereToOpenLink already handles the aEvent==undefined case.

I know but this still gave problems with F5 (Ctrl+F5 didn't work anymore).

Comment 32

[Klaas Heidstra]

(In reply to comment #30)
> Oh, and while you're at it, please move the bracket to the function head's
> line:
> 
> function BrowserReload(aEvent) {

But all other functions in browser.js don't have the bracket in the functions head.

Comment 33

[Klaas Heidstra]

(In reply to comment #29)
> (In reply to comment #27)
> > Should I file a followup bug for using command in the other places as well (and
> > making middle-click possible on back/forward and view background image context
> > menuitems)?
> 
> Yep.

> I filed bug440702

Comment 34

[Klaas Heidstra]

Attachment: patch v4 - removed the observes elements

I removed the observes elements, but I still use aEvent = {}; because otherwise Ctrl+F5 won't work.

Comment 35

[Dão Gottwald [:dao]]

(In reply to comment #31)
> (In reply to comment #28)
> > The <observes> element isn't needed anymore, right?
> 
> I think so but it's used for the back/forward buttons as well

That's because it's not using the command attribute.

> > whereToOpenLink already handles the aEvent==undefined case.
> 
> I know but this still gave problems with F5 (Ctrl+F5 didn't work anymore).

Ok, how about not altering BrowserReloadSkipCache? I.e., don't pass the event there.

Unless I'm missing something this should work then:

function BrowserReloadWithFlags(reloadFlags, aEvent) {
  if (whereToOpenLink(aEvent, false, true) == "current") {
    // reload
  } else {
    // duplicate
  }
}

(In reply to comment #32)
> (In reply to comment #30)
> > Oh, and while you're at it, please move the bracket to the function head's
> > line:
> > 
> > function BrowserReload(aEvent) {
> 
> But all other functions in browser.js don't have the bracket in the functions
> head.

No, not all, mostly ancient ones.

Comment 36

[Klaas Heidstra]

(In reply to comment #35)
> Ok, how about not altering BrowserReloadSkipCache? I.e., don't pass the event
> there.
> 
> Unless I'm missing something this should work then:
> 
> function BrowserReloadWithFlags(reloadFlags, aEvent) {
>   if (whereToOpenLink(aEvent, false, true) == "current") {
>     // reload
>   } else {
>     // duplicate
>   }
> }

But then it is not possible to open a tab in the background anymore with Ctrl+Shift+Reload or Shift+Middle+Reload.

Comment 37

[Klaas Heidstra]

Attachment: patch v5 - moved brackets to function heads

I moved the brackets to the function heads, but I didn't do this:

> Ok, how about not altering BrowserReloadSkipCache? I.e., don't pass the event there.
>
> Unless I'm missing something this should work then:
>
> function BrowserReloadWithFlags(reloadFlags, aEvent) {
>   if (whereToOpenLink(aEvent, false, true) == "current") {
>     // reload
>   } else {
>     // duplicate
>   }
> }

Because then Ctrl+Shift+Reload and Shift+Middle+Reload won't work anymore.

Comment 38

[Dão Gottwald [:dao]]

Comment on attachment 325985 [details] [diff] [review]
patch v5 - moved brackets to function heads

>+function BrowserReload(aEvent) {
>+  if (!aEvent)
>+    aEvent = {};
>   const reloadFlags = nsIWebNavigation.LOAD_FLAGS_NONE;
...
>+  return BrowserReloadWithFlags(reloadFlags, aEvent);
>+}

The 'return' is superfluous.

Maybe you can clean this up a bit, I find it quite hard to follow:

>+  if ((where == "current" || aEvent.shiftKey) && aEvent.button != 1 && !aEvent.ctrlKey) {

Otherwise I don't think that "open same URL in new background tab" is a requirement for this bug, and for the sake of simplicity, I'd drop it.

Comment 39

[Klaas Heidstra]

Attachment: patch v6 - simplified by making load tabs in background impossible

> The 'return' is superfluous.

> Otherwise I don't think that "open same URL in new background tab" is a
> requirement for this bug, and for the sake of simplicity, I'd drop it.

I did what you say above so I think this adresses all comments.

Comment 40

[Dão Gottwald [:dao]]

Comment on attachment 326138 [details] [diff] [review]
patch v6 - simplified by making load tabs in background impossible

Ok, this is looking much more friendly... Unfortunately it breaks Ctrl+R. To address this, I suggest we add a Browser:ReloadOrDuplicate command, which would work just like Browser:Reload except for calling BrowserReloadOrDuplicate(event) instead of BrowserReload(). BrowserReloadOrDuplicate either duplicates the tab or calls BrowserReload().

Comment 41

[Klaas Heidstra]

Attachment: patch v7 - add a BrowserReloadOrDuplicate command

> Ok, this is looking much more friendly... Unfortunately it breaks Ctrl+R. To
> address this, I suggest we add a Browser:ReloadOrDuplicate command, which would
> work just like Browser:Reload except for calling
> BrowserReloadOrDuplicate(event) instead of BrowserReload().
> BrowserReloadOrDuplicate either duplicates the tab or calls BrowserReload().

Something like this?

Comment 42

[Klaas Heidstra]

Attachment: patch v8 - reload bypassing cache didn't work with previous patch

I'm now using this function for Browser:ReloadOrDuplicate:
> +function BrowserReloadOrDuplicate(aEvent) {
> +  var where = whereToOpenLink(aEvent, false, true);
> +
> +  if (aEvent.shiftKey) {
> +    BrowserReloadSkipCache();
> +  }
> +  else if (where == "current") {
> +    BrowserReload();
> +  }
> +  else {
> +    var url = getWebNavigation().currentURI.spec;
> +    openUILinkIn(url, where);
> +  }
> +}
Opening a tab in background can easily be added by replacing:
> +  if (aEvent.shiftKey) {
with:
> +  if (aEvent.shiftKey && !aEvent.button == 1) {
But I don't know if that is really necessary.

Comment 43

[Dão Gottwald [:dao]]

Comment on attachment 326170 [details] [diff] [review]
patch v8 - reload bypassing cache didn't work with previous patch

Almost there :)

The Browser:ReloadOrDuplicate command needs to be added to nsBrowserStatusHandler (browser.js) or it needs to observe Browser:Reload's disabled attribute with an <observes> element (in which case you should update Browser:ReloadSkipCache to do the same for consistency), and gnomestripe needs an update again.

>+function BrowserReloadOrDuplicate(aEvent) {
>+  var where = whereToOpenLink(aEvent, false, true);
>+
>+  if (aEvent.shiftKey) {
>+    BrowserReloadSkipCache();
>+  }
>+  else if (where == "current") {
>+    BrowserReload();
>+  }
>+  else {
>+    var url = getWebNavigation().currentURI.spec;
>+    openUILinkIn(url, where);
>+  }
>+}

nit: "where" isn't needed in the first case, so this might be slightly better:

function BrowserReloadOrDuplicate(aEvent) {
  if (aEvent.shiftKey) {
    BrowserReloadSkipCache();
    return;
  }

  var where = whereToOpenLink(aEvent, false, true);
  if (where == "current") {
    ...

Comment 44

[Klaas Heidstra]

One problem with ignoring load page in background is that Shift+middle-click reloads the page bypassing cache, I don't think that is desired. So maybe I should put the check for middle-click back?

Comment 45

[Dão Gottwald [:dao]]

Makes sense...

> Opening a tab in background can easily be added by replacing:
> > +  if (aEvent.shiftKey) {
> with:
> > +  if (aEvent.shiftKey && !aEvent.button == 1) {

I assume you mean aEvent.button != 1.

Comment 46

[Klaas Heidstra]

(In reply to comment #45)
> Makes sense...
> 
> > Opening a tab in background can easily be added by replacing:
> > > +  if (aEvent.shiftKey) {
> > with:
> > > +  if (aEvent.shiftKey && !aEvent.button == 1) {
> 
> I assume you mean aEvent.button != 1.
> 
More something like this:
> +#ifdef XP_MACOSX
> +  if (aEvent.shiftKey && aEvent.button != 1 && !aEvent.metaKey) {
> +#else
> +  if (aEvent.shiftKey && aEvent.button != 1 && !aEvent.ctrlKey) {
> +#endif

Comment 47

[Dão Gottwald [:dao]]

(In reply to comment #46)
> More something like this:
> > +#ifdef XP_MACOSX
> > +  if (aEvent.shiftKey && aEvent.button != 1 && !aEvent.metaKey) {
> > +#else
> > +  if (aEvent.shiftKey && aEvent.button != 1 && !aEvent.ctrlKey) {
> > +#endif

It's getting opaque again... so either add a comment or make the code itself self-explanatory:

> var backgroundTabModifier = aEvent.button == 1 ||
> #ifdef XP_MACOSX
>   aEvent.metaKey;
> #else
>   aEvent.ctrlKey;
> #endif
> if (aEvent.shiftKey && !backgroundTabModifier) {

Comment 48

[John Mellor (Jomel)]

(In reply to comment #42)
> I'm now using this function for Browser:ReloadOrDuplicate:
> (...)
> > +    var url = getWebNavigation().currentURI.spec;
> > +    openUILinkIn(url, where);

Hang on - there are much better ways to duplicate a tab!

Bug 393716 added the duplicateTab method (documented at http://developer.mozilla.org/en/docs/nsISessionStore ) which will duplicate a tab including it's history, scroll position, form contents, etc.

This method can be used directly for the "tab" and "tabshifted" cases (for the foreground tab case you'll have to set selectedTab manually). For the window case, while duplicateTab can copy tabs to different windows, it currently requires the window to already be open. The cleanest approach is probably to extend duplicateTab so that it creates a new window if the passed window is null. This would also be useful to Bug 225680 / extensions wanting to allow tabs to be "detached" as new windows.

Note however that the session service requires the browser.sessionstore.enabled preference to be true (the default); if not we should fall back on your existing code which just copies the uri (and this can continue to handle the "save" case).

I'm happy to have a go at implementing this if you want.

Comment 49

[Klaas Heidstra]

(In reply to comment #48)
> 
> Hang on - there are much better ways to duplicate a tab!
>
> ...
> 
> I'm happy to have a go at implementing this if you want.
> 

But then this also needs to be implemented for other elements with the middle click feature, like the back/forward buttons and links in web pages. So maybe it is better to file a new bug for this behavior. 

Comment 50

[Klaas Heidstra]

Attachment: patch v9 - addresses all the comments

I've added ReloadOrDuplicate to nsBrowserStatusHandler and the new styling for gnomestripe. Do think you think I should implement the suggestion from John Mellor in comment #48 within this bug?

Comment 51

[Dão Gottwald [:dao]]

Comment on attachment 326358 [details] [diff] [review]
patch v9 - addresses all the comments

> I've added ReloadOrDuplicate to nsBrowserStatusHandler

As I said, you could also use a XUL observer:

<command id="Browser:ReloadOrDuplicate" ...>
  <observes element="Browser:Reload" attribute="disabled" />
</command>

... which I think would actually be more straightforward.

>+  else {
>+    var url = getWebNavigation().currentURI.spec;
>+    openUILinkIn(url, where);
>+  }

nit: "let url = ..." or just "openUILinkIn(getWebNavigation().currentURI.spec, where)".

> Do think you think I should implement the suggestion from John
> Mellor in comment #48 within this bug?

Depends on whether we really want to clone the session rather than forking it, and could be done in a separate bug.

Comment 52

[Klaas Heidstra]

Attachment: patch v10 - using observes elements for command

I'm now using this: openUILinkIn(getWebNavigation().currentURI.spec, where);
And I added observes elements to the command elements instead of adding stuff to nsBrowserStatusHandler. So this should be the final patch.

Comment 53

[:Gavin Sharp [email: [e-mail]]]

I don't think we should be supporting middle clicking on menu items, that just seems weird (the back/forward dropdown menu items are somewhat exceptional). That would also simplify the patch somewhat. Otherwise this looks OK.

(nit: "if (aEvent.shiftKey && !backgroundTabModifier)" isn't indented properly)

Comment 54

[Dão Gottwald [:dao]]

(In reply to comment #53)
> I don't think we should be supporting middle clicking on menu items, that just
> seems weird (the back/forward dropdown menu items are somewhat exceptional).

Note that Back and Forward in the History menu have middle click support, too.
As for the context menu, right click -> middle click seems quite convenient to me.

Comment 55

[Klaas Heidstra]

Attachment: patch v11 - removed 'Browser:ReloadSkipCache' from var disabledItems in nonBrowserWindowStartup()

> #ifdef XP_MACOSX
> ...
> function nonBrowserWindowStartup()
> {
> // Disable inappropriate commands / submenus
> var disabledItems = ['Browser:SavePage', 'Browser:SendLink',
> 'cmd_pageSetup', 'cmd_print', 'cmd_find', 'cmd_findAgain', 'viewToolbarsMenu',
> 'cmd_toggleTaskbar', 'viewSidebarMenuMenu', 'Browser:Reload',
> 'Browser:ReloadSkipCache',
I'm not sure if should remove the 'Browser:ReloadSkipCache' here(I've removed it in this patch), or add a 'Browser:ReloadOrDuplicate'.

(In reply to comment #53)
> I don't think we should be supporting middle clicking on menu items, that just
> seems weird (the back/forward dropdown menu items are somewhat exceptional).
> That would also simplify the patch somewhat. Otherwise this looks OK.
As Dão Gottwald already said this is also supported in the history meny, so I don't see a reason not supporting it in the contect menu.

> (nit: "if (aEvent.shiftKey && !backgroundTabModifier)" isn't indented
> properly)
I've replaced it with this:
> + if (aEvent.shiftKey &&
> +     !backgroundTabModifier) {

Comment 56

[Dão Gottwald [:dao]]

(In reply to comment #55)
> > (nit: "if (aEvent.shiftKey && !backgroundTabModifier)" isn't indented
> > properly)
> I've replaced it with this:
> > + if (aEvent.shiftKey &&
> > +     !backgroundTabModifier) {

What Gavin meant is that there's a space missing before the "if".

Comment 57

[Dão Gottwald [:dao]]

(In reply to comment #55)
> > #ifdef XP_MACOSX
> > ...
> > function nonBrowserWindowStartup()
> > {
> > // Disable inappropriate commands / submenus
> > var disabledItems = ['Browser:SavePage', 'Browser:SendLink',
> > 'cmd_pageSetup', 'cmd_print', 'cmd_find', 'cmd_findAgain', 'viewToolbarsMenu',
> > 'cmd_toggleTaskbar', 'viewSidebarMenuMenu', 'Browser:Reload',
> > 'Browser:ReloadSkipCache',
> I'm not sure if should remove the 'Browser:ReloadSkipCache' here(I've removed
> it in this patch), or add a 'Browser:ReloadOrDuplicate'.

Removing it is fine.

Comment 58

[Klaas Heidstra]

Attachment: patch v12 - corrected wrong indentation

> What Gavin meant is that there's a space missing before the "if".
Sorry I didn't see it, this patch corrects it.

Comment 59

[Simon Bünzli]

(In reply to comment #48)
Please file a new bug for the API change and another new bug for the front-end change, should you want to follow through with this. BTW: Duplicating a tab into an existing window is best done through the tabbrowser's duplicateTab method (which already includes the mentioned fall-back solution).

Comment 60

[Klaas Heidstra]

For some reason the "back-forward-dropmarker" won't work in this patch, I've tried replacing Browser:Forward with Browser:ForwardOrForwardDuplicate in the code beneath but that didn't change anything.
>var backForwardDropmarker = document.getElementById("back-forward-dropmarker");
>if (backForwardDropmarker)
>  backForwardDropmarker.disabled =
>    document.getElementById('Browser:Back').hasAttribute('disabled') &&
>    document.getElementById('Browser:Forward').hasAttribute('disabled');

I was under the impression that because of: "<observes element="Browser:Forward" attribute="disabled"/>", the Browser:Forward command and the Browser:ForwardOrForwardDuplicate command "inherit" the disabled attribute from each other as soon as it changes. But apparently the disabled attribute from Browser:Forward doesn't change when the one from Browser:ForwardOrForwardDuplicate changes.

Comment 61

[Klaas Heidstra]

(In reply to comment #60)
> For some reason the "back-forward-dropmarker" won't work in this patch, I've
> ...
> Browser:ForwardOrForwardDuplicate changes.
Sorry this was meant for bug 440702.

Comment 62

[:Gavin Sharp [email: [e-mail]]]

The history menu is also somewhat exceptional since it mostly lists history items. I'd rather not extend this behavior, but I guess beltzner or mconnor should make that call.

Comment 63

[Klaas Heidstra]

> I don't think we should be supporting middle clicking on menu items, that just
> seems weird (the back/forward dropdown menu items are somewhat exceptional).

> The history menu is also somewhat exceptional since it mostly lists history
> items. I'd rather not extend this behavior, but I guess beltzner or mconnor
> should make that call.

If middle-clicking on menu items in menus other then the history menu is unwanted, then maybe there should be filed another bug. But I believe for consistency it would be best to support middle clicking on menuitems. Currently middle-clicking "View Background Image" in the context menu does work, but middle-clicking back/forward/reload doesn't, that is why I added it.

Comment 64

[Klaas Heidstra]

(In reply to comment #59)
> (In reply to comment #48)
> Please file a new bug for the API change and another new bug for the front-end
> change, should you want to follow through with this. BTW: Duplicating a tab
> into an existing window is best done through the tabbrowser's duplicateTab
> method (which already includes the mentioned fall-back solution).

It would be very nice for this feature to clone the current tab (with it's history, scroll position, form contents, etc) so I filed bug 448546. 

Comment 65

[Klaas Heidstra]

There is a discussion about this functionality in dev.apps.firefox see: http://groups.google.com/group/mozilla.dev.apps.firefox/browse_thread/thread/76f02c8fb692701b#

Comment 66

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

Comment on attachment 326489 [details] [diff] [review]
patch v12 - corrected wrong indentation

uir=beltzner for:

 - middle click on reload button opens the URL in a new tab

Comment 67

[:Gavin Sharp [email: [e-mail]]]

The patch with a pending review request still implements middle clicking on menu items. Can you split that part off into a separate bug? I still don't think it's a good idea.

Comment 68

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

Gavin: see bug 440702 comment 39; I do agree with you that it's strange, and I'm positive it isn't discoverable, but I'm having a hard time coming up with an argument about the cost (or end-user cost) of implementing it such that middle click does this. Elsewhere in the product we have a consistent mapping of "middle click" = "open target in new", as well, so in that respect, it's consistent with our UI.

We might go back on this later, but for now, let's give it a whirl.

Comment 69

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #68)
> Gavin: see bug 440702 comment 39; I do agree with you that it's strange, and
> I'm positive it isn't discoverable, but I'm having a hard time coming up with
> an argument about the cost (or end-user cost) of implementing it such that
> middle click does this.

Well, it adds to implementation complexity (though granted not by much given the existence of checkForMiddleClick), and opens up a can of worms for adding this to all menus. I still think it's a bad idea, but won't insist.

Comment 70

[:Gavin Sharp [email: [e-mail]]]

(I'm also slightly worried about the potential extension compatibility problems caused by changing the command used by the menuitems/toolbarbuttons, but I suppose there's not much that can be done about that without ugly hacks in openUILink, given comment 40.)

Comment 71

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #70)
> without ugly hacks in openUILink

Er, I meant whereToOpenLink.

Comment 72

[Dão Gottwald [:dao]]

http://hg.mozilla.org/mozilla-central/rev/903096fec9b4