599833 - Add "Paste and Go" to the context menu of the URLbar.

Status: VERIFIED FIXED

(SeaMonkey :: Location Bar, enhancement)

Description

[Philip Chee]

Adapt the following Firefox fixes for SeaMonkey:
[Bug 492544] Add 'Paste and Go' + 'Paste and Search' to context menu on location field + search field.
[Bug 597129] "Paste & Go" or "Paste & Search" trigger two pastes, including to content.

Comment 1

[Philip Chee]

Attachment: Patch v1.0 proposed fix.

Comment 2

[Philip Chee]

Comment on attachment 478752 [details] [diff] [review]
Patch v1.0 proposed fix.

># HG changeset patch
># User Philip Chee <[e-mail]>
># Date 1285582698 -28800
># Node ID 41f0e3ace61e2120b3cb1b9acdd7a7c197eee58b
># Parent  f4a93ad5efb50d350a875c94f1dfdfe8a9fcbec2
>Bug 599833 Add "Paste and Go" to the context menu of the URLbar.
>
>diff --git a/suite/browser/navigator.css b/suite/browser/navigator.css
>--- a/suite/browser/navigator.css
>+++ b/suite/browser/navigator.css
>@@ -19,16 +19,20 @@ tabbrowser {
> }
> 
> /* ::::: urlbar autocomplete ::::: */
> 
> #urlbar {
>   -moz-binding: url("chrome://navigator/content/urlbarBindings.xml#urlbar");
> }
> 
>+#urlbar > .autocomplete-textbox-container > .textbox-input-box {
>+  -moz-binding: url("chrome://navigator/content/urlbarBindings.xml#input-box-paste");
>+}
>+
> panel[for="urlbar"] {
>   -moz-binding: url("chrome://navigator/content/urlbarBindings.xml#autocomplete-result-popup") !important;
> }
> 
> .autocomplete-search-box {
>   -moz-binding: url("chrome://navigator/content/urlbarBindings.xml#autocomplete-search-box");
> }
> 
>diff --git a/suite/browser/urlbarBindings.xml b/suite/browser/urlbarBindings.xml
>--- a/suite/browser/urlbarBindings.xml
>+++ b/suite/browser/urlbarBindings.xml
>@@ -1,28 +1,35 @@
> <?xml version="1.0"?>
> 
>+<!DOCTYPE bindings [
>+  <!ENTITY % textcontextDTD SYSTEM "chrome://communicator/locale/utilityOverlay.dtd">
>+  %textcontextDTD;
>+]>
>+
> <bindings id="urlbarBindings"
>           xmlns="http://www.mozilla.org/xbl"
>           xmlns:xul="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul"
>           xmlns:xbl="http://www.mozilla.org/xbl">
> 
>   <binding id="urlbar" extends="chrome://global/content/autocomplete.xml#autocomplete">
>     <implementation>
>       <constructor><![CDATA[
>         var pbi = this.mPrefs.QueryInterface(Components.interfaces.nsIPrefBranch2);
>         if (pbi)
>           pbi.addObserver("browser.urlbar", this.mPrefObserver, false);
>-        
>+
>         this.updatePref("browser.urlbar.showPopup");
>         this.updatePref("browser.urlbar.autoFill");
>         this.updatePref("browser.urlbar.showSearch");
>+        this.inputField.controllers.insertControllerAt(0, this._editItemsController);
>       ]]></constructor>
> 
>       <destructor><![CDATA[
>+        this.inputField.controllers.removeController(this._editItemsController);
>         var pbi = this.mPrefs.QueryInterface(Components.interfaces.nsIPrefBranch2);
>         if (pbi)
>           pbi.removeObserver("browser.urlbar", this.mPrefObserver);
>       ]]></destructor>
> 
>       <field name="mPrefs">
>         var svc = Components.classes["@mozilla.org/preferences-service;1"]
>                             .getService(Components.interfaces.nsIPrefService);
>@@ -88,19 +95,47 @@
>                 this.mInputElt.setSelectionRange(entry.length, this.value.length);
>                 this.mDefaultMatchFilled = true;
>               }
>               this.mNeedToComplete = true;
>             }
>           }
>         ]]></body>
>       </method>
>+
>+      <field name="_editItemsController"><![CDATA[
>+        ({
>+          doCommand: function(aCommand) {
>+            switch (aCommand) {
>+              case "cmd_pasteAndGo":
>+                goDoCommand("cmd_paste");
>+                handleURLBarCommand(null, null);
>+            }
>+          },
>+          supportsCommand: function(aCommand) {
>+            switch (aCommand) {
>+              case "cmd_pasteAndGo":
>+                return true;
>+            }
>+            return false;
>+          },
>+          isCommandEnabled: function(aCommand) {
>+            switch (aCommand) {
>+              case "cmd_pasteAndGo":
>+                return document.commandDispatcher
>+                               .getControllerForCommand("cmd_paste")
>+                               .isCommandEnabled("cmd_paste");
>+            }
>+          },
>+          onEvent: function(aEventName) {}
>+        })
>+      ]]></field>
>     </implementation>
>   </binding>
>-  
>+
>   <binding id="autocomplete-result-popup" extends="chrome://global/content/autocomplete.xml#autocomplete-result-popup">
>     <content>
>       <xul:tree anonid="tree" class="autocomplete-tree plain" flex="1">
>         <xul:treecols anonid="treecols">
>           <xul:treecol class="autocomplete-treecol" id="treecolAutoCompleteValue" flex="2"/>
>           <xul:treecol class="autocomplete-treecol" id="treecolAutoCompleteComment" flex="1" hidden="true"/>
>         </xul:treecols>
>         <xul:treechildren anonid="treebody" class="autocomplete-treebody" flex="1"/>
>@@ -483,10 +518,32 @@
>         this.parentNode.selectedIndex = Number(this.getAttribute("engineIndex"));
>       </handler>
> 
>       <handler event="mouseout">
>         this.parentNode.selectedIndex = -1;
>       </handler>
>     </handlers>
>   </binding>
>-  
>+
>+  <binding id="input-box-paste" extends="chrome://global/content/bindings/textbox.xml#input-box">
>+    <content context="_child">
>+      <children/>
>+      <xul:menupopup anonid="input-box-contextmenu"
>+                     class="textbox-contextmenu"
>+                     onpopupshowing="if (document.commandDispatcher.focusedElement != this.parentNode.firstChild)
>+                                       this.parentNode.firstChild.focus();
>+                                     this.parentNode._doPopupItemEnabling(this);"
>+                     oncommand="var cmd = event.originalTarget.getAttribute('cmd'); if(cmd) { this.parentNode.doCommand(cmd); event.stopPropagation(); }">
>+        <xul:menuitem label="&undoCmd.label;" accesskey="&undoCmd.accesskey;" cmd="cmd_undo"/>
>+        <xul:menuseparator/>
>+        <xul:menuitem label="&cutCmd.label;" accesskey="&cutCmd.accesskey;" cmd="cmd_cut"/>
>+        <xul:menuitem label="&copyCmd.label;" accesskey="&copyCmd.accesskey;" cmd="cmd_copy"/>
>+        <xul:menuitem label="&pasteCmd.label;" accesskey="&pasteCmd.accesskey;" cmd="cmd_paste"/>
>+        <xul:menuitem label="&pasteGoCmd.label;" accesskey="&pasteGoCmd.accesskey;" cmd="cmd_pasteAndGo"/>
>+        <xul:menuitem label="&deleteCmd.label;" accesskey="&deleteCmd.accesskey;" cmd="cmd_delete"/>
>+        <xul:menuseparator/>
>+        <xul:menuitem label="&selectAllCmd.label;" accesskey="&selectAllCmd.accesskey;" cmd="cmd_selectAll"/>
>+      </xul:menupopup>
>+    </content>
>+  </binding>
>+
> </bindings>
>diff --git a/suite/locales/en-US/chrome/common/utilityOverlay.dtd b/suite/locales/en-US/chrome/common/utilityOverlay.dtd
>--- a/suite/locales/en-US/chrome/common/utilityOverlay.dtd
>+++ b/suite/locales/en-US/chrome/common/utilityOverlay.dtd
>@@ -31,16 +31,18 @@
> <!ENTITY cutCmd.key							"X">  
> <!ENTITY cutCmd.accesskey					"t"> 
> <!ENTITY copyCmd.label						"Copy">  
> <!ENTITY copyCmd.key						"C">  
> <!ENTITY copyCmd.accesskey					"c"> 
> <!ENTITY pasteCmd.label						"Paste">  
> <!ENTITY pasteCmd.key						"V">  
> <!ENTITY pasteCmd.accesskey					"p"> 
>+<!ENTITY pasteGoCmd.label					"Paste &amp; Go">
>+<!ENTITY pasteGoCmd.accesskey				"G"> 
> <!ENTITY deleteCmd.label					"Delete">  
> <!ENTITY deleteCmd.accesskey				"d"> 
> <!ENTITY selectAllCmd.label					"Select All">  
> <!ENTITY selectAllCmd.key					"A">  
> <!ENTITY selectAllCmd.accesskey				"a"> 
> <!ENTITY preferencesCmd.label				"Preferencesâ¦">
> <!ENTITY preferencesCmd.key					"E">  
> <!ENTITY preferencesCmd.accesskey			"e">

Comment 3

[Philip Chee]

Comment on attachment 478752 [details] [diff] [review]
Patch v1.0 proposed fix.

Attempt to add comment one more time!

Differences with the Firefox version of this patch:

1. Instead of generating the menu item dynamically I use an additional derived binding:


> +#urlbar > .autocomplete-textbox-container > .textbox-input-box {
> +  -moz-binding: url("chrome://navigator/content/urlbarBindings.xml#input-box-paste");
> +}

2. Instead of enabling/disabling the menu item via a popup event listener I use a custom controller:

> +        this.inputField.controllers.insertControllerAt(0, this._editItemsController);

> +        this.inputField.controllers.removeController(this._editItemsController);

> +      <field name="_editItemsController"><![CDATA[
> +        ({
> +          doCommand: function(aCommand) {
> +            switch (aCommand) {

I am using a switch here to make it easier to port further patches from Firefox e.g. Bug 531210.


> +              case "cmd_pasteAndGo":
> +                goDoCommand("cmd_paste");
> +                handleURLBarCommand(null, null);

I don't know how to get an event to use as the second parameter of handleURLBarCommand()

> +<!ENTITY pasteGoCmd.label					"Paste &amp; Go">
> +<!ENTITY pasteGoCmd.accesskey				"G">

Firefox UX team prefers an ampersand over "and".

Comment 4

[[e-mail]]

Comment on attachment 478752 [details] [diff] [review]
Patch v1.0 proposed fix.

>+          doCommand: function(aCommand) {
Nit: doCommand after isCommandEnabled

>+                goDoCommand("cmd_paste");
I seem to remember seeing a bug that you have to select all first?

>+                handleURLBarCommand(null, null);
[Rather than calling handleURLBarCommand directly, might be neater to call this._fireEvent("textentered", "pasting");]
Nit: missing break; (other cases use return so not a problem)

Comment 5

[Philip Chee]

Attachment: Patch v1.1

> [e-mail]      2010-09-28 03:38:29 PDT
> 
> Comment on attachment 478752 [details] [diff] [review]
> Patch v1.0 proposed fix.
> 
>> +          doCommand: function(aCommand) {
> Nit: doCommand after isCommandEnabled
Fixed.

>> +                goDoCommand("cmd_paste");
> I seem to remember seeing a bug that you have to select all first?
I found one Firefox bug that seem relevant:
[NEW] Bug 599793 - Paste & Go and Paste & Search should always replace the entire contents of the textbox.
However there are no patches yet for this bug. So I've decided to unconditionally delete the entire current contents of the textbox before pasting.

>> +                handleURLBarCommand(null, null);
> [Rather than calling handleURLBarCommand directly, might be neater to call
> this._fireEvent("textentered", "pasting");]
Fixed.

> Nit: missing break; (other cases use return so not a problem)
Fixed.

Comment 6

[[e-mail]]

Comment on attachment 479734 [details] [diff] [review]
Patch v1.1

>+          _fireEvent: this._fireEvent.bind(this),
Sneaky! I wonder whether you would be better off binding doCommand ;-)

>+                this.editor.selectAll();
>+                this.editor.selection.deleteFromDocument();
>+                goDoCommand("cmd_paste");
Paste already deletes the selection, does it not?
Or you could just set the value to "".

>+<!ENTITY pasteGoCmd.accesskey				"G"> 
Nit: trailing space.

Comment 7

[[e-mail]]

(In reply to comment #6)
>(From update of attachment 479734 [details] [diff] [review])
>>+          _fireEvent: this._fireEvent.bind(this),
>Sneaky! I wonder whether you would be better off binding doCommand ;-)
Or you could be boring and store a reference to the element in the object...

Comment 8

[Philip Chee]

Attachment: Patch v1.2 bind the whole doCommand.

Bug 599833 Add "Paste and Go" to the context menu of the URLbar.

>>+          _fireEvent: this._fireEvent.bind(this),
> Sneaky! I wonder whether you would be better off binding doCommand ;-)
I bound the whole doCommand rather than hold a reference to the urlbar. It's much simpler to just do:

|doCommand: function(aCommand) { ... }.bind(this);|

>>+                this.editor.selectAll();
>>+                this.editor.selection.deleteFromDocument();
>>+                goDoCommand("cmd_paste");
> Paste already deletes the selection, does it not?
> Or you could just set the value to "".
Fixed using |this.value = "";|

>>+<!ENTITY pasteGoCmd.accesskey				"G"> 
> Nit: trailing space.
Fixed.

Comment 9

[Philip Chee]

Pushed to comm-central
http://hg.mozilla.org/comm-central/rev/53261a78051d

Comment 10

[Robert Kaiser]

You should not have landed this during a string freeze!

That said, it's done, and most localizers caught up and localized it already. Still, please at least post to m.d.l10n and tell them you did break the string freeze, so those who haven't know to catch up.

Comment 11

[Philip Chee]

Oops sorry. I checked the Tinderbox page and the topic in #seamonkey, but forgot to check the dates for the L10n freeze.

Comment 12

[Philip Chee]

Attachment: Patch 2.0 isCommandEnabled does not always return a value

From irc://moznet/seamonkey :
> Ratty: hmm, your isCommandEnabled does not always return a value...

Comment 13

[Philip Chee]

> Created attachment 482237 [details] [diff] [review]
> Patch 2.0 isCommandEnabled does not always return a value

Pushed to comm-central:
http://hg.mozilla.org/comm-central/rev/c01b6b5f2b8e

Comment 14

[Jens Hatlak (:InvisibleSmiley)]

OK, so we already fixed bug 599793 on our side here. Should we port bug 601695 (Paste & Go should only appear when clipboard is URL)? That would make some special cases (host names, bookmark keywords etc.) not work with P&G anymore so I thought I'd ask here first. If in doubt we can just open a follow-up.

Comment 15

[Philip Chee]

My take is that if someone really wants to paste a non url e.g. a keyword search we shouldn't try to prevent them from doing so. Micromanaging this sort of stuff just causes end user confusion e.g. "Why is the Paste&Go option greyed out sometimes?".