1051187 - "Match case" button does not refresh the number of occurrences

Status: VERIFIED FIXED

(Toolkit :: Find Toolbar, defect)

Description

[Sylvestre Ledru [:Sylvestre]]

1) on this page:
https://wiki.mozilla.org/Release_Management/Release_Day
search for Release occurrences 
30 are found

2) Click on "Match case"
30 is still show

3) Search for the next one
The number of match is updated (17)

The refresh should be done once we click on "Match case"

Comment 1

[Mike de Boer [:mikedeboer]]

I agree. A fix for this should be trivial.

You scared me a bit with the huge number of CCs :/

Comment 2

[Sylvestre Ledru [:Sylvestre]]

Sorry, I did a lame clone of the implementation :)
I cleared the CC list (besides you and me)

Comment 3

[Mike de Boer [:mikedeboer]]

If you plan on taking this bug, leave a comment with any question(s) you might have and I'll help.

What needs to happen here is that once the 'Match Case' button is clicked, the find count label needs to be hidden. Any subsequent find in page action will cause the label to show up again with the correct numbers.

Comment 4

[Tomasz Kołodziejski [:tomasz]]

I'd like to work on it.

Comment 5

[Mike de Boer [:mikedeboer]]

(In reply to Tomasz Kołodziejski [:tomasz] from comment #4)
> I'd like to work on it.

Fantastic! If you need help with anything, please ping me on IRC: 'mikedeboer' in #developers or #fx-team.

Comment 6

[Tomasz Kołodziejski [:tomasz]]

Attachment: 0001-Bug-1051187-Match-case-button-does-not-refresh-the-n.patch

I just started working on it after reading some some introduction to XBL and I noticed interesting thing: if "highlight all" button is pressed the "match case" refreshes the match count.

The reason for that, as I get it, is that _setHighlightTimeout (file: toolkit/content/widgets/findbar.xml) is called (with 500ms timeout) and after its updated the ui gets an update too. I understand that this timeout is so that the "highlight all" is not run every keystroke and is more responsive, right?

In the attachment you can find my patch. Please have a look. However, I'm not entirely sure if that's the right place to call find. The whole file uses some observers so I'm a little bit unsure.

Thoughts:
1. I think we can get rid of this._updateCaseSensitivity in _find. It seems that the case sensitivity should be updated only on startup or via observer. I think we can remove it.
2. I see stuff like "browser.finder.addResultListener(this)" which probably means that this should always take care of updating matches count for us once the search is done (onFindResult does that). That updates matches count which is also asynchronous with default timeout of 250 ms. In this case I'm not really sure why we don't do this just synchronously.
3. How do I test it? It looks that there are two modes but in Firefox I see only on of them.

Please keep in mind that it's our third day at Mozilla :-)

Comment 7

[Mike de Boer [:mikedeboer]]

Comment on attachment 8472736 [details] [diff] [review]
0001-Bug-1051187-Match-case-button-does-not-refresh-the-n.patch

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

Hi Tomasz, thanks for providing a patch! <3 the fact you're using vanilla Mercurial (which I *think* you're using, right?).

OK, so you're almost there. I provided a suggestion below, but you'll also need to update the unit test(s) to make sure this change doesn't regress in the future.

You can find the test case in http://mxr.mozilla.org/mozilla-central/source/toolkit/content/tests/chrome/findbar_window.xul#200. This is a mochitest-chrome unit test which you can run from the command line using mach: `mach mochitest-chrome toolkit/content/tests/chrome/test_findbar.xul`.

Usually we split the bug fix and the test patches for separate review, but when the patch is really small like this, feel free to include a test update in the same patch.
However, one important benefit of keeping the patches separate is that you can run the test 1) without the fix applied and 2) with the fix so you can make sure that it fails without the fix applied and succeeds with the fix.
That's what I'll be doing when reviewing your patch ;)

PS: I'm from The Netherlands, so my timezone might be quite different from yours. If you're in the US, it's usually best to ping me early in your morning.

::: toolkit/content/widgets/findbar.xml
@@ +306,5 @@
>                break;
>              case "accessibility.typeaheadfind.casesensitive":
>                this._self._typeAheadCaseSensitive = prefsvc.getIntPref(aPrefName);
>                this._self._updateCaseSensitivity();
> +              this._self._find(this._self._findField.value);

Almost there! If you do

```js
this._self.onMatchesCountResult({total: 0});
```

... you're golden :)

Comment 8

[Tomasz Kołodziejski [:tomasz]]

I'm currently using git because I know it and convert a patch using moz-git-tools. I read an article 'please stop using mq' that told me not to start using mq. By the way: can I just post patches in git format? I've read that hg has the ability to import them.

I'll post the test as a separate patch but digging into the test file takes me some time. It uses old, school, synchronous, manually driven test functions so I have to wrap my head around it. I'm heading home now so I'm not sure if I can make it before you having weekend.

What do you suggest running onMatchesCountResult? It will be fired automatically once browser.finder finishes its job. I may be missing something so please clarify.

Thanks for your help!

Comment 9

[Tomasz Kołodziejski [:tomasz]]

Attachment: test.patch

This is a test case for the bug.

Comment 10

[Tomasz Kołodziejski [:tomasz]]

Attachment: find.patch

I actually removed the argument from find. It defaults to that value.

Comment 11

[Tomasz Kołodziejski [:tomasz]]

Attachment: less-async.patch

Sorry for such a stream of updates/thoughts but this is my whole day work:

Side notes (what to do about them? open separate bugs?):
1. I've found some dead (?) code: updateControlState.
2. I've noticed _findResetTimeout having being set to -1 as no timeout. And then somewhere in the code there's a check `if (_findResetTimeout != -1)` and also `if (_findResetTimeout)`. This is wrong.

Bug related:
1. I've found that even with the above (I mean previous) patch it does not work smoothly. When you click quickly on match case it breaks...
2. This lead me to first getting rid of calling setHighlightTimeout which is already called by _find and just resetting _findFailedString. This is much better.
3. I don't understand updateMatchesCountTimeout. Why is it there? I guess the reason is that it may take long time to calculate it and we don't want to do it on every keystroke just like the search itself, right? But right now it causes the weird user experience that it's out of sync with updating notfound string and so you can see at the same time: "Phrase not found." and "1 of 9 matches" which didn't yet disappeared. I think we should get rid of that timeout completely and move updating of those numbers to onFindResult. I'm not sure of the mechanics there but it should be pretty natural that once we get this callback fired getting the matches count should be instantaneous. This patch does that, but...
4. _updateMatchesCountWorker throws an Error: HTMLAnchorElement is not defined. In fact it’s the finder which throws it. It is being thrown even before my changes. In my patch I ugly catch it and do nothing about ti. Add a new bug and leave a TODO?

Note to myself: check how many times is find actually called on every keystroke. The code is kinda convoluted and I'm not sure if it's really just once.

Comment 12

[Tomasz Kołodziejski [:tomasz]]

Ok, I guess that you don't want me to have this less async version because of bug 1004520 references to the original discussion where it was introduced. I'll leave it here for reference.

Comment 13

[Drew Willcoxon :adw]

Comment on attachment 8473985 [details] [diff] [review]
less-async.patch

Tom, I think you might know some of the find bar code?  Apologies if not.  Do you think you could give Tom some feedback on this patch and the others while Mike is away?

Comment 14

[Tom S [:evilpie]]

Comment on attachment 8473985 [details] [diff] [review]
less-async.patch

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

I haven't seen the "match count" code before, but updating the match count from onFindResult seems reasonable. This is a weak f+, because I don't really know how this works right now. Sorry.

Comment 15

[Tomasz Kołodziejski [:tomasz]]

After reading through the internals I no longer feel that's a good solution. I started some work on bug 429732, comment 24 to make the overall search experience better.

So requestingMatchesCount from onFindResult is not instantaneous and so we shouldn't do that. We'll see what will happen in bug 429732 and will get back here.

Comment 17

[Mike de Boer [:mikedeboer]]

Tomasz, thanks for working on this! I cleared all feedback requests, because I think you're right in comment 15.

I'm looking forward to new patches after you fixed & landed bug 429732.

Comment 18

[Tomasz Kołodziejski [:tomasz]]

Attachment: match-case-button.patch

I'm back with this bug. The fix was easy with the exception of catching the case of clearing _findFailedString -- I added a special test for it.

I also Taskified the test a little bit and onPageShow is now much nicer. Please have a look mikedeboer.

Comment 19

[Tomasz Kołodziejski [:tomasz]]

Attachment: match-case-button.patch

I had to update one test since it was failing on try. With the update it looks good: https://tbpl.mozilla.org/?tree=Try&rev=8721a3a44d46

Comment 20

[Mike de Boer [:mikedeboer]]

Comment on attachment 8494783 [details] [diff] [review]
match-case-button.patch

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

Tomasz, I like the improvements you made in the tests! I do have a question about the actual fix you implemented... Could you answer that please? Meanwhile, f+

::: toolkit/content/tests/chrome/findbar_window.xul
@@ +23,5 @@
>      const Cc = Components.classes;
>      const Cr = Components.results;
> +    const Cu = Components.utils;
> +    const { Task } = Cu.import("resource://gre/modules/Task.jsm", {});
> +    Cu.import("resource://gre/modules/Promise.jsm");

Why mix these two import styles?

@@ +59,5 @@
>        window.opener.wrappedJSObject.SimpleTest.ok(condition, message);
>      }
> +    function is() {
> +      window.opener.wrappedJSObject.SimpleTest.is.apply(
> +          window.opener.wrappedJSObject.SimpleTest.is, arguments);

Let's just do this:

```js
function is(a, b, message) {
  window.opener.wrappedJSObject.SimpleTest.is(a, b, message);
}
```

@@ +169,5 @@
>        gFindBar.close();
>      }
>  
>      function testQuickFindClose() {
> +      let deffered = Promise.defer();

nit: `deferred`. Please correct that throughout your patch.

@@ +491,5 @@
> +      enterStringIntoFindField(SEARCH_TEXT);
> +      gFindBar.clear();
> +
> +      var prefsvc = Cc["@mozilla.org/preferences-service;1"].
> +                    getService(Components.interfaces.nsIPrefBranch);

nit: please use the following notation:

```js
let prefsvc = Cc["@mozilla.org/preferences-service;1"]
                .getService(Ci.nsIPrefBranch);
```

::: toolkit/content/widgets/findbar.xml
@@ +538,5 @@
>          <body><![CDATA[
>            this._typeAheadCaseSensitive = aCaseSensitivity;
>            this._updateCaseSensitivity();
> +          this._findFailedString = null;
> +          this._find();

Isn't this going to make the found range jump to the next match every time you hit "Match Case" button?

Comment 21

[Tomasz Kołodziejski [:tomasz]]

Comment on attachment 8494783 [details] [diff] [review]
match-case-button.patch

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

::: toolkit/content/tests/chrome/findbar_window.xul
@@ +23,5 @@
>      const Cc = Components.classes;
>      const Cr = Components.results;
> +    const Cu = Components.utils;
> +    const { Task } = Cu.import("resource://gre/modules/Task.jsm", {});
> +    Cu.import("resource://gre/modules/Promise.jsm");

Copy-paste artifact. Fixing.

@@ +59,5 @@
>        window.opener.wrappedJSObject.SimpleTest.ok(condition, message);
>      }
> +    function is() {
> +      window.opener.wrappedJSObject.SimpleTest.is.apply(
> +          window.opener.wrappedJSObject.SimpleTest.is, arguments);

Done.

I don't like the fact that we have to do it at all. I don't really understand the reason that we have test_findbar.xul and findbar_window.xml. Do we really need to open a new window so that SpecialPowers.pushPrefEnv takes effect?

@@ +169,5 @@
>        gFindBar.close();
>      }
>  
>      function testQuickFindClose() {
> +      let deffered = Promise.defer();

Yay! They -- promises aplus -- had to chose deferred and fulfill. At least we use resolve instead of fulfill.

Out of curiosity I searched for the misspelled (another difficult one!) words:
* "deffered" = 6 results,
* "fullfill" = 10 results.

Fixed.

@@ +491,5 @@
> +      enterStringIntoFindField(SEARCH_TEXT);
> +      gFindBar.clear();
> +
> +      var prefsvc = Cc["@mozilla.org/preferences-service;1"].
> +                    getService(Components.interfaces.nsIPrefBranch);

Do we have some js{h,l}int to catch those?

::: toolkit/content/widgets/findbar.xml
@@ +538,5 @@
>          <body><![CDATA[
>            this._typeAheadCaseSensitive = aCaseSensitivity;
>            this._updateCaseSensitivity();
> +          this._findFailedString = null;
> +          this._find();

I tested it and it looks like it works correctly -- it does not jump to the next one. There is findAgain to find next occurrence (difficult word again!).

Comment 22

[Tomasz Kołodziejski [:tomasz]]

Attachment: match-case-button.patch

The patch with nits addressed.

Comment 24

[Mike de Boer [:mikedeboer]]

(In reply to Tomasz Kołodziejski [:tomasz] from comment #21)
> Done.
> 
> I don't like the fact that we have to do it at all. I don't really
> understand the reason that we have test_findbar.xul and findbar_window.xml.
> Do we really need to open a new window so that SpecialPowers.pushPrefEnv
> takes effect?

I'm not aware that changed recently. I also never bothered to try doing this differently, as the cost-benefit ratio didn't seem appealing to me.

> Yay! They -- promises aplus -- had to chose deferred and fulfill. At least
> we use resolve instead of fulfill.
> 
> Out of curiosity I searched for the misspelled (another difficult one!)
> words:
> * "deffered" = 6 results,
> * "fullfill" = 10 results.
> 
> Fixed.

BTW, the new awesome way to this is

```js
yield new Promise(resolve => somethingAsync(resolve));
```

...and possible variations thereof. Nice, eh? Something to keep in mind for the next thing you'll work on.

> Do we have some js{h,l}int to catch those?

Nope, it's up to us, the style-cops, to catch 'em :/

> I tested it and it looks like it works correctly -- it does not jump to the
> next one. There is findAgain to find next occurrence (difficult word again!).

Alright, we'll have to keep an eye on possible regressions that might be filed - like the intermittent test failure you fixed recently. But the amount of regressions reported by users still counts _0_ - keep up the good work!!

Comment 25

[Mike de Boer [:mikedeboer]]

Comment on attachment 8496253 [details] [diff] [review]
match-case-button.patch

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

Apology for the late review here - I've been very busy with other work on a project called 'Loop' (you might've heard of it).

Thanks much for the test improvements here!

Comment 26

[Tomasz Kołodziejski [:tomasz]]

Thanks Mike for helping me with this bug, it really helped me getting started with Mozilla!

I'm marking it as checkin-needed.

Comment 27

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/fx-team/rev/7f987505ac94

Comment 28

[Carsten Book [:Tomcat]]

sorry had to backout this changes for test failures like https://tbpl.mozilla.org/php/getParsedLog.php?id=49391885&tree=Fx-Team

Comment 29

[Tomasz Kołodziejski [:tomasz]]

Attachment: match-case-button.patch

I should have pushed to try after those changes. I thought those are just nits but they weren't.

Here's the updated patch with try run:
https://tbpl.mozilla.org/?tree=Try&rev=c3315a7d852c

Comment 30

[Tomasz Kołodziejski [:tomasz]]

Checkin-needed. Try is has problems running os x tests now but this shouldn't fail on that specific platform.

Comment 31

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d5a592a65044

Comment 32

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/d5a592a65044

Comment 33

[[SV Manager] Florin Mezei, QA (:FlorinMezei)]

Reassigning as Petruta is busy with other work.

Comment 34

[[SV Manager] Florin Mezei, QA (:FlorinMezei)]

Reproduced the issue on Win 7 x64, with Nightly from September 3rd (BuildID=20140903072057), using scenario from comment 0. In this case refresh was performed only after clicking "Next"/"Previous", <Enter>, "Highlight All". 

Verified as fixed in latest Firefox 35 Nightly (BuildID=20141008065430), with same scenario, on Win 7 x64, Mac OS X 10.9.5, Ubuntu 13.04 x64. The refresh (and highlight when applicable) is now correctly applied as soon as "Match Case" is clicked, or as soon as text is changed.