Closed Bug 614356 Opened 9 years ago Closed 9 years ago

default to next action hint for form inputs

Categories

(Core :: Widget: Android, defect, P1)

defect

Tracking

()

RESOLVED FIXED
Tracking Status
fennec 2.0+ ---

People

(Reporter: blassey, Assigned: blassey)

Details

(Whiteboard: [softkb])

Attachments

(1 file, 3 obsolete files)

Attached patch patch (obsolete) — Splinter Review
No description provided.
Attachment #492747 - Flags: review?(roc)
tracking-fennec: --- → ?
OS: Linux → All
Hardware: x86_64 → All
Comment on attachment 492747 [details] [diff] [review]
patch

+            context.mActionHint.Assign(NS_LITERAL_STRING("next"));

{}
Attachment #492747 - Flags: review?(roc) → review+
tracking-fennec: ? → 2.0b3+
tracking-fennec: 2.0b3+ → 2.0+
Whiteboard: [softkb]
Assignee: nobody → blassey.bugs
madhava, we need a specification for when to show the next button and when to show a return and/or done button in web forms
Priority: -- → P1
Attached patch patch v2 (obsolete) — Splinter Review
you already reviewed the previous patch, this revision adds some logic to not use the next button if the form has a default submit element.

The effect is that test inputs where an enter key has an action have enter keys and ones that have no action have a next key.
Attachment #492747 - Attachment is obsolete: true
Attachment #508072 - Flags: review?(roc)
s/test/text
Comment on attachment 508072 [details] [diff] [review]
patch v2

+                if (formSubmit)
+                  hasSubmit = PR_TRUE;

{}

+            if (!hasSubmit)
+              context.mActionHint.Assign(NS_LITERAL_STRING("next"));

{}

Looks good to me, but you really need a content peer I think
Attachment #508072 - Flags: review?(roc)
Attachment #508072 - Flags: review?(Olli.Pettay)
Attachment #508072 - Flags: review+
Comment on attachment 508072 [details] [diff] [review]
patch v2

I don't know what this is about.
Attachment #508072 - Flags: review?(masayuki)
Attachment #508072 - Flags: review?(Olli.Pettay)
Attachment #508072 - Flags: feedback?(mounir.lamouri)
After discussing with blassey on IRC, I may now sort of understand what this all
is about.
The patch clearly needs to add some good comment why it is doing whatever it
is doing, and there should be some explanation also in this bug.
Comment on attachment 508072 [details] [diff] [review]
patch v2

What's the mozactionhint? I don't find the document...

>@@ -282,6 +284,28 @@ nsIMEStateManager::SetIMEState(PRUint32 
>                         context.mHTMLInputType);
>       aContent->GetAttr(kNameSpaceID_None, nsGkAtoms::moz_action_hint,
>                         context.mActionHint);
>+
>+      if (context.mActionHint.IsEmpty()) {
>+        // if we don't have an action hint and tab index isn't -1, use "next"
>+        if (aContent->Tag() == nsGkAtoms::input) {
>+          if (!aContent->AttrValueIs(kNameSpaceID_None, nsGkAtoms::tabindex, 
>+                                     NS_LITERAL_STRING("-1"), eCaseMatters)) {

Why don't you check both condition in an if? In other words, why don't you use &&?

And aContent can be null.

>+            PRBool hasSubmit = PR_FALSE;
>+            nsCOMPtr<nsIFormControl> control(do_QueryInterface(aContent));
>+            if (control) {
>+              nsCOMPtr<nsIForm> form(do_QueryInterface(control->GetFormElement()));
>+              if (form) {
>+                nsCOMPtr<nsIContent> formSubmit =
>+                  do_QueryInterface(form->GetDefaultSubmitElement());
>+                if (formSubmit)
>+                  hasSubmit = PR_TRUE;
>+              }
>+            }
>+            if (!hasSubmit)
>+              context.mActionHint.Assign(NS_LITERAL_STRING("next"));

I'm not sure whether this logic is correct. Would you explain about the mozactionhint attribute or give me a URL of the document?

And I worry about a case. Even if the aContent in an HTML editor, is this fine? Furthermore, the previous is if statement checks the namespace of aContent. Don't you need it here?
Comment on attachment 508072 [details] [diff] [review]
patch v2

>+      if (context.mActionHint.IsEmpty()) {
>+        // if we don't have an action hint and tab index isn't -1, use "next"
>+        if (aContent->Tag() == nsGkAtoms::input) {
>+          if (!aContent->AttrValueIs(kNameSpaceID_None, nsGkAtoms::tabindex, 
>+                                     NS_LITERAL_STRING("-1"), eCaseMatters)) {

Why not use the next action hint for element with tabindex=-1? If you select an element with tabindex=-1, you can go to the next element.
Try: data:text/html,<input tabindex=1><input tabindex=-1><input tabindex=2>

>+            PRBool hasSubmit = PR_FALSE;
>+            nsCOMPtr<nsIFormControl> control(do_QueryInterface(aContent));

Couldn't you use static_cast given that you know aContent is implementing nsIFormControl (it's an input element)?

>+            if (control) {
>+              nsCOMPtr<nsIForm> form(do_QueryInterface(control->GetFormElement()));
>+              if (form) {
>+                nsCOMPtr<nsIContent> formSubmit =
>+                  do_QueryInterface(form->GetDefaultSubmitElement());
>+                if (formSubmit)

You probably don't really want to call do_QueryInterface here.
I think |if (form->GetDefaultSubmitElement())| should be enough.

>+                  hasSubmit = PR_TRUE;
>+              }
>+            }

I don't use Firefox Mobile but I guess the idea here is to replace the "ENTER" key for a "NEXT" key in the virtual keyboard when the 'action' would actually not submit the form?

f+ with at least the tabindex issue fixed or explained.
Attachment #508072 - Flags: feedback?(mounir.lamouri) → feedback+
(In reply to comment #8)
> And aContent can be null.

I think it's checked a few lines before so it should not be null in this block.

> Would you explain about the
> mozactionhint attribute or give me a URL of the document?

FWIW, I've found that setting mozactionhint changes mIMEActionHint here:
https://mxr.mozilla.org/mozilla-central/source/embedding/android/GeckoSurfaceView.java#96
I'm guessing this is changing the 'action' key in the virtual keyboard in Android but I've no strong clues.
(In reply to comment #8)
> And aContent can be null.

> Furthermore, the previous is if statement checks the namespace of aContent.
> Don't you need it here?

Oops, sorry, ignore these comments. I misunderstood the indentation level.
(In reply to comment #10)
> (In reply to comment #8)
> > And aContent can be null.
> 
> I think it's checked a few lines before so it should not be null in this block.
> 
> > Would you explain about the
> > mozactionhint attribute or give me a URL of the document?
> 
> FWIW, I've found that setting mozactionhint changes mIMEActionHint here:
> https://mxr.mozilla.org/mozilla-central/source/embedding/android/GeckoSurfaceView.java#96
> I'm guessing this is changing the 'action' key in the virtual keyboard in
> Android but I've no strong clues.

You assume correctly. Here's an example of where its used https://mxr.mozilla.org/mobile-browser/source/chrome/content/browser.xul#239

I've re-added the dev-doc-needed keyword to bug 589879 so this can be documented correctly.
Thanks, I read the patch of bug 589879 and
http://developer.android.com/reference/android/view/inputmethod/EditorInfo.html

I think there are some cases which the action button shouldn't be "next". E.g., if the focused element is final element of the tabindex or next focusable element is in another form element. But unfortunately, we don't get the next focusable element by nsIFocusManager.
http://mxr.mozilla.org/mozilla-central/source/dom/interfaces/base/nsIFocusManager.idl

I think you should fix this issue in next cycle.
Comment on attachment 508072 [details] [diff] [review]
patch v2

r=masayuki

if you combine the 3 if conditions

+      if (context.mActionHint.IsEmpty()) {
+        // if we don't have an action hint and tab index isn't -1, use "next"
+        if (aContent->Tag() == nsGkAtoms::input) {
+          if (!aContent->AttrValueIs(kNameSpaceID_None, nsGkAtoms::tabindex, 
+                                     NS_LITERAL_STRING("-1"), eCaseMatters)) {

to one if statement and reduce the indentation level of its block.
Attachment #508072 - Flags: review?(masayuki) → review+
Comment on attachment 508072 [details] [diff] [review]
patch v2


>+                nsCOMPtr<nsIContent> formSubmit =
>+                  do_QueryInterface(form->GetDefaultSubmitElement());
>+                if (formSubmit)
>+                  hasSubmit = PR_TRUE;
>+              }
Based on the IRC discussion, I believe this is wrong.

Do we have any tests for this action_hint stuff?
(In reply to comment #15)
> Comment on attachment 508072 [details] [diff] [review]
> patch v2
> 
> 
> >+                nsCOMPtr<nsIContent> formSubmit =
> >+                  do_QueryInterface(form->GetDefaultSubmitElement());
> >+                if (formSubmit)
> >+                  hasSubmit = PR_TRUE;
> >+              }
> Based on the IRC discussion, I believe this is wrong.
We should talk on IRC again then, because I thought this settled when we talked

> Do we have any tests for this action_hint stuff?
asking for it to be added to litmus since we don't have any good automated way to test if the keyboard behavior changes
Flags: in-litmus?(mozaakash)
Attached patch patch (obsolete) — Splinter Review
changed the logic based on discussion yesterdays on IRC and the rules spelled out here https://mxr.mozilla.org/mozilla-central/source/content/html/content/src/nsHTMLInputElement.cpp#2350

Asking for smaug's review and carrying everyone else's.

Aakash, here's a page that can be the basis of a litmus test http://lassey.us/forms.html
Attachment #508072 - Attachment is obsolete: true
Attachment #508838 - Flags: review?(Olli.Pettay)
Checkbox and radio buttons do submit on ENTER to or that is not the behavior you are trying to imitate?
Comment on attachment 508838 [details] [diff] [review]
patch


>+            nsCOMPtr<nsIContent> formSubmit =
>+              do_QueryInterface(form->GetDefaultSubmitElement());
Why this QI? Couldn't you just check
if (form->GetDefaultSubmitElement()) ...


>+            if (formSubmit) {
>+              willSubmit = PR_TRUE;
>+            } else {
>+              PRUint32 elCount = form->GetElementCount();
>+              int thisIndex = form->IndexOfControl(control);
>+              willSubmit = PR_TRUE;
>+              for (int i = 0; i < elCount && willSubmit; i++) {
>+                if (i != thisIndex && form->GetElementAt(i)->IsSingleLineTextControl(PR_FALSE))
>+                  willSubmit = PR_FALSE;
>+              }
This certainly needs some comments. And you need to address volkmar's comment some way
(it might be ok to say you don't care that case).
And why aren't you using HasSingleTextControl? Or even better, change nsHTMLInputElement a bit
by adding a method, say "IsAbleToSubmitForm"
(this code really should be some where in form controls) and just call that here.
nsHTMLInputElement::MaybeSubmitForm should probably utilize the same method.
Attachment #508838 - Flags: review?(Olli.Pettay) → review-
(In reply to comment #18)
> Checkbox and radio buttons do submit on ENTER to or that is not the behavior
> you are trying to imitate?

I only care about text boxes here because the software keyboard won't show up for checkboxes and radio buttons
Attached patch patchSplinter Review
Attachment #508838 - Attachment is obsolete: true
Attachment #508923 - Flags: review?(Olli.Pettay)
(In reply to comment #9)
> >+            PRBool hasSubmit = PR_FALSE;
> >+            nsCOMPtr<nsIFormControl> control(do_QueryInterface(aContent));
> 
> Couldn't you use static_cast given that you know aContent is implementing
> nsIFormControl (it's an input element)?

you can't static_cast from an nsIContent to an nsIFormControl. There used to be a dont_QueryInterface, but I don't know that there is a modern equivalent now.
Comment on attachment 508923 [details] [diff] [review]
patch

>diff --git a/content/events/src/nsIMEStateManager.cpp b/content/events/src/nsIMEStateManager.cpp
>+        nsCOMPtr<nsIFormControl> control(do_QueryInterface(aContent));
>+        mozilla::dom::Element* formElement = control->GetFormElement();
>+        nsCOMPtr<nsIForm> form;
>+        if (control) {

You probably don't want to call |control->GetFormElement()| then checking for control being not null.
Actually, you could just assert if control is null because it should not happen AFAIK.

>+          // is this a form and does it have a default submit element?
>+          if ((form = do_QueryInterface(formElement)) && form->GetDefaultSubmitElement())

GetFormElement() should return a form element so |form| can't be null if |formElement| isn't null. I would check |formElement| and assert if |form| is null and |formElement| isn't. In addition, I think |(form = do_QueryInterface(formElement))| in a condition is ugly.

>+            willSubmit = PR_TRUE;
>+          // is this an html form and does it only have a single text input element?
>+          else if (formElement && formElement->Tag() == nsGkAtoms::form && formElement->IsHTML() &&
>+                   static_cast<nsHTMLFormElement*>(formElement)->HasSingleTextControl())t 

You can do |formElement->IsHTML(nsGkAtoms::form)|.
But I don't really like to have the logic of indirect form submission copied here. It would be better to have a method in the content side or at least open a follow-up to do that.
Comment on attachment 508923 [details] [diff] [review]
patch

># HG changeset patch
># User Brad Lassey <blassey@mozilla.com>
># Date 1296483838 28800
># Node ID 09803c97a8626a7b74c50036e92c24cabce7ad25
># Parent  a79b46eef8f2b15d7d88a2979c4c5bde2515e1cb
>bug 614356 - default to next action hint for form inputs r=roc,masayuki f=volkmar a=blocking-fennec
>
>diff --git a/content/events/src/Makefile.in b/content/events/src/Makefile.in
>--- a/content/events/src/Makefile.in
>+++ b/content/events/src/Makefile.in
>@@ -97,6 +97,7 @@ include $(topsrcdir)/config/rules.mk
> 
> LOCAL_INCLUDES	+= \
>              -I$(srcdir)/../../base/src \
>+             -I$(srcdir)/../../html/content/src \
>              -I$(srcdir)/../../html/base/src \
>              -I$(srcdir)/../../xul/content/src \
>              -I$(srcdir)/../../xml/content/src \
>diff --git a/content/events/src/nsIMEStateManager.cpp b/content/events/src/nsIMEStateManager.cpp
>--- a/content/events/src/nsIMEStateManager.cpp
>+++ b/content/events/src/nsIMEStateManager.cpp
>@@ -65,6 +65,9 @@
> #include "nsContentEventHandler.h"
> #include "nsIObserverService.h"
> #include "mozilla/Services.h"
>+#include "nsIFormControl.h"
>+#include "nsIForm.h"
>+#include "nsHTMLFormElement.h"
> 
> /******************************************************************/
> /* nsIMEStateManager                                              */
>@@ -282,6 +285,24 @@ nsIMEStateManager::SetIMEState(PRUint32 
>                         context.mHTMLInputType);
>       aContent->GetAttr(kNameSpaceID_None, nsGkAtoms::moz_action_hint,
>                         context.mActionHint);
>+
>+      // if we don't have an action hint and  return won't submit the form use "next"
>+      if (context.mActionHint.IsEmpty() && aContent->Tag() == nsGkAtoms::input) {
>+        PRBool willSubmit = PR_FALSE;
>+        nsCOMPtr<nsIFormControl> control(do_QueryInterface(aContent));
>+        mozilla::dom::Element* formElement = control->GetFormElement();
>+        nsCOMPtr<nsIForm> form;
>+        if (control) {
>+          // is this a form and does it have a default submit element?
>+          if ((form = do_QueryInterface(formElement)) && form->GetDefaultSubmitElement())
>+            willSubmit = PR_TRUE;

if (expr) {

}


>+          // is this an html form and does it only have a single text input element?
>+          else if (formElement && formElement->Tag() == nsGkAtoms::form && formElement->IsHTML() &&
>+                   static_cast<nsHTMLFormElement*>(formElement)->HasSingleTextControl())
>+            willSubmit = PR_TRUE;


Same here.
Attachment #508923 - Flags: review?(Olli.Pettay) → review+
pushed http://hg.mozilla.org/mozilla-central/rev/00f207cb2898
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Brad, did you ignored comment 23 on purpose or you just missed it?
(In reply to comment #26)
> Brad, did you ignored comment 23 on purpose or you just missed it?

Looks like I missed it, sorry. Can you file a follow up bug for those points?
Flags: in-litmus?(mozaakash) → in-litmus?(nhirata.bugzilla)
Created litmus test case : 22058
Showing strange behavior and hang; need to revisit the case.
Flags: in-litmus?(nhirata.bugzilla) → in-litmus+
You need to log in before you can comment on or make changes to this bug.