The default bug view has changed. See this FAQ.

Status

()

Firefox for Android
General
P3
normal
VERIFIED FIXED
6 years ago
8 months ago

People

(Reporter: elan, Assigned: Margaret)

Tracking

({pp, relnote})

unspecified
Firefox 15
ARM
Android
pp, relnote
Points:
---
Dependency tree / graph
Bug Flags:
in-moztrap +

Firefox Tracking Flags

(firefox21 verified, blocking-fennec1.0 -)

Details

(Whiteboard: [QA+] [sumo][parity-xul][parity-stock][parity-desktop])

Attachments

(4 attachments, 4 obsolete attachments)

Comment hidden (empty)
Priority: P1 → P3

Updated

6 years ago
Whiteboard: [QA+]
Assignee: nobody → kgupta
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
(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?
(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!
Keywords: uiwanted
Created attachment 581456 [details] [diff] [review]
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.
Some sample designs can be found here:
http://www.flickr.com/photos/patrykdesign/6538751287/in/set-72157628485933281/
tracking-fennec: --- → 11+
tracking-fennec: 11+ → 12+
Keywords: uiwanted
blocking-fennec1.0: --- → ?
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

5 years ago
not blocking fennec-native1.0.
blocking-fennec1.0: ? → -
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.
Whiteboard: [QA+] → [QA+] [sumo]
Keywords: pp, relnote
Whiteboard: [QA+] [sumo] → [QA+] [sumo][parity-xul][parity-stock][parity-desktop]

Comment 9

5 years ago
SUMO: I've documented this in the Search and Find tools and Fix Problems articles on SUMO.
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.
Assignee: bugmail.mozilla → nobody
tracking-fennec: 12+ → ?
(Assignee)

Comment 11

5 years ago
I can pick this up!
Assignee: nobody → margaret.leibovic
(Assignee)

Comment 12

5 years ago
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).

-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.
Attachment #581456 - Attachment is obsolete: true
Attachment #626549 - Flags: feedback?(mark.finkle)
(Assignee)

Comment 13

5 years ago
Created attachment 626551 [details]
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.
Attachment #626551 - Flags: feedback?(padamczyk)
Attachment #626551 - Flags: feedback?(ibarlow)
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....
(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 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)
Attachment #626549 - Flags: feedback?(mark.finkle) → feedback+
Created attachment 627223 [details]
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 :)
(Assignee)

Updated

5 years ago
Depends on: 759513
(Assignee)

Comment 18

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

Updated

5 years ago
No longer depends on: 759513
(Assignee)

Comment 19

5 years ago
Created attachment 628155 [details] [diff] [review]
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?
Attachment #626549 - Attachment is obsolete: true
Attachment #628155 - Flags: review?(mark.finkle)
(Assignee)

Comment 20

5 years ago
Created attachment 628156 [details]
screenshot

Ian, what do you think? I took the EditText styles from the AwesomeBar as you suggested.
Attachment #626551 - Attachment is obsolete: true
Attachment #626551 - Flags: feedback?(padamczyk)
Attachment #626551 - Flags: feedback?(ibarlow)
Attachment #628156 - Flags: feedback?(ibarlow)
(Assignee)

Comment 21

5 years ago
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!
Attachment #628155 - Flags: feedback?(eitan)
(Assignee)

Comment 22

5 years ago
(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 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.
Attachment #628155 - Flags: feedback?(eitan) → feedback?(marco.zehe)
(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"
(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 on attachment 628155 [details] [diff] [review]
patch

r- mainly to get a new patch with the GeckoViewsFactory changes. Also make the string changes too.
Attachment #628155 - Flags: review?(mark.finkle) → review-
(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 on attachment 628155 [details] [diff] [review]
patch

f- for the reason given above, too verbose.
Attachment #628155 - Flags: feedback?(marco.zehe) → feedback-
(Assignee)

Comment 29

5 years ago
Created attachment 628399 [details] [diff] [review]
patch v2

Revised string and added view to GeckoViewsFactory.
Attachment #628155 - Attachment is obsolete: true
Attachment #628399 - Flags: review?(mark.finkle)
(Assignee)

Comment 30

5 years ago
Created attachment 628400 [details] [diff] [review]
Get rid of unused stuff in GeckoViewsFactory

Some cleanup.
Attachment #628400 - Flags: review?(mark.finkle)
Attachment #628399 - Flags: review?(mark.finkle) → review+
Attachment #628400 - Flags: review?(mark.finkle) → review+
(Assignee)

Comment 31

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/2389f26e4efa
https://hg.mozilla.org/integration/mozilla-inbound/rev/14a55152ab57
Target Milestone: --- → Firefox 15
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().
(Assignee)

Comment 33

5 years ago
(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.
https://hg.mozilla.org/mozilla-central/rev/2389f26e4efa
https://hg.mozilla.org/mozilla-central/rev/14a55152ab57
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED

Updated

5 years ago
Depends on: 760085

Updated

5 years ago
Depends on: 760087

Updated

5 years ago
Depends on: 760089

Updated

5 years ago
Depends on: 760090
Comment on attachment 628156 [details]
screenshot

This looks awesome!
Attachment #628156 - Flags: feedback?(ibarlow) → feedback+
(Assignee)

Comment 36

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

Updated

5 years ago
Depends on: 760196

Updated

5 years ago
Depends on: 760205

Updated

5 years ago
Depends on: 760238
(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
(Assignee)

Updated

5 years ago
Depends on: 760612
Depends on: 762309
(Assignee)

Updated

5 years ago
Depends on: 764638
(Assignee)

Updated

5 years ago
Depends on: 765270

Updated

5 years ago
Depends on: 766723
Depends on: 767797
Depends on: 768835
Depends on: 760223
(Assignee)

Updated

5 years ago
Depends on: 769388
(Assignee)

Updated

5 years ago
No longer depends on: 768835

Updated

5 years ago
Depends on: 771525

Updated

5 years ago
Alias: find
Summary: Find on page → Find In Page

Updated

5 years ago
Depends on: 771997

Updated

5 years ago
Depends on: 771989

Updated

5 years ago
Depends on: 774096

Updated

5 years ago
Depends on: 772449

Updated

5 years ago
Flags: in-moztrap?(kbrosnan)
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
Status: RESOLVED → VERIFIED
status-firefox21: --- → verified
https://moztrap.mozilla.org/manage/case/299/ covers this feature in Tablet, ARMv6/7 and x86 test runs.
Flags: in-moztrap?(kbrosnan) → in-moztrap+
tracking-fennec: ? → ---
You need to log in before you can comment on or make changes to this bug.