Closed Bug 541817 Opened 14 years ago Closed 14 years ago

Fennec needs find in page functionality (CTRL+F)

Categories

(Firefox for Android Graveyard :: General, enhancement)

Product:
Firefox for Android Graveyard
Component:
General
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED

People

(Reporter: webmaster, Assigned: vingtetun)

References

Details

Attachments

(1 file, 1 obsolete file)

Patch
30.47 KB, patch
mfinkle
: review+
Details | Diff | Splinter Review 
User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.3a1pre) Gecko/20100123 Minefield/3.7a1pre
Build Identifier: Fennec 1.0 on n900

There is no way to find in page using Fennec. CTRL + F is not working and I can find no other option.

Reproducible: Always

Steps to Reproduce:
Press CTRL +F 

Actual Results:  
Nothing

Expected Results:  
A box where I can enter a search term should open

It's CTRL+F on MicroB
Note bug 519833.
Status: UNCONFIRMED → NEW
Ever confirmed: true
OK, I did not search for closed bugs... However, the functionality is needed! I've found myself wanting to use it several times.
Blocks: 519833
I don't think just adding a search box would be a good idea, however the functionality is definitely needed (ever tried to navigate a Wikipedia page without it?)

How about adding this to the Awesomebar? Something like
1. Focus location bar
2. Enter string
3. An entry "Find in LOCATION" pops up, where location could either be the url of the currently viewed tab or the page title (title probably makes more sense).
4. If selected, the user is returned to the tab with all matches highlighted.
5. A previous/next/close panel is shown at the bottom.
6. Previous/Next jumps between matches (with the active match highlighted differently)
7. Closing the panel removes the highlights.
8. Leaving the page/closing the tab/... closes the panel.
Attached patch wip-0.1 - dirty wip (obsolete) — — Splinter Review 
This wip add a very basic Find in the site menu (and it only look for "test" at the moment! :))

Also the code looks duplicate with the FormHelper's code in browser-ui but I want to see what need to be share between these functionalities to try to do a better abstraction later.
I assume that the actual finding is broken right now.  I should probably rebase and land bug 514925.
Depends on: 514925
Attached patch Patch — — Splinter Review 
(In reply to comment #5)
> I assume that the actual finding is broken right now.  I should probably rebase
> and land bug 514925.

To be honest it works quite well for me, I wonder if this is related to the fact that I've get rid of the findbar.xml binding (it was doing too much) and communicate with the content through messages.


The patch add a working Find-In-Page functionality and a beginning of merge with the FormHelper code for the navigation button part to allow extensions developers to use it.
The style of the assistant has a few very small changes (mostly borders) but I guess this is not really important since this will change in a near future.
Assignee: nobody → 21
Attachment #457592 - Attachment is obsolete: true
Attachment #458306 - Flags: review?(mark.finkle)
Comment on attachment 458306 [details] [diff] [review]
Patch

>+      <field name="_contentSpacerHelper">
>+        document.getElementById(this.getAttribute("spacer"));
>+      </field>

"_spacer" is probably good enough. Matches the attribute

>+      <field name="_currentObject">null</field>

"_currentObject" is not descriptive enough. what about "_model" or "_ui" ?

>+      <method name="show">
>+        <parameter name="aObject" />

Change the name of this param too

>diff -r 36723271befb chrome/content/browser-ui.js

>+var FindHelperUI = {

>+  receiveMessage: function findHelperReceiveMessage(aMessage) {

>+      case "FindAssist:Show":
>+        if (json.rect) {
>+          this._zoom(Rect.fromRect(json.rect));
>+        }

No braces for single line

>+  show: function findHelperShow() {
>+    let bv = Browser._browserView;
>+    bv.ignorePageScroll(true);

No need for "bv" ? Just make it a one-liner

>+  hide: function findHelperHide() {
>+    let bv = Browser._browserView;
>+    bv.ignorePageScroll(false);

No need for "bv" ? Just make it a one-liner

>+  goToPrevious: function findHelperGoToPrevious() {
>+    Browser.selectedBrowser.messageManager.sendAsyncMessage("FindAssist:Previous", { });
>+  },
>+
>+  goToNext: function fingHelperGoToNext() {
>+    Browser.selectedBrowser.messageManager.sendAsyncMessage("FindAssist:Next", { });
>+  },

Might be nice to combine these with the FormHelper versions in a shared base class someday.

>+  _zoom: function _findHelperZoom(aElementRect) {
>+    let bv = Browser._browserView;
>+    let zoomRect = bv.getVisibleRect();
>+
>+    // Zoom to a specified Rect
>+    if (aElementRect && bv.allowZoom && Services.prefs.getBoolPref("findhelper.autozoom")) {
>+      let zoomLevel = Browser._getZoomLevelForRect(bv.browserToViewportRect(aElementRect.clone()));
>+      zoomLevel = Math.min(Math.max(kBrowserFormZoomLevelMin, zoomLevel), kBrowserFormZoomLevelMax);
>+
>+      zoomRect = Browser._getZoomRectForPoint(aElementRect.center().x, aElementRect.y, zoomLevel);
>+      Browser.animatedZoomTo(zoomRect);
>+    }
>+  }

This _zoom code is similar to the FormHelper._zoom code too, except for the caretRect part. Maybe we could push this code into a shared base class too - someday.

>-              <box id="form-helper-spacer" hidden="true"/>
>+              <box id="content-helper-spacer" hidden="true"/>

>-        <!-- popup for form helper -->
>-        <vbox id="form-helper-container" hidden="true" class="window-width" top="0" pack="end">
>+        <!-- popup for content navigator helper -->
>+        <vbox id="content-navigator" class="window-width" top="0" spacer="content-helper-spacer">

Can we name the spacer "content-navigator-spacer" ?

>diff -r 36723271befb chrome/content/content.js

>+      for (let i = 0; i < nodes.length; i++) {
>+        let node = nodes[i];
>+        if (node instanceof Components.interfaces.nsIDOMNSEditableElement && node.editor) {
>+          controller = node.editor.selectionController.getSelection(Components.interfaces.nsISelectionController.SELECTION_NORMAL);

Use Ci. instead of Components.interfaces.

>+#content-navigator:not([type="form"]) > #form-helper-autofill,
>+#content-navigator:not([type="form"]) > #select-container {
>+  visibility: collapse;
>+}
>+
>+#content-navigator:not([type="find"]) > #find-helper-textbox {
>+  visibility: collapse;
>+}

is "visibility: collapse" what we want here? not "display:none"? Just asking


Tests for FormHelper are currently broken. We should add some find tests when we get the formhelper tests fixed.
Attachment #458306 - Flags: review?(mark.finkle) → review+
>goToNext: function fingHelperGoToNext() {

s/fing/find
> >+#content-navigator:not([type="form"]) > #form-helper-autofill,
> >+#content-navigator:not([type="form"]) > #select-container {
> >+  visibility: collapse;
> >+}
> >+
> >+#content-navigator:not([type="find"]) > #find-helper-textbox {
> >+  visibility: collapse;
> >+}
> 
> is "visibility: collapse" what we want here? not "display:none"? Just asking
> 

We need it just for the autofill case otherwise the arrows of the arrowscrollbox are not updated correctly, I've update the code for that.

> 
> Tests for FormHelper are currently broken. We should add some find tests when
> we get the formhelper tests fixed.

Yes you're right, I will do that as soon as possible.

http://hg.mozilla.org/mobile-browser/rev/5df86aa2c582
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Can we also change the string in the site menu from "Find" to "Find in Page"
Flags: in-testsuite?
Flags: in-litmus?
Depends on: 581064
Can these be added?
-showing the number of matches on the page, 
-looking for a word while typing and narrowing down the results 
-firing the Find in page field when starting typing. 

I think the number of matches info can be particularly useful on the small screen.
(In reply to comment #11)
> Can these be added?

Not in this bug. Can you open a new bug for adding enhancements to the Find-in-page feature?

The current code should look for the word while typing, but you need to delay typing for about 500ms, I think. Finding on each key could be bad, since we zoom and scroll every time we search for the word.
Filed bug 582198.

Finding on each key and focusing in on the first result is not confusing and it reduces key strokes by allowing the user to observe when the results have been narrowed down to the term he is looking for, without having to type the whole word and lets him know if the word he's looking for is not there after the first few strokes. It's consistent without our "less typing required" strategy.
For implementation on looking for a word on each key see the default Android browser.
Verified fix on Mozilla/5.0 (X11; U; Linux armv7l; en-US; rv:2.0b4pre) Gecko/20100810 Namoroka/4.04pre Fennec/2.0a1pre.  

still need to check on android
Vlad, can you verify the fix on the android nightly?   when you're done, you can change Status to VERIFIED.  Thanks.
i just tried this on a android board, with a hard keyboard wired to it.  CTRL-F doesn't do anything.   

If someone has a android device with a hard keyboard, please verify if its working there.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: