All input fields labeled with a "go" button, which doesn't do anything without special handling

VERIFIED FIXED

Status

VERIFIED FIXED
8 years ago
7 years ago

People

(Reporter: aaronmt, Assigned: blassey)

Tracking

({dev-doc-complete, inputmethod, polish})

Trunk
ARM
Android
dev-doc-complete, inputmethod, polish
Dependency tree / graph
Bug Flags:
in-testsuite ?
in-litmus ?

Details

(Whiteboard: [VKB])

Attachments

(2 attachments, 6 obsolete attachments)

(Reporter)

Description

8 years ago
Mozilla/5.0 (Android; Linux arm7l; rv.2.0b5pre) Gecko/20100823 Namoroka/4.0b5pre Fennec/2.0a1pre

Currently on the above build the button on the virtual keyboard for URL input is titled "Execute" on my Android phone. This is poor choice and should be renamed to "Go" or "Open" to match that of the Firefox desktop experience, not "Execute".
tracking-fennec: --- → ?
Created attachment 468509 [details] [diff] [review]
Patch v1

Patch makes the keyboard use "Go" as the label.

However, in other browsers the button is also used for navigating forms (i.e. the button says "Next" if there are more than one input fields). I'm not sure if we should integrate that here.
Assignee: nobody → jchen
Keywords: inputmethod
(Assignee)

Updated

8 years ago
tracking-fennec: ? → 2.0b2+
(Assignee)

Updated

8 years ago
Duplicate of this bug: 596626
(Assignee)

Updated

