250910 - Find toolbar: initialize with the current selection

Status: RESOLVED FIXED

(Toolkit :: Find Toolbar, enhancement)

Description

[Bernard Alleysson]

1) Select some text in the web page
2) Ctrl + F (display the find toolbar)

Result:
Focus is moved to the text field of the find toolbar but the content is empty

Result I would like to see:
The find toolbar text field is initialized with the selected text

Remarks:
This feature would need to be smart enough to work only when a small amount of
text is selected (word ? sentence ?). If the text selected is too long (ie Page
|Select All), then it should stick with the current behaviour (empty text field)

Comment 1

[Ben Goodger (use ben at mozilla dot org for email)]

-> Find Toolbar component.

Comment 2

[Jesse Ruderman]

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

Comment 3

[Michael Clackler]

Attachment: patch to input user-selected text into find string with ctrl-F

This patch will initialize the find bar with any user-selected text when the
bar is opened using ctrl-F.

Only the first 15 characters of the users selection will be used.  I am
debating whether or not this length should be available to the user in a user
pref to allow it to be changed.

Comment 4

[Jaap A. Haitsma]

Can't you use the function searchSelected from browser.js it is essentially
doing almost the same as your userSelectedFind()

The searchSelected function you can see here
http://lxr.mozilla.org/mozilla/source/browser/base/content/browser.js#3755

I would go for a bit more then 15 characters. You have languages with very long
words. In Dutch a famous example is "hottentottententententoonstelling" (Also
very good for scrabble)

Maybe just only do the first word so:

findStr=searchSelected();
findStr=findStr.split(" ");
findStr=findStr[0];

You don't need to introduce another preference then and you are reusing existing
code :-)

Comment 5

[Ryan Polk (Quark)]

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

Comment 6

[Taku0]

Re: #4
>Maybe just only do the first word so:
>
>findStr=searchSelected();
>findStr=findStr.split(" ");
>findStr=findStr[0];

The idea is not good because of two reason:
1. There are many languages that does not split words by " ".
2. It doesn't work well when trying to search phrase.

Comment 7

[Phil Ringnalda (:philor)]

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

Comment 8

[nrlz]

Some points to consider for the implementation of this:

1. Should the find function begin searching immediately as soon as Ctrl+F is
pressed with a text selection?

Immediately would save the user one less step. But it may cause accidental
searching if the user forgot they previously selected some text (either
deliberately or accidentally) on a long webpage and the search ends up scrolling
the webpage to an arbitrary location.

2. If we do search immediately, should the resulting focus be on the textbox
(with the text selected) or should the focus be where ever the Find function
focuses on?

Focusing and selecting the textbox allows the user to immediately start typing
something else when they open the Find bar. Allowing the Find function to
control the focus allows launching of links.

Comment 9

[Asaf Romano (gone)]

Comment on attachment 156683 [details] [diff] [review]
patch to input user-selected text into find string with ctrl-F

The _proto_ hack isn't supported anymore, please see
http://developer-test.mozilla.org/docs/Safely_accessing_content_DOM_from_chrome

Comment 10

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

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

Comment 11

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

Attachment: checkpoint patch

This is half of what needs to be done. It isn't hooked up to the find bar yet, mostly because the issues in comment 8 need to be addressed.

Comment 12

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

Masayuki-san, do you have any thoughts about this bug? Ideally opening the search bar with selected text would make that text be highlighted, and would allow pressing "enter" or F3 to find the next occurence. I am not sure how best to accomplish this.

Comment 13

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

# Umm... I don't like this feature.
# I think that this should be selectable by pref.

I don't know what is best way in browser.js.
But findBar.js looks good for me.

Howerver, please wait to fix bug 297369. I will clean up the findBar event handler in that bug. You can use following code after bug 297369.

> -  onFindCmd: function ()
> +  onFindCmd: function (aSelectedText)
>    {
>      this.setFindMode(FIND_NORMAL);
>      this.openFindBar();
>      if (this.mFlashFindBar) {
>        this.mFlashFindBarTimeout = setInterval(this.flashFindBar, 500);
>        var prefService =
>              Components.classes["@mozilla.org/preferences-service;1"]
>                        .getService(Components.interfaces.nsIPrefBranch);
>  
>        prefService.setIntPref("accessibility.typeaheadfind.flashBar",
>                               --this.mFlashFindBar);
>      }
>      this.selectFindBar();
>      this.focusFindBar();
> +
> +    if (this.mPrefUseSelectionText && aSelectedText) {
> +      var findField = document.getElementById("find-field");
> +      findField.value = aSelectedText;
> +      this.onFindBarInput(aSelectedText);
> +    }
>    },

Note that this feature will not often function well in Japanese or Chinese content.
See bug 156369.
# But this is not a scope of this bug. However I hope that this will optional
# function(that means the default value of mPrefUseSelectionText is false) for
# Japanese and Chinese people.

Comment 14

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

Attachment: checkpoint #2

Dumping this here to get it out of my tree. I think the getBrowserSelection refactoring is useful with or without findbar changes, they should probably go in a blocker bug. I don't plan on working on this for a while, so if anyone else wants to sort out a working prototype go ahead.

Comment 15

[Robin Monks]

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

Comment 16

[Robin Monks]

Sister bug for SeaMonkey: bug 115630

Comment 17

[Peter Kasting]

Taking this in hopes of looking at it someday.

Comment 18

[Michael Ventnor]

Attachment: Patch

This implements it and changes around a few things to accommodate this new feature. We can't just change the value of the textbox since Find Next will read from nsITypeAheadFind rather than the textbox value.

Thoroughly tested and no regressions encountered as of yet.

Comment 19

[Asaf Romano (gone)]

Comment on attachment 244659 [details] [diff] [review]
Patch

Sorry, this will have to wait until the new findbar lands (see bug 288254).

Comment 20

[Michael Ventnor]

Attachment: Patch for XML widget

New find bar has landed. With 10 votes, I would've thought this bug deserved more than P5...

Comment 21

[Michael Ventnor]

Attachment: Patch for XML widget 2

I think I reintroduced something similar to bug 297369 with the previous patch, this patch fixes it by checking to make sure the buttons are disabled when the find bar is opened.

Comment 22

[Asaf Romano (gone)]

Comment on attachment 247376 [details] [diff] [review]
Patch for XML widget 2

Few issues:
 * The findbar binding should get the selection, i.e. callers should not need to pass it.
 * the selection length should be limited.
 * selections inside textfields should be handled as well.
 * we should either select the findfield's contents so you could easily search for something else or (and maybe I should say "and") actually search for the selection once you press Accel+F. This needs some input from beltzner and mconnor.

Comment 23

[Michael Ventnor]

Attachment: Patch for XML widget 3

> Few issues:
>  * The findbar binding should get the selection, i.e. callers should not need
> to pass it.

Done.

>  * the selection length should be limited.

This was already implemented in the getBrowserSelection function, but that doesn't matter anymore since the patch no longer relies on browser code.

>  * selections inside textfields should be handled as well.

Partly done. I can get the selection in textareas but I don't know how to do so with normal input elements. Unless you or someone else knows, it will have to be a known issue.

>  * we should either select the findfield's contents so you could easily search
> for something else or (and maybe I should say "and") actually search for the
> selection once you press Accel+F.
> 

The first half of what you mentioned already happens. As for the second half, I don't think that's a good idea since they may not want to begin searching immediately.

Comment 24

[Asaf Romano (gone)]

Something like this should work (untested):

var focusedElement = document.commnadDispatcher.focusedElement;
if (focusedElement instanceof Components.interfaces.nsIDOMNSEditableElement &&
    focusedElement.ownerDocument.defaultView.top == this.browser.contentWindow) {
  selection = focusedElement.editor.selectionController.
    getSelection(this.nsISelectionController.SELECTION_NORMAL).toString();
}
else {
  var focusedWindow = document.commandDispatcher.focusedWindow;
  if (focusedWindow.top == this.browser.contentWindow)
    selection = focusedWindow.getSelection().toString();
}

Comment 25

[Asaf Romano (gone)]

Comment on attachment 250165 [details] [diff] [review]
Patch for XML widget 3

>Index: toolkit/content/widgets/findbar.xml
>===================================================================
>       <method name="_findAgain">
>         <parameter name="aFindPrevious"/>
>         <body><![CDATA[
>           var fastFind = this._browser.fastFind;
>           var findFieldFocused =
>             this._findField.getAttribute("focused") == "true";
>-          var res = fastFind.findAgain(aFindPrevious,
>-                                       this._findMode == this.FIND_LINKS,
>-                                       findFieldFocused);
>-          this._updateFoundLink(res);
>-          this._updateStatusUI(res, aFindPrevious);
>+
>+          // Ensure the stored SearchString is in sync with what we want to find
>+          if (this._findField.value == this._browser.fastFind.searchString || this.hidden) {
>+            var res = fastFind.findAgain(aFindPrevious,
>+                                         this._findMode == this.FIND_LINKS,
>+                                         findFieldFocused);
>+            this._updateFoundLink(res);
>+            this._updateStatusUI(res, aFindPrevious);
>+          } else {
>+            var res = this._find(this._findField.value);
>+          }

General comment: 1 ) please avoid |} else| code style (hey gavin ;) ). 2) Remove braces around single-line blocks.

this belongs to onFindAgainCommand IMO.


>@@ -1167,16 +1176,52 @@
>               this._findStatusIcon.removeAttribute("status");
>               this._findStatusLabel.value = "";
>               this._findField.removeAttribute("status");
>               break;
>           }
>         ]]></body>
>       </method>
> 
>+      <method name="_getInitialSelection">
>+        <body><![CDATA[
>+          const selLen = 100;


hrm, "Web Search for..." uses 150. Either lengths might be too long (beltzner: ping).

>+          var selText =
>+            document.commandDispatcher.focusedWindow.getSelection().toString();
>+          if (!selText) {
...
>+            }
>+          }


comment 24

>+          if (!selText)
>+            return false;

So this method returns a string or a boolean? ;) |return "";| should do.

>+          var initialString = this._getInitialSelection();
>+          if (initialString) {
>+            this._findField.value = initialString;
>+            this._enableFindButtons(true);
>+          } else if (this._findField.value == "") {

nit: else if (!this._findField.value)


We may want to disable this behavior in certain windows.

Comment 26

[Michael Ventnor]

Attachment: Patch v4

I should just reassign this bug to you. :P
With a little adjustment your code works beautifully. I also addressed your review concerns.

Comment 27

[Asaf Romano (gone)]

Any reason you cleared the review request?

Comment 28

[Michael Ventnor]

Comment on attachment 250238 [details] [diff] [review]
Patch v4

Sorry. I didn't mean to do that. :/

Comment 29

[Asaf Romano (gone)]

Comment on attachment 250238 [details] [diff] [review]
Patch v4

This needs both ui-r and a unit test before it can land.

>Index: toolkit/content/widgets/findbar.xml
>=================================================================== 
>+      <method name="_getInitialSelection">
>+        <body><![CDATA[

Declare selText here.

>         <body><![CDATA[
>-          var findString = this._browser.fastFind.searchString;
>+          var findString = this._browser.fastFind.searchString || this._findField.value;
>           if (!findString) {
>             this.startFind();
>             return;
>           }
> 
>-          var res = this._findAgain(aFindPrevious);

Declare res here.

Please file a bug on making this behavior optional (per-findbar via a property/attribute and globally via a hidden preference).

Many thanks for your work on this, Michael.

Comment 30

[Michael Ventnor]

Attachment: Patch 5

(In reply to comment #29)
> (From update of attachment 250238 [details] [diff] [review])
> This needs both ui-r and a unit test before it can land.

Will ask Beltzner for ui-r. As for a unit test, I can't find any examples of toolkit widgets having unit tests so I wouldn't know where to put them nor what they're made of. I've tested this patch in every possible scenario I can think of so I don't think it'll need one anyway :)

> 
> >Index: toolkit/content/widgets/findbar.xml
> >=================================================================== 
> >+      <method name="_getInitialSelection">
> >+        <body><![CDATA[
> 
> Declare selText here.
> 
> >         <body><![CDATA[
> >-          var findString = this._browser.fastFind.searchString;
> >+          var findString = this._browser.fastFind.searchString || this._findField.value;
> >           if (!findString) {
> >             this.startFind();
> >             return;
> >           }
> > 
> >-          var res = this._findAgain(aFindPrevious);
> 
> Declare res here.
> 

Done.

> Please file a bug on making this behavior optional (per-findbar via a
> property/attribute and globally via a hidden preference).
> 

Already taken care of in this patch.

Comment 31

[Asaf Romano (gone)]

Comment on attachment 252590 [details] [diff] [review]
Patch 5

1) prefillWithSelection should be an attribute with a corresponding property.
2. set the pref in all.js and remove the try/catch blocks
3) Optimization: don't read the pref if prefillWithSelection is set to false.

Comment 32

[Asaf Romano (gone)]

See bug 288254 for an example of a findbar unit-test.

Comment 33

[Michael Ventnor]

(In reply to comment #31)
> (From update of attachment 252590 [details] [diff] [review])
> 1) prefillWithSelection should be an attribute with a corresponding property.

Sorry but I don't quite understand what you mean here?
Do you simply mean "var attribute = this._prefillWithSelection;"? 

Comment 34

[Asaf Romano (gone)]

DOM attribute, default to true.

<property name="prefillwithselection"
          onget="return this.getAttribute('prefillwithselection') != 'false'"
          onset="this.setAttribute('prefillwithselection', val); return val;"/>

Comment 35

[Michael Ventnor]

Attachment: Unit test

Unit test for this bug (I hope I did this right).

Comment 36

[Michael Ventnor]

Attachment: Patch 6

Comment 37

[Asaf Romano (gone)]

Comment on attachment 254488 [details] [diff] [review]
Patch 6

>Index: toolkit/content/widgets/findbar.xml
>===================================================================
 
>+      <method name="_getInitialSelection">
>+        <body><![CDATA[
>+          var focusedElement = document.commandDispatcher.focusedElement;
>+          var Ci = Components.interfaces;
...
>+            // The user may have a selection in an input or textarea
>+            selText = focusedElement.editor.selectionController.
>+              getSelection(Ci.nsISelectionController.SELECTION_NORMAL).toString();

s/Ci/Components.interfaces. Ci is defined in browser-specific code.

>+          var initialString = (this.prefillWithSelection && userWantsPrefill) ?
>+                              this._getInitialSelection() :
>+                              null;

nit: |null| on the same line with |this._getInitialSelection()|

Comment 38

[Michael Ventnor]

Attachment: Patch 6.1

You always have _something_ to complain about, don't you. :)

Comment 39

[Asaf Romano (gone)]

Wouldn't you complain about findbar being broken in view-source ;) ?

Comment 40

[Asaf Romano (gone)]

Comment on attachment 254486 [details]
Unit test

The unit test should also test the prefillwithselection property (it would also be nice to test selection in the content area as well as in a textfield).

Comment 41

[Asaf Romano (gone)]

I get
patching file toolkit/content/widgets/findbar.xml
Hunk #4 FAILED at 1094.
patch: **** malformed patch at line 179:          -

when trying to apply v6.1

Comment 42

[Michael Ventnor]

Attachment: Second unit test

I don't really know how to cause an automatic selection of content, but this does check the pref. I'll attach a new patch shortly.

Comment 43

[Michael Ventnor]

Attachment: Patch 6.1 again

Freshly diffed patch. Also thank you Mike for the speedy ui-r.

Comment 44

[Asaf Romano (gone)]

mozilla/toolkit/content/widgets/findbar.xml 1.10
mozilla/modules/libpref/src/init/all.js 3.665

Comment 45

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

I landed tests for this as part of bug 440334.