The default bug view has changed. See this FAQ.

Find toolbar: initialize with the current selection

RESOLVED FIXED in mozilla1.9alpha3

Status

()

Toolkit
Find Toolbar
--
enhancement
RESOLVED FIXED
13 years ago
8 years ago

People

(Reporter: Bernard Alleysson, Assigned: Michael Ventnor)

Tracking

(Depends on: 1 bug)

Trunk
mozilla1.9alpha3
Points:
---
Dependency tree / graph
Bug Flags:
blocking-aviary1.5 -
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 11 obsolete attachments)

6.72 KB, patch
Details | Diff | Splinter Review
3.73 KB, application/vnd.mozilla.xul+xml
Details
8.54 KB, patch
Details | Diff | Splinter Review
(Reporter)

Description

13 years ago
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)
(Reporter)

Updated

13 years ago
OS: Windows Server 2003 → All
-> Find Toolbar component.
Component: General → Find Toolbar / FastFind

Comment 2

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

Comment 3

13 years ago
Created attachment 156683 [details] [diff] [review]
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.

Updated

13 years ago
Attachment #156683 - Flags: review?(firefox)

Comment 4

13 years ago
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

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

Comment 6

13 years ago
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.
*** Bug 274503 has been marked as a duplicate of this bug. ***

Comment 8

12 years ago
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.

Updated

12 years ago
Flags: blocking-aviary1.1?
Flags: blocking-aviary1.1? → blocking-aviary1.1+
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
Attachment #156683 - Flags: review?(firefox) → review-

Updated

12 years ago
Flags: blocking-aviary1.1+ → blocking-aviary1.1-
*** Bug 305615 has been marked as a duplicate of this bug. ***
Assignee: firefox → gavin.sharp
QA Contact: general → fast.find
Hardware: PC → All
Target Milestone: --- → Firefox1.6-
Version: unspecified → Trunk
Priority: -- → P2
Status: NEW → ASSIGNED
Created attachment 201968 [details] [diff] [review]
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.
Attachment #156683 - Attachment is obsolete: true
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.
# 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.
Depends on: 297369
Created attachment 207273 [details] [diff] [review]
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.
Attachment #201968 - Attachment is obsolete: true
Priority: P2 → P5
Target Milestone: Firefox1.6- → Future

Comment 15

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

Updated

11 years ago
Blocks: 225321

Comment 16

11 years ago
Sister bug for SeaMonkey: bug 115630

Comment 17

11 years ago
Taking this in hopes of looking at it someday.
Assignee: gavin.sharp → pkasting
Status: ASSIGNED → NEW
(Assignee)

Comment 18

11 years ago
Created attachment 244659 [details] [diff] [review]
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.
Attachment #244659 - Flags: review?(mano)
Comment on attachment 244659 [details] [diff] [review]
Patch

Sorry, this will have to wait until the new findbar lands (see bug 288254).
Attachment #244659 - Flags: review?(mano)
(Assignee)

Updated

11 years ago
Depends on: 288254
(Assignee)

Comment 20

10 years ago
Created attachment 247324 [details] [diff] [review]
Patch for XML widget

New find bar has landed. With 10 votes, I would've thought this bug deserved more than P5...
Assignee: pkasting → ventnor.bugzilla
Attachment #244659 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #247324 - Flags: review?(mano)
(Assignee)

Comment 21

10 years ago
Created attachment 247376 [details] [diff] [review]
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.
Attachment #247324 - Attachment is obsolete: true
Attachment #247376 - Flags: review?(mano)
Attachment #247324 - Flags: review?(mano)
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.
Attachment #247376 - Flags: review?(mano) → review-
(Assignee)

Comment 23

10 years ago
Created attachment 250165 [details] [diff] [review]
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.
Attachment #247376 - Attachment is obsolete: true
Attachment #250165 - Flags: review?(mano)
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 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.
Attachment #250165 - Flags: review?(mano) → review-
(Assignee)

Comment 26

10 years ago
Created attachment 250238 [details] [diff] [review]
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.
Attachment #250165 - Attachment is obsolete: true
Attachment #250238 - Flags: review?(mano)
(Assignee)

Updated

10 years ago
Attachment #250238 - Flags: review?(mano)
Any reason you cleared the review request?
(Assignee)

Comment 28

10 years ago
Comment on attachment 250238 [details] [diff] [review]
Patch v4

Sorry. I didn't mean to do that. :/
Attachment #250238 - Flags: review?(mano)
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.
Attachment #250238 - Flags: review?(mano) → review+
(Assignee)

Comment 30

10 years ago
Created attachment 252590 [details] [diff] [review]
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.
Attachment #250238 - Attachment is obsolete: true
Attachment #252590 - Flags: ui-review?(beltzner)
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.
Attachment #252590 - Flags: review-
See bug 288254 for an example of a findbar unit-test.
Flags: in-testsuite?
Priority: P5 → --
Target Milestone: Future → Firefox 3
(Assignee)

Comment 33

10 years ago
(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;"? 

DOM attribute, default to true.

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

Comment 35

10 years ago
Created attachment 254486 [details]
Unit test

Unit test for this bug (I hope I did this right).
Attachment #252590 - Attachment is obsolete: true
Attachment #252590 - Flags: ui-review?(beltzner)
(Assignee)

Comment 36

10 years ago
Created attachment 254488 [details] [diff] [review]
Patch 6
Attachment #254488 - Flags: ui-review?(beltzner)
Attachment #254488 - Flags: review?(mano)
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()|
Attachment #254488 - Flags: review?(mano) → review+
(Assignee)

Comment 38

10 years ago
Created attachment 254523 [details] [diff] [review]
Patch 6.1

You always have _something_ to complain about, don't you. :)
Attachment #254488 - Attachment is obsolete: true
Attachment #254523 - Flags: ui-review?(beltzner)
Attachment #254488 - Flags: ui-review?(beltzner)
Wouldn't you complain about findbar being broken in view-source ;) ?
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).

Updated

10 years ago
Attachment #254523 - Flags: ui-review?(beltzner) → ui-review+
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
(Assignee)

Comment 42

10 years ago
Created attachment 254625 [details]
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.
Attachment #254486 - Attachment is obsolete: true
(Assignee)

Comment 43

10 years ago
Created attachment 254626 [details] [diff] [review]
Patch 6.1 again

Freshly diffed patch. Also thank you Mike for the speedy ui-r.
Attachment #254523 - Attachment is obsolete: true
mozilla/toolkit/content/widgets/findbar.xml 1.10
mozilla/modules/libpref/src/init/all.js 3.665
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED
Whiteboard: [a3]
Whiteboard: [a3]
Target Milestone: Firefox 3 → Firefox 3 alpha3

Updated

9 years ago
Depends on: 440334
I landed tests for this as part of bug 440334.
Flags: in-testsuite? → in-testsuite+
Product: Firefox → Toolkit

Updated

8 years ago
Depends on: 495230
You need to log in before you can comment on or make changes to this bug.