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

VERIFIED FIXED in Firefox 32

Status

()

enhancement
VERIFIED FIXED
15 years ago
5 years ago

People

(Reporter: Peter6, Assigned: mikedeboer)

Tracking

(Depends on 1 bug, Blocks 1 bug, {feature})

Trunk
mozilla32
Points:
---
Dependency tree / graph
Bug Flags:
firefox-backlog +
blocking1.9.1 -
wanted1.9.1 -
blocking1.9 -

Firefox Tracking Flags

(firefox32 verified, relnote-firefox 32+)

Details

(Whiteboard: [parity-chrome][parity-safari][has patch] p=0 s=it-32c-31a-30b.1 [qa!], )

Attachments

(3 attachments, 19 obsolete attachments)

898 bytes, patch
beltzner
: review+
beltzner
: ui-review+
beltzner
: approval1.9+
Details | Diff | Splinter Review
14.45 KB, image/png
Details
41.50 KB, patch
mikedeboer
: review+
Details | Diff | Splinter Review
Reporter

Description

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

Extremely usefull extra
Reporter

Updated

15 years ago
Summary: count number of found items with FIND and display the number → count and display the number of found items in the FIND toolbar
Reporter

Comment 1

14 years ago
*** Bug 281534 has been marked as a duplicate of this bug. ***
*** Bug 322201 has been marked as a duplicate of this bug. ***
QA Contact: fast.find
Assignee: bross2 → nobody

Updated

12 years ago
Assignee: nobody → ehsan.akhgari
Component: Find Toolbar / FastFind → XUL Widgets
Product: Firefox → Toolkit
Hardware: PC → All
Target Milestone: --- → mozilla1.9beta1
Version: 1.0 Branch → Trunk

Comment 3

12 years ago
This should be handled in the findbar XUL widget, which is part of the toolkit.  I'll be posting a patch soon.
Status: NEW → ASSIGNED

Comment 4

12 years ago
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.
Attachment #272773 - Flags: review?(mano)

Comment 5

12 years ago
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.
Attachment #272773 - Attachment is obsolete: true
Attachment #272782 - Flags: review?(mano)
Attachment #272773 - Flags: review?(mano)
QA Contact: fast.find → xul.widgets
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).
Attachment #272782 - Flags: review?(mano) → review-

Comment 7