8 years ago
Duplicate of this bug: 596629
Summary: Rename "Execute" on the Android Virtual Keyboard for URL input → [VKB]Rename "Execute" on the Android Virtual Keyboard for URL input
Duplicate of this bug: 599366
Flags: in-litmus?(nhirata.bugzilla)
(In reply to comment #1)
> However, in other browsers the button is also used for navigating forms (i.e.
> the button says "Next" if there are more than one input fields). I'm not sure
> if we should integrate that here.
If we aren't integrating, bug 599366 should be unduped.

Comment 7

8 years ago
This patch looks fine to me - anything is better than Execute. I think we should just switch to Go for now and fix it correctly in another bug.
set in-litmus flag back to ? until the bug lands.  When the bug lands both litmus test cases mentioned above should be revisited.
Flags: in-litmus+ → in-litmus?
Summary: [VKB]Rename "Execute" on the Android Virtual Keyboard for URL input → Rename "Execute" on the Android Virtual Keyboard for URL input
Whiteboard: [VKB]
(Assignee)

Updated

8 years ago
tracking-fennec: 2.0b2+ → 2.0b3+
Duplicate of this bug: 605614

Updated

8 years ago
Assignee: jimnchen+bmo → blassey.bugs

Updated

8 years ago
Keywords: polish
(Assignee)

Updated

8 years ago
Attachment #468509 - Flags: review-

Updated

8 years ago
Assignee: blassey.bugs → mwu
taking
Assignee: mwu → blassey.bugs
Created attachment 491118 [details] [diff] [review]
patch

not sure if this is ready for review yet, but I'd like to get some feedback on it.
Attachment #468509 - Attachment is obsolete: true
Attachment #491118 - Flags: feedback?(roc)
This seems kinda reasonable but you really should be asking a DOM peer who knows HTML5 forms well ... Jonas, Mounir, etc
Created attachment 491195 [details] [diff] [review]
patch
Attachment #491118 - Attachment is obsolete: true
Attachment #491195 - Flags: review?(roc)
Attachment #491118 - Flags: feedback?(roc)
(Assignee)

Updated

8 years ago
Attachment #491195 - Flags: review?(jonas)
Created attachment 491211 [details] [diff] [review]
patch

updated so textareas have return keys
Attachment #491195 - Attachment is obsolete: true
Attachment #491211 - Flags: review?(roc)
Attachment #491211 - Flags: review?(jonas)
Attachment #491195 - Flags: review?(roc)
Attachment #491195 - Flags: review?(jonas)
Created attachment 491226 [details] [diff] [review]
m-b patch

label fennec's url bar with "go"
Attachment #491226 - Flags: review?(mark.finkle)
(Assignee)

Updated

8 years ago
Blocks: 599365
(Assignee)

Updated

8 years ago
Depends on: 581596
Attachment #491226 - Flags: review?(mark.finkle) → review+
Comment on attachment 491211 [details] [diff] [review]
patch

The interface changes are not ok unfortunately. We aren't supposed to change any interfaces past beta7 unless really needed. I'd recommend you just remove them and call getAttribute instead, that'll work as well.

Could we only honor this attribute for chrome-inputs? My concern is what happens if a webpage sticks a very long string as hint on an <input>. Will that break the keyboard?
Comment on attachment 491211 [details] [diff] [review]
patch

anyhow, r- on the content parts since they are entirely the interface changes. So just change the code to use getAttribute instead.

Unfortunately I'm not qualified to review the IME changes. Maybe smaug is the right person for those?
Attachment #491211 - Flags: review?(jonas)
Attachment #491211 - Flags: review?(Olli.Pettay)
Attachment #491211 - Flags: review-
Created attachment 492529 [details] [diff] [review]
patch
Attachment #491211 - Attachment is obsolete: true
Attachment #492529 - Flags: review?(roc)
Attachment #491211 - Flags: review?(roc)
Attachment #491211 - Flags: review?(Olli.Pettay)
(In reply to comment #16)
> Comment on attachment 491211 [details] [diff] [review]
> patch
> 
> The interface changes are not ok unfortunately. We aren't supposed to change
> any interfaces past beta7 unless really needed. I'd recommend you just remove
> them and call getAttribute instead, that'll work as well.
> 
> Could we only honor this attribute for chrome-inputs? My concern is what
> happens if a webpage sticks a very long string as hint on an <input>. Will that
> break the keyboard?

for android (which is all this implements) we're translating these strings into set flags using by the OS. If the string doesn't match we pass it to the keyboard in the actionLabel field, which is ignored by the keyboard I've been testing on.

I'd consider it to be up to the keyboard implementation to make sure that label doesn't break things, but given that we're not relying or expecting it to work, I wouldn't mind dropping that fallback.
+        if (aContent->Tag() == nsGkAtoms::input) {    nsAutoString nextCheck;

Insert line break

+    if (aContent && (aContent->Tag() == nsGkAtoms::input ||
+                     aContent->Tag() == nsGkAtoms::textarea)) {

I think we should be checking the namespace here too. We don't want to alter things for non-HTML content.

+          aContent->GetAttr(kNameSpaceID_None, nsGkAtoms::moz_action_hint,
+                            nextCheck);

I'm confused. You just set context.mActionHint to moz_action_hint, now you're getting it again?
Created attachment 492570 [details] [diff] [review]
patch
Attachment #492529 - Attachment is obsolete: true
Attachment #492570 - Flags: review?(roc)
Attachment #492529 - Flags: review?(roc)
What is a "multiline" action?

I'm a bit nervous here that you're creating Web-accessible API that maps directly onto some Android API. And at a very late date in the release cycle. Should we restrict it to chrome documents?
(In reply to comment #22)
> What is a "multiline" action?
I suppose "multiline" isn't actually an action, but the intent is to make sure we have a return key for multi-line text boxes.

> I'm a bit nervous here that you're creating Web-accessible API that maps
> directly onto some Android API. And at a very late date in the release cycle.
> Should we restrict it to chrome documents?

not using actionLabel for the open ended fallback would probably limit the risk. But I can see if you also want to restrict it to chrome.

Comment 24

8 years ago
This bug has morphed into a number of issues, apparently, and I don't believe it should block b3. Per the current bug description, this bug is actually fixed by bug 581596 since there should be no instance in which we actually show an Execute button.

The patch that is currently attached addresses these issues instead:

1. Removes the extract text UI in portrait mode.

While I agree with this change, I don't think this should land for b3 as it actually breaks the usability of entering text in landscape. This is because both the form helper and awesomebar pop up in landscape, leaving maybe 20 pixels or so for the form itself. Helpfully, fennec doesn't even center the form field correctly within the tiny available space. Even if the field were centered correctly, as soon as the user begins typing, the suggestions popup consumes the remaining space available for content.

2. Attempts to set the action button to next where possible. I discussed this with madhava on irc and we've agreed that the next button is redundant when the form helper is visible. In that case, labeling the button as "Go" is preferred.

3. Attempts to inform the IME of a multiline input field. A good idea. The implementation is unnecessarily tangled with mozactionhint.

I would prefer having separate bugs for each of these issues.
tracking-fennec: 2.0b3+ → ?
Yeah that sounds good.

Clearly "multiline" should be an independent flag rather than an action hint.
Three must be some mis communication, because I also discussed this with madhava on irc, After the landing of bug 581596 we label any HTML5 input with a go button. In that Go == navigate, this isn't right. Also, this button is treated like the return key, so it doesn't do anything in normal fields if there isn't a submit in the form and it just adds new lines to text areas.

If we were to simplify this to only correct that issue, we'd drop the extract text UI removal, the auto "next"ing of form inputs and the multiline bit. The rest can be added and discussed in follow ups. The effect would be that normal textfields would have a return key and we can add mozactionhit to the url bar to get the "go" key.
Summary: Rename "Execute" on the Android Virtual Keyboard for URL input → All input fields labeled with a "go" button, which doesn't do anything without special handling
Created attachment 492749 [details] [diff] [review]
patch

here's a reduced patch. Filed bug 614355 for not using fullscreen landscape keyboards and bug 614356 to default to the next button for form inputs. I'll follow up with something to deal with multiline inputs later
Attachment #492570 - Attachment is obsolete: true
Attachment #492749 - Flags: review?(roc)
Attachment #492570 - Flags: review?(roc)
Comment on attachment 492749 [details] [diff] [review]
patch

We're still exposing this to Web content. I think that's OK, given the prefix, but we should still document what the supported values are and what they do ... somewhere in the code as well as on MDC/MDN.
Attachment #492749 - Flags: review?(roc) → review+

Updated

8 years ago
tracking-fennec: ? → 2.0b3+
(Assignee)

Updated

8 years ago
Keywords: dev-doc-needed
pushed http://hg.mozilla.org/mozilla-central/rev/2aceea4f527b and http://hg.mozilla.org/mobile-browser/rev/93c37330e654
Status: NEW → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED

Updated

8 years ago
Blocks: 614488

Comment 30

8 years ago
Removing dev-doc-needed because I think we should not expose mozactionhint to web content at all. Bug 614488 will remove mozactionhint.
Keywords: dev-doc-needed
Comment on attachment 492570 [details] [diff] [review]
patch


>@@ -288,32 +289,50 @@ class GeckoSurfaceView
>     @Override
>     public InputConnection onCreateInputConnection(EditorInfo outAttrs) {
>         outAttrs.inputType = InputType.TYPE_CLASS_TEXT;
>-        outAttrs.imeOptions = EditorInfo.IME_ACTION_GO;
>+        outAttrs.imeOptions = EditorInfo.IME_ACTION_NONE;
>+

for whatever reason (bad merge I guess) the patch that landed was missing this hunk. I'll land it when the tree reopens.
(Assignee)

Updated

8 years ago
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
pushed http://hg.mozilla.org/mozilla-central/rev/50d50f789265
Status: REOPENED → RESOLVED
Last Resolved: 8 years ago8 years ago
Resolution: --- → FIXED
(Assignee)

Updated

8 years ago
Keywords: dev-doc-needed
I see a comment saying "removing dev-doc-needed" but it's still there. Is doc needed or not here?
(In reply to comment #33)
> I see a comment saying "removing dev-doc-needed" but it's still there. Is doc
> needed or not here?

yes, documentation is needed
Documented:

https://developer.mozilla.org/en/HTML/Element/Input#attr-mozactionhint
Keywords: dev-doc-needed → dev-doc-complete
This issue still occurs on the latest Nightly build. Should I reopen this bug?

--
Build id : Mozilla/5.0 (Android;Linux armv7l;rv:8.0a1)Gecko/20110816
Firefox/8.0a1 Fennec/8.0a1
Device: HTC Desire Z
OS: Android 2.3.3
Please file a new bug.
Verified fixed on:
Mozilla/5.0 (Android;Linux armv7l;rv:9.0a1)Gecko/20110919
Firefox/9.0a1 Fennec/9.0a1
Device: Samsung Galaxy S
OS: Android 2.2
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.