695172 - (find) Find In Page

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect, P3)

Comment 1

[Madhava Enros [:madhava]]

For reference, here is what the ICS browser does:
* before launched: http://i.imgur.com/eohqY.png
* user picks item from menu
* comes up: http://i.imgur.com/N5ZUR.png
* mid-searching: http://i.imgur.com/mQhOK.png

Comment 2

[Madhava Enros [:madhava]]

(In reply to Madhava Enros [:madhava] from comment #1)
> For reference, here is what the ICS browser does:
> * before launched: http://i.imgur.com/eohqY.png
> * user picks item from menu
> * comes up: http://i.imgur.com/N5ZUR.png
> * mid-searching: http://i.imgur.com/mQhOK.png

Which is not to say that I particularly like this approach. I.e., what is that checkmark for? Done? Something found? And how do you change tabs while this is up?

Comment 3

[Aaron Train [:aaronmt]]

(In reply to Madhava Enros [:madhava] from comment #2)
> (In reply to Madhava Enros [:madhava] from comment #1)
>... 
> Which is not to say that I particularly like this approach. I.e., what is
> that checkmark for? Done? Something found? And how do you change tabs while
> this is up?

Checkmark closes the panel; all match selections are cleared. That checkmark is always there. You can not change tabs, because the menu is inaccessible while that panel is visible!

Comment 4

[Kartikaya Gupta (email:[e-mail])]

Attachment: WIP

Did some work on this to see how hard it would be to fully implement, and have a WIP patch. It's usable on the galaxy tab, but for some reason the popup doesn't show on the droid pro (I think the position/anchoring is just off). Gonna leave it at this until discussion with Madhava tomorrow since changing the UX will likely result in a lot of churn anyway and I don't want to spend more time here unless it's definitely going in v1.

Comment 5

[Patryk Adamczyk [:patryk] UX]

Some sample designs can be found here:
http://www.flickr.com/photos/patrykdesign/6538751287/in/set-72157628485933281/

Comment 6

[Zhenshuo Fang (:fang) - Firefox UX Team]

More references:
Opera: http://cl.ly/3T1o2i150X3J2N302344
Chrome: http://cl.ly/1M0H1Q230D3u443e3d0R
Dolphin: http://cl.ly/1P2I3D0a443s150J1R2c
Firefox Beta: http://cl.ly/0N351v3o1c130E1h260g

In terms of UI, I like Patryk's mock-up better than the half bar we have in beta now. One thing we need to think about is do we want to provide more controls to the user, things like number of matches, match case, match whole word, etc. But since we have limited space, we shouldn't provide an option to clutter the UI unless it's really necessary for the user.

I'm not sure whether the need is different for mobile user when find in page compare to desktop. But in terms of interaction, I think we should by default show all matches so the user can easily scroll the page and find what they are looking for. Scrolling is much easy than click on the up and down arrow on mobile. 

And also currently when I find on page in Beta, it automatically zoom in to the first result as I type(which is very annoying). I don't think this is the behavior we want because people find and read things in content, zooming in does exactly the opposite which make people lose their content.

Comment 7

[Doug Turner (:dougt)]

not blocking fennec-native1.0.

Comment 8

[Kartikaya Gupta (email:[e-mail])]

Note to SUMO: we don't have find-in-page in the 1.0 release, and a lot of users will probably be looking for it at some point or another.

Comment 9

[Michelle Luna]

SUMO: I've documented this in the Search and Find tools and Fix Problems articles on SUMO.

Comment 10

[Kartikaya Gupta (email:[e-mail])]

I'm going to unassign myself from this for now since somebody else can probably pick up the WIP I have and run with it. The WIP is pretty severely bitrotted but shows the work that needs to be done to get a basic working find-in-page; the rest will be UI tweaking.

Comment 11

[:Margaret Leibovic]

I can pick this up!

Comment 12

[:Margaret Leibovic]

Attachment: WIP (mostly done, still needs resources)

This patch implements Patryk's design from comment 5, but it still needs resources for the buttons and potentially the text field.

Besides the resources, a few things left to address/consider:

-When should we hide this bar? In this patch we only when the user taps the "x", which is similar to what desktop does. I don't think we should hide it when the user taps outside, since sometimes you want to scroll the page to see some context around the find (I had a version of the patch that did this, and I found it surprising when playing around with it).

-Do we want to keep the keyboard open when the user hits the next/previous buttons? By default it is hiding itself, but I could figure out how to keep it open if that's what we want.

-I decided to disable the menuitem for about:home, since it wouldn't work on Java content. Any other pages we want to disable it for? On desktop we disable the find bar on in-content pages like about:addons.

Comment 13

[:Margaret Leibovic]

Attachment: screenshot

Ian or Patryk, could one of you help me out with polish/button resources here?

Patryk's mockup also had a different looking input box, but I think the native input looks fine, at least fine enough for a first pass.

Comment 14

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 626549 [details] [diff] [review]
WIP (mostly done, still needs resources)

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

::: mobile/android/chrome/content/browser.js
@@ +3221,5 @@
> +
> +  findClosed: function() {
> +    if (!this._findInProgress) {
> +      // this should never happen
> +      Cu.reportErro("Warning: findClosed() called while _findInProgress is false!");

typo alert! damn weakly typed languages....

Comment 15

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to Margaret Leibovic [:margaret] from comment #12)
> Created attachment 626549 [details] [diff] [review]
> WIP (mostly done, still needs resources)
> 
> This patch implements Patryk's design from comment 5, but it still needs
> resources for the buttons and potentially the text field.
> 
> Besides the resources, a few things left to address/consider:
> 
> -When should we hide this bar? In this patch we only when the user taps the
> "x", which is similar to what desktop does. I don't think we should hide it
> when the user taps outside, since sometimes you want to scroll the page to
> see some context around the find (I had a version of the patch that did
> this, and I found it surprising when playing around with it).

I agree. Let's use the 'x'

> -Do we want to keep the keyboard open when the user hits the next/previous
> buttons? By default it is hiding itself, but I could figure out how to keep
> it open if that's what we want.

I kinda think that if you are moving next/prev that you are done typing for now and giving you more space to view content is the priority.

> -I decided to disable the menuitem for about:home, since it wouldn't work on
> Java content. Any other pages we want to disable it for? On desktop we
> disable the find bar on in-content pages like about:addons.

The Java impl'd pages are the main ones from my point of view. We can blacklist others as needed.

Comment 16

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 626549 [details] [diff] [review]
WIP (mostly done, still needs resources)

>diff --git a/mobile/android/base/locales/en-US/android_strings.dtd b/mobile/android/base/locales/en-US/android_strings.dtd

>+<!ENTITY find_in_page "Find In Page">

"Find in Page"

>diff --git a/mobile/android/base/resources/layout-v11/gecko_app.xml b/mobile/android/base/resources/layout-v11/gecko_app.xml

>+    <org.mozilla.gecko.FindInPageBar android:id="@+id/find_in_page"

I think we need to add this to our GeckoViewFactory for performance reason.

>+var FindHelper = {

Do we have any way to send a "nothing found" message to Java?

f+ (mind the typo kats found)

Comment 17

[Ian Barlow (:ibarlow)]

Attachment: find in page mockup

Margaret, here are the resources you should need for the find in page bar: http://cl.ly/0g3y1I3u3u3e200n1h2s. The input field is the same one as the URL bar, so you should be able to reuse that element.

And attached is a mockup for reference :)

Comment 18

[:Margaret Leibovic]

(In reply to Mark Finkle (:mfinkle) from comment #16)

> >diff --git a/mobile/android/base/resources/layout-v11/gecko_app.xml b/mobile/android/base/resources/layout-v11/gecko_app.xml
> 
> >+    <org.mozilla.gecko.FindInPageBar android:id="@+id/find_in_page"
> 
> I think we need to add this to our GeckoViewFactory for performance reason.

I'm not sure what you're referring to here. Do you mean just lazily adding the view to the main layout?

> >+var FindHelper = {
> 
> Do we have any way to send a "nothing found" message to Java?

Yes, we can send a message if the find result is Ci.nsITypeAheadFind.FIND_NOTFOUND. The mockup doesn't include UI for what to do if there's no match, but I think it would be a nice follow-up to indicate that there's no match.

Comment 19

[:Margaret Leibovic]

Attachment: patch

I copied the EditText styling from the way we do it in AwesomeBar. It seems kinda gross, but I'm assuming that was the only way to make it do what we want?

Comment 20

[:Margaret Leibovic]

Attachment: screenshot

Ian, what do you think? I took the EditText styles from the AwesomeBar as you suggested.

Comment 21

[:Margaret Leibovic]

Comment on attachment 628155 [details] [diff] [review]
patch

>diff --git a/mobile/android/base/locales/en-US/android_strings.dtd b/mobile/android/base/locales/en-US/android_strings.dtd
>--- a/mobile/android/base/locales/en-US/android_strings.dtd
>+++ b/mobile/android/base/locales/en-US/android_strings.dtd
>@@ -89,16 +89,24 @@

>+<!-- Localization note (find_text, find_prev, find_next, find_close) : These strings are used
>+     as alternate text for accessibility. They are not visible in the UI. -->
>+<!ENTITY find_text "Enter Find Text">
>+<!ENTITY find_prev "Find Previous Match">
>+<!ENTITY find_next "Find Next Match">
>+<!ENTITY find_close "Close Find Bar">

Eitan, are these strings good for accessibility? I'm sure what conventions are around labeling UI widgets, so I just took a guess!

Comment 22

[:Margaret Leibovic]

(In reply to Margaret Leibovic [:margaret] from comment #21)

> I'm sure what conventions
> are around labeling UI widgets, so I just took a guess!

I'm *not* sure.

Comment 23

[Eitan Isaacson [:eeejay]]

Comment on attachment 628155 [details] [diff] [review]
patch

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

(In reply to Margaret Leibovic [:margaret] from comment #21)
> Comment on attachment 628155 [details] [diff] [review]
> patch
> 
> >diff --git a/mobile/android/base/locales/en-US/android_strings.dtd b/mobile/android/base/locales/en-US/android_strings.dtd
> >--- a/mobile/android/base/locales/en-US/android_strings.dtd
> >+++ b/mobile/android/base/locales/en-US/android_strings.dtd
> >@@ -89,16 +89,24 @@
> 
> >+<!-- Localization note (find_text, find_prev, find_next, find_close) : These strings are used
> >+     as alternate text for accessibility. They are not visible in the UI. -->
> >+<!ENTITY find_text "Enter Find Text">
> >+<!ENTITY find_prev "Find Previous Match">
> >+<!ENTITY find_next "Find Next Match">
> >+<!ENTITY find_close "Close Find Bar">
> 
> Eitan, are these strings good for accessibility? I'm sure what conventions
> are around labeling UI widgets, so I just took a guess!

Looks good. Might be a bit too verbose. I think it makes sense to have one item be descriptive, for example the text entry (maybe "Find In Page"), and then the buttons would be understood in the entry's context. So "Previous", "Next" would be enough.

Typically, labels should be as descriptive as a real label you would put on such a button instead of an image, and less descriptive than tooltips.

I'm going to loop Marco in since he probably has stronger opinions.

Comment 24

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to Eitan Isaacson [:eeejay] from comment #23)

> Looks good. Might be a bit too verbose. I think it makes sense to have one
> item be descriptive, for example the text entry (maybe "Find In Page"), and
> then the buttons would be understood in the entry's context. So "Previous",
> "Next" would be enough.

I agree with Eitan on the strings: "Find in Page", "Next", "Previous", "Close"

Comment 25

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to Margaret Leibovic [:margaret] from comment #18)
> (In reply to Mark Finkle (:mfinkle) from comment #16)
> 
> > >diff --git a/mobile/android/base/resources/layout-v11/gecko_app.xml b/mobile/android/base/resources/layout-v11/gecko_app.xml
> > 
> > >+    <org.mozilla.gecko.FindInPageBar android:id="@+id/find_in_page"
> > 
> > I think we need to add this to our GeckoViewFactory for performance reason.
> 
> I'm not sure what you're referring to here. Do you mean just lazily adding
> the view to the main layout?

We have several "custom classes" we use in places like gecko_app.xml and Sriram found that the factory Android uses to "lookup" the class associated with the XML tag is slow. Slow enough that using a custom factory improved our UI startup time.

See http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/GeckoViewsFactory.java#38
for examples of other custom classes we support in our own factory

Comment 26

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 628155 [details] [diff] [review]
patch

r- mainly to get a new patch with the GeckoViewsFactory changes. Also make the string changes too.

Comment 27

[Marco Zehe (:MarcoZ)]

(In reply to Eitan Isaacson [:eeejay] from comment #23)
> > >+<!ENTITY find_text "Enter Find Text">
> > >+<!ENTITY find_prev "Find Previous Match">
> > >+<!ENTITY find_next "Find Next Match">
> > >+<!ENTITY find_close "Close Find Bar">

Oh yes, these are too verbose.

> Looks good. Might be a bit too verbose. I think it makes sense to have one
> item be descriptive, for example the text entry (maybe "Find In Page"), and
> then the buttons would be understood in the entry's context. So "Previous",
> "Next" would be enough.

Right. Localizers should be able to find the proper translation for their contexts easily. So, good with me.

Comment 28

[Marco Zehe (:MarcoZ)]

Comment on attachment 628155 [details] [diff] [review]
patch

f- for the reason given above, too verbose.

Comment 29

[:Margaret Leibovic]

Attachment: patch v2

Revised string and added view to GeckoViewsFactory.

Comment 30

[:Margaret Leibovic]

Attachment: Get rid of unused stuff in GeckoViewsFactory

Some cleanup.

Comment 31

[:Margaret Leibovic]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2389f26e4efa
https://hg.mozilla.org/integration/mozilla-inbound/rev/14a55152ab57

Comment 32

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 628399 [details] [diff] [review]
patch v2

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

::: mobile/android/chrome/content/browser.js
@@ +3222,5 @@
> +      this._findInProgress = true;
> +      this._targetTab = BrowserApp.selectedTab;
> +      this._find = Cc["@mozilla.org/typeaheadfind;1"].createInstance(Ci.nsITypeAheadFind);
> +      this._find.init(this._targetTab.browser.docShell);
> +      this._initialViewport = JSON.stringify(this._targetTab.viewport);

Does this viewport stuff work? I just realized that since I wrote the original WIP the viewport getter/setter have been replaced with getViewport() and setViewport().

Comment 33

[:Margaret Leibovic]

(In reply to Kartikaya Gupta (:kats) from comment #32)

> ::: mobile/android/chrome/content/browser.js
> @@ +3222,5 @@
> > +      this._findInProgress = true;
> > +      this._targetTab = BrowserApp.selectedTab;
> > +      this._find = Cc["@mozilla.org/typeaheadfind;1"].createInstance(Ci.nsITypeAheadFind);
> > +      this._find.init(this._targetTab.browser.docShell);
> > +      this._initialViewport = JSON.stringify(this._targetTab.viewport);
> 
> Does this viewport stuff work? I just realized that since I wrote the
> original WIP the viewport getter/setter have been replaced with
> getViewport() and setViewport().

Hrm, I'm not sure. When I tested the patch, I ensured find/next/previous all worked, but I didn't pay too close attention to viewport stuff. This probably needs a follow-up to get fixed.

Comment 34

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/2389f26e4efa
https://hg.mozilla.org/mozilla-central/rev/14a55152ab57

Comment 35

[Ian Barlow (:ibarlow)]

Comment on attachment 628156 [details]
screenshot

This looks awesome!

Comment 36

[:Margaret Leibovic]

A bad line made in into the Makefile:
https://hg.mozilla.org/integration/mozilla-inbound/rev/33e2ec979081

Comment 37

[Ed Morley [:emorley]]

(In reply to Margaret Leibovic [:margaret] from comment #36)
> A bad line made in into the Makefile:
> https://hg.mozilla.org/integration/mozilla-inbound/rev/33e2ec979081

https://hg.mozilla.org/mozilla-central/rev/33e2ec979081

Comment 38

[Cristian Nicolae (:xti)]

Find in Page feature was implemented and it's working fine on the latest Nightly. Closing bug as verified fixed on:

Firefox for Android
Version: 21.0a1 (2013-01-29)
Device: Galaxy R
OS: Android 2.3.4

Comment 39

[Kevin Brosnan [Ex-Mozilla]]

https://moztrap.mozilla.org/manage/case/299/ covers this feature in Tablet, ARMv6/7 and x86 test runs.