704879 - HTML5 form validation is broken in Fennec Android Native

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect, P2)

Description

[Markus Popp]

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:9.0) Gecko/20100101 Firefox/9.0
Build ID: 20111122192043

Steps to reproduce:

Triggering a form is possible, even though fields don't validate, e.g. if form fields are set required.


Actual results:

A form like http://var.mpopp.net/bdemo/forms.php which includes required fields (among other restrictions) should not submit while not all conditions are met. However, in the Fennec Android Native Nightly as of November 23, 2011 (on Samsung Galaxy S2, Android 2.3.4) it triggers with no fields filled out at all.


Expected results:

The form should not submit while not all conditions set by HTML5 form attributes are met.

This was fixed for Fennec 9.0 XUL version in Bug 605365, but is appearing again in Fennec Android Native.

Comment 1

[Catalin Suciu [:csuciu]]

I can reproduce this on:
Mozilla/5.0 (Android;Linux armv7l;rv:11.0a1)Gecko/20111123 Firefox/11.0a1 Fennec/11.0a1
Samsung GalaxyS, Android 2.2

Comment 2

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

It's ignoring the required for the input.  http://www.w3schools.com/html5/att_input_required.asp; does not occur on desktop nightly.

Comment 3

[Markus Popp]

Works on desktop, works on the Android XUL versions (>= version 9.0), only fails on Android Native.

It's not only the required attribute which isn't validated. The "Code:" field at http://var.mpopp.net/bdemo/forms.php doesn't have a required attribute, but

pattern="[A-Z][a-z0-9]{0,8}"

instead. However if you enter a value which doesn't match this pattern (while meeting the required criteria in the fields "Email", "URL" and "Text-Number", last of which also has a pattern="[0-9]+" attribute), the form still submits.

Looks to me like form validation is broken as a whole, and not only limited to a special type.

Comment 4

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

This bug depends on bug 695444, since HTML5 validation is somewhat built on form history. Bug 605365 shows how this was implemented in XUL Fennec.

Comment 5

[Mounir Lamouri (:mounir)]