12 years ago
(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?
We usually just do nsIFooBar2. That said, I'm not a peer for this code (embedding/components).

Comment 9

12 years ago
(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?
I think the embedding/ work here is worth a separate bug, yes.

Updated

12 years ago
Depends on: 389159
Duplicate of this bug: 359493

Updated

12 years ago
Target Milestone: mozilla1.9 M8 → mozilla1.9 M9

Updated

12 years ago
Flags: blocking1.9?
Not a blocker, though nice to have if you can get the reviews and nominate for approval.
Flags: blocking1.9? → blocking1.9-
Target Milestone: mozilla1.9 M9 → ---

Updated

12 years ago
Whiteboard: [parity-safari]

Comment 13

12 years ago
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.
Attachment #272782 - Attachment is obsolete: true
Attachment #294747 - Flags: ui-review?(beltzner)
Attachment #294747 - Flags: review?(mconnor)

Comment 16

12 years ago
String changes to land before the l10n freeze...
Attachment #298775 - Flags: ui-review?(beltzner)
Attachment #298775 - Flags: review?(beltzner)

Comment 17

12 years ago
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!
Reporter

Comment 18

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

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

12 years ago
(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 on attachment 298775 [details] [diff] [review]
String changes (checked in)

r+uir+a=beltzner for 1.9
Attachment #298775 - Flags: ui-review?(beltzner)
Attachment #298775 - Flags: ui-review+
Attachment #298775 - Flags: review?(beltzner)
Attachment #298775 - Flags: review+
Attachment #298775 - Flags: approval1.9+
Checkin needed on strings only portion.
Keywords: checkin-needed

Updated

12 years ago
Attachment #298775 - Attachment description: String changes → String changes (for check-in before the l10n freeze)
Reporter

Updated

12 years ago
Blocks: 414109
Reporter

Comment 23

12 years ago
(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
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
Keywords: checkin-needed
Target Milestone: --- → mozilla1.9 M11

Updated

12 years ago
Attachment #298775 - Attachment description: String changes (for check-in before the l10n freeze) → String changes (checked in)

Comment 25

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

12 years ago
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?
Whiteboard: [parity-safari] → [parity-safari][has patch][needs review mconnor][needs review beltzner]

Updated

12 years ago
Whiteboard: [parity-safari][has patch][needs review mconnor][needs review beltzner] → [parity-safari][has patch][needs review mconnor][needs ui-review beltzner]

Comment 27

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

Updated

11 years ago
Target Milestone: mozilla1.9beta3 → mozilla1.9beta4

Comment 28

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

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

11 years ago
(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.
Reporter

Comment 31

11 years ago
Find In Numbers 1.1 prevents / and ' to call the findbar with the current nightly
Whiteboard: [parity-safari][has patch][needs review mconnor][needs ui-review beltzner] → [parity-safari][has patch][needs review mconnor][needs ui-review beltzner][not needed for 1.9]

Updated

11 years ago
Duplicate of this bug: 440629
Reporter

Comment 33

11 years ago
not really a blocker, but it would be nice to get this feature and bug 414109 into 1.9.1
Flags: wanted1.9.1?
Flags: blocking1.9.1?

Comment 34

11 years ago
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).
Target Milestone: mozilla1.9beta4 → mozilla1.9.1
Attachment #294747 - Flags: ui-review?(beltzner) → ui-review-
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)
Adding Aza, as I know he's got an itch to scratch with the find toolbar :)

Updated

11 years ago
Keywords: uiwanted
Whiteboard: [parity-safari][has patch][needs review mconnor][needs ui-review beltzner][not needed for 1.9] → [parity-safari][has patch][needs review mconnor][not needed for 1.9]
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.
Flags: wanted1.9.1?
Flags: wanted1.9.1-
Flags: blocking1.9.1?
Flags: blocking1.9.1-

Updated

11 years ago
Duplicate of this bug: 473928
Attachment #294747 - Flags: review?(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.

Updated

10 years ago
Whiteboard: [parity-safari][has patch][needs review mconnor][not needed for 1.9] → [parity-safari][has patch][needs ui mockup]

Comment 40

9 years ago
Updating to reality: I won't have the time to work on this for the foreseeable future!
Assignee: ehsan.akhgari → nobody
Status: ASSIGNED → NEW
Component: XUL Widgets → Find Toolbar
QA Contact: xul.widgets → fast.find
Duplicate of this bug: 558703

Comment 42

9 years ago
If this is implemented, it should also detail which instance is currently highlighted, like in Google Chrome.
That's a different bug, already on file.

Updated

9 years ago
Duplicate of this bug: 569802
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

9 years ago
How has this still not landed? All the necessary patches are done and this really should be blocking.
Assuming the polish aspects are addressed (cc'ing shorlander) I don't see why this couldn't make it into Firefox 4.

Comment 49

9 years ago
(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!  :-)

Updated

9 years ago
Duplicate of this bug: 595941

Comment 51

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

Updated

9 years ago
Duplicate of this bug: 619663
Whiteboard: [parity-safari][has patch][needs ui mockup] → [parity-chrome][parity-safari][has patch][needs ui mockup]
Duplicate of this bug: 644575

Updated

8 years ago
Duplicate of this bug: 652435
Status: NEW → RESOLVED
Last Resolved: 7 years ago
Keywords: uiwanted
Resolution: --- → FIXED
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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 57

6 years ago
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.
Assignee

Comment 58

6 years ago
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.
Assignee: nobody → mdeboer
Attachment #294747 - Attachment is obsolete: true
Status: REOPENED → ASSIGNED
Attachment #741917 - Flags: review?
Attachment #741917 - Flags: feedback?(ehsan)
Assignee

Comment 59

6 years ago
Attachment #294748 - Attachment is obsolete: true
Attachment #294749 - Attachment is obsolete: true
Attachment #741917 - Attachment is obsolete: true
Attachment #741917 - Flags: review?
Attachment #741917 - Flags: feedback?(ehsan)
Attachment #741922 - Flags: feedback?(ehsan)
Assignee

Comment 60

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

Updated

6 years ago
Depends on: 776708
Thanks for working on this!  I've been waiting for this feature to be added to Firefox for years!
Target Milestone: mozilla1.9.1 → ---

Comment 62

6 years ago
(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

6 years ago
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...  :-)
Assignee

Updated

6 years ago
Attachment #741922 - Flags: feedback?(ehsan) → review?(mano)
Assignee

Comment 64

6 years ago
mochi test coming soon...
Assignee

Updated

6 years ago
Duplicate of this bug: 414109
Assignee

Updated

6 years ago
Attachment #741922 - Flags: review?(mano) → review?(dao)
Assignee

Comment 66

6 years ago
fixed a JS error
Attachment #741922 - Attachment is obsolete: true
Attachment #741922 - Flags: review?(dao)
Attachment #746304 - Flags: review?(dao)
This is looking good. Are you waiting for review or is something else blocking this?
Assignee

Comment 68

6 years ago
(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 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.
Attachment #746304 - Flags: review?(dao)
Attachment #746304 - Flags: review?(bmcbride)
Attachment #746304 - Flags: review-
Assignee

Updated

6 years ago
Duplicate of this bug: 389159
Assignee

Comment 71

6 years ago
Addressed review comments and added tests.
Attachment #746304 - Attachment is obsolete: true
Attachment #774625 - Flags: review?(bmcbride)
Assignee

Comment 72

6 years ago
(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! ;)
(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.
(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 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)
Attachment #774625 - Flags: review?(bmcbride) → review-
(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.
Assignee

Updated

6 years ago
Depends on: 899286
Assignee

Comment 77

6 years ago
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?
Attachment #774625 - Attachment is obsolete: true
Attachment #788133 - Flags: feedback?(gkrizsanits)
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...)
Attachment #788133 - Flags: feedback?(gkrizsanits) → feedback-
Assignee

Comment 79

6 years ago
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?
Attachment #788133 - Attachment is obsolete: true
Attachment #788234 - Flags: feedback?(gkrizsanits)
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...
Attachment #788234 - Flags: feedback?(gkrizsanits) → feedback+
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;
Assignee

Comment 82

6 years ago
(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! :)
Assignee

Comment 83

6 years ago
Attachment #788234 - Attachment is obsolete: true
Attachment #788359 - Flags: review?(bmcbride)
Attachment #788359 - Flags: feedback+
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"
Attachment #788359 - Flags: review?(bmcbride) → review-
Assignee

Comment 85

6 years ago
(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 :)
Assignee

Comment 86

6 years ago
This one *should* have all your comments addressed, sir!
Attachment #788359 - Attachment is obsolete: true
Attachment #789499 - Flags: review?(bmcbride)
Attachment #789499 - Flags: feedback+
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 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 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.
Attachment #789499 - Flags: review?(bmcbride) → review-
QA Contact: manuela.muntean
Assignee

Updated

6 years ago
Blocks: 384458
Assignee

Comment 90

6 years ago
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.
Flags: needinfo?(gavin.sharp)
(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.
Flags: needinfo?(gavin.sharp)
Assignee

Comment 92

6 years ago
(In reply to :Gavin Sharp (use gavin@gavinsharp.com 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.
Flags: needinfo?(gavin.sharp)
Assignee

Comment 93

6 years ago
Posted patch IsRangeVisible WIP patch (obsolete) — Splinter Review
Assignee

Comment 94

6 years ago
Nvmd, found it! \o/
Flags: needinfo?(gavin.sharp)
Assignee

Comment 95

6 years ago
Attachment #789499 - Attachment is obsolete: true
Attachment #806677 - Attachment is obsolete: true
Attachment #806710 - Flags: review?(bmcbride)
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.
Assignee

Comment 97

6 years ago
Posted patch e10s compat WIP (obsolete) — Splinter Review
Assignee

Updated

6 years ago
Attachment #806710 - Flags: review?(bmcbride)

Comment 98

6 years ago
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.
Assignee

Comment 99

6 years ago
(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 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.
Assignee

Comment 101

6 years ago
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.
Attachment #806710 - Attachment is obsolete: true
Attachment #806753 - Attachment is obsolete: true
Attachment #806944 - Flags: review?(bmcbride)
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.
Attachment #806944 - Flags: review?(bmcbride) → review-
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.
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.
(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.
(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.
Whiteboard: [parity-chrome][parity-safari][has patch][needs ui mockup] → [triage] [parity-chrome][parity-safari][has patch][needs ui mockup]
Whiteboard: [triage] [parity-chrome][parity-safari][has patch][needs ui mockup] → [parity-chrome][parity-safari][has patch][needs ui mockup]
Blocks: 924149
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.
Flags: needinfo?(mdeboer)
No longer blocks: fxdesktopbacklog
Whiteboard: [parity-chrome][parity-safari][has patch][needs ui mockup] → [parity-chrome][parity-safari][has patch][needs ui mockup] [feature] p=0
No longer blocks: fxdesktopbacklog
Whiteboard: [parity-chrome][parity-safari][has patch][needs ui mockup] [feature] p=0 → [parity-chrome][parity-safari][has patch][needs ui mockup]
Flags: firefox-backlog+

Comment 108

5 years ago
-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
Assignee

Updated

5 years ago
Duplicate of this bug: 924149
Assignee

Comment 110

5 years ago
Attachment #8406153 - Flags: review?(bmcbride)
Flags: needinfo?(mdeboer)
Assignee

Comment 111

5 years ago
Unbitrotted, for your convenience.
Attachment #8406153 - Attachment is obsolete: true
Attachment #8406153 - Flags: review?(bmcbride)
Attachment #8407420 - Flags: review?(bmcbride)
The whiteboard here states that this needs UI work. Is that still the case?
The screenshot from Unfocused looks good to me!
Assignee

Comment 113

5 years ago
(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! :)
Whiteboard: [parity-chrome][parity-safari][has patch][needs ui mockup] → [parity-chrome][parity-safari][has patch]
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!!!!!
Attachment #8407420 - Flags: review?(bmcbride) → review+
Assignee

Comment 115

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

I hope it sticks!
Assignee

Comment 116

5 years ago
(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.
Assignee

Comment 118

5 years ago
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
Assignee

Comment 119

5 years ago
Patch with mochitest-bc fix. Carrying over r=Unfocused.
Attachment #8407420 - Attachment is obsolete: true
Attachment #8415851 - Flags: review+
Assignee

Updated

5 years ago
Depends on: 1004512
Assignee

Updated

5 years ago
Depends on: 1004515
Assignee

Updated

5 years ago
Blocks: 1004520
So happy about this!  Thank you!!
https://hg.mozilla.org/mozilla-central/rev/3c60a233a26d
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
Depends on: 1004915
thanks guys, I did a follow up for reordering of items on the Find Toolbar in bug 1005222
Whiteboard: [parity-chrome][parity-safari][has patch] → [parity-chrome][parity-safari][has patch] S=IT-32C-31A-30B.1
Flags: needinfo?(jbecerra)
Whiteboard: [parity-chrome][parity-safari][has patch] S=IT-32C-31A-30B.1 → [parity-chrome][parity-safari][has patch] S=IT-32C-31A-30B.1 [qa?]
Flags: needinfo?(jbecerra)
Whiteboard: [parity-chrome][parity-safari][has patch] S=IT-32C-31A-30B.1 [qa?] → [parity-chrome][parity-safari][has patch] S=IT-32C-31A-30B.1 [qa+]
QA Contact: manuela.muntean → camelia.badau
Assignee

Updated

5 years ago
relnote-firefox: --- → ?
Assignee

Updated

5 years ago
Added in the release notes with the wording "Display the number of found items in the find toolbar"
Assignee

Comment 125

5 years ago
(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"

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

Updated

5 years ago
Depends on: 1007101
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.
Status: RESOLVED → VERIFIED
Whiteboard: [parity-chrome][parity-safari][has patch] S=IT-32C-31A-30B.1 [qa+] → [parity-chrome][parity-safari][has patch] S=IT-32C-31A-30B.1 [qa!]
Whiteboard: [parity-chrome][parity-safari][has patch] S=IT-32C-31A-30B.1 [qa!] → [parity-chrome][parity-safari][has patch] s=it-32c-31a-30b.1 [qa!]
Whiteboard: [parity-chrome][parity-safari][has patch] s=it-32c-31a-30b.1 [qa!] → [parity-chrome][parity-safari][has patch] p=0 s=it-32c-31a-30b.1 [qa!]
Assignee

Updated

5 years ago
Depends on: 1014788
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.

Updated

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