If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Implement desktop's FindInPage result limiting

RESOLVED FIXED in Firefox 38

Status

()

Firefox for Android
General
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: capella, Assigned: capella)

Tracking

unspecified
Firefox 38
All
Android
Points:
---

Firefox Tracking Flags

(firefox38 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

3 years ago
Created attachment 8543473 [details] [diff] [review]
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.
Attachment #8543473 - Flags: review?(wjohnston)
(Assignee)

Comment 1

3 years ago
fyi https://www.dropbox.com/s/sobyep3xdj9cltd/bug1117274.png?dl=0
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
Attachment #8543473 - Flags: review?(wjohnston) → review-
(Assignee)

Comment 3

3 years ago
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 ?
(Assignee)

Comment 4

3 years ago
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
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.
Let's get Robin to weigh in as a break from her other labors!
Flags: needinfo?(randersen)
Hardware: ARM → All
Summary: Implement desktops FindInPage matchString limit pref → Implement desktop's FindInPage result limiting
(Assignee)

Comment 7

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

Comment 8

3 years ago
ping: randerson?
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+.
Flags: needinfo?(randersen)
(Assignee)

Comment 10

3 years ago
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+.
(Assignee)

Comment 11

3 years ago
Created attachment 8550026 [details] [diff] [review]
bug1117274.diff

Simplified.
Attachment #8543473 - Attachment is obsolete: true
Attachment #8550026 - Flags: review?(wjohnston)
(Assignee)

Comment 12

3 years ago
Moar review nag ping?
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.
Attachment #8550026 - Flags: review?(wjohnston) → review+
(Assignee)

Comment 14

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

Comment 15

3 years ago
try-push
https://tbpl.mozilla.org/?tree=Try&rev=1f1ccb1f0dd1
(Assignee)

Comment 16

3 years ago
https://hg.mozilla.org/integration/fx-team/rev/f24e06712083
https://hg.mozilla.org/mozilla-central/rev/f24e06712083
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox38: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 38
You need to log in before you can comment on or make changes to this bug.