Closed Bug 1077574 Opened 5 years ago Closed 5 years ago

Add current and total counts to Find-in-page

Categories

(Firefox for Android :: General, defect)

ARM
Android
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 36

People

(Reporter: capella, Assigned: capella)

References

Details

Attachments

(1 file, 5 obsolete files)

Attached patch find.diff (obsolete) — Splinter Review
I thought we had an old bug open for this but I couldn't find it.

Also, while looking for the "old bug", I see that there are now several new Find-In-Page bugs ( bug 1050481, bug 1050480 ). I didn't consider them while putting this together.
Attachment #8499714 - Flags: review?(wjohnston)
Nice screenshot. Thoughts:
1. Can we get by with just "X of Y" and drop the "matches"?
2. Can this be put on the same "row" as the other controls? Maybe between the textview and the buttons. I don't know that the extra row is worth losing the vertical space.

Paging UX
Flags: needinfo?(randersen)
Flags: needinfo?(alam)
(In reply to Mark Finkle (:mfinkle) from comment #2)
> Nice screenshot. Thoughts:
> 1. Can we get by with just "X of Y" and drop the "matches"?
> 2. Can this be put on the same "row" as the other controls? Maybe between
> the textview and the buttons. I don't know that the extra row is worth
> losing the vertical space.
> 
> Paging UX

Agreed. 'matches' is helpful but not needed. What if we shortened the text field a wee bit and placed it next to that? I am concerned about the real estate on phones. This is where I expect to see it.

See mock: http://cl.ly/XrSm
Flags: needinfo?(randersen)
Attached patch bug1077574.diff (obsolete) — Splinter Review
Fantastic! I used an extra line previously, as I was trying to mimic desktop wording and was worried about phones ... and I >always< need help with visual design :-D

Your suggestion works out like this:
https://www.dropbox.com/s/6iadhyuh8mkt5zs/bug1077574_Phone.png?dl=0
https://www.dropbox.com/s/syoxewxhai0jypy/bug1077574_Tablet.png?dl=0

(Holding off on dev review? request, until UI (tecgirl?) likes it ...
Assignee: nobody → markcapella
Attachment #8499714 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #8499714 - Flags: review?(wjohnston)
Tweaked the fail message:
TEST-UNEXPECTED-FAIL | testSelectionHandler | Robocop tests on your device need network/wifi access to reach: [http://192.168.2.5:8888/tests]. -
 org.apache.http.conn.HttpHostConnectException: Connection to http://192.168.2.5:8888 refused

https://hg.mozilla.org/integration/fx-team/rev/63061f730d01
Wrong bug, please ignore  :-(
Flags: needinfo?(randersen)
Looks good. Can we make the text the same colour as the nav icons? I am guessing it's #afb1b3.
Flags: needinfo?(randersen)
Attached patch bug1077574.diff (obsolete) — Splinter Review
Absolutely !
Attachment #8499846 - Attachment is obsolete: true
Attachment #8500546 - Flags: review?(wjohnston)
Comment on attachment 8500546 [details] [diff] [review]
bug1077574.diff

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

::: mobile/android/base/FindInPageBar.java
@@ +93,5 @@
>          if (!mInflated) {
>              return;
>          }
> +        EventDispatcher.getInstance().unregisterGeckoThreadListener((GeckoEventListener) this,
> +            "TextSelection:Data", "FindHelper:Results");

Do you need this cast? I don't think we do here. GeckoApp implements two listener interfaces so has to do this hoop jumping, but this only implements one of them.

::: mobile/android/base/resources/layout/find_in_page_content.xml
@@ +20,5 @@
>                android:imeOptions="actionSearch"
>                android:selectAllOnFocus="true"
>                android:gravity="center_vertical|left"/>
>  
> +    <TextView android:id="@+id/find_status"

TBH, I would rather we added this "hint" text in the right side of the textbox. That will be more difficult though.

::: mobile/android/chrome/content/FindHelper.js
@@ +68,5 @@
>    onFindResult: function(aData) {
>      if (aData.result == Ci.nsITypeAheadFind.FIND_NOTFOUND) {
> +      // Clear and hide FindInPageBar results.
> +      Messaging.sendRequest({
> +        type: "FindHelper:Results",

I'd like to use the Java-JS response api for this. You'd have to return a promise from the observe message here and resolve it (with the match count data) when you were finished. Want to try?
Attachment #8500546 - Flags: review?(wjohnston) → review+
Attached patch bug1077574.diff (v2) (obsolete) — Splinter Review
mmm .... I can simplify it this way ...

I've not used Promises enough to see how they simplify things ... I tried to figure out how to advantageously inject them into this situation, but nothing jumped out at me.

Is there a simple trick I need to learn?
Attachment #8501051 - Flags: review?(wjohnston)
Flags: needinfo?(alam)
Comment on attachment 8501051 [details] [diff] [review]
bug1077574.diff (v2)

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

I would probably just make the next/prev/find messages return this data to java instead of having a separate message for it.

::: mobile/android/chrome/content/FindHelper.js
@@ +41,5 @@
> +
> +      // String match count requested, get and return the results.
> +      Messaging.addListener((data) => {
> +        this._finder.requestMatchesCount(data);
> +        return this._matchesCountResult;

Is requestMatchesCount synchronous or are you just using the old count here? I think you would do something like this:

return new Promise((resolve, reject) => {
  this.resolver = resolve;
});

Then: 
onMatchesCountResult: function(result) {
  if (this.resolver) this.resolver(result);
},
Attachment #8501051 - Flags: review?(wjohnston)
Attached patch bug1077574v3.diff (obsolete) — Splinter Review
It does appear to be synchronous. Is that why I still can't get the final Promise/resolve piece to work? (Or am I /extremely/ obtuse ??)
Attachment #8500546 - Attachment is obsolete: true
Attachment #8501051 - Attachment is obsolete: true
Attachment #8502025 - Flags: review?(wjohnston)
Comment on attachment 8502025 [details] [diff] [review]
bug1077574v3.diff

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

::: mobile/android/base/FindInPageBar.java
@@ +76,5 @@
>          mFindText.requestFocus();
>  
>          // handleMessage() receives response message and determines initial state of softInput
>          GeckoAppShell.sendEventToGecko(GeckoEvent.createBroadcastEvent("TextSelection:Get", REQUEST_ID));
> +        GeckoAppShell.sendEventToGecko(GeckoEvent.createBroadcastEvent("FindInPage:Openned", null));

One 'n' in Opened

@@ +187,5 @@
> +            public void onResponse(NativeJSObject nativeJSObject) {
> +                final int total = nativeJSObject.optInt("total", 0);
> +                final int current = nativeJSObject.optInt("current", 0);
> +
> +                final Boolean statusVisibility = (total > 0);

Follow up bug. Chrome turns there text red if there are no matches. That might be nice for us too. But hiding is fine for now. Does this jank back and forth some as the number of matches changes?

@@ +188,5 @@
> +                final int total = nativeJSObject.optInt("total", 0);
> +                final int current = nativeJSObject.optInt("current", 0);
> +
> +                final Boolean statusVisibility = (total > 0);
> +                final String statusText = current + "/" + total;

You might localize this string. i.e. %d/%d. Then you'd hvae to use getResources().getString(R.string.myString, current, total).... not sure if its worth it or not (we cheat like this in other places as well).

::: mobile/android/base/resources/layout/find_in_page_content.xml
@@ +24,5 @@
> +    <TextView android:id="@+id/find_status"
> +              android:layout_width="wrap_content"
> +              android:layout_height="wrap_content"
> +              android:layout_marginRight="5dip"
> +              android:textColor="#afb1b3"

I still want to use dimens/colors throughout here.

::: mobile/android/chrome/content/FindHelper.js
@@ +35,5 @@
> +
> +    Messaging.addListener((data) => {
> +      this.doFind(data);
> +      this._finder.requestMatchesCount(data);
> +      return this._matchesCountResult;

Hmm.. You're right it is sync! Add a note at least that it is right now. I wonder if we should reset _matchesCountResult before each call to ensure we don't have stale data.
Attachment #8502025 - Flags: review?(wjohnston) → review+
I fixed the spelling nit and will file the followup bug. I moved to dimens/colors items that I've touched in this patch, added a note re: the sync call, and now reset _matchesCountResult before each call.

I'm gonna have to bug you for one more r? as during testing I found an issue involving page switching that had me re-jigger the open/init and uninit/close logic.

(Now we open/close from FindInPageBar.java, but FindHelper.js will further uninit/init between pages.)

push to try-server:
   https://tbpl.mozilla.org/?tree=Try&rev=b882a723b97e
Attachment #8502025 - Attachment is obsolete: true
Attachment #8502697 - Flags: review?(wjohnston)
Looks like earlier issues on the try-server wound up cancelling my push

Let's try again
   https://tbpl.mozilla.org/?tree=Try&rev=62e3f05c2212
Comment on attachment 8502697 [details] [diff] [review]
bug1077574v4.diff

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

::: mobile/android/chrome/content/FindHelper.js
@@ +58,5 @@
> +      // Sync call to Finder, results available immediately.
> +      this._matchesCountResult = null;
> +      this._finder.requestMatchesCount(data);
> +
> +      return this._matchesCountResult;

Since this is repeated, I'd just put it in a shared function somewhere.
Attachment #8502697 - Flags: review?(wjohnston) → review+
Wasn't sure it was worth throwing another method in there, but it looks pretty good  :-)
https://hg.mozilla.org/integration/fx-team/rev/dc0dec53a37c
https://hg.mozilla.org/mozilla-central/rev/dc0dec53a37c
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 36
Duplicate of this bug: 760612
Duplicate of this bug: 869927
Current and total counts are displayed in the "Find in Page" toolbar only when the search string matches any word on the page. When there are no find in page matches, only the "Aa", up/down arrows and "x" symbols are displayed.
Verified as fixed on:
Device: Alcatel One Touch
OS: Android 4.1.2
Build: Firefox for Android 37.0a1 (2015-01-04) and Firefox for Android 36.0a2 (2014-01-04)
Duplicate of this bug: 1005231
You need to log in before you can comment on or make changes to this bug.