Closed Bug 1305194 Opened 4 years ago Closed 4 years ago

Find highlights too quickly while I'm typing

Categories

(Toolkit :: Find Toolbar, defect)

defect
Not set
normal
Points:
2

Tracking

()

VERIFIED FIXED
mozilla52
Iteration:
52.2 - Oct 17
Tracking Status
firefox52 --- disabled
firefox53 --- disabled
firefox54 --- verified

People

(Reporter: Dolske, Assigned: mikedeboer)

References

(Blocks 1 open bug)

Details

Attachments

(3 files)

The performance of find-in-page is too fast! ;)

I've noticed a few times that while typing a search term, it's highlighting stuff in the page before I've finished typing. E.G. I just noticed it highlighting the letter "i" (which was all over the page!), while I was typing a term beginning with that letter.

At best this is a little distracting, at worst it's slowing things down searching for intermediate strings that are just going to be discarded for a future search. (Although the only slowdown I've seen is that it the previously highlighted short string may linger for a fraction of a second. So again more distraction than "slowness").

The usual fix for this kind of thing is to simply add a timeout, so that input is ignored a few milliseconds until the user hasn't typed more. (And perhaps with a timeout that's a bit longer for the first few characters?) Do we do anything like this already?
Yes we do this already - can you play around with the `findbar.iteratorTimeout` pref a bit? You do need to restart your browser for it to take effect.
Flags: needinfo?(dolske)
Attached patch WIPSplinter Review
Ah, cool, so maybe something simple like this?

Aside: I'm not sure the (existing) clearTimeout() is quite right... Seems like this will cause the pending promise (for the setTimeout) to get stuck and fail to ever complete. (Is that a leak?) This should probably be storing the actual promise, and rejecting it instead of clearing the timeout?
Flags: needinfo?(dolske)
Attachment #8795534 - Flags: feedback?(mdeboer)
Comment on attachment 8795534 [details] [diff] [review]
WIP

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

::: toolkit/modules/FinderIterator.jsm
@@ +426,5 @@
> +      let searchTerm = this._currentParams.word;
> +      if (searchTerm.length == 1)
> +        timeout *= 4;
> +      else if (searchTerm.length == 2)
> +        timeout *= 2;

I hadn't thought about this type of granularity, but I like it!!
r=me for this part.

@@ +429,5 @@
> +      else if (searchTerm.length == 2)
> +        timeout *= 2;
> +dump("Searching for <" + searchTerm + "> with timeout " + timeout + "\n");
> +
> +      yield new Promise(resolve => this._timer = setTimeout(resolve, timeout));

You're right, I was trying to be smart here. However, this is causing a very specific type of leak: JS call-stack explosion. This is not the type of leak we're used to (doesn't directly impact memory or CPU usage), but might be a cause for odd bugs after months of intensive findbar usage.
The simple fix would be:

```js
if (this._timer)
  clearTimeout(this._timer);
if (this._runningFindResolver)
  this._runningFindResolver();

// Yer-code.

yield new Promise(resolve => {
  this._runningFindResolver = resolve;
  this._timer = setTimeout(resolve, timeout)
});
this._timer = this._runningFindResolver = null;
```

And the if-statement below will do the rest of the work for us, because the spawnId will be different for the previous find.
Attachment #8795534 - Flags: feedback?(mdeboer) → feedback+
Justin, I hope you don't mind me stealing.
Assignee: nobody → mdeboer
Blocks: 1291278
Status: NEW → ASSIGNED
Iteration: --- → 52.2 - Oct 17
Points: --- → 2
Flags: qe-verify+
Flags: firefox-backlog+
Comment on attachment 8797956 [details]
Bug 1305194 - wait a little longer when the finder iterator is requested to find a query of only one or two characters, which improves usability due to less flickering of highlighter results and performance due to avoiding the most costly nsFind operation

https://reviewboard.mozilla.org/r/83564/#review82872

::: toolkit/content/tests/chrome/findbar_window.xul:562
(Diff revision 1)
> +        if (test.text.length == 1)
> +          timeout = ((timeout - 20) * 4) + 20;
> +        else if (test.text.length == 2)
> +          timeout = ((timeout - 20) * 2) + 20;

What is the point of subtracting 20 from the timeout before multiplying by 4 and then adding 20 back?

This will now be 60ms less than the iterator_timeout, whereas before this patch the timeout was equal to iterator_timeout.
Comment on attachment 8797956 [details]
Bug 1305194 - wait a little longer when the finder iterator is requested to find a query of only one or two characters, which improves usability due to less flickering of highlighter results and performance due to avoiding the most costly nsFind operation

https://reviewboard.mozilla.org/r/83564/#review82872

> What is the point of subtracting 20 from the timeout before multiplying by 4 and then adding 20 back?
> 
> This will now be 60ms less than the iterator_timeout, whereas before this patch the timeout was equal to iterator_timeout.

Because 20ms is added at the top of the file too. I can also remove that additional 20ms, because I don't think we need it...
Comment on attachment 8797956 [details]
Bug 1305194 - wait a little longer when the finder iterator is requested to find a query of only one or two characters, which improves usability due to less flickering of highlighter results and performance due to avoiding the most costly nsFind operation

https://reviewboard.mozilla.org/r/83564/#review83470

Okay, I see now. So with this change the iterator timeout that is used within the test matches that of the pref? I think we should still add 20ms to the iterator timeout, just do it after we have gone through the if-elseif lines.
Attachment #8797956 - Flags: review?(jaws) → review+
Pushed by mdeboer@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/a7882c91ca20
wait a little longer when the finder iterator is requested to find a query of only one or two characters, which improves usability due to less flickering of highlighter results and performance due to avoiding the most costly nsFind operations there are on a page. r=jaws
Pushed by mdeboer@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/d428a03f0f4b
wait a little longer when the finder iterator is requested to find a query of only one or two characters, which improves usability due to less flickering of highlighter results and performance due to avoiding the most costly nsFind operations there are on a page. r=jaws
https://hg.mozilla.org/mozilla-central/rev/d428a03f0f4b
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
QA Contact: brindusa.tot
Flags: needinfo?(mdeboer)
Attached video bug1305194.mp4
I was trying to verify this issue and indeed the delay that was desired can be noticed and it kind of reduces the flickering.

But, on the following searches that same delay makes the previous search be displayed again. Please see the screen cast. For e.g, if I search for "i", I clear the Find bar and after that I search for "a", "i" is highlighted for a short period of time just before "a" being highlighted. 

Is this the result you expected with the landing of this patch?
Flags: needinfo?(mdeboer)
Yes. Thanks for verifying! And apologies for the super late reply...
Flags: needinfo?(mdeboer)
Based on Comment 19, setting the status-firefox54: to verified. 
Since the Find Toolbar feature is enabled by default only on the latest Nightly 54.0a1, setting the status-firefox52 and status-firefox53 to disabled.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.