Use any current Text Selection when main menu Find In Page is selected

RESOLVED FIXED in Firefox 25

Status

()

defect
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: capella, Assigned: capella)

Tracking

Trunk
Firefox 25
ARM
Android
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

Posted patch bugfind (v1) (obsolete) — Splinter Review
This would make mobile similar to desktop ... any selected text string would be used when the main menu 'Find in Page' is clicked to start the search.
Attachment #779469 - Flags: review?(margaret.leibovic)
Comment on attachment 779469 [details] [diff] [review]
bugfind (v1)

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

I haven't tried out a build with this patch, but it's looking good so far. I have some comments, but I'd also like to get some UX feedback just to make sure this is a feature we want (it sounds like a good feature to me, especially since it matches desktop).

::: mobile/android/base/FindInPageBar.java
@@ +61,5 @@
>          mInflated = true;
> +        registerEventListener("TextSelection:SelectedText");
> +    }
> +
> +    private void registerEventListener(String event) {

No need to make this helper method if it's only used once. You can just put the GeckoAppShell call directly in inflateContent.

@@ +62,5 @@
> +        registerEventListener("TextSelection:SelectedText");
> +    }
> +
> +    private void registerEventListener(String event) {
> +        GeckoAppShell.getEventDispatcher().registerEventListener(event, this);

We should unregister this listener when this view is destroyed. You can do this by creating an onDestroy method and calling mFindInPageBar.onDestroy in BrowserApp.onDestroy.

@@ +84,5 @@
> +                    if (!hasFocus)
> +                        return;
> +
> +                    mFindText.setOnWindowFocusChangeListener(null);
> +                    getInputMethodManager(mFindText).showSoftInput(mFindText, 0);

Please put the formatting fixes in a separate cleanup patch.

@@ +89,5 @@
>                 }
>              });
>          }
> +
> +        GeckoAppShell.sendEventToGecko(GeckoEvent.createBroadcastEvent("TextSelection:GetSelectedText", LOGTAG));

It seems like it would be better to do this earlier, then handle setting the text and focus, and showing the keyboard, all in the message handler. I am hoping it wouldn't feel slow to wait for the message to come back, but it would also look bad for there to be a delay in the text showing up in the EditText.

