Closed Bug 263942 Opened 15 years ago Closed 11 years ago

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

Categories

(Firefox :: General, enhancement)

x86
All
enhancement
Not set

Tracking

()

RESOLVED FIXED
Firefox 3.1b1

People

(Reporter: fishos, Assigned: klaas1988)

References

(Blocks 1 open bug, )

Details

Attachments

(1 file, 12 obsolete files)

14.71 KB, patch
Gavin
: review+
beltzner
: ui-review+
Details | Diff | Splinter Review
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:
This is also a good way to help people duplicate the current tab without adding
options for that.
*** Bug 278131 has been marked as a duplicate of this bug. ***
Attachment #171189 - Flags: review?(mconnor)
Attachment #171189 - Attachment is patch: false
Comment on attachment 171189 [details] [diff] [review]
Disable reload command on blank pages/tabs

Sorry, submitted this to the wrong bug somehow.
Attachment #171189 - Attachment is obsolete: true
Attachment #171189 - Attachment is patch: true
Attachment #171189 - Flags: review?(mconnor)
*** Bug 251207 has been marked as a duplicate of this bug. ***
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.
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).
Severity: minor → enhancement
OS: Windows XP → All
Assignee: firefox → nobody
*** Bug 324373 has been marked as a duplicate of this bug. ***
Keywords: uiwanted
Summary: Reload button should have middle click support → Reload button should have middle click support (open same URL in new tab, clone tab)
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.
Depends on: 332498
Duplicate of this bug: 377028
Version: unspecified → Trunk
Shouldn't this bug block the tracking bug 70501?
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.
Attachment #325797 - Flags: ui-review?(dao)
Attachment #325797 - Flags: review?(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.
Attachment #325797 - Flags: ui-review?(dao)
Attachment #325797 - Flags: review?(dao)
Attachment #325797 - Flags: review-
Assignee: nobody → klaas1988
> 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? 

Attachment #325797 - Flags: ui-review?(beltzner)
(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
Status: NEW → ASSIGNED
Keywords: regression
With this patch it is not possible anymore to use Alt with the reload button. It also fixes the problem with gnomestripe.
Attachment #325797 - Attachment is obsolete: true
Attachment #325819 - Flags: ui-review?(beltzner)
Attachment #325819 - Flags: review?(dao)
Attachment #325797 - Flags: ui-review?(beltzner)
Whiteboard: [has patch] [needs review 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?
(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.
(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.
(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?
(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.
Assignee: klaas1988 → nobody
Status: ASSIGNED → NEW
(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.
Oops, wrong button. Sorry for bugspam.
Assignee: nobody → klaas1988
(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.
(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.
This patch makes checkForMiddleClick() also work for the command attribute.
Attachment #325819 - Attachment is obsolete: true
Attachment #325965 - Flags: ui-review?(beltzner)
Attachment #325965 - Flags: review?(dao)
Attachment #325819 - Flags: ui-review?(beltzner)
Attachment #325819 - Flags: review?(dao)
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)?
Status: NEW → ASSIGNED
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.
Attachment #325965 - Flags: review?(dao) → review-
(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.
(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) {
(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).
(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.
(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
I removed the observes elements, but I still use aEvent = {}; because otherwise Ctrl+F5 won't work.
Attachment #325965 - Attachment is obsolete: true
Attachment #325965 - Flags: ui-review?(beltzner)
Attachment #325973 - Flags: ui-review?(beltzner)
Attachment #325973 - Flags: review?(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.
Blocks: 440702
(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.
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.
Attachment #325973 - Attachment is obsolete: true
Attachment #325985 - Flags: ui-review?(beltzner)
Attachment #325985 - Flags: review?(dao)
Attachment #325973 - Flags: ui-review?(beltzner)
Attachment #325973 - Flags: review?(dao)
No longer depends on: 332498
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.
> 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.
Attachment #325985 - Attachment is obsolete: true
Attachment #326138 - Flags: review?(dao)
Attachment #325985 - Flags: ui-review?(beltzner)
Attachment #325985 - Flags: review?(dao)
Attachment #326138 - Flags: ui-review?(beltzner)
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().
Attachment #326138 - Flags: review?(dao) → review-
> 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?
Attachment #326138 - Attachment is obsolete: true
Attachment #326162 - Flags: ui-review?(beltzner)
Attachment #326162 - Flags: review?(dao)
Attachment #326138 - Flags: ui-review?(beltzner)
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.
Attachment #326162 - Attachment is obsolete: true
Attachment #326170 - Flags: ui-review?(beltzner)
Attachment #326170 - Flags: review?(dao)
Attachment #326162 - Flags: ui-review?(beltzner)
Attachment #326162 - Flags: review?(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") {
    ...
Attachment #326170 - Flags: review?(dao) → review-
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?
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.
(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
(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) {
(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.
(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. 
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?
Attachment #326170 - Attachment is obsolete: true
Attachment #326358 - Flags: ui-review?(beltzner)
Attachment #326358 - Flags: review?(dao)
Attachment #326170 - Flags: ui-review?(beltzner)
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.
Attachment #326358 - Flags: review?(gavin.sharp)
Attachment #326358 - Flags: review?(dao)
Attachment #326358 - Flags: review+
Whiteboard: [has patch] [needs review dao] → [has patch]
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.
Attachment #326358 - Attachment is obsolete: true
Attachment #326358 - Flags: ui-review?(beltzner)
Attachment #326358 - Flags: review?(gavin.sharp)
Attachment #326413 - Flags: ui-review?(beltzner)
Attachment #326413 - Flags: review?(gavin.sharp)
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)
(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.
> #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) {
Attachment #326413 - Attachment is obsolete: true
Attachment #326471 - Flags: ui-review?(beltzner)
Attachment #326471 - Flags: review?(gavin.sharp)
Attachment #326413 - Flags: ui-review?(beltzner)
Attachment #326413 - Flags: review?(gavin.sharp)
(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".
(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.
> What Gavin meant is that there's a space missing before the "if".
Sorry I didn't see it, this patch corrects it.
Attachment #326471 - Attachment is obsolete: true
Attachment #326489 - Flags: ui-review?(beltzner)
Attachment #326489 - Flags: review?(gavin.sharp)
Attachment #326471 - Flags: ui-review?(beltzner)
Attachment #326471 - Flags: review?(gavin.sharp)
(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).
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.
(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.
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.
Whiteboard: [has patch] → [has patch] [needs review beltzner]
> 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.
(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. 
Blocks: 448546
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 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
Attachment #326489 - Flags: ui-review?(beltzner) → ui-review+
Whiteboard: [has patch] [needs review beltzner] → [has patch] [needs review gavin.sharp]
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.
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.
Attachment #326489 - Flags: review?(gavin.sharp) → review+
(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.
Whiteboard: [has patch] [needs review gavin.sharp] → [has patch]
Keywords: checkin-needed
Whiteboard: [has patch] → [has patch][has review]
(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.)
(In reply to comment #70)
> without ugly hacks in openUILink

Er, I meant whereToOpenLink.
http://hg.mozilla.org/mozilla-central/rev/903096fec9b4
URL:
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Whiteboard: [has patch][has review]
Target Milestone: --- → Firefox 3.1b1
Duplicate of this bug: 460139
You need to log in before you can comment on or make changes to this bug.