(In reply to Mark Finkle (:mfinkle) from comment #4)
> This bug depends on bug 695444, since HTML5 validation is somewhat built on
> form history. Bug 605365 shows how this was implemented in XUL Fennec.

HTML5 form validation isn't built on form history. The content code sends a notification through the observer service. The UI code just has to listen to it and do what is needed.

Comment 6

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

(In reply to Mounir Lamouri (:volkmar) (:mounir) from comment #5)
> (In reply to Mark Finkle (:mfinkle) from comment #4)
> > This bug depends on bug 695444, since HTML5 validation is somewhat built on
> > form history. Bug 605365 shows how this was implemented in XUL Fennec.
> 
> HTML5 form validation isn't built on form history. The content code sends a
> notification through the observer service. The UI code just has to listen to
> it and do what is needed.

Ugh, you're right of course. It depends on an arrowbox popup UI, so we can show the message while pointing to the right element.

Comment 7

[Mardeg]

I should mention that on Galaxy Nexus when switching focus away from a field with invalid input and the "required" attribute, I see the red invalidation outline and there seems to be some kind of notification box that shows for a second positioned in the centre of the screen and then fades, but except for its upper border it's entirely obscured by the onscreen keyboard so I have no idea what it says.

Comment 8

[Andreea Pod]

Added testcase: https://litmus.mozilla.org/show_test.cgi?id=44973

Comment 9

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

bumping priority for release parity with XUL

Comment 10

[:Margaret Leibovic]

Attachment: WIP

Lucas, I saw you did the HTML5 form validation messages for XUL fennec, so I'm also asking you for feedback, since Finkle is probably too busy at MWC :)

I decided to just re-purpose AutoCompletePopup to also show form validation messages. I want to rename it to something more appropriate, like FormAssistPopup, but I think I'll just do that in a second patch to avoid getting bit-rotted while I work on this (there are references to mAutoCompletePopup in a bunch of places). I also decided to move the "FormAssist:Foo" message handling in there to clean things up a bit.

Issues left to address:
-add an arrow or other styling to the popup when we're showing form validation messages
-focus events don't seem to be working
-invalidformsubmit is fired before the document is done rendering, so the position from _getElementPositionData can end up being wrong

Comment 11

[Mounir Lamouri (:mounir)]

Margaret, would that be possible to see a screenshot?

Comment 12

[Lucas Rocha (:lucasr)]

Comment on attachment 601130 [details] [diff] [review]
WIP

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

I like the direction. My micro-committer heart tells me that you can split this patch into a series of self-contained smaller steps :-)

::: mobile/android/base/AutoCompletePopup.java
@@ +60,2 @@
>  
> +public class AutoCompletePopup extends ListView implements GeckoEventListener {

Rename to FormAssistPopup (as you mentioned) in a separate patch.

@@ +75,5 @@
>      private static final int AUTOCOMPLETE_ROW_HEIGHT_IN_DPI = 32;
>  
> +    private static int TYPE_NONE = 0;
> +    private static int TYPE_AUTOCOMPLETE = 1;
> +    private static int TYPE_VALIDATION = 2;

Maybe use an enum?

@@ +84,1 @@
>      public AutoCompletePopup(Context context, AttributeSet attrs) {

Maybe AutocompletePopup is not appropriate anymore? FormPopup maybe? Rename it in a separate patch.

@@ +92,5 @@
>  
>          setOnItemClickListener(new OnItemClickListener() {
>              public void onItemClick(AdapterView<?> parentView, View view, int position, long id) {
> +                if (mTypeShowing == TYPE_AUTOCOMPLETE) {
> +                    String value = ((TextView) view).getText().toString();

I generally try to avoid those in-place castings for the sake of readability. Cast to a temp variable then use it here.

@@ +130,5 @@
> +                    }
> +                });
> +            }
> +        } catch (Exception e) { 
> +            Log.i(LOGTAG, "handleMessage throws " + e + " for message: " + event);

Use Log.e() instead?

@@ +150,5 @@
>          for (int i = 0; i < suggestions.length(); i++) {
>              try {
>                  adapter.add(suggestions.get(i).toString());
>              } catch (JSONException e) {
>                  Log.i(LOGTAG, "JSONException: " + e);

Not in this patch but: break the loop if an exception happens? Use Log.e()?

@@ +166,5 @@
> +
> +        // Update popup contents with validation message
> +        ArrayAdapter<String> adapter = new ArrayAdapter<String>(mContext, R.layout.autocomplete_list_item);
> +        adapter.add(validationMessage);
> +        setAdapter(adapter);

Something to ask UX team about: maybe the validation message popup should look a bit different than the autocomplete one? Different color?

@@ +260,1 @@
>              GeckoAppShell.sendEventToGecko(GeckoEvent.createBroadcastEvent("FormAssist:Closed", null));

Not in this patch but: maybe the terminology for the messages is slightly inconsistent? FormAssist:Closed in a hide() method? There's already a FormAssist:Hide message coming from FormAssist. Maybe rename FormAssist:Closed to FormAssist:Hidden for consistency and clarity? Just thinking out loud here.

::: mobile/android/base/GeckoApp.java
@@ -1022,5 @@
> -                            if (!imm.isFullscreenMode())
> -                                mAutoCompletePopup.show(suggestions, rect, zoom);
> -                        }
> -                    });
> -                }

I'd move these message handler from GeckoApp to AutoComplete popup (with no functional change) in a separate a patch.

::: mobile/android/chrome/content/browser.js
@@ +2930,5 @@
> +        suggestions: suggestions,
> +        rect: positionData.rect,
> +        zoom: positionData.zoom
> +      }
> +    });

You seem to be doing a lot of autocomplete-related plumbing here which should probably go on a separate patch. Consider splitting this into a series of incremental changes.

Comment 13

[:Margaret Leibovic]

Attachment: WIP screenshot

Here's a screenshot with this patch. I haven't done anything yet to style the popup differently than the autocomplete popup.

Comment 14

[:Margaret Leibovic]

UX friends, could you help me decide how to style this popup? We could add an arrow to it like we have in XUL fennec, but I'll need assets for that.

Comment 15

[:Margaret Leibovic]

Attachment: (1/7) Rename AutoCompletePopup -> FormAssistPopup

Comment 16

[:Margaret Leibovic]

Attachment: (2/7) Move "FormAssist:AutoComplete" message handling to FormAssistPopup

Comment 17

[:Margaret Leibovic]

Attachment: (3/7) Clean up FormAssistant

This patch refactors FormAssistant so that it will be easier to introduce the form validation code.

Comment 18

[:Margaret Leibovic]

Attachment: (4/7) Make FormAssistant in charge of hiding an empty popup

This is also in preparation for when we add the form validation code. We want the input listener to be in charge of deciding if the popup is empty and should be hidden, since if there are no suggestions, we may still want to show it if there is a form validation. (This avoids the flickering that would happen if we send a message to java saying there are no suggestions, then another message that says we should show a validation message.)

I also included the "FormAssist:Closed"->"FormAssist:Hidden" changes you suggested in here.

Comment 19

[:Margaret Leibovic]

Attachment: (5/7) Add form validation messages

This is the real meat of this bug. However, this doesn't include the styling changes.

Comment 20

[:Margaret Leibovic]

Attachment: (6/7) Show form validation message when an input element is focused

Comment 21

[:Margaret Leibovic]

I didn't add a (7/7) yet, since I'm planning on making that the popup style changes, but I still need UX input.

Comment 22

[Lucas Rocha (:lucasr)]

Comment on attachment 601414 [details] [diff] [review]
(1/7) Rename AutoCompletePopup -> FormAssistPopup

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

Excellent!

Comment 23

[Lucas Rocha (:lucasr)]

Comment on attachment 601415 [details] [diff] [review]
(2/7) Move "FormAssist:AutoComplete" message handling to FormAssistPopup

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

Great.

::: mobile/android/base/FormAssistPopup.java
@@ +90,5 @@
>                  hide();
>              }
>          });
> +
> +        GeckoAppShell.registerGeckoEventListener("FormAssist:AutoComplete", this);

No matching unregisterGeckoEventListener anywhere? Not needed?

Comment 24

[Lucas Rocha (:lucasr)]

Comment on attachment 601416 [details] [diff] [review]
(3/7) Clean up FormAssistant

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

I think the commit message doesn't match what this patch is doing. This is not just cleaning code up, is it?

::: mobile/android/chrome/content/browser.js
@@ +2845,5 @@
> +    // Only show autocomplete suggestions for certain elements
> +     if (!(aElement instanceof HTMLInputElement) ||
> +         (aElement.getAttribute("type") == "password") ||
> +         (aElement.hasAttribute("autocomplete") &&
> +          aElement.getAttribute("autocomplete").toLowerCase() == "off"))

Maybe keep this as a separate _isAutocomplete() method? nit: maybe rename it isAutoComplete for consistency?

@@ +2865,5 @@
> +      if (value == searchString)
> +        continue;
> +
> +      suggestions.push(value);
> +    }

Maybe keep the separate _getAutocompleteSuggestions() method? _showAutoCompleteSuggestions() got a bit too chunky. nit: rename it to getAutoCompleteSuggestions() for consistency?

Comment 25

[Lucas Rocha (:lucasr)]

Comment on attachment 601418 [details] [diff] [review]
(4/7) Make FormAssistant in charge of hiding an empty popup

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

Looks good but I'd like to get an answer to my question about the possible race between Hide and Hidden before giving r+.

::: mobile/android/chrome/content/browser.js
@@ +2811,5 @@
>          this._currentInputElement.blur();
>          this._currentInputElement.value = aData;
>          break;
>  
> +      case "FormAssist:Hidden":

What would happen if you send a FormAssist:Hide message to Java and an FormAssist:AutoComplete is received before FormAssist:Hidden?

@@ +2873,5 @@
>      }
>  
> +    // Return false if there are no suggestions to show
> +    if (!suggestions.length)
> +      return false;

We're handling empty list of suggestions on Java side as well. Given that we don't send FormAssist:AutoComplete when there are no suggestions anymore, maybe you can remove the equivalent (duplicate?) code on the Java side now?

Comment 26

[Lucas Rocha (:lucasr)]

Comment on attachment 601419 [details] [diff] [review]
(5/7) Add form validation messages

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

::: mobile/android/base/FormAssistPopup.java
@@ +76,5 @@
>  
> +    private static enum PopupType { NONE, AUTOCOMPLETE, VALIDATION };
> +
> +    // Keep track of the type of popup we're currently showing
> +    private PopupType mTypeShowing = PopupType.NONE;

I usually prefer to initialize state on the constructor. No big deal though.

@@ +114,5 @@
> +                     public void run() {
> +                         showAutoCompleteSuggestions(suggestions, rect, zoom);
> +                     }
> +                });
> +            } else if (event.equals("FormAssist:ValidationMessage")) {

Suggestion: add a separate patch that refactors the code for each type of message into separate private methods (handleAutocompleteMessage(), handleValidationMessage(), handleHideMessage(), etc). This would make handleMessage much more readable.

@@ +150,5 @@
> +            mTypeShowing = PopupType.AUTOCOMPLETE;
> +    }
> +
> +    private void showValidationMessage(String validationMessage, JSONArray rect, double zoom) {
> +        // TODO: style the validation message popup differently

File a bug and add reference to the new bug here?

Comment 27

[Lucas Rocha (:lucasr)]

Comment on attachment 601420 [details] [diff] [review]
(6/7) Show form validation message when an input element is focused

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

Good.

::: mobile/android/chrome/content/browser.js
@@ +2851,5 @@
>    handleEvent: function(aEvent) {
>      switch (aEvent.type) {
> +      case "focus":
> +        let currentElement = aEvent.target;
> +        this._showValidationMessage(currentElement);

Autocomplete will still have precedence over validation message when user starts typing, right?

Comment 28

[Lucas Rocha (:lucasr)]

Awesome patch series Margaret! I wish more Mozillians submitted their patches like this :-)

Comment 29

[:Margaret Leibovic]

I decided to just file a follow-up for styling the popup, so that we won't be blocked on landing this work. See bug 731654.

Comment 30

[:Margaret Leibovic]

(In reply to Lucas Rocha (:lucasr) from comment #23)
> > +        GeckoAppShell.registerGeckoEventListener("FormAssist:AutoComplete", this);
> 
> No matching unregisterGeckoEventListener anywhere? Not needed?

Since FormAssistPopup is never destroyed during the lifetime of the app, we don't need to unregister the event listener. We do the same things in Tabs. Maybe we should make FormAssistPopup into a proper singleton like we did with tabs, but we can look into that in another bug.

(In reply to Lucas Rocha (:lucasr) from comment #25)
> ::: mobile/android/chrome/content/browser.js
> @@ +2811,5 @@
> >          this._currentInputElement.blur();
> >          this._currentInputElement.value = aData;
> >          break;
> >  
> > +      case "FormAssist:Hidden":
> 
> What would happen if you send a FormAssist:Hide message to Java and an
> FormAssist:AutoComplete is received before FormAssist:Hidden?

Okay, let's think this through:
- We send FormAssist:Hide when there are no suggestions/validation message to show
- We only send FormAssist:AutoComplete back to gecko when the user taps on a suggestion
- We would only get a FormAssist:AutoComplete before a FormAssist:Hidden if the user tapped on a suggestion before we managed to receive a FormAssist:Hide to hide the popup
- In this case, we would fill the input with the suggestion string that was tapped, then we would set the current element to null

I think a bigger concern would be receiving a FormAssist:Hidden before a FormAssist:AutoComplete, since the current element would be null but since we are hiding the popup in Java before sending FormAssist:Hidden, it should be impossible for a user to select a suggestion to fire a FormAssist:AutoComplete.

> @@ +2873,5 @@
> >      }
> >  
> > +    // Return false if there are no suggestions to show
> > +    if (!suggestions.length)
> > +      return false;
> 
> We're handling empty list of suggestions on Java side as well. Given that we
> don't send FormAssist:AutoComplete when there are no suggestions anymore,
> maybe you can remove the equivalent (duplicate?) code on the Java side now?

Oops, the point of this patch was to change it so that we don't deal with an empty suggestions list in Java anymore, but that change made it into the next patch in the series instead of this one. I can update that to make it clearer.


(In reply to Lucas Rocha (:lucasr) from comment #27)
> > +      case "focus":
> > +        let currentElement = aEvent.target;
> > +        this._showValidationMessage(currentElement);
> 
> Autocomplete will still have precedence over validation message when user
> starts typing, right?

Yeah, once the user starts typing, we get input events, which give autocomplete precedence. This focus event only gets fired when focus changes. I suppose if the user enters some text, switches focus, then comes back to the input, we're going to show a validation message instead of autocomplete suggestions. Maybe we should change that? We also don't show suggestions for an empty input like we do in XUL fennec. Some bugzilla digging led me to bug 711177, and it doesn't seem clear that the current behavior was an active decision, so maybe that will change.

Comment 31

[:Margaret Leibovic]

Attachment: (3/7) Refactor FormAssistant autocomplete logic

Here's an updated patch that addresses comment 24. I guess this is more refactoring than cleaning up, but the functionality isn't changing in this patch.

Comment 32

[:Margaret Leibovic]

Attachment: (4/7) Make FormAssistant in charge of hiding an empty popup (v2)

I updated this to address what I said in comment 30.

Comment 33

[:Margaret Leibovic]

Attachment: (4.5/7) Refactor message handling code into separate private messages

Refactoring suggested in comment 26. I'll update patch (5/7) to deal with this.

Comment 34

[:Margaret Leibovic]

Attachment: (5/7) Add form validation messages (v2)

Updated to apply on top of patch (4.5/7). I also factored out some of _showValidationMessage into an _isValidateable function, to be more consistent with _isAutoComplete (this is also how it was in XUL fennec).

Comment 35

[Lucas Rocha (:lucasr)]

Comment on attachment 601714 [details] [diff] [review]
(4.5/7) Refactor message handling code into separate private messages

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

Great.

Comment 36

[Lucas Rocha (:lucasr)]

Comment on attachment 601731 [details] [diff] [review]
(5/7) Add form validation messages (v2)

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

Nice.

::: mobile/android/base/FormAssistPopup.java
@@ +262,5 @@
>      public void hide() {
>          if (isShown()) {
>              setVisibility(View.GONE);
>              GeckoAppShell.sendEventToGecko(GeckoEvent.createBroadcastEvent("FormAssist:Hidden", null));
> +            mTypeShowing = PopupType.NONE;

Just being a bit pedantic: mTypeShowing should probably be set just after hiding the popup.

Comment 37

[:Margaret Leibovic]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0c7d8f0751a8
https://hg.mozilla.org/integration/mozilla-inbound/rev/596f99994571
https://hg.mozilla.org/integration/mozilla-inbound/rev/c39f37a0d9f4
https://hg.mozilla.org/integration/mozilla-inbound/rev/44923daa1770
https://hg.mozilla.org/integration/mozilla-inbound/rev/c251670745e0
https://hg.mozilla.org/integration/mozilla-inbound/rev/a979f7ffdba1
https://hg.mozilla.org/integration/mozilla-inbound/rev/f0d2d132bb7e

Comment 38

[Marco Bonardo [:mak]]

https://hg.mozilla.org/mozilla-central/rev/0c7d8f0751a8
https://hg.mozilla.org/mozilla-central/rev/596f99994571
https://hg.mozilla.org/mozilla-central/rev/c39f37a0d9f4
https://hg.mozilla.org/mozilla-central/rev/44923daa1770
https://hg.mozilla.org/mozilla-central/rev/c251670745e0
https://hg.mozilla.org/mozilla-central/rev/a979f7ffdba1
https://hg.mozilla.org/mozilla-central/rev/f0d2d132bb7e

Comment 39

[Adrian Tamas (:AdrianT)]

Verified/fixed on:
Nightly Fennec 15.0a1 (2012-04-25)
Device: HTC Desire (Android 2.2.2)