780906 - Unable to invoke the context menu by long tapping on input or textareas in content

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: Text Selection, defect)

Description

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

Bug 765079 changed the behaviour of long-press on a text field so that it invokes the text-selection code. However this breaks the ability to add arbitrary text fields as search engines because now the context menu doesn't come up. I'm not sure what the desired fix is to be able to get both behaviours, but I think at least we should show the context menu if the text field is empty.

Comment 1

[Wesley Johnston (:wesj)]

I wonder if we can change to showing an actionbar in this case rather than the context menu? We have a custom actionbar implementation, so things should work across all Android versions, right?

Comment 2

[Sriram Ramasubramanian [:sriram]]

(In reply to Wesley Johnston (:wesj) from comment #1)
> I wonder if we can change to showing an actionbar in this case rather than
> the context menu? We have a custom actionbar implementation, so things
> should work across all Android versions, right?

Just today morning I was thinking of this issue, and felt, we should be showing an actionbar :D

Comment 3

[:Margaret Leibovic]

(In reply to Wesley Johnston (:wesj) from comment #1)
> I wonder if we can change to showing an actionbar in this case rather than
> the context menu? We have a custom actionbar implementation, so things
> should work across all Android versions, right?

Would we add some custom actionbar item for "add as search engine"? On the stock browser on gingerbread, a long press always opens a context menu, and if no text is selected yet, it offers a context menu with options to select text. Maybe we can do that until we fix bug 768667?

(In reply to Sriram Ramasubramanian [:sriram] from comment #2)
> (In reply to Wesley Johnston (:wesj) from comment #1)
> > I wonder if we can change to showing an actionbar in this case rather than
> > the context menu? We have a custom actionbar implementation, so things
> > should work across all Android versions, right?
> 
> Just today morning I was thinking of this issue, and felt, we should be
> showing an actionbar :D

You can take on bug 768667 next if you want ;)

Comment 4

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

Do not change to actionbars on all versions of Android, only where appropriate

Comment 5

[Wesley Johnston (:wesj)]

Y(In reply to Margaret Leibovic [:margaret] from comment #3)
> Would we add some custom actionbar item for "add as search engine"?

Yeah, I'd probably show Cut/Copy/Paste, and put Add Search Engine in the overflow. That seems to fit better with what Google suggests doing (although, like you say, they don't always follow it in their own apps, so much so that I'd say their apps are horrible things to look to for good Android design patterns).

See: http://developer.android.com/design/patterns/actionbar.html#contextual

Comment 6

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

Also, I point out in bug 765079 that a long tap on a textbox always makes the contextmenu appear on Gingerbread and Froyo. Two menu items, "Select All" and "Select Word", are used to enter text selection mode.

Comment 7

[:Margaret Leibovic]

(In reply to Mark Finkle (:mfinkle) from comment #6)
> Also, I point out in bug 765079 that a long tap on a textbox always makes
> the contextmenu appear on Gingerbread and Froyo. Two menu items, "Select
> All" and "Select Word", are used to enter text selection mode.

See comment 3. I think we should just do this for now, then add the action bar support for ICS/JB in bug 768667.

Comment 8

[:Margaret Leibovic]

Attachment: WIP to add pre-ICS contextmenu functionality

The problem with this patch is that SelectionHandler's custom context menu with Select All/Copy/Share items doesn't appear anymore because the input's default context menu is taking over. That context menu should probably just be added with the contextmenu API instead of doing it in startSelection like we're currently doing.

Comment 9

[Wesley Johnston (:wesj)]

Comment on attachment 649749 [details] [diff] [review]
WIP to add pre-ICS contextmenu functionality

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

I like passing the coordinates. If we have that, I think we can yank the custom menu stuff in SelectionHelper. Then everything should just work!

::: mobile/android/chrome/content/browser.js
@@ +1379,5 @@
>        }
>  
>        // only send the contextmenu event to content if we are planning to show a context menu (i.e. not on every long tap)
> +      if (this.menuitems) {
> +        this._tapPoint = { x: aX, y: aY };

I don't think we need to cache this. We can get it from the event later?

@@ +1574,5 @@
> +    let selectWord = Strings.browser.GetStringFromName("contextmenu.selectWord");
> +    let selectAll = Strings.browser.GetStringFromName("contextmenu.selectAll");
> +
> +    // |this| in a contextmenu callback refers to the contextmenu item
> +    let callback = function(aElement, aPoint) {

name this function

Comment 10

[:Margaret Leibovic]

Attachment: move text selection context menu logic into ClipboardHelper

My one concern with this patch is that we show lots of context menu items when long tapping on some selected text. In the stock browser on gingerbread, they show a "Edit text" context menu that only has cut/copy/paste when long tapping on an active selection, so maybe we do want to do more to treat text selection as a special case. However, you'll definitely be able to access all your context menu items with this patch!

Also, I don't like having both "Copy" and "Copy All". Now that we can select text, I feel like we can get rid of "Copy All".

Comment 12

[Zac C (:zac)]

I found this same issue while trying to change the Input Method on an empty field in facebook.

Although it sounds like Margaret's patch repairs this, we can add changing input method as another use case for opening the context menu when no text is present in the text field.

Comment 13

[:Margaret Leibovic]

https://hg.mozilla.org/integration/mozilla-inbound/rev/295552661955

I'm going to file a follow-up and talk to Ian about these long context menus, but at least now you can access them again!

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/295552661955