Last Comment Bug 807326 - Context menu Search should be available in textareas/inputs as well (Port Bug 565717)
: Context menu Search should be available in textareas/inputs as well (Port Bug...
Status: RESOLVED FIXED
:
Product: SeaMonkey
Classification: Client Software
Component: General (show other bugs)
: Trunk
: All All
: -- enhancement (vote)
: seamonkey2.19
Assigned To: Edmund Wong (:ewong)
:
Mentors:
: 125487 429244 (view as bug list)
Depends on: 856587
Blocks:
  Show dependency treegraph
 
Reported: 2012-10-31 08:37 PDT by Daniel B.
Modified: 2013-07-27 10:11 PDT (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
proposed patch to enable search for inputs. (v1) (5.64 KB, patch)
2012-11-28 00:36 PST, Edmund Wong (:ewong)
neil: review-
Details | Diff | Splinter Review
Proposed patch (v2) (4.99 KB, patch)
2013-01-03 00:35 PST, Edmund Wong (:ewong)
no flags Details | Diff | Splinter Review
Context menu search should be available in textareas/inputs as well (Port Bug 565717) (v3) (28.31 KB, patch)
2013-01-04 19:35 PST, Edmund Wong (:ewong)
neil: review-
Details | Diff | Splinter Review
Context menu search should be available in textareas/inputs as well (Port Bug 565717) (v4) (26.01 KB, patch)
2013-01-10 19:52 PST, Edmund Wong (:ewong)
no flags Details | Diff | Splinter Review
Context menu search should be available in textareas/inputs as well (Port Bug 565717) (v5) (26.99 KB, patch)
2013-01-10 20:00 PST, Edmund Wong (:ewong)
no flags Details | Diff | Splinter Review
Context menu search should be available in textareas/inputs as well (Port Bug 565717) (v6) (27.12 KB, patch)
2013-02-19 19:47 PST, Edmund Wong (:ewong)
neil: review+
Details | Diff | Splinter Review
Context menu search should be available in textareas/inputs as well (Port Bug 565717) (v7) (27.08 KB, patch)
2013-02-28 18:32 PST, Edmund Wong (:ewong)
neil: review+
Details | Diff | Splinter Review

Description Daniel B. 2012-10-31 08:37:17 PDT
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 2012-10-31 13:50:54 PDT
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) 2012-11-28 00:36:32 PST
Created attachment 686020 [details] [diff] [review]
proposed patch to enable search for inputs. (v1)
Comment 3 neil@parkwaycc.co.uk 2012-11-28 01:37:58 PST
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) 2013-01-03 00:35:31 PST
Created attachment 697354 [details] [diff] [review]
Proposed patch (v2)
Comment 5 Philip Chee 2013-01-03 04:53:54 PST
> +      <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) 2013-01-04 19:35:22 PST
Created attachment 698206 [details] [diff] [review]
Context menu search should be available in textareas/inputs as well (Port Bug 565717) (v3)

Fixed tests.
Comment 7 neil@parkwaycc.co.uk 2013-01-06 10:06:43 PST
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) 2013-01-10 19:52:57 PST
Created attachment 700829 [details] [diff] [review]
Context menu search should be available in textareas/inputs as well (Port Bug 565717) (v4)
Comment 9 Edmund Wong (:ewong) 2013-01-10 20:00:48 PST
Created attachment 700830 [details] [diff] [review]
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 neil@parkwaycc.co.uk 2013-01-11 09:56:50 PST
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) 2013-02-19 19:47:56 PST
Created attachment 715850 [details] [diff] [review]
Context menu search should be available in textareas/inputs as well (Port Bug 565717) (v6)
Comment 12 neil@parkwaycc.co.uk 2013-02-20 04:00:26 PST
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) 2013-02-28 18:32:54 PST
Created attachment 719790 [details] [diff] [review]
Context menu search should be available in textareas/inputs as well (Port Bug 565717) (v7)
Comment 14 Edmund Wong (:ewong) 2013-03-01 19:11:51 PST
Pushed to comm-central:
http://hg.mozilla.org/comm-central/rev/55a83408986b
Comment 15 Philip Chee 2013-07-26 08:17:10 PDT
*** Bug 125487 has been marked as a duplicate of this bug. ***
Comment 16 Philip Chee 2013-07-27 10:11:31 PDT
*** Bug 429244 has been marked as a duplicate of this bug. ***

Note You need to log in before you can comment on or make changes to this bug.