Closed
Bug 1051187
Opened 10 years ago
Closed 9 years ago
"Match case" button does not refresh the number of occurrences
Categories
(Toolkit :: Find Toolbar, defect)
Toolkit
Find Toolbar
Tracking
()
Tracking | Status | |
---|---|---|
firefox35 | --- | verified |
People
(Reporter: Sylvestre, Assigned: tomasz, Mentored)
References
Details
(Whiteboard: [good first bug] [lang=js])
Attachments
(1 file, 7 obsolete files)
15.22 KB,
patch
|
tomasz
:
review+
|
Details | Diff | Splinter Review |
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•10 years ago
|
||
I agree. A fix for this should be trivial. You scared me a bit with the huge number of CCs :/
Assignee: nobody → mdeboer
Status: NEW → ASSIGNED
Reporter | ||
Comment 2•10 years ago
|
||
Sorry, I did a lame clone of the implementation :) I cleared the CC list (besides you and me)
Comment 3•10 years ago
|
||
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.
Assignee: mdeboer → nobody
Mentor: mdeboer
Status: ASSIGNED → NEW
Whiteboard: [good first bug] [lang=js]
Updated•10 years ago
|
Status: NEW → ASSIGNED
Comment 5•10 years ago
|
||
(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.
Assignee | ||
Comment 6•9 years ago
|
||
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 :-)
Attachment #8472736 -
Flags: feedback?(mdeboer)
Comment 7•9 years ago
|
||
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 :)
Attachment #8472736 -
Flags: feedback?(mdeboer) → feedback+
Assignee | ||
Comment 8•9 years ago
|
||
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!
Assignee | ||
Comment 9•9 years ago
|
||
This is a test case for the bug.
Attachment #8473401 -
Flags: feedback?(mdeboer)
Assignee | ||
Comment 10•9 years ago
|
||
I actually removed the argument from find. It defaults to that value.
Attachment #8472736 -
Attachment is obsolete: true
Attachment #8473403 -
Flags: feedback?(mdeboer)
Assignee | ||
Comment 11•9 years ago
|
||
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.
Attachment #8473985 -
Flags: feedback?(mdeboer)
Assignee | ||
Comment 12•9 years ago
|
||
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•9 years ago
|
||
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?
Attachment #8473985 -
Flags: feedback?(evilpies)
Comment 14•9 years ago
|
||
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.
Attachment #8473985 -
Flags: feedback?(evilpies) → feedback+
Assignee | ||
Comment 15•9 years ago
|
||
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.
Updated•9 years ago
|
Flags: qe-verify?
Flags: firefox-backlog+
Updated•9 years ago
|
Points: --- → 1
Updated•9 years ago
|
Iteration: --- → 35.1
Updated•9 years ago
|
Flags: qe-verify? → qe-verify+
Updated•9 years ago
|
Attachment #8473401 -
Flags: feedback?(mdeboer)
Updated•9 years ago
|
Attachment #8473403 -
Flags: feedback?(mdeboer)
Updated•9 years ago
|
Attachment #8473985 -
Flags: feedback?(mdeboer)
Comment 17•9 years ago
|
||
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.
![]() |
||
Updated•9 years ago
|
QA Contact: petruta.rasa
Updated•9 years ago
|
Iteration: 35.1 → 35.2
Assignee | ||
Comment 18•9 years ago
|
||
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.
Attachment #8473401 -
Attachment is obsolete: true
Attachment #8473403 -
Attachment is obsolete: true
Attachment #8473985 -
Attachment is obsolete: true
Attachment #8493930 -
Flags: review?(mdeboer)
Assignee | ||
Comment 19•9 years ago
|
||
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
Attachment #8493930 -
Attachment is obsolete: true
Attachment #8493930 -
Flags: review?(mdeboer)
Assignee | ||
Updated•9 years ago
|
Attachment #8494783 -
Flags: review?(mdeboer)
Comment 20•9 years ago
|
||
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?
Attachment #8494783 -
Flags: review?(mdeboer) → feedback+
Assignee | ||
Comment 21•9 years ago
|
||
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!).
Assignee | ||
Comment 22•9 years ago
|
||
The patch with nits addressed.
Attachment #8494783 -
Attachment is obsolete: true
Attachment #8496253 -
Flags: review?(mdeboer)
Reporter | ||
Updated•9 years ago
|
Flags: in-testsuite+
Updated•9 years ago
|
Iteration: 35.2 → 35.3
Comment 24•9 years ago
|
||
(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•9 years ago
|
||
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!
Attachment #8496253 -
Flags: review?(mdeboer) → review+
Assignee | ||
Comment 26•9 years ago
|
||
Thanks Mike for helping me with this bug, it really helped me getting started with Mozilla! I'm marking it as checkin-needed.
Keywords: checkin-needed
Comment 27•9 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/7f987505ac94
Keywords: checkin-needed
Whiteboard: [good first bug] [lang=js] → [good first bug] [lang=js][fixed-in-fx-team]
Comment 28•9 years ago
|
||
sorry had to backout this changes for test failures like https://tbpl.mozilla.org/php/getParsedLog.php?id=49391885&tree=Fx-Team
Whiteboard: [good first bug] [lang=js][fixed-in-fx-team] → [good first bug] [lang=js]
Assignee | ||
Comment 29•9 years ago
|
||
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
Attachment #8496253 -
Attachment is obsolete: true
Attachment #8499271 -
Flags: review+
Assignee | ||
Comment 30•9 years ago
|
||
Checkin-needed. Try is has problems running os x tests now but this shouldn't fail on that specific platform.
Keywords: checkin-needed
Comment 31•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/d5a592a65044
Keywords: checkin-needed
Comment 32•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/d5a592a65044
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
Comment 33•9 years ago
|
||
Reassigning as Petruta is busy with other work.
QA Contact: petruta.rasa → florin.mezei
Comment 34•9 years ago
|
||
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.
Status: RESOLVED → VERIFIED
status-firefox35:
--- → verified
You need to log in
before you can comment on or make changes to this bug.
Description
•