1117274 - Implement desktop's FindInPage result limiting

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Mark Capella [:capella]]

Attachment: limitFindInPage.diff

Desktop uses the pref |accessibility.typeaheadfind.matchesCountLimit| to avoid lengthy Find-In-Page searches for common strings.

I think Mobile should do this also, to avoid taking forever to count the number of |e|'s for example in a long page of content.

Comment 1

[Mark Capella [:capella]]

fyi https://www.dropbox.com/s/sobyep3xdj9cltd/bug1117274.png?dl=0

Comment 2

[Wesley Johnston (:wesj)]

Comment on attachment 8543473 [details] [diff] [review]
limitFindInPage.diff

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

I'm going to veto this until we have some better way to show this on small screens.

::: mobile/android/base/locales/en-US/android_strings.dtd
@@ +569,5 @@
>  <!ENTITY find_matchcase "Aa">
> +<!-- LOCALIZATION NOTE (find_matchcase): This is meant to preceed a number that limits how high
> +     we count when display how many strings were found. If the limit is 3 and we found 3, 4, 5...
> +     matches, we'll display "At least 3". -->
> +<!ENTITY find_matchedLimit "At least">

I doubt we have room to show this on a lot of screens. I think we need something better on mobile.

::: mobile/android/chrome/content/FindHelper.js
@@ +120,5 @@
>     * Pass along the count results to FindInPageBar for display.
>     */
>    onMatchesCountResult: function(result) {
>      this._matchesCountResult = result;
> +    this._matchesCountResult["limit"] = this._matchesCountLimit;

just use _matchesCountResult.limit = this._matchesCountLimit. Or better I think we can remove most of these matchesCount prefixes and just have

result.limit = limit
result.total
result.current

Comment 3

[Mark Capella [:capella]]

By better, do you mean a better message (ala: ">=n", or "many") ? Or "better" as in just wait for something like:
Bug 1050479 - Make find-in-page an action mode ?

Comment 4

[Mark Capella [:capella]]

I wasn't worried about message length, as the large message only occurs on simple search strings such as 'e' where the limit runs up immediately ... After the user types a few keys, either we match far less or none at all and revert to normal match count display.

Worst case on my GS3:
https://www.dropbox.com/s/l9mh9nq1zrhx4ih/bug1117274Phone.png?dl=0

Comment 5

[Richard Newman [:rnewman]]

Can we just show "99+"?

I mean, even something like "99" is gonna be obvious to anyone who's used something like Facebook Messenger or other apps with badges for unread counts, so a plus sign should seal the deal.

Comment 6

[Richard Newman [:rnewman]]

Let's get Robin to weigh in as a break from her other labors!

Comment 7

[Mark Capella [:capella]]

Ok, let me toss in a bit of trivia while we're at it :)

On desktop, if you have a limit of 100 and you go over, the string reads "More than 100 matches". As Finder.jsm sets the "over count" flag from Finder.jsm even if you match the limit exactly, if there's 100 matches, you'll still see "More than 100 matches".

For mobile, 99+ seems odd but technically correct. I'd rather just do 100+. (Which wouldn't be any worse than desktop.)

But I'm a dev, not a UI designer, so let me know :-D

Comment 8

[Mark Capella [:capella]]

ping: randerson?

Comment 9

[Robin Andersen [:tecgirl]]

So if I'm following this correctly, we're trying to speed up the time it takes to count instances, and if we cap it to 100, we need to come up with a way to say that. TBO it seems odd and I'm curious as to how much time we're saving by capping the string at 100, it makes more sense to just display the count. I wonder how often people search for just one character? What if we started the count at 2 or more characters?

I have l10n concerns when using common language on mobile, since it's a very limited space - example, Indonesian: "Lebih dari 100 pertandingan". So, if we're back to using the numerical representation, I don't favor 100+/99+, since there is no 'norm' for this, but in the sake of saving space, I would say go with 99+.

Comment 10

[Mark Capella [:capella]]

re: |I wonder how often people search for just one character?|

   Quite often. We start all Find-In-Page actions with no match string and basically re-do the search as the user types, so based on their typing speed, the second they type the first character we search, then they type a second character, we search again.

   With large articles I've noticed considerable lag in response, hence why I dug into desktops implementation a little closer.


Re: |Using #+ notation|

   I think we all agree here then. If the pref match-limit is set to max of 99, and the actual match count goes over, it'll say 99+. If the pref match-limit is set to max of 100, and the actual match count goes over, it'll say 100+.

Comment 11

[Mark Capella [:capella]]

Attachment: bug1117274.diff

Simplified.

Comment 12

[Mark Capella [:capella]]

Moar review nag ping?

Comment 13

[Wesley Johnston (:wesj)]

Comment on attachment 8550026 [details] [diff] [review]
bug1117274.diff

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

Sorry for the delay. Thanks for the hard work mark :)

::: mobile/android/base/FindInPageBar.java
@@ +229,5 @@
> +                    final int limit = nativeJSObject.optInt("limit", 0);
> +                    statusText.append(Integer.toString(limit)).append("+");
> +                } else if (total > 0) {
> +                    final int current = nativeJSObject.optInt("current", 0);
> +                    statusText.append(Integer.toString(current)).append("/").append(Integer.toString(total));

I don't think using a StringBuilder like this gains you much over just using string concatination. i.e.

Integer.toString(current) + "/" + Integer.toString(total)

the builders only advantage (AFAIK) is in cases where you're doing lots of separate String + String calls (which will allocate a new string for each call)

@@ +231,5 @@
> +                } else if (total > 0) {
> +                    final int current = nativeJSObject.optInt("current", 0);
> +                    statusText.append(Integer.toString(current)).append("/").append(Integer.toString(total));
> +                }
> +

Add a note here that if total == 0, we show nothing, and maybe point out that that will also happen if the limit is off.

::: mobile/android/chrome/content/FindHelper.js
@@ +35,5 @@
> +    try {
> +      this._limit = Services.prefs.getIntPref("accessibility.typeaheadfind.matchesCountLimit");
> +    } catch (e) {
> +      // Pref not available, assume 0, no match counting.
> +      this._limit = 0;

So if you've turned off the limit, we also don't count at all. Is that the behavior we want? I'm fine with it I think, but just want to be clear.

Comment 14

[Mark Capella [:capella]]

Cool, thanks.

I'm not sure I understand your last question re: |So if you've turned off the limit, we also don't count at all|, but ...

... Like on Desktop, if the user sets the pref to 0, or deletes the pref / it's unavailable, the matchCount routine works as always (when it matches # of occurences past the limit we pass), it stops counting.

Comment 15

[Mark Capella [:capella]]

try-push
https://tbpl.mozilla.org/?tree=Try&rev=1f1ccb1f0dd1

Comment 16

[Mark Capella [:capella]]

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

Comment 17

[Carsten Book [:Tomcat]]

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