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

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Aaron Train [:aaronmt]]

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".

Comment 1

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: 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.

Comment 5

[Shawn Wilsher :sdwilsh]

(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 6

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

litmus test cases created:
https://litmus.mozilla.org/show_test.cgi?searchType=by_id&id=13542
https://litmus.mozilla.org/show_test.cgi?searchType=by_id&id=13543

Comment 7

[Michael Wu [:mwu]]

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.

Comment 8

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

set in-litmus flag back to ? until the bug lands.  When the bug lands both litmus test cases mentioned above should be revisited.

Comment 10

[Brad Lassey [:blassey] (use needinfo?)]

taking

Comment 11

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: patch

not sure if this is ready for review yet, but I'd like to get some feedback on it.

Comment 12

[Robert O'Callahan (:roc) (email my personal email if necessary)]

This seems kinda reasonable but you really should be asking a DOM peer who knows HTML5 forms well ... Jonas, Mounir, etc

Comment 13

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: patch

Comment 14

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: patch

updated so textareas have return keys

Comment 15

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: m-b patch

label fennec's url bar with "go"

Comment 16

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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 17

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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?

Comment 18

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: patch

Comment 19

[Brad Lassey [:blassey] (use needinfo?)]

(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.

Comment 20

[Robert O'Callahan (:roc) (email my personal email if necessary)]

+        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?

Comment 21

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: patch

Comment 22

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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?

Comment 23

[Brad Lassey [:blassey] (use needinfo?)]

(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

[Michael Wu [:mwu]]

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.

Comment 25

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Yeah that sounds good.

Clearly "multiline" should be an independent flag rather than an action hint.

Comment 26

[Brad Lassey [:blassey] (use needinfo?)]

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.

Comment 27

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: 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

Comment 28

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 29

[Brad Lassey [:blassey] (use needinfo?)]

pushed http://hg.mozilla.org/mozilla-central/rev/2aceea4f527b and http://hg.mozilla.org/mobile-browser/rev/93c37330e654

Comment 30

[Michael Wu [:mwu]]

Removing dev-doc-needed because I think we should not expose mozactionhint to web content at all. Bug 614488 will remove mozactionhint.

Comment 31

[Brad Lassey [:blassey] (use needinfo?)]

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.

Comment 32

[Brad Lassey [:blassey] (use needinfo?)]

pushed http://hg.mozilla.org/mozilla-central/rev/50d50f789265

Comment 33

[Eric Shepherd [:sheppy]]

I see a comment saying "removing dev-doc-needed" but it's still there. Is doc needed or not here?

Comment 34

[Brad Lassey [:blassey] (use needinfo?)]

(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

Comment 35

[Eric Shepherd [:sheppy]]

Documented:

https://developer.mozilla.org/en/HTML/Element/Input#attr-mozactionhint

Comment 36

[Cristian Nicolae (:xti)]

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

Comment 37

[Martijn Wargers (dead)]

Please file a new bug.

Comment 38

[Cristian Nicolae (:xti)]

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