@@ +143,5 @@
> +    // GeckoEventListener implementation
> +
> +    @Override
> +    public void handleMessage(String event, JSONObject message) {
> +        if (event.equals("TextSelection:SelectedText") && message.optString("origin").equals(LOGTAG)) {

It's safer to do LOGTAG.equals(message...), since we know LOGTAG will never be null. Also, if you're only ever handling one message in this method, I would prefer if we return early if we don't get what we expect, just to reduce the indentation level for the rest of the method.

@@ +146,5 @@
> +    public void handleMessage(String event, JSONObject message) {
> +        if (event.equals("TextSelection:SelectedText") && message.optString("origin").equals(LOGTAG)) {
> +            final String text = message.optString("text");
> +            if (!TextUtils.isEmpty(text)) {
> +                mFindText.setOnWindowFocusChangeListener(null);

What is this line for?

::: mobile/android/chrome/content/SelectionHandler.js
@@ +131,5 @@
> +      case "TextSelection:GetSelectedText": {
> +        sendMessageToJava({
> +          type: "TextSelection:SelectedText",
> +          origin: aData,
> +          text: (this._activeType == this.TYPE_SELECTION) ? this._getSelectedText() : null

I think it's safe to just call this._getSelectedText(), which will just return an empty string if there's no selected text.
Attachment #779469 - Flags: review?(margaret.leibovic) → feedback+
Ian, I just want to put you in the loop about this change. Capella is working on a patch here to automatically fill the find bar with any text that is currently selected, which is what desktop does. I think this sounds like a nice feature, what do you think?
Flags: needinfo?(ibarlow)
Fantastic feedback as usual !

re: What is this line for?
mFindText.setOnWindowFocusChangeListener(null);

If selected text is available, that particular string is |found| and highlit in green. This line of code prevents the softKBD from openning and obscurring that found text if it's near the bottom of the screen.

I found it fairly natural, in that if I select text and select |find| in the menu, then more often than not, I don't need the KBD open, as my next choices will be clicking the search up and down keys to scan for further instances of the string, versus immediately modifying the string. (Tapping the custom edittext to modify the contents then opens the KBD as always).

re: It seems like it would be better to do this earlier ...

We can move up generating the request for the selected text earlier in the method before the |open KBD /or set focus listener to do it later| piece, but I think in testing I had seen an odd problem where we can't guarantee a response message being responded to / generated in SelectionHandler... so I left the default FindInPage bar behaviour to assume no initial text selection, and to open the keyboard for the user unless I null the focuschangelistener.

I'm open of course to changing the UI behaviour to streamline, (don't open automatically ever?), but didn't go that extra mile yet, waiting for your advice.

The rest is cool! Don't need the extra registerListener method I cut'n'paste from somewhere else, hadn't figured out how to unregister now I do, format fixs will come first standalone, LOGTAG compare, early escape, and SelectionHandler tweak WFM  :-D
Posted patch bug896764 (obsolete) — Splinter Review
Oh! This is better ... I factored in the fact that SelectionHandler is lazy loaded by browser.js by passing the Selection:Get request through there ... this always provides a response message back to FindInPageBar and lets it properly handle the decision to open / not open the softInput accordingly.
Attachment #779469 - Attachment is obsolete: true
Attachment #781484 - Flags: review?(margaret.leibovic)
(In reply to :Margaret Leibovic from comment #2)
> Ian, I just want to put you in the loop about this change. Capella is
> working on a patch here to automatically fill the find bar with any text
> that is currently selected, which is what desktop does. I think this sounds
> like a nice feature, what do you think?

Yes! :)
Flags: needinfo?(ibarlow)
Comment on attachment 781484 [details] [diff] [review]
bug896764

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

Looking good! I mostly have comments about renaming things, but I'll be ready to give this an r+ with these comments addressed.

Let's rename these message "SelectedText:Data" and the other one "SelectedText:Get". This would be consistent with our current Preferences:Get/Data, CharEncoding:Get/Data and SearchEngines:Get/Data messages.

::: mobile/android/base/FindInPageBar.java
@@ +67,5 @@
>              inflateContent();
>  
>          setVisibility(VISIBLE);
>          mFindText.requestFocus();
> +        GeckoAppShell.sendEventToGecko(GeckoEvent.createBroadcastEvent("Selection:GetSelectedText", LOGTAG));

Instead re-purposing LOGTAG here, I'd rather see us declare a REQUEST_ID string. In fact, LOGTAG isn't even used (and it's named "GeckoFindInPagePopup", which doesn't really make sense), so let's replace that with a REQUEST_ID string (updated to "FindInPageBar" or something like that) and use that here.

Also, add a comment that the response handler will deal with the rest of the stuff we want to do when showing the find bar.

@@ +125,5 @@
> +    // GeckoEventListener implementation
> +
> +    @Override
> +    public void handleMessage(String event, JSONObject message) {
> +        if (event.equals("Selection:SelectedText") && LOGTAG.equals(message.optString("origin"))) {

Just return early if the event/request id don't match what you expect.

@@ +156,5 @@
> +                @Override
> +                public void run() {
> +                    mFindText.setText(text);
> +                }
> +            });

Since this bit is much smaller, let's put this in an |if (!TextUtils.isEmpty(text))| statement that returns early, then leave the existing logic you moved in here as a default case.

::: mobile/android/chrome/content/SelectionHandler.js
@@ +233,5 @@
>    _getSelection: function sh_getSelection() {
>      if (this._targetElement instanceof Ci.nsIDOMNSEditableElement)
>        return this._targetElement.QueryInterface(Ci.nsIDOMNSEditableElement).editor.selection;
>      else
> +      return (this._contentWindow ? this._contentWindow.getSelection() : null);

Oh, I forgot that this._contentWindow could be null if the selection isn't active... instead of doing this, let's just have a check to see if there's an active selection in getSelectedText, and if there isn't, we should just bail early and return an empty string.

@@ +238,3 @@
>    },
>  
> +  getSelectedText: function sh_getSelectedText() {

Nice call updating this to get rid of the underscore, since it's used publicly now.

::: mobile/android/chrome/content/browser.js
@@ +1443,5 @@
>  
> +      case "Selection:GetSelectedText":
> +        sendMessageToJava({
> +          type: "Selection:SelectedText",
> +          origin: aData,

Is there any particular reason why you decided to pass this origin property along? It looks similar to how we pass a requestId for the Preferences:Get/Data messages, but that was introduced mainly because we had multiple consumers calling Preferences:Get with requests for different kinds of data, whereas only one kind of data will be returned here.

However, thinking about this a bit, this seems like a smart preventative move, in case anyone else ever starts requesting the selected text (we wouldn't want that to end up populating the find bar), so we can keep this. But let's rename this property requestId, to be consistent with the Preferences messages.
Attachment #781484 - Flags: review?(margaret.leibovic) → feedback+
awesome.
Posted patch bug896764Splinter Review
So now the patch looks like this ...
Attachment #781484 - Attachment is obsolete: true
Attachment #782096 - Flags: review?(margaret.leibovic)
Comment on attachment 782096 [details] [diff] [review]
bug896764

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

Nice work!

::: mobile/android/base/FindInPageBar.java
@@ +24,1 @@
>      private static final String LOGTAG = "GeckoFindInPagePopup";

Delete this line, since LOGTAG isn't used.

::: mobile/android/chrome/content/SelectionHandler.js
@@ +239,5 @@
>  
> +  getSelectedText: function sh_getSelectedText() {
> +    if (!this._contentWindow) {
> +      return "";
> +    }

Nit: no braces, to be consistent with the other code around this.
Attachment #782096 - Flags: review?(margaret.leibovic) → review+
Finkle's late feedback:

Could we file a followup to let SelectionHandler.js handle the "SelectedText:Xxx" messages? Maybe renaming to "TextSelection:Xxx"?

If that makes sense. I like to move as much out of BrowserApp as possible. We could probably make the followup move even more out of BrowserApp. I see "keyword-search" in there, but we have a SearchEngines object. We might be able to move some of those Session and Tab messages out too. Just thinking out loud here. I don't want to rathole on this idea though :)
(In reply to Mark Finkle (:mfinkle) from comment #12)
> Finkle's late feedback:
> 
> Could we file a followup to let SelectionHandler.js handle the
> "SelectedText:Xxx" messages?

Oops, yeah, I should have caught that. Let's file a follow-up to do that. And then we could make _getSelectedText "private" again.

> Maybe renaming to "TextSelection:Xxx"?

But I don't know if I like the renmaing, since these messages are about getting the selected text itself, and "SelectedText:Xxx" makes that clearer.

> If that makes sense. I like to move as much out of BrowserApp as possible.
> We could probably make the followup move even more out of BrowserApp. I see
> "keyword-search" in there, but we have a SearchEngines object.

That one was originally added to set the userSearch propertly on the selected tab, so that might actually sense to be in BrowserApp (otherwise SearchEngines would need to make a call to BrowserApp.selectedTab). Although that "Search:Keyword" message doesn't have anything to do with BrowserApp (that was added for FHR), so maybe it is time for this notification handling to move now.

> We might be
> able to move some of those Session and Tab messages out too. Just thinking
> out loud here. I don't want to rathole on this idea though :)

File some mentor bugs about these!
Wait, I'm confused.

The new messages we settled on "SelectedText:Get" and "SelectedText:Data" are being requested and returned to FindInPageBar.java by browser.js ...

I originally had them being caught / processed / returned by SelectionHandler.js but moved them into browser.js since SelectionHandler is lazy loaded and we can't be guaranteed that the messages will have a handler available unless browser.js does it's thing.

Did I miss a trick?
Ohhhhh ! Maybe crossover chatter from Bug 582581 - Auto-populate the text in the Find-in-page textbox with search engine query ?
But there is a trick too:
https://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/browser.js#97

That list of objects and scripts are lazy loaded when either:
1. JS script uses the object name (this is normally lazy load)
2. A notification is received that the object can handle
nope - still not getting it ... (my JS isn't my strongest point) ... I'll have to chat with you or margaret in IRC ...

What I saw in testing was that GeckoMessages weren't being received in SelectionHandler in the first patch https://bugzilla.mozilla.org/page.cgi?id=splinter.html&bug=896764&attachment=779469 when requested from FindInPageBar.java at FF startup if no text selection had yet been made (current use case).

Basically, how would SelectionHandler be lazy loaded to receive / handle a message that it hasn't yet (been loaded and) added an observer for?

So again, I'm sure my JS is letting me down.
Oh! hah ... I see the trick now :-)
https://hg.mozilla.org/mozilla-central/rev/c28a75832426
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 25
You need to log in before you can comment on or make changes to this bug.