807326 - Context menu Search should be available in textareas/inputs as well (Port Bug 565717)

Status: RESOLVED FIXED

(SeaMonkey :: General, enhancement)

Description

[Daniel B.]

The function of initiating a Google (etc.) search on the text that
is currently selected in a web page is not available for text in
text boxes (one-line boxes and textareas).

(That is, the context menu for a text selection in a text box does
not have the "Search ..." menu item that a text selection in 
"regular" text would have.)

There doesn't seem to be any reason that text in a text box 
shouldn't have this functionality the same way that regular text 
does.

Comment 1

[Philip Chee]

From Bug 565717 Comment 0:

> (Note: this is filed as part of the “Paper Cut” bugs — we assume that there may be
> multiple existing bugs on this. Please make them block this bug, and we will de-dupe
> if they are indeed exactly the same. Thanks!)

> To reproduce:
> 1. open a page with a form
> 2. select some text on the page that isn't in a text input
> 3. notice the "Search Google for"
> 4. select some text within a text input
> 5. notice the "Search Google for" is missing

> Recommendation:
> Add the "Search Google for" context menu when selecting text within a text input and
>  text area.

Comment 2

[Edmund Wong (:ewong)]

Attachment: proposed patch to enable search for inputs. (v1)

Comment 3

[[e-mail]]

Comment on attachment 686020 [details] [diff] [review]
proposed patch to enable search for inputs. (v1)

>     this.initMiscItems();
>+    this.initSearchItems();
Can't you tweak initMiscItems to show the existing search option?

>   searchSelected: function(aCharlen) {
>     var focusedWindow = document.commandDispatcher.focusedWindow;
>     var searchStr = focusedWindow.getSelection();
>     searchStr = searchStr.toString();
>+
>+    if (!searchStr) {
>+      var focusedElement = document.commandDispatcher.focusedElement;
Please use this.target instead.

>+        if (fElem.selectionStart < fElem.selectionEnd)
selectionStart is never greater than selectionEnd, although as it happens substring will swap them anyway, so no need to check.

[I wonder whether it's worth checking for a text input first.]

Comment 4

[Edmund Wong (:ewong)]

Attachment: Proposed patch (v2)

Comment 5

[Philip Chee]

> +      <menuseparator id="search-separator"/>
> +      <menuitem id="search-for-text"
> +                oncommand="BrowserSearch.loadSearch(gContextMenu.searchSelected(), true, event);"/>

I think you need to update the tests as well:

suite/browser/test/mochitest/test_contextmenu.html
suite/browser/test/mochitest/subtst_contextmenu.html

Comment 6

[Edmund Wong (:ewong)]

Attachment: Context menu search should be available in textareas/inputs as well (Port Bug 565717) (v3)

Fixed tests.

Comment 7

[[e-mail]]

Comment on attachment 698206 [details] [diff] [review]
Context menu search should be available in textareas/inputs as well (Port Bug 565717) (v3)

>     var menuLabel = bundle.getFormattedString("contextMenuSearchText",
>                                               [engineName, searchSelectText]);
>+    var menuLabelSearch = bundle.getFormattedString("searchForText",
>+                                              [engineName, searchSelectText]);
>+
>     this.setItemAttr("context-searchselect", "label", menuLabel);
>     this.setItemAttr("context-searchselect", "accesskey",
>                      bundle.getString("contextMenuSearchText.accesskey"));
> 
>+    this.setItemAttr("search-for-text", "label", menuLabelSearch);
>+    this.setItemAttr("search-for-text", "accesskey",
>+                     bundle.getString("searchForText.accesskey"));
You're just duplicating effort here, you should just tweak the code that shows the existing context search item to show when you have a selection in a text field as well.

>+    if (!searchStr) {
>+      var focusedElement = this.target;
>+      if ((focusedElement instanceof HTMLInputElement &&
>+           focusedElement.mozIsTextField(true)) ||
>+           focusedElement instanceof HTMLTextAreaElement) {
>+        var fElem = focusedElement;
>+        searchStr = fElem.value.substring(fElem.selectionStart, fElem.selectionEnd);
Unnecessary variable(s). "this.target" is actually less typing than "focusedElement"!

We have a problem because we now get the context menu for disabled form controls. One approach is to use if (this.onTextInput) instead if if (!searchStr).

Comment 8

[Edmund Wong (:ewong)]

Attachment: Context menu search should be available in textareas/inputs as well (Port Bug 565717) (v4)

Comment 9

[Edmund Wong (:ewong)]

Attachment: Context menu search should be available in textareas/inputs as well (Port Bug 565717) (v5)

same as v4 but needed to change the key=value pair for contextMenuSearchText as
the accesskey was changed.

Comment 10

[[e-mail]]

Comment on attachment 700830 [details] [diff] [review]
Context menu search should be available in textareas/inputs as well (Port Bug 565717) (v5)

We're getting close!

>+    this.showItem("context-searchselect", this.onTextInput && this.isTextSelected);
There's already a this.showItem for context-searchselect, you need to tweak the existing one instead of adding a new one. Otherwise search selected stops working for normal selection ;-)

>+    if (this.onTextInput) {
>+      if ((this.target instanceof HTMLInputElement &&
>+           this.target.mozIsTextField(true)) ||
>+           this.target instanceof HTMLTextAreaElement) {
>+        var fElem = this.target;
>+        searchStr = fElem.value.substring(fElem.selectionStart, fElem.selectionEnd);
If you want to keep fElem, then move it up three lines so that you can switch from this.target to fElem in more places.

> # LOCALIZATION NOTE (contextMenuSearchText): %1$S is the search engine,
> # %2$S is the selection string.
>-contextMenuSearchText=Search %1$S for "%2$S"
>-contextMenuSearchText.accesskey=S
>+contextMenuSearchTextValue=Search %1$S for "%2$S"
>+contextMenuSearchTextValue.accesskey=e
You forgot to rename the l10n note. Also, that's a lame string name, any chance you could simplify it to searchSelected[.accesskey]?

Comment 11

[Edmund Wong (:ewong)]

Attachment: Context menu search should be available in textareas/inputs as well (Port Bug 565717) (v6)

Comment 12

[[e-mail]]

Comment on attachment 715850 [details] [diff] [review]
Context menu search should be available in textareas/inputs as well (Port Bug 565717) (v6)

This is functionally correct, but please fix these before check in!

>+    this.showItem("context-searchselect",
>+                   (this.isTextSelected && !this.onTextInput) ||
>+                   (this.onTextInput && this.isTextSelected));
This expression is unnecessarily complex.
> (this.isTextSelected && !this.onTextInput) ||
> (this.onTextInput && this.isTextSelected)
commutativity of && is allowed in this case:
> (this.isTextSelected && !this.onTextInput) ||
> (this.isTextSelected && this.onTextInput)
&& distributes over ||:
> this.isTextSelected && (!this.onTextInput || this.onTextInput)
Hopefully you can figure the rest out.

>+    var fElem = this.target;
>+    if (this.onTextInput) {
Nit: put fElem inside this if because you only use it there.

> # LOCALIZATION NOTE (contextMenuSearchText): %1$S is the search engine,
> # %2$S is the selection string.
>-contextMenuSearchText=Search %1$S for "%2$S"
>-contextMenuSearchText.accesskey=S
>+searchSelected=Search %1$S for "%2$S"
>+searchSelected.accesskey=e
Nit: need to fix the L10N note.

Comment 13

[Edmund Wong (:ewong)]

Attachment: Context menu search should be available in textareas/inputs as well (Port Bug 565717) (v7)

Comment 14

[Edmund Wong (:ewong)]

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