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

VERIFIED FIXED in Firefox 17

Status

()

Firefox for Android
Text Selection
VERIFIED FIXED
6 years ago
2 years ago

People

(Reporter: kats, Assigned: Margaret)

Tracking

(Depends on: 1 bug, Blocks: 1 bug, {regression})

17 Branch
Firefox 17
All
Android
regression
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox17 verified, fennec17+)

Details

Attachments

(1 attachment, 1 obsolete attachment)

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.
Keywords: regression
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?

Updated

6 years ago
tracking-fennec: --- → ?
status-firefox17: --- → affected
(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
(Assignee)

Comment 3

6 years ago
(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 ;)
Do not change to actionbars on all versions of Android, only where appropriate
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
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.
Blocks: 718437
(Assignee)

Comment 7

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

Comment 8

6 years ago
Created attachment 649749 [details] [diff] [review]
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.
Assignee: nobody → margaret.leibovic
Attachment #649749 - Flags: feedback?(wjohnston)
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
Attachment #649749 - Flags: feedback?(wjohnston) → feedback+
(Assignee)

Comment 10

6 years ago
Created attachment 649833 [details] [diff] [review]
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".
Attachment #649749 - Attachment is obsolete: true
Attachment #649833 - Flags: review?(wjohnston)
tracking-fennec: ? → 17+

Updated

6 years ago
Duplicate of this bug: 782161

Comment 12

6 years ago
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.
Summary: Long-press on a text field to "Add as search engine" doesn't work any more → Unable to invoke the context menu by long tapping on input or textareas in content
Attachment #649833 - Flags: review?(wjohnston) → review+
(Assignee)

Comment 13

6 years ago
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!
Target Milestone: --- → Firefox 17
(Assignee)

Updated

6 years ago
Depends on: 782650
https://hg.mozilla.org/mozilla-central/rev/295552661955
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED

Updated

6 years ago
Status: RESOLVED → VERIFIED
status-firefox17: affected → verified
Duplicate of this bug: 783846
Depends on: 784016
You need to log in before you can comment on or make changes to this bug.