257061 - count and display the number of found items in the FIND toolbar

Status: VERIFIED FIXED

(Toolkit :: Find Toolbar, enhancement)

Description

[Peter van der Woude [:Peter6]]

With Find , display the number of matches found on a page.

Extremely usefull extra

Comment 1

[Peter van der Woude [:Peter6]]

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

Comment 2

[Dave Townsend [:mossop]]

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

Comment 3

[(no longer active)]

This should be handled in the findbar XUL widget, which is part of the toolkit.  I'll be posting a patch soon.

Comment 4

[(no longer active)]

Attachment: Patch to show the matches count in the findbar

This patch shows a match count in the find bar, both in the normal and quick find modes.  The match count is updated as the user types the search phrase, and will be updated if the Match Case checkbox is checked/unchecked both before and after a search operation.

This patch also handles "find in links" setting.  It also tries to exclude the invisible elements in the page from the total match count.  The "brackets" that the matches are displayed in are achieved using CSS.

Comment 5

[(no longer active)]

Attachment: Patch to show the matches count in the findbar (updated)

Oops, I found out that my patch is affected with the same problem described in bug 251862.  Here's a new patch to fix this problem.  This patch adds the following:

* a maximum number of matches to find is set through the matchLimit property, which defaults to 100.  When a page has more matches, the string "More than 100 matches" is shown.
* the match count calculation is performed in a background thread via window.setTimeout() with a delay of 500ms.

Comment 6

[Asaf Romano (gone)]

Comment on attachment 272782 [details] [diff] [review]
Patch to show the matches count in the findbar (updated)

Thanks for this work. I don't like this living in findbar.xml though. Most of this functionality would be useful for other consumers of nsIWebBrowserFind, and probably better get written in C++ (likely in a ns*Frame accessible component).

Comment 7

[(no longer active)]

(In reply to comment #6)
> (From update of attachment 272782 [details] [diff] [review])
> Thanks for this work. I don't like this living in findbar.xml though. Most of
> this functionality would be useful for other consumers of nsIWebBrowserFind,
> and probably better get written in C++ (likely in a ns*Frame accessible
> component).
> 

Here is what I have in mind in order to implement this.  Since the nsIWebBrowserFind interface is frozen, I'm going to define a new one, named something like nsIWebBrowserFindCountMatches and implement it in the nsWebBrowserFind (@mozilla.org/embedcomp/find;1) interface.  Then, modify the nsITypeAheadFind interface to support returning the match count, using the nsIWebBrowserFindCountMatches implementation.  Then, modify findbar.xml to use the nsITypeAheadFind functionality to display the match count in the UI.

This way, all of the nsIWebBrowserFind consumers can get the match count by querying for the nsIWebBrowserFindCountMatches interface.

What do you think about this plan?

Comment 8

[Asaf Romano (gone)]

We usually just do nsIFooBar2. That said, I'm not a peer for this code (embedding/components).

Comment 9

[(no longer active)]

(In reply to comment #8)
> We usually just do nsIFooBar2. That said, I'm not a peer for this code
> (embedding/components).
> 

Thanks for the guide.  Is it OK to attach the patch for the backend to this bug, or should I file a new bug for the backend code, and attach the patch to show the matches count in the UI to the current bug once that one lands?

Comment 10

[Asaf Romano (gone)]

I think the embedding/ work here is worth a separate bug, yes.

Comment 12

[Mike Beltzner [:beltzner, not reading bugmail]]

Not a blocker, though nice to have if you can get the reviews and nominate for approval.

Comment 13

[(no longer active)]

Attachment: Unbitrotted and updated patch for reconsideration

This patch is updated to apply cleanly on the latest trunk code.  I think because bug 389159 is not likely to get fixed in the Firefox 3 time frame, and this is a useful feature to users, this patch should be reconsidered for inclusion in Firefox 3.  The current maximum match count of 100 (which is what Safari does as well, for example) prevents this from using up too much CPU, therefore I think a JavaScript implementation works acceptably as well.  Of course, the code responsible for counting the matches can be updated when bug 389159 gets fixed.

I'm also requesting ui-review, and I'll attach a couple of screenshots to ease the ui-r process in a moment.

I hope to be able to get this landed for Firefox 3.

Comment 14

[(no longer active)]

Attachment: Screenshot of the patch in effect in the full find bar

Comment 15

[(no longer active)]

Attachment: Screenshot of the patch in effect in the quick find bar

Comment 16

[(no longer active)]

Attachment: String changes (checked in)

String changes to land before the l10n freeze...

Comment 17

[(no longer active)]

Beltzner: can you do a review on the string changes (attachment 298775 [details] [diff] [review]) so that this has a change to land in Firefox 3?  Thanks!

Comment 18

[Peter van der Woude [:Peter6]]

Ehsan, would it be hard to display which of the results is selected and display "match X of n" or "X of n matches"
Or would this require a followup bug ?

Comment 19

[robertuhl]

Hm, I would like to go with that if possible:

"Match X of n"

Having "X of n matches" displayed does not really make sense because you can only select one item at a time.

But --- I actually would use this feature as an indicator of how important the displayed page 'could' be. So the more information you provide the more you have to concentrate to find what you I want. To hit easily 'n' as my main information with my eyes wouldn't it be good to print it "bold" or change the color of it?

Comment 20

[(no longer active)]

(In reply to comment #18)
> Ehsan, would it be hard to display which of the results is selected and display
> "match X of n" or "X of n matches"
> Or would this require a followup bug ?

Actually I prefer to defer that to a followup bug.  The concept of "match X of N" is a bit hard to define, because the search is not initiated from the beginning of the document in all cases, and therefore determining what the correct X value should be is not as easy as it may appear at first sight.

Anyway, unless Beltzner reviews the strings in this bug and we land them in the next 15 hours, it's not likely that this would make it into Firefox 3.  In that case, I'll try to knock up an extension using the same implementation and submit it to AMO.

Comment 21

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 298775 [details] [diff] [review]
String changes (checked in)

r+uir+a=beltzner for 1.9

Comment 22

[Mike Beltzner [:beltzner, not reading bugmail]]

Checkin needed on strings only portion.

Comment 23

[Peter van der Woude [:Peter6]]

(In reply to comment #20)
> (In reply to comment #18)
> > Ehsan, would it be hard to display which of the results is selected and display
> > "match X of n" or "X of n matches"
> > Or would this require a followup bug ?
> 
> Actually I prefer to defer that to a followup bug.  The concept of "match X of
> N" is a bit hard to define, because the search is not initiated from the
> beginning of the document in all cases, and therefore determining what the
> correct X value should be is not as easy as it may appear at first sight.
> 
> Anyway, unless Beltzner reviews the strings in this bug and we land them in the
> next 15 hours, it's not likely that this would make it into Firefox 3.  In that
> case, I'll try to knock up an extension using the same implementation and
> submit it to AMO.
> 
->Bug 414109

Comment 24

[Reed Loden [:reed]]

Checking in toolkit/locales/en-US/chrome/global/findbar.properties;
/cvsroot/mozilla/toolkit/locales/en-US/chrome/global/findbar.properties,v  <--  findbar.properties
new revision: 1.5; previous revision: 1.4
done

Comment 25

[Rimas Kudelis [use needinfo to ping]]

Plurals support has recently been added to the toolkit with a fix to the Bug #394516. I think it would be nice to use plural support at least for the newly added strings...

Comment 26

[(no longer active)]

Marking the status in the whiteboard...  The patch in attachment 294747 [details] [diff] [review] minus the strings parts still applies cleanly on the trunk.  Waiting for the reviews...

(In reply to comment #25)
> Plurals support has recently been added to the toolkit with a fix to the Bug
> #394516. I think it would be nice to use plural support at least for the newly
> added strings...

Hmmm, this would make the bug late-l10n, wouldn't it?

Comment 27

[Rimas Kudelis [use needinfo to ping]]

(In reply to comment #26)
> (In reply to comment #25)
> > Plurals support has recently been added to the toolkit with a fix to the Bug
> > #394516. I think it would be nice to use plural support at least for the newly
> > added strings...
> 
> Hmmm, this would make the bug late-l10n, wouldn't it?

Yes...

Comment 28

[(no longer active)]

I have created an extension based on my patch which works both in Firefox 2 and 3.  I have uploaded it to AMO.  Please install and test it, and write a review if you think it's useful, so that I can nominate it to appear on the public site (it's currently in the sandbox).

<https://addons.mozilla.org/en-US/firefox/addon/6738>

Thanks!

Comment 29

[robertuhl]

Wonderful!!! small thing --> huge impact.

I wrote at comment #19 about telling about what found item is displayed or selected from the top to bottom. Would it be hard to implement?

Thanks for the add-on.

(Can you tell me how get access to the source of the add-on?)

Comment 30

[(no longer active)]

(In reply to comment #29)
> Wonderful!!! small thing --> huge impact.

:-)

> I wrote at comment #19 about telling about what found item is displayed or
> selected from the top to bottom. Would it be hard to implement?

Hmmm.  Like I said before, it's hard to define which number is the current match, because the matching does not always start at the beginning of the document.  I guess it would be implementable by keeping all of the ranges found in the document in memory and compare them to the current match, but that could have a perf impact on large pages where the number of matches is huge.  This problem already happens for the highlight feature (see bug 251862), and I don't like making this twice worse.

There may be other ways of doing this, but I can't think of any right now.  Any suggestions are appreciated.

> Thanks for the add-on.
> 
> (Can you tell me how get access to the source of the add-on?)

Yeah.  The XPI files for the add-ons are really ZIP files which you can extract and take a look at.  The jar file inside the chrome dir is also a ZIP file.  I have also enabled viewing the sources via the web: <https://addons.mozilla.org/en-US/firefox/files/browse/25131>.

To give you a quick startup, here is how the extension works.  There are two separate implementations for Firefox 3 Alpha 1 and below (including Firefox 2), and Firefox 3 Alpha2 and above.

Firefox 3 Alpha 1 and below: findinnumbers.xul is an overlay for browser.xul, and pulls in findinnumbers.js, which extends the gFindBar object with some new methods and properties to add the new functionality.  The code in findinnumbers.js is mostly based on this patch, modified in a few places to match the older non-XBL findbar implementation.

Firefox 3 Alpha2 and above: findinnumbers.xml implements a new XBL binding for the findbar named findbarnumbers.  This binding inherits from findbar, and adds the new functionality.  This is almost identical to the code in the patch for this bug.  findinnumbers.css makes the XUL <findbar> element use the new binding.

The code of this extension is licensed under MPL/GPL/LGPL tri-license.

Comment 31

[Peter van der Woude [:Peter6]]

Find In Numbers 1.1 prevents / and ' to call the findbar with the current nightly

Comment 33

[Peter van der Woude [:Peter6]]

not really a blocker, but it would be nice to get this feature and bug 414109 into 1.9.1

Comment 34

[Régis Caspar]

I added a patch proposal to bug 414109 adding match(es) count and selected match number. 
Sorry Ehsan, I didn't see this bug before starting to code (we talked about this enhancement with Peter at the summit).

Comment 35

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 294747 [details] [diff] [review]
Unbitrotted and updated patch for reconsideration

I like the idea, but not the current presentation. It's not properly aligned, and too far removed from the controls that would actually be useful with the information (the next and previous buttons)

Comment 36

[Mike Beltzner [:beltzner, not reading bugmail]]

Adding Aza, as I know he's got an itch to scratch with the find toolbar :)

Comment 37

[(not currently active) Ted Mielczarek]

Minusing, doesn't seem like something we really need. Beltzner, feel free to mark wanted if this is something you'd like to get in.

Comment 39

[Mike Connor [:mconnor]]

Comment on attachment 294747 [details] [diff] [review]
Unbitrotted and updated patch for reconsideration

Let's get some UI love on this, and then we can get the code done.

Comment 40

[(no longer active)]

Updating to reality: I won't have the time to work on this for the foreseeable future!

Comment 42

[eviatarbach]

If this is implemented, it should also detail which instance is currently highlighted, like in Google Chrome.

Comment 43

[Mike Beltzner [:beltzner, not reading bugmail]]

That's a different bug, already on file.

Comment 46

[Asa Dotzler [:asa]]

Seems like this is being done by all the other browsers now (Safari, Chrome, and IE9) and it's actually very useful.  We should probably try to get it on the roadmap for 4.next.

Comment 47

[imradyurrad]

How has this still not landed? All the necessary patches are done and this really should be blocking.

Comment 48

[Mike Beltzner [:beltzner, not reading bugmail]]

Assuming the polish aspects are addressed (cc'ing shorlander) I don't see why this couldn't make it into Firefox 4.

Comment 49

[(no longer active)]

(In reply to comment #48)
> Assuming the polish aspects are addressed (cc'ing shorlander) I don't see why
> this couldn't make it into Firefox 4.

We need an owner here.  I'm pretty sure that my three year old patch isn't still good enough as is, and I'm pretty sure that I can find enough blockers to work on so that I wouldn't have the time to update the patch to trunk in the Firefox 4 timeframe.

That said, I'd really like to see this in Firefox 4 too!  :-)

Comment 51

[JJM]

Is the UI locked in yet for this bug?

My suggestion as per dupe Bug 595941 (see Asa's comment above):

The Find Toolbar should give an indication of how many matches there are in the
current document and which match is currently highlighted.

Suggested: "Find: FooBar (Next|Previous) (1/3) (Highlight all) []Match Case"

Where "1" is the match that is currently highlighted (or where you are "at" in
any case) and "3" is the total number of matches. "(X/Y)" here simply indicates
text, not a button.

This would also make (Next|Previous) that much more clearer and easier to
understand.

Comment 55

[Tanvi Vyas[:tanvi]]

Is anyone available to work on this?  It's been open for years and would be a great feature enhancement to find.  I find myself having to open a different browser sometimes when I need this feature, since firefox doesn't offer it.

Comment 56

[Csaba Kozák [:WonderCsabo]]

Check out Bug 565552.

Comment 57

[(no longer active)]

Mike, please ping me if you needed my help to port the code that currently lives in Find In Numbers for the purposes of this bug.

Comment 58

[Mike de Boer [:mikedeboer]]

Attachment: adding a counter of found matches to the find in page bar

Here's what I did: I manually re-applied Ehsans' patch, and re-added the visual elements manually.
After that I added a `foundRange` attribute to nsITypeAheadFind, used that to implement a 'currently highlighted occurence' marker.
Please watch the screencast I will post together with this patch.

Comment 59

[Mike de Boer [:mikedeboer]]

Attachment: adding a counter of found matches to the find in page bar

Comment 60

[Mike de Boer [:mikedeboer]]

The screencast I promised, which shows off the feature on OSX: http://www.screenr.com/CTi7

Comment 61

[Tanvi Vyas[:tanvi]]

Thanks for working on this!  I've been waiting for this feature to be added to Firefox for years!

Comment 62

[u428464]

(In reply to Mike de Boer [:mikedeboer] from comment #60)
> The screencast I promised, which shows off the feature on OSX:
> http://www.screenr.com/CTi7

Very nice. The search box could also appear red if no matches are found, but it's more something to add in bug 776708 (I can't remember if it is already present).

Comment 63

[(no longer active)]

Mike, what kind of feedback would you like me to provide here?  I'd rather somebody else review this since I wrote the code myself...  :-)

Comment 64

[Mike de Boer [:mikedeboer]]

mochi test coming soon...

Comment 66

[Mike de Boer [:mikedeboer]]

Attachment: adding a counter of found matches to the find in page bar

fixed a JS error

Comment 67

[Tom S [:evilpie]]

This is looking good. Are you waiting for review or is something else blocking this?

Comment 68

[Mike de Boer [:mikedeboer]]

(In reply to Tom Schuster [:evilpie] from comment #67)
> This is looking good. Are you waiting for review or is something else
> blocking this?

Tom: Thanks! I'm awaiting review.

Comment 69

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 746304 [details] [diff] [review]
adding a counter of found matches to the find in page bar

Review of attachment 746304 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/components/typeaheadfind/nsITypeAheadFind.idl
@@ +63,5 @@
>                                          // Most recent elem found, if a link
>    readonly attribute nsIDOMElement foundEditable;
>                                          // Most recent elem found, if editable
> +  readonly attribute nsIDOMRange foundRange;
> +                                        // Range of most recent match

Hmm, I think this needs to be a method that returns a clone of the nsIDOMRange - otherwise the range is going to be able to be modified, which will lead to unexpected results. (Making it a method in the IDL forces JS code to actually use it as a clone, rather than accidentally re-cloning multiple times.)

::: toolkit/content/widgets/findbar.xml
@@ +977,5 @@
> +      </method>
> +
> +      <!-- the maximum number of matches shown (for speed reasons) -->
> +      <field name="_matchLimit">100</field>
> +      <property name="matchLimit" onget="return this._matchLimit;">

Neither getter or setter for this is doing any processing, so you don't really need a property - just the field.

@@ +1002,5 @@
> +
> +          var countFound = 0;
> +          var countCurrent = 0;
> +          for (var i = 0, count; win.frames && i < win.frames.length; i++) {
> +            if ((count = this._countMatches(aWord, win.frames[i])) != -1)

Shouldn't this be checking if the frame is hidden?

@@ +1094,5 @@
> +              }
> +              else {
> +                this._foundMatches.value = this.strBundle.formatStringFromName
> +                       ("FoundMatchesCount", [matchesCount.current.toString(),
> +                       matchesCount.total.toString()], 2);

Don't need toString() here - numbers are automatically coerced to strings.

@@ +1103,5 @@
> +              this._foundMatches.hidden = true;
> +              this._foundMatches.value = "";
> +            }
> +
> +            window.clearTimeout(this._updateMatchCountTimeout);

Doesn't seem like doing this should be conditional. Also, set _updateMatchCountTimeout to null after clearing the timeout, so the check in _updateMatchesCount works.

@@ +1121,5 @@
> +            window.clearTimeout(this._updateMatchCountTimeout);
> +          this._updateMatchCountTimeout =
> +            window.setTimeout(function(aRes, aSelf) {
> +              aSelf._updateMatchesCountWorker(aRes);
> +            }, 0, aRes, this);

Can just bind() this:
  window.setTimeout(this._updateMatchesCountWorker.bind(this), 0, aRes);

Also, an earlier version of this patch delayed this by 500ms, which seems like a good idea - it'd reduce the amount of unnecessary processing done while text is being typed in.

::: toolkit/locales/en-US/chrome/global/findbar.properties
@@ +11,5 @@
>  FastFindLinks=Quick Find (links only)
>  CaseSensitive=(Case sensitive)
> +FoundMatchCount=%S of %S match
> +FoundMatchesCount=%S of %S matches
> +FoundTooManyMatches=More than %S matches
\ No newline at end of file

You need to change the string IDs for these, otherwise the string changes won't get picked up by all localizers.

Also, these should be using plural form. See https://developer.mozilla.org/en-US/docs/Localization_and_Plurals

FoundMatchCount/FoundMatchesCount not using plural form is an existing bug (mentioned in comment 25, couldn't find a separate bug for it). Should fix that here, since you're changing the strings.

And FoundTooManyMatches will also need to be in plural form.

Comment 71

[Mike de Boer [:mikedeboer]]

Attachment: Adding a counter of found matches to the find in page bar

Addressed review comments and added tests.

Comment 72

[Mike de Boer [:mikedeboer]]

(In reply to Blair McBride [:Unfocused] from comment #69)
> 
> Hmm, I think this needs to be a method that returns a clone of the
> nsIDOMRange - otherwise the range is going to be able to be modified, which
> will lead to unexpected results. (Making it a method in the IDL forces JS
> code to actually use it as a clone, rather than accidentally re-cloning
> multiple times.)

Instead of making it a method, I updated the code itself to always clone the `returnRange` when `mFoundRange` is set (since it's a new pointer, it needs to be addref'ed).
This will make the implementation safe as it always returns a clone.

> 
> Doesn't seem like doing this should be conditional. 

I think you mean UNconditional? There is no escape-hatch (`return;`) in `_countMatches`, so it'll always clear the timeout. I did implement null-ing it.

> 
> Also, an earlier version of this patch delayed this by 500ms, which seems
> like a good idea - it'd reduce the amount of unnecessary processing done
> while text is being typed in.

That's indeed better. I set the timeout to 100ms, because 500ms seemed too arbitrary to me. Not sayin' 100ms is less arbitrary, but this makes sure that the lag between a typing-stop and counter update is as minimal as possible, while avoiding unnecessary processing. I felt that 500ms was just way too much.

> 
> And FoundTooManyMatches will also need to be in plural form.

I don't understand why `FoundTooManyMatches` needs to be in plural form; it'll never be anything else than plural. If there is a language that doesn't do plurals, than this string only having one form will cover the case. The number that will be put in place of '%S' is just for decoration purposes.

Thanks for the review! I've got a fresh one lined up for you! ;)

Comment 73

[Blair McBride [:Unfocused] (UNAVAILABLE)]

(In reply to Mike de Boer [:mikedeboer] from comment #72)
> (In reply to Blair McBride [:Unfocused] from comment #69)
> > 
> > Hmm, I think this needs to be a method that returns a clone of the
> > nsIDOMRange - otherwise the range is going to be able to be modified, which
> > will lead to unexpected results. (Making it a method in the IDL forces JS
> > code to actually use it as a clone, rather than accidentally re-cloning
> > multiple times.)
> 
> Instead of making it a method, I updated the code itself to always clone the
> `returnRange` when `mFoundRange` is set (since it's a new pointer, it needs
> to be addref'ed).
> This will make the implementation safe as it always returns a clone.

Still not safe, IMO. If any code modifies the range returned by the foundRange property, it still modifies mFoundRange. Then if any other code accesses the foundRange property, it gets the modified range (not the range originally found by FindItNow). So the cloning needs to happen every time that function is called. 

Then if that's happening, we should try to make it obvious what the following code is doing (horribly contrived example follows):

  for (let i = 0; i < 10; i++) {
    do_something(i, find.foundRange); // Non-obvious: clones every time this is accessed
  }

vs:

  let range = find.getFoundRange(); // More obvious: clones every time this is called
  for (let i = 0; i < 10; i++) {
    do_something(i, range); 
  }


> > Doesn't seem like doing this should be conditional. 
> 
> I think you mean UNconditional? There is no escape-hatch (`return;`) in
> `_countMatches`, so it'll always clear the timeout. I did implement null-ing
> it.

No, I meant it shouldn't be conditional :) ATM it won't get cleared if there's no match - IMO it should be cleared no matter what.

> 
> > 
> > Also, an earlier version of this patch delayed this by 500ms, which seems
> > like a good idea - it'd reduce the amount of unnecessary processing done
> > while text is being typed in.
> 
> That's indeed better. I set the timeout to 100ms, because 500ms seemed too
> arbitrary to me. Not sayin' 100ms is less arbitrary, but this makes sure
> that the lag between a typing-stop and counter update is as minimal as
> possible, while avoiding unnecessary processing. I felt that 500ms was just
> way too much.

Yep, fair. Should probably make that a (memorized) pref too.



> > And FoundTooManyMatches will also need to be in plural form.
> 
> I don't understand why `FoundTooManyMatches` needs to be in plural form;
> it'll never be anything else than plural. If there is a language that
> doesn't do plurals, than this string only having one form will cover the
> case. The number that will be put in place of '%S' is just for decoration
> purposes.

"20 matches" and "21 matches" do not use the same plural rules in all languages. For instance, some languages special case 21, 31, 41, etc. https://developer.mozilla.org/en-US/docs/Localization_and_Plurals#List_of_Plural_Rules has an extensive list.

So basically: If the string deals with anything but the singular, it has to use plural form.

Comment 74

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to Blair McBride [:Unfocused] from comment #73)
> (In reply to Mike de Boer [:mikedeboer] from comment #72)
> > (In reply to Blair McBride [:Unfocused] from comment #69)
> > > Also, an earlier version of this patch delayed this by 500ms, which seems
> > > like a good idea - it'd reduce the amount of unnecessary processing done
> > > while text is being typed in.
> > 
> > That's indeed better. I set the timeout to 100ms, because 500ms seemed too
> > arbitrary to me. Not sayin' 100ms is less arbitrary, but this makes sure
> > that the lag between a typing-stop and counter update is as minimal as
> > possible, while avoiding unnecessary processing. I felt that 500ms was just
> > way too much.
> 
> Yep, fair. Should probably make that a (memorized) pref too.

According to the research discussed at http://imlocation.wordpress.com/2007/12/05/how-fast-do-people-type/, the average typing speed is 38wpm, which comes to 228 characters per minute. This would translate to 3.8 characters per second, or about 263 milliseconds per character. Based on this, I think we should settle at 250ms, so that users can see it update per keystroke, but also reduce the amount of work that the browser will have to do when the user types two consecutive characters in quick succession.

Comment 75

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 774625 [details] [diff] [review]
Adding a counter of found matches to the find in page bar

Review of attachment 774625 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/content/widgets/findbar.xml
@@ +983,5 @@
> +      <property name="pluralForm">
> +        <getter><![CDATA[
> +          if (!this._pluralForm) {
> +            this._pluralForm = Components.utils.import(
> +                               "resource://gre/modules/PluralForm.jsm").PluralForm;

Careful here - this will actually import it into the global scope, unless you pass a scope object as the second argument.

@@ +1009,5 @@
> +          let countFound = 0;
> +          let countCurrent = 0;
> +          for (let i = 0, count; win.frames && i < win.frames.length; i++) {
> +            // Don't count matches in hidden frames.
> +            if (!win.frames[i] || win.frames[i].hidden)

.hidden isn't that great a check, as evidenced by _rangeIsVisible - the frame visibility check here should be doing what the range visibility function does.

@@ +1129,5 @@
> +          }
> +          this._updateMatchCountTimeout =
> +            window.setTimeout(function(aRes) {
> +              this._updateMatchesCountWorker(aRes);
> +            }.bind(this, aRes), 100);

Why not just |window.setTimeout(this._updateMatchesCountWorker.bind(this), 100, aRes);| like I mentioned in comment 69?

::: toolkit/locales/en-US/chrome/global/findbar.properties
@@ +12,5 @@
>  CaseSensitive=(Case sensitive)
> +# LOCALIZATION NOTE (FoundMatches): Semicolon-separated list of plural forms.
> +# #1 is FoundMatchesPair, where %S is substituted by a number.
> +FoundMatches=#1 match;#1 matches
> +FoundMatchesPair=%S of %S

It's not safe to assume this will work in all locales - just use one plural form string (#1 of #2 match;#1 of #2 matches)

Comment 76

[Blair McBride [:Unfocused] (UNAVAILABLE)]

(In reply to Jared Wein [:jaws] from comment #74)
> According to the research discussed at
> http://imlocation.wordpress.com/2007/12/05/how-fast-do-people-type/, the
> average typing speed is 38wpm, which comes to 228 characters per minute.
> This would translate to 3.8 characters per second, or about 263 milliseconds
> per character. Based on this, I think we should settle at 250ms, so that
> users can see it update per keystroke, but also reduce the amount of work
> that the browser will have to do when the user types two consecutive
> characters in quick succession.

Data is nice :) But I do expect it'll need tweaking after it lands anyway, since its a tradeoff.

Comment 77

[Mike de Boer [:mikedeboer]]

Attachment: Patch v3: add a counter of found matches to the find in page bar

Gabor, I'm asking you for feedback, because I know you're familiar with XPCOM and related things... :)
I've added a GetFoundRange method to the nsITypeAheadFind idl and cpp. It works, but when I run the unit test (./mach mochitest-chrome toolkit/content/tests/chrome/test_findbar.xul) I think it crashes Fx and reports leaks. I've tried many things, but in the end my knowledge of C++ falls short. Please help?

Comment 78

[Gabor Krizsanits (INACTIVE)]

Comment on attachment 788133 [details] [diff] [review]
Patch v3: add a counter of found matches to the find in page bar

Review of attachment 788133 [details] [diff] [review]:
-----------------------------------------------------------------

It's hard to tell without a call stack, but I see 2 big issues here:

+NS_IMETHODIMP
+nsTypeAheadFind::GetFoundRange(nsIDOMRange** aFoundRange)
+{
+  NS_ENSURE_ARG_POINTER(aFoundRange);
+  if (mFoundRange == nullptr) {
+    aFoundRange = nullptr;

You probably wanted to write: *aFoundRange = nullptr;

+  } else {
+    mFoundRange->CloneRange(aFoundRange);
+    NS_IF_ADDREF(*aFoundRange);

You don't need to addref here, according to the COM rules these type of getters have to do the addref for you.
If that weren't the case CloneRange must have been fixed. Also, try avoid using NS_IF_ADDREF, and stick to smart
pointers instead like nsCOMPtr and nsRefPtr.

I hope that helps, if it does not solve your problem please get a call stack and I'll try to help you tracing the issue.
(I have not yet looked into the cycle collector part, I'm not an expert in that field...)

Comment 79

[Mike de Boer [:mikedeboer]]

Attachment: Patch v4: add a counter of found matches to the find in page bar

OK, you made me think in the right direction... I hope ;)

With the implementation I uploaded here there's no leaks, crashes or other weirdness!

I'd love to stear away form NS_IF_ADDREF, but since nsIDOMRange::CloneRange() likes nsCOMPtr<nsIDOMRange> instances (wrapped) I decided to go create an instance for it, pass that along to CloneRange() and after the cloning is done, unwrap it by means of statically casting it to an nsIDOMRange.

After that I still do NS_IF_ADDREF, similar to all the other methods in this class.

How does this look?

Comment 80

[Gabor Krizsanits (INACTIVE)]

Comment on attachment 788234 [details] [diff] [review]
Patch v4: add a counter of found matches to the find in page bar

Review of attachment 788234 [details] [diff] [review]:
-----------------------------------------------------------------

Closer... good news: this works! more good news: it can be done simpler!

+  *aFoundRange = static_cast<nsIDOMRange*>(returnRange);
+  NS_IF_ADDREF(*aFoundRange);

returnRange.forget(aFoundRange);

This line sets aFoundRange, and tells returnRange to null out its pointer
without dropping its refcount, your version works too, but this is the preferred
way these days...

Comment 81

[Gabor Krizsanits (INACTIVE)]

But actually you don't need all these here.

+  nsCOMPtr<nsIDOMRange> returnRange;
+  returnRange = nullptr;
+  mFoundRange->CloneRange(getter_AddRefs(returnRange));
+  *aFoundRange = static_cast<nsIDOMRange*>(returnRange);
+  NS_IF_ADDREF(*aFoundRange);
+  return NS_OK;

mFoundRange->CloneRange(aFoundRange);
return NS_OK;

Is all you really need.

and in general you never need to null nsCOMPtr to initialize them like this:
+  returnRange = nullptr;

Comment 82

[Mike de Boer [:mikedeboer]]

(In reply to Gabor Krizsanits [:krizsa :gabor] (moving to Berlin: in and out for a few weeks) from comment #81)
> mFoundRange->CloneRange(aFoundRange);
> return NS_OK;
> 
> Is all you really need.
> 
> and in general you never need to null nsCOMPtr to initialize them like this:
> +  returnRange = nullptr;

By Jove, you're right! This is in fact turning out to be a rather elegant implementation! I owe you many thanks, mate! :)

Comment 83

[Mike de Boer [:mikedeboer]]

Attachment: Patch v5: add a counter of found matches to the find in page bar

Comment 84

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 788359 [details] [diff] [review]
Patch v5: add a counter of found matches to the find in page bar

Review of attachment 788359 [details] [diff] [review]:
-----------------------------------------------------------------

r- just because I think you missed something from a previous review. If that's not the case, then r+ with the following small things fixed.

::: toolkit/components/typeaheadfind/nsITypeAheadFind.idl
@@ +36,5 @@
>  
>    /* Find another match in the page. */
>    unsigned short findAgain(in boolean findBackwards, in boolean aLinksOnly);
>  
> +  /* Return a clone of the range of most recent match. */

Nit: That it's a clone is an implementation detail.

::: toolkit/components/typeaheadfind/nsTypeAheadFind.cpp
@@ +67,3 @@
>                             mCurrentWindow, mStartFindRange, mSearchRange,
>                             mStartPointRange, mEndPointRange, mSoundInterface,
> +                           mFind, mFoundRange)

Nit: Fix indentation.

::: toolkit/content/widgets/findbar.xml
@@ +369,5 @@
>            prefsvc.getIntPref("accessibility.typeaheadfind.timeout");
>          this._flashFindBar =
>            prefsvc.getIntPref("accessibility.typeaheadfind.flashBar");
> +        this._matchCountTimeoutLength =
> +          prefsvc.getIntPref("accessibility.typeaheadfind.matchCountTimeout");

Surprise, you just broke every non-Firefox app using this binding. Either set this in /modules/libpref/src/init/all.js (preferred) or wrap this call in a try/catch with a sensible hard-coded default.

@@ +1031,5 @@
> +          let countFound = 0;
> +          let countCurrent = 0;
> +          for (let i = 0, count; win.frames && i < win.frames.length; i++) {
> +            // Don't count matches in hidden frames.
> +            if (!win.frames[i] || win.frames[i].hidden)

Missed this from the last review? Or are you intentionally leaving it like this?

@@ +1149,5 @@
> +            window.clearTimeout(this._updateMatchCountTimeout);
> +            this._updateMatchCountTimeout = null;
> +          }
> +          this._updateMatchCountTimeout =
> +            window.setTimeout(aRes => this._updateMatchesCountWorker(aRes),

Er, this doesn't seem right. aRes will get bound to the first argument setTimeout gives to the callback function. If it's working as expected right now, it's by accident :) Can't rely on that behaviour. It should be:

  window.setTimeout(() => this._updateMatchesCountWorker(aRes),

::: toolkit/locales/en-US/chrome/global/findbar.properties
@@ +14,5 @@
> +# #1 is currently selected match and #2 the total amount of matches.
> +FoundMatches=#1 of #2 match;#1 of #2 matches
> +# LOCALIZATION NOTE (FoundTooManyMatches): Semicolon-separated list of plural
> +# forms. #1 is the total amount of matches allowed before counting stops.
> +FoundTooManyMatches=More than #1 matches;More than #1 matches

Not expected to be used, but technically still possible to hit the singular (plus we don't want to confuse localizers with a poor example of English). "More than #1 match;More than #1 matches"

Comment 85

[Mike de Boer [:mikedeboer]]

(In reply to Blair McBride [:Unfocused] from comment #84)
> Surprise, you just broke every non-Firefox app using this binding. Either
> set this in /modules/libpref/src/init/all.js (preferred) or wrap this call
> in a try/catch with a sensible hard-coded default.

Yikes, will do!

> Missed this from the last review? Or are you intentionally leaving it like
> this?

So sorry, I missed it while addressing/ implementing your comments :S

> Er, this doesn't seem right. aRes will get bound to the first argument
> setTimeout gives to the callback function. If it's working as expected right
> now, it's by accident :) Can't rely on that behaviour. It should be:
> 
>   window.setTimeout(() => this._updateMatchesCountWorker(aRes),

Whoopsie! ^-- that's what I meant to write.


Thanks! I WILL address all your comments next time :)

Comment 86

[Mike de Boer [:mikedeboer]]

Attachment: Patch v6: add a counter of found matches to the find in page bar

This one *should* have all your comments addressed, sir!

Comment 87

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 789499 [details] [diff] [review]
Patch v6: add a counter of found matches to the find in page bar

Review of attachment 789499 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/content/widgets/findbar.xml
@@ +995,5 @@
> +
> +          // There is no perfect way to check if a node is visible in JavaScript,
> +          // so use the best measures we can have
> +          if (aNode) {
> +            aWindow = aWindow || aNode.ownerDocument.defaultView;

Hm, just built this revision (Win8, UX tree), and am getting the following error here:

[13:04:42.092] TypeError: aNode.ownerDocument is undefined @ chrome://global/content/bindings/findbar.xml:975

(Tested on http://edition.cnn.com/)

Comment 88

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

Comment on attachment 789499 [details] [diff] [review]
Patch v6: add a counter of found matches to the find in page bar

>diff --git a/toolkit/content/widgets/findbar.xml b/toolkit/content/widgets/findbar.xml

>+      <method name="_nodeIsVisible">

>+          // There is no perfect way to check if a node is visible in JavaScript,
>+          // so use the best measures we can have
>+          if (aNode) {
>+            aWindow = aWindow || aNode.ownerDocument.defaultView;

Beyond just needing to handle a null ownerDocument, this code seems quite error-prone. There has to be some prior art, here. It also feels like we could delegate to a core helper function if necessary (nsTypeAheadFind::IsRangeVisible() does similar checks that seem more robust).

Comment 89

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 789499 [details] [diff] [review]
Patch v6: add a counter of found matches to the find in page bar

Review of attachment 789499 [details] [diff] [review]:
-----------------------------------------------------------------

As per comment 87 and comment 88.

Comment 90

[Mike de Boer [:mikedeboer]]

Gavin, might it be an option to roll with the current - less optimal - JS version of `_nodeIsVisible` and get to a C++ version in a follow-up bug?
I'm suggesting this, because the thread on dev-platform - https://groups.google.com/forum/#!topic/mozilla.dev.platform/y4QjMBKqe1s - did not yield a notion of prior art we could use here.

Comment 91

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

(In reply to Mike de Boer [:mikedeboer] from comment #90)
> Gavin, might it be an option to roll with the current - less optimal - JS
> version of `_nodeIsVisible` and get to a C++ version in a follow-up bug?
> I'm suggesting this, because the thread on dev-platform -
> https://groups.google.com/forum/#!topic/mozilla.dev.platform/y4QjMBKqe1s -
> did not yield a notion of prior art we could use here.

Is it functionally equivalent to the current code?

My takeaway from that thread is that we should just script-expose the existing nsTypeAheadFind::IsRangeVisible somehow. Adding to e.g. nsIDomWindowUtils shouldn't be too complicated. Can happen in a followup, I guess, assuming we're not regressing behavior here.

Comment 92

[Mike de Boer [:mikedeboer]]

(In reply to :Gavin Sharp (use [e-mail] for email) from comment #91)
> Is it functionally equivalent to the current code?
> 
> My takeaway from that thread is that we should just script-expose the
> existing nsTypeAheadFind::IsRangeVisible somehow.

Ah! Well, that part I did already. I thought you wanted nsIDomWindowUtils::IsNodeVisible too, for the case where I check the visibility of a frame.
One thing that bothered me a bit; now I have a function which is called like `fastFind.isRangeVisible(aRange)`, which throws assertion failures when the active context in the nsITypeAheadFind class is different than the docShell context to which the range belongs (like when the range is in an (i)frame). So I also made a version with the signature `fastFind.isRangeVisible(aRange, aDocShell)`, and get the docShell of a range first via `range.startContainer.contentWindow.docShell`. But this throws an assertion failure for EVERY range I pass to it! I did not understand, so I threw it away.

Assertions from console:

###!!! ASSERTION: prescontext mismatch?: 'aFrame->PresContext() == GetPresContext()', file /Users/mdeboer/Projects/mozilla-central/layout/base/nsPresShell.cpp, line 3349
###!!! ASSERTION: GetOffsetTo called on frames in different documents: 'PresContext() == aOther->PresContext()', file /Users/mdeboer/Projects/mozilla-central/layout/generic/nsFrame.cpp, line 4342

Do you know what's going on here by any chance?

> Adding to e.g.
> nsIDomWindowUtils shouldn't be too complicated. Can happen in a followup, I
> guess, assuming we're not regressing behavior here.

Very true, but I guess I need to understand why passing a docShell throws these assertions that the context is different/ wrong.

Comment 93

[Mike de Boer [:mikedeboer]]

Attachment: IsRangeVisible WIP patch

Comment 94

[Mike de Boer [:mikedeboer]]

Nvmd, found it! \o/

Comment 95

[Mike de Boer [:mikedeboer]]

Attachment: Patch v7: add a counter of found matches to the find in page bar

Comment 96

[Luís Miguel [:quicksaver] (not reading bugmail)]

I have a question/suggestion. After looking at the code I see that the matchLimit value is hardcoded. When this value is modified (for any reason; I would advocate this be made a preference in about:config as well but it's not as critical), when "matchLimit == 0" it would act as a turn off switch for the counter, in this case not showing it at all instead of the "FoundTooManyMatches" string.

Or as an alternative, at least some form of on/off switch to this feature would be good. I was thinking specifically in an event, much like the one for _find(): "if(!this._dispatchFindEvent('counter')) return;" where if something preventDefault()'ed that event the method would return, so the counter wouldn't be used/updated.

I ask only because in my add-on the match counter is already a feature, and as everything else in it, it is optional. The only way that I see, as it is now, to turn it off would be to modify the existing code (hiding the counter by CSS would still cause it to calculate everything, and changing the matchLimit value to 0 would still show the "FoundTooManyMatches" string). Of course, if modifying the code is the only way to go for an add-on to change this behavior then so be it, but I try to avoid these cases as much as possible, so I thought I'd ask.

Comment 97

[Mike de Boer [:mikedeboer]]

Attachment: e10s compat WIP

Comment 98

[Art K]

I am not clear what the discussion or necessity regarding visibility is about but I am concerned that it implies complications far beyond the original bug request.

From a user's viewpoint the "find count" simply means the number of ocurrences of the search string in the entire current page/tab - and only in the current page/tab - regardless of whether it is visible or not. IOW, the same number I would get if I copied the rendered page HTML into a text editor and searched there. Please don't make it any more complicated than that.

Comment 99

[Mike de Boer [:mikedeboer]]

(In reply to Art K from comment #98)
> I am not clear what the discussion or necessity regarding visibility is
> about but I am concerned that it implies complications far beyond the
> original bug request.
> 
> From a user's viewpoint the "find count" simply means the number of
> ocurrences of the search string in the entire current page/tab - and only in
> the current page/tab - regardless of whether it is visible or not. IOW, the
> same number I would get if I copied the rendered page HTML into a text
> editor and searched there. Please don't make it any more complicated than
> that.

Hi Art, thanks for your comment! Please be assured that we're doing all we can here to match the users' expectations here and nothing more complicated than that.

The thing that is important here is that the total amount of matches is exactly the same as when you'd turn 'Highlight All' on and count all the highlighted words in the page by hand.

Things that may seem simple on the surface may be very complex beneath it; one might pick an iceberg as a good metaphor. What the find code does for 'Highlight All' across various layers of code is
1) go through the DOM in a smart way to find all text excerpts that contain the text that is searched for
2) check for obvious reasons why the found text ranges may not be visible
3) ask the graphics layer that's in charge of drawing the text on your screen if it has drawn it
4) yield the results
5) draw 'nicely' colored rectangles around the found text ranges

All this is done is such a way that any overhead can and will be eliminated to keep the feature as responsive as possible.

The steps I mentioned above apply to the 'Highlight All' feature, which is not enabled by default. This feature will be and it uses (roughly) steps 1-4. But instead of drawing colored rectangles it will count the results and keep track at which point you are while cycling through them.

Comment 100

[Luís Miguel [:quicksaver] (not reading bugmail)]

Comment 98 from Art K reminded me of something that I'm not sure is being considered (I barely have any knowledge of C++ so this may have been already taken into account, I'm sorry if that is the case).

Firefox does account for a few invisible results sometimes. For example, at http://www.google.com/, if I search for "f" and cycle through the results using F3, I have at least two instances where the range isn't actually visible on screen (no text selected anywhere) but firefox still counts them as valid results and "cycles to" them. FindBar Tweak's counter/sights features are useful for this if you'd like to confirm, I have five "f"'s in that page, reported by firefox's native search (nsIFind), where matches number 1 and 2 aren't visible (they don't produce a sight when I cycle to them) but are still "accessible".

Whether this is unwanted behavior that needs to be fixed (in another bug of course) or not, IMO this bug's counter should never differ in the number of results from what nsITypeAheadFind/nsIFind instances report. As Art K said, this would at best be confusing for the user, at worst be considered faulty behavior in a browser (as a reviewer, I'd see a browser reporting two different results for the same operation).

Mike, you posted comment 99 as I was typing, so let me comment on it as well. The results from "Highlight All" (which come from an nsIFind instance) seem to always be the same as those that come from fastFind (nsITypeAheadFind), but actual visibility doesn't seem to be taken into account by either of them sometimes, as evidenced by that example on the google page. If you say that this counter will report the same as what nsIFind reports, then the extra "isRangeVisibile()" calls on the results returned from nsIFind will make it differ for sure, as it will be the only thing that uses these "visibility filters".

Again, I'm sorry if this is already being considered in the current code.

Comment 101

[Mike de Boer [:mikedeboer]]

Attachment: Patch v8: add a counter of found matches to the find in page bar

Rebased patch on top of work done in bug 666816. Introduced event to cancel counting of matches (by add-ons, for example). `matchLimit` is now a pref.

Comment 102

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 806944 [details] [diff] [review]
Patch v8: add a counter of found matches to the find in page bar

Review of attachment 806944 [details] [diff] [review]:
-----------------------------------------------------------------

I assume adding this functionality to RemoteFinder will be done in a followup bug?

::: toolkit/content/widgets/findbar.xml
@@ +440,5 @@
>          ]]></body>
>        </method>
>  
> +      <!-- the maximum number of matches shown (for speed reasons) -->
> +      <field name="_pluralForm">null</field>

Er, this comment doesn't exactly match any more....

@@ +472,5 @@
> +        - Updates the search match count after each find operation on a new string.
> +        - @param aRes
> +        -        the result of the find operation
> +        -->
> +      <method name="_updateMatchesCount">

Should short-circuit this method if _matchesCountLimit == 0, to avoid doing unneeded work.

::: toolkit/modules/Finder.jsm
@@ +147,5 @@
>  
> +  requestMatchesCount: function(aWord, aMatchLimit, aLinksOnly) {
> +    let result = this._countMatchesInWindow(aWord, aMatchLimit, aLinksOnly, this._getWindow());
> +    for (let l of this._listeners)
> +      l.onMatchesCountResult(result);

Should have a try/catch every time we call a listener's method (I consider it a bug that _notify doesn't do this).

@@ +161,5 @@
> +   * @param aLinksOnly
> +   *        whether we should only search through links.
> +   * @param aWindow
> +   *        the window to search in. Passing undefined will search the
> +   *        current content window. Optional.

Really? Cos at the moment it looks like it would break ;)

@@ +167,5 @@
> +   *        the Object that is returned by this function. It may be passed as an
> +   *        argument here in the case of a recursive call.
> +   * @returns an object stating the number of matches and a vector for the current match.
> +   */
> +  _countMatchesInWindow: function(aWord, aMatchLimit, aLinksOnly, aWindow, aStats) {

This seems to traverse the DOM differently - the current index jumps around when hitting "Next", instead of reliably incrementing by one each time. Tested on cnn.com, which has a bunch of iframes.

@@ +174,5 @@
> +      current: 0,
> +      currentFound: false
> +    };
> +
> +    for (let i = 0, count; aWindow.frames && i < aWindow.frames.length; i++) {

Ew. for-of loop please.

@@ +239,5 @@
> +        ++aStats.total;
> +        if (!aStats.currentFound) {
> +          ++aStats.current;
> +          if (isSameRange)
> +            aStats.currentFound = true;

currentFound doesn't seem useful to expose via the API.

@@ +541,5 @@
> +    var node = aRange.startContainer;
> +
> +    if (node.nodeType == node.ELEMENT_NODE) {
> +      if (node.hasChildNodes) {
> +        var childNode = node.childNodes[aRange.startOffset];

Use |.childNodes.item()| , it's safer (it'll return null instead of throwing if something goes wrong).

@@ +573,5 @@
> +   * @param aWindow
> +   *        the window to check the visibility in. Optional.
> +   * @returns true if the node is visible
> +   */
> +  _nodeIsVisible: function(aNode, aWindow) {

Should be able to re-use the mechanics of isRangeVisisble() to do this - either by re-factoring that method to allow adding a nsITypeAheadFind.isNodeVisible(), or by constructing a nsIRange here that only covers this one node.

Comment 103

[Luís Miguel [:quicksaver] (not reading bugmail)]

If I can ask a question in Finder.jsm line 250-251:
>      startPt = doc.createRange();
>      startPt.setStart(retRange.startContainer, retRange.startOffset + 1);

You're just collapsing the range to the start of the container to plus one character of what it was. Basically: "abcd" -> [cursor here]"bcd". Wouldn't it be easier on nsIFind to collapse it to the end of the range, like it does currently in Finder._highlight()? Since there won't be any matches in that specific range for sure because it was cropped like that, it's useless for nsIFind to start there, or am I wrong?

Also a couple of comments on these points from Blair's review:
> @@ +167,5 @@
> > +   *        the Object that is returned by this function. It may be passed as an
> > +   *        argument here in the case of a recursive call.
> > +   * @returns an object stating the number of matches and a vector for the current match.
> > +   */
> > +  _countMatchesInWindow: function(aWord, aMatchLimit, aLinksOnly, aWindow, aStats) {
> 
> This seems to traverse the DOM differently - the current index jumps around
> when hitting "Next", instead of reliably incrementing by one each time.
> Tested on cnn.com, which has a bunch of iframes.

I think it should count matches in frames only after it has counted matches in the main document, and not before like it does now. nsITypeAheadFind seems to always start there (the first matches to have focus seem to always be in the main window when there are any there) so they should be given a lower index in the counter.

> Should be able to re-use the mechanics of isRangeVisisble() to do this -
> either by re-factoring that method to allow adding a
> nsITypeAheadFind.isNodeVisible(), or by constructing a nsIRange here that
> only covers this one node.

I won't press on this issue any further, as I'm not trying to be stubborn or anything, but I still honestly believe my comment 100 is still valid here, found ranges visibility should not be taken into account only for this counter, but for everything that uses the nsIFind/nsITypeAheadFind interfaces.

Comment 104

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Attachment: Screenshot 1 - reached end of page

Polish bug: This is what it looks like when searching past the end of the page. Feels like there should be something more separating the two messages - a full-stop, or more white space, or something.

Comment 105

[Blair McBride [:Unfocused] (UNAVAILABLE)]

(In reply to Luís Miguel [:Quicksaver] from comment #103)
> I won't press on this issue any further, as I'm not trying to be stubborn or
> anything, but I still honestly believe my comment 100 is still valid here,
> found ranges visibility should not be taken into account only for this
> counter, but for everything that uses the nsIFind/nsITypeAheadFind
> interfaces.

Agreed, FWIW - they should be the same. And in some cases it is... but our find implementation is far from bug-free :\ But IMO that's a separate bug.

Comment 106

[Luís Miguel [:quicksaver] (not reading bugmail)]

(In reply to Blair McBride [:Unfocused] from comment #104)
> Created attachment 809098 [details]
> Screenshot 1 - reached end of page
> 
> Polish bug: This is what it looks like when searching past the end of the
> page. Feels like there should be something more separating the two messages
> - a full-stop, or more white space, or something.

Suggestion: the match counter already implies this "Reached end of page" information, so it really won't be necessary anymore IMO; it could just be removed/not shown when the counter is used.

(In reply to Blair McBride [:Unfocused] from comment #105)
> (In reply to Luís Miguel [:Quicksaver] from comment #103)
> Agreed, FWIW - they should be the same. And in some cases it is... but our
> find implementation is far from bug-free :\ But IMO that's a separate bug.

That's precisely my point, it should be a separate bug. The counter should show the same results as everything else that uses nsIFind/nsITypeAheadFind, and the visibility filters should be applied on top of it to everything in a separate bug; or vice versa, the order of what happens first doesn't really matter.

Of course, you could make the case that this patch includes a partial fix for these visibility issues already, and in the future you wouldn't be "fixing the overall find results visibility issues", but rather "synchronizing the results from the counter and everything else". But I think that applying these visibility filters here to the counter only would make the find operation more confusing (for both users and developers IMO) in the meantime.

Comment 107

[Luís Miguel [:quicksaver] (not reading bugmail)]

I think if the patch in here is eventually to work for both desktop and metro modes (I'm not sure, so needinfo'ing you Mike), then bug 924149 should be marked as a duplicate, not as depending on here.

Comment 108

[MAx]

-Ehm- Excuse me, usually i'm not a polemist, but it's almost TEN YEARS since it has been opened, as per Reported Date in the header of the present page... 

Hope we'll see a solution soon! :-)

Kind regards,
MAx - Italy

Comment 110

[Mike de Boer [:mikedeboer]]

Attachment: Patch v9: add a counter of found matches to the find in page bar

Comment 111

[Mike de Boer [:mikedeboer]]

Attachment: Patch v9: add a counter of found matches to the find in page bar

Unbitrotted, for your convenience.

Comment 112

[(Currently slow to respond) Philipp Sackl [:phlsa] (Firefox UX) please use needinfo]

The whiteboard here states that this needs UI work. Is that still the case?
The screenshot from Unfocused looks good to me!

Comment 113

[Mike de Boer [:mikedeboer]]

(In reply to Philipp Sackl [:phlsa] (on PTO Apr 16-23) from comment #112)
> The whiteboard here states that this needs UI work. Is that still the case?
> The screenshot from Unfocused looks good to me!

Apparently not! :)

Comment 114

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 8407420 [details] [diff] [review]
Patch v9: add a counter of found matches to the find in page bar

Review of attachment 8407420 [details] [diff] [review]:
-----------------------------------------------------------------

Ship it!

::: toolkit/locales/en-US/chrome/global/findbar.properties
@@ +14,5 @@
> +# #1 is currently selected match and #2 the total amount of matches.
> +FoundMatches=#1 of #2 match;#1 of #2 matches
> +# LOCALIZATION NOTE (FoundTooManyMatches): Semicolon-separated list of plural
> +# forms. #1 is the total amount of matches allowed before counting stops.
> +FoundTooManyMatches=More than #1 match;More than #1 matches

I found out recently that some localisation tools rely on the comments for pluralform strings to be very specifically formatted - and it's expected to include the MDN documentation link. eg:
http://dxr.mozilla.org/mozilla-central/source/browser/locales/en-US/chrome/browser/browser.properties#87

::: toolkit/modules/Finder.jsm
@@ +299,5 @@
> +   *        the Object that is returned by this function. It may be passed as an
> +   *        argument here in the case of a recursive call.
> +   * @returns an object stating the number of matches and a vector for the current match.
> +   */
> +  _countMatchesInWindow: function(aWord, aMatchLimit, aLinksOnly, aWindow, aStats) {

Nit: I think we've had default params introduced since the past patch revision - it's a good habit to get into specifying the default as null for optional params, so the function signature becomes self-documenting in that way.

@@ +342,5 @@
> +
> +  /**
> +   * Basic wrapper around nsIFind that provides invoking a callback `aOnFind`
> +   * each time an occurence of `aWord` string is found.
> +   * 

Nit: HORROR OF HORRORS, A TRAILING WHITESPACE! KILL IT! KILL IT WITH FIRE!!!!!

Comment 115

[Mike de Boer [:mikedeboer]]

oboyoboyoboy, I just landed this puppy as https://hg.mozilla.org/integration/fx-team/rev/8d131b6770d7

I hope it sticks!

Comment 116

[Mike de Boer [:mikedeboer]]

(In reply to Blair McBride [:Unfocused] from comment #114)
> Nit: HORROR OF HORRORS, A TRAILING WHITESPACE! KILL IT! KILL IT WITH
> FIRE!!!!!

:)) I triple-checked the final patch for any whitespace that might be trailing. I believe none are left standing in our way!

I also addressed your other comments, of course. Thanks so much to everyone who participated here to get this to completion, Blair most of all. It took a while, but I'm pretty sure it was well worth it.

As for follow-ups: I will file two; one to experiment getting the delay down between successive count operations, so it'll feel more snappy and another to remove the status text 'Reached end of page', as Luís suggested in comment 106.

Comment 117

[Ed Morley [:emorley]]

Backed out for exceptions in findbar.xml, eg:
https://tbpl.mozilla.org/php/getParsedLog.php?id=38863074&tree=Fx-Team

remote:   https://hg.mozilla.org/integration/fx-team/rev/4cc15c7ea4d8

Comment 118

[Mike de Boer [:mikedeboer]]

Ed, thanks for backing it out and sorry for the trouble.

Pushed a Try build with the (simple) fix: https://tbpl.mozilla.org/?tree=Try&rev=0ecc87549202

Comment 119

[Mike de Boer [:mikedeboer]]

Attachment: Patch v9.1: add a counter of found matches to the find in page bar

Patch with mochitest-bc fix. Carrying over r=Unfocused.

Comment 120

[Mike de Boer [:mikedeboer]]

Re-landed as: https://hg.mozilla.org/integration/fx-team/rev/3c60a233a26d

Comment 121

[Tanvi Vyas[:tanvi]]

So happy about this!  Thank you!!

Comment 122

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/3c60a233a26d

Comment 123

[Peter van der Woude [:Peter6]]

thanks guys, I did a follow up for reordering of items on the Find Toolbar in bug 1005222

Comment 124

[Sylvestre Ledru [:Sylvestre]]

Added in the release notes with the wording "Display the number of found items in the find toolbar"

Comment 125

[Mike de Boer [:mikedeboer]]

(In reply to Sylvestre Ledru [:sylvestre] from comment #124)
> Added in the release notes with the wording "Display the number of found
> items in the find toolbar"

       _
      /(|
     (  :
    __\  \  _____
  (____)  `|
 (____)|   |
  (____).__|
   (___)__.|_____

Comment 126

[Camelia Badau [:cbadau], Desktop Test Engineering]

This implementation works correctly on Windows 7 64bit, Ubuntu 13.10 32bit and Mac OSX 10.8.5 using latest Nightly 32.0a1 (build ID: 20140507030202),except for the scenario described in bug 1007101, which seems to be a site issue.

Comment 127

[mcdavis941 (sporadically reading bugmail)]

Is there some reason the selector is different for Linux vs. the other platforms?

I just tried on Linux and it looks fine, but that selector doesn't seem to match the DOM.  Just an observation.