Closed Bug 605365 Opened 14 years ago Closed 13 years ago

Implement a UI for HTML5 Forms Validation in Fennec

Categories

(Firefox for Android Graveyard :: General, defect)

defect
Not set
normal

Tracking

(fennec+)

VERIFIED FIXED
Firefox 9
Tracking Status
fennec + ---

People

(Reporter: m_kato, Assigned: lucasr)

References

(Blocks 2 open bugs)

Details

Attachments

(7 files, 21 obsolete files)

61.37 KB, image/png
Details
3.73 KB, patch
lucasr
: review+
Details | Diff | Splinter Review
5.14 KB, patch
lucasr
: review+
Details | Diff | Splinter Review
15.26 KB, patch
lucasr
: review+
Details | Diff | Splinter Review
1.97 KB, patch
lucasr
: review+
Details | Diff | Splinter Review
1.51 KB, patch
lucasr
: review+
Details | Diff | Splinter Review
1.76 KB, patch
lucasr
: review+
Details | Diff | Splinter Review
Fennec UI doesn't handle "invalidformsubmit" observer, so validation and required attirubte doesn't work well.
When a form is invalid, form submission should be blocked. However, we need a proper UI. Given that the UI is done in browser.js (for Firefox Desktop), I tried to prevent breaking other Gecko consumers so the form submission isn't blocked if 'invalidformsubmit' event has no observer. This is the case for Fennec.

I think we should have this done for Firefox 4 Mobile for feature parity with Firefox 4 Destkop.

See bug 561636 for the implementation of the UI in Firefox Desktop
Blocks: 587671
tracking-fennec: --- → ?
Summary: Don't work HTML5 Form validation on Fennec → Implement a UI for HTML5 Forms Validation in Fennec
Current ideas on Fennec is to have an indicator you have to act on to have the suggestion displayed. The indicator will live could live inside the suggestion area of Form Helper or next to the field.
Attached patch wip that display an alert (obsolete) — Splinter Review
The wip just display an alert for the moment. 
I'm pretty sure the code should not be linked too much to form helper because this one can be disabled while form validation is an html5 feature.
tracking-fennec: ? → 2.0+
Comment on attachment 484312 [details] [diff] [review]
wip that display an alert

>diff --git a/chrome/content/browser-ui.js b/chrome/content/browser-ui.js
>+    // Limit the message to 256 characters.
>+    let message = element.validationMessage.substring(0, 256);

This is no longer needed. Since bug 606817 has been fixed, messages are truncated by the content if needed.
We either need a simple UI or we need to stub out the validation.
(In reply to comment #5)
> We either need a simple UI or we need to stub out the validation.

What do you mean by stub out?
What does this look like with the WIP?
(In reply to comment #6)
> (In reply to comment #5)
> > We either need a simple UI or we need to stub out the validation.
> 
> What do you mean by stub out?

stub out == do whatever might be needed to make forms work in the absence of any validation UI
(In reply to comment #7)
> What does this look like with the WIP?

Madhava - The WIP uses an alert dialog, not an alert notification, to display the validation message.
(In reply to comment #8)
> (In reply to comment #6)
> > (In reply to comment #5)
> > > We either need a simple UI or we need to stub out the validation.
> > 
> > What do you mean by stub out?
> 
> stub out == do whatever might be needed to make forms work in the absence of
> any validation UI

What I did in bug 561636 takes care of that: if there is no observer of the event sent by the content, the form is submitted. IOW, the behavior doesn't change from what you have with Gecko 1.9.2. If you want to follow this path, you might want to disable the default style of :-moz-ui-invalid (which is set in forms.css).

However, I don't think being inconsistent between Firefox Desktop and Firefox Mobile would be good.
tracking-fennec: 2.0+ → 2.0-
Whiteboard: [fennec-4.1?]
Blocks: 616348
Madhava, I think we need a spec to move forward with this
tracking-fennec: - → 7+
Keywords: uiwanted
Whiteboard: [fennec-4.1?]
Assignee: 21 → lucasr.at.mozilla
tracking-fennec: 7+ → +
Attached patch Updated version Vivien's patch (obsolete) — Splinter Review
Updated Vivien's patch to apply to latest code.
Attachment #484312 - Attachment is obsolete: true
Attachment #548645 - Attachment is obsolete: true
Attachment #555835 - Flags: review?(mark.finkle)
Attachment #555836 - Flags: review?(mark.finkle)
Attachment #555837 - Flags: review?(mark.finkle)
Attachment #555839 - Flags: review?(mark.finkle)
Attachment #555841 - Flags: review?(mark.finkle)
Attached image Screenshot
Here's how it looks. I'm using the same type of arrow box than the "new tab created" one. It's visually consistent with the other UI elements on form assistant. Need input from design team. This notification is only shown once the form is submitted. It's then "replaced" by any autocomplete suggestions once the user starts typing on the field. It more or less matches the behaviour on desktop Firefox btw.
Attachment #555835 - Flags: review?(mark.finkle) → review+
Attachment #555836 - Flags: review?(mark.finkle) → review+
Comment on attachment 555837 [details] [diff] [review]
(3/5) Change Content's formAssistant to be a public property

You missed some spots in the test files:

http://mxr.mozilla.org/mozilla-central/search?string=_formAssistant&find=mobile

r+, but fix those too.
Attachment #555837 - Flags: review?(mark.finkle) → review+
Comment on attachment 555839 [details] [diff] [review]
(4/5) Show notification for invalid form field on submit


>+    if (this._currentElement.validationMessage) {
>+      this._updateFormValidationFor(this._currentElement);
>+    } else {
>+      this._updateSuggestionsFor(this._currentElement);
>+    }

No need for {} for one liners. We use {} in both blocks if either the "if" or the "else" blocks are more than one line

>+
>+          if (json.current.validationMessage) {
>+            this._updateFormValidationFor(json.current);
>+          } else {
>+            this._updateSuggestionsFor(json.current);
>+          }

Same

>+  _updateFormValidationFor: function _formHelperUpdateFormValidationFor(aElement) {
>+    if (!aElement.validationMessage) {
>+      return;
>+    }

Same

r+, but fix the nits
Attachment #555839 - Flags: review?(mark.finkle) → review+
Comment on attachment 555841 [details] [diff] [review]
(5/5) Centralize popup updates on one method

>diff --git a/mobile/chrome/content/common-ui.js b/mobile/chrome/content/common-ui.js

>-    if (this._currentElement.validationMessage) {
>-      this._updateFormValidationFor(this._currentElement);
>-    } else {
>-      this._updateSuggestionsFor(this._currentElement);
>-    }
>+    this._updatePopupsFor(aElement);

ignore the some of the {} changes in the previous patch :)

>   _updateSuggestionsFor: function _formHelperUpdateSuggestionsFor(aElement) {
>     let suggestions = this._getAutocompleteSuggestions(aElement);
>     if (!suggestions.length) {
>-      ContentPopupHelper.popup = null;
>-      return;
>+      return false;
>     }

remove the {} since this becomes a 1-liner

>   _updateFormValidationFor: function _formHelperUpdateFormValidationFor(aElement) {
>     if (!aElement.validationMessage) {
>-      return;
>+      return false;
>     }

same here, even though this wasn't your code

r+ with nits
Attachment #555841 - Flags: review?(mark.finkle) → review+
Lucas - I assume we could make some browser-chrome tests for the validation UI. What tests does desktop use? Can we steal/copy some of those for mobile?
(In reply to Mark Finkle (:mfinkle) from comment #23)
> Lucas - I assume we could make some browser-chrome tests for the validation
> UI. What tests does desktop use? Can we steal/copy some of those for mobile?

There are some browser-chrome tests for that, here:
browser/base/content/test/browser_bug561636.js
Update chrome tests as suggested. Keeping the review+.
Attachment #555837 - Attachment is obsolete: true
Attachment #556041 - Flags: review+
Fixed all nits as per review. Keeping the review+.
Attachment #555839 - Attachment is obsolete: true
Attachment #556042 - Flags: review+
Fixed all nits as per review. Keeping the review+.
Attachment #555841 - Attachment is obsolete: true
Attachment #556043 - Flags: review+
I'm having trouble downloading the screenshot. Would someone mind emailing it to me or repost?

bdils@mozilla.com
Blocks: 668283
No real changes, just updated for new patch queue. Keeping the review+.
Attachment #555835 - Attachment is obsolete: true
Attachment #555836 - Attachment is obsolete: true
Attachment #557556 - Flags: review+
No real changes, updated for new patch queue. Keeping the review+.
Attachment #556041 - Attachment is obsolete: true
Attachment #557559 - Flags: review+
I've changed this patch quite a bit in response to the bugs I found while writing the chrome tests.
Attachment #556042 - Attachment is obsolete: true
Attachment #556043 - Attachment is obsolete: true
Attachment #557560 - Flags: review?(mark.finkle)
Attachment #557561 - Flags: review?(mark.finkle)
Attachment #557563 - Flags: review?(mark.finkle)
No directly related to this bug. Just a code improvement I did on the way.
Attachment #557566 - Flags: review?(mark.finkle)
No directly related to this patch. Just something I fixed on the way.
Attachment #557567 - Flags: review?(mark.finkle)
Remove some debugging leftovers and update commit message.
Attachment #557563 - Attachment is obsolete: true
Attachment #557577 - Flags: review?(mark.finkle)
Attachment #557563 - Flags: review?(mark.finkle)
Generally speaking, my implementation of form validation UI in Fennec is pretty much equivalent to Firefox on desktop, with a few necessary adaptations for the mobile context. Here's how it works:

- When a HTML form is submitted and it has at least one invalid element (text entry, text area, etc), the first invalid element will be automatically focused and zoomed into with a respective dark arrowbox (visually consistent with the other UI elements from the form assistant) showing the validation message.

- The invalid elements will be shown with red borders just like in Firefox Mobile (this is not something I implemented. It's just how gecko does it).

- Once the form is submitted and there are form elements with invalid content (e.g. an email entry that doesn't have an email in it), focusing on invalid elements will automatically show the arrowbox with the validation message. This arrowbox will only disappear once the content of the form element is valid. The message might update as the user enters content. For instance, if a text entry is both required and of email type, it will initially show a validation message requesting user to fill in the empty entry and as the user starts typing on it, it will change into a message regarding the email pattern until the text is actually an email address.

- I had to handle the case where a form element (say, a text entry) has both a validation message and autocomplete suggestions. Given that we can only show one arrow box at a time (to avoid cluttering the UI), I've done it so that when the invalid element is first focused it will always show the arrowbox with validation message first. Once the user starts entering text, autocomplete suggestions will have precedence and will be shown (just like it is implemented now) instead of the validation message.

The behaviour described above is fully covered in the browser-chrome tests I wrote btw.
Small simplification of the updatePopupsFor() method.
Attachment #557560 - Attachment is obsolete: true
Attachment #557687 - Flags: review?(mark.finkle)
Attachment #557560 - Flags: review?(mark.finkle)
Comment on attachment 557561 [details] [diff] [review]
(4/7) Hide form assistant when focused element is blurred

We just need to keep an eye open for weird states that cause a text field to briefly lose focus. Hopefully, nothing like this will happen.
Attachment #557561 - Flags: review?(mark.finkle) → review+
Attachment #557566 - Flags: review?(mark.finkle) → review+
Attachment #557567 - Flags: review?(mark.finkle) → review+
Attachment #557687 - Flags: review?(mark.finkle) → review+
Attachment #557577 - Flags: review?(mark.finkle) → review+
Brian, I'm requesting check-in. Feel free to give feedback once the code lands on nightly. We can definitely tweak/fix any issue that you find.
Keywords: checkin-needed
Let's make sure the test passes on Try before checking this in:
https://tbpl.mozilla.org/?usebuildbot=1&tree=Try&rev=2bc3a3b01789
Status: NEW → ASSIGNED
Keywords: checkin-needed
The test timed out on Try.  It looks like maybe it just exceeded the 30-second limit, so it might just need to call requestLongerTimeout.  There are also some JS exceptions in the log, but it looks like they did not cause any test failures:
https://ftp.mozilla.org/pub/mozilla.org/mobile/try-builds/mbrubeck@mozilla.com-2bc3a3b01789/try-android/try_tegra_android_test-browser-chrome-build1944.txt.gz
Try run for 2bc3a3b01789 is complete.
Detailed breakdown of the results available here:
    http://tbpl.allizom.org/?tree=Try&usebuildbot=1&rev=2bc3a3b01789
Results (out of 15 total builds):
    success: 11
    warnings: 3
    failure: 1
Builds available at http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/mbrubeck@mozilla.com-2bc3a3b01789
JavaScript Error: "Content.formAssistant is undefined" {file: "chrome://browser/content/content.js" line: 1070}

attachment 557559 [details] [diff] [review] should have changed that
I'll download the build and give it a try, but this sounds like a good solution. Sorry for the delay Lucas....
(In reply to Mark Finkle (:mfinkle) from comment #47)
> JavaScript Error: "Content.formAssistant is undefined" {file:
> "chrome://browser/content/content.js" line: 1070}
> 
> attachment 557559 [details] [diff] [review] should have changed that

Actually, something more fundamental is going wrong here. This exception happens on the very first form validation test:

[JavaScript Error: "uncaught exception: [Exception... "Component returned failure code: 0x80004003 (NS_ERROR_INVALID_POINTER) [inIDOMUtils.setContentState]"  nsresult: "0x80004003 (NS_ERROR_INVALID_POINTER)"  location: "JS frame :: chrome://browser/content/content.js :: _cancelTapHighlight :: line 622"  data: no]"]

No idea where this is coming from, to be honest.
Lucas, on the desktop version of Firefox the validation message doesn't show up until you try to submit the form and there are invalid fields. But on the fennec build I'm getting it on every field even before I tried to submit it. 

It's a little heavy-handed and covers the field below which would be a problem.  I would simply suggest that we have no UI for the form field until it is submitted. At which point we can use the red border only. I don't even think the validation message is necessary at all. It's pretty standard behavior that of the form doesn't get submitted correctly and form fields are in red, they need to be filled out. If the website owners want to take a step further and add dialog boxes or help text, they should do that themselves. I don't think we should do it on the browser side. What do you think?
It looks like the HTML5 spec says we should be validating on submission, or at least I see that mentioned a lot. However, all examples show some form of error message, shown in a popup of some kind.

Some docs:
https://developer.mozilla.org/en/HTML/HTML5/Forms_in_HTML5
http://blog.mozilla.com/webdev/2011/03/14/html5-form-validation-on-sumo/
UA's do not _have_ to show an error message when validating but they should not submit an invalid form so it seems pretty reasonable to show an error message, otherwise the user might not understand.

I think the form validation message should not appear until the user tries to submit the form like in Firefox Desktop. There is a notification the chrome can listen for that.
Yeah its the notification popping up before the submission that's the most jarring. In the screenshot it didn't seem so bad, but playing with live builds it's pretty distracting...
Comment on attachment 557687 [details] [diff] [review]
(3/7) Show notification for invalid form field on submit

Lucas, let's make sure we are only sending the "FormAssist:ValidationMessage" when receiving the "invalidformsubmit" notification. Don't do it when changing fields or entering data.
Attachment #557687 - Flags: review+ → review-
(In reply to Brian Dils [:briandils] from comment #50)
> Lucas, on the desktop version of Firefox the validation message doesn't show
> up until you try to submit the form and there are invalid fields. But on the
> fennec build I'm getting it on every field even before I tried to submit it. 

This is actually a regression between my original patch and the latest one. Fixing that now.
Only show validation message after form is submitted.
Attachment #557687 - Attachment is obsolete: true
Attachment #559422 - Flags: review?(mark.finkle)
Comment on attachment 559422 [details] [diff] [review]
(3/7) Show notification for invalid form field on submit

waiting for try results too
Attachment #559422 - Flags: review?(mark.finkle) → review+
(In reply to Mark Finkle (:mfinkle) from comment #58)
> Comment on attachment 559422 [details] [diff] [review]
> (3/7) Show notification for invalid form field on submit
> 
> waiting for try results too

FYI: I've made a small change in the browser tests that will hopefully make them work in Try. I'll submit the updated patch if Try goes green.
Try run for c0c3060d47d6 is complete.
Detailed breakdown of the results available here:
    http://tbpl.allizom.org/?tree=Try&usebuildbot=1&rev=c0c3060d47d6
Results (out of 19 total builds):
    exception: 2
    success: 10
    warnings: 6
    failure: 1
Builds available at http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/lrocha@mozilla.com-c0c3060d47d6
Just a minor update to the tests to get make them more reliable. Keeping the review+.
Attachment #557577 - Attachment is obsolete: true
Attachment #560447 - Flags: review+
Requesting check-in for my patches. I reported a separate bug to ensure that the new browser-chrome tests added here work fine once bug 687194 is fixed (which seems to be the reason why those tests are failing on Try).
Keywords: checkin-needed
The separate bug I referred to is bug 687201 by the way.
(In reply to Lucas Rocha (:lucasr) from comment #62)
> (which seems to be the reason why those tests are failing on Try).

Comment 62 seems slightly ambiguous - is this in a landable state, or are things going to fail until bug 687201 lands? 

The "once bug 687194 is fixed" implies the former, but the "reason why those tests are failing on Try" implies it fails even now?!?

Thanks :-)
I thought I'd just send to try and see what it said, but at least four of the patches don't apply cleanly to inbound, so I'm going to leave it for now. Removing checkin-needed for now.

Thanks :-)
Keywords: checkin-needed
(In reply to Ed Morley [:edmorley] from comment #65)
> I thought I'd just send to try and see what it said, but at least four of
> the patches don't apply cleanly to inbound, so I'm going to leave it for
> now. Removing checkin-needed for now.

Thanks Ed, I'll submit updated files. About whether to check in those patches or not, maybe a better way to handle this is to check-in all patches except the tests. We can check-in the patch with the tests (5/7) as part of bug 687201 which depends on bug 687194.

Mark, what do you think?
Just updated to apply to latest trunk. Keeping review+
Attachment #557556 - Attachment is obsolete: true
Attachment #560899 - Flags: review+
Just updated to apply to latest trunk. Keeping review+
Attachment #557559 - Attachment is obsolete: true
Attachment #560900 - Flags: review+
Just updated to apply to latest trunk. Keeping review+
Attachment #559422 - Attachment is obsolete: true
Attachment #560901 - Flags: review+
Just updated to apply to latest trunk. Keeping review+
Attachment #557561 - Attachment is obsolete: true
Attachment #560902 - Flags: review+
Attachment #557566 - Attachment is obsolete: true
Attachment #560903 - Flags: review+
Just updated to apply to latest trunk. Keeping review+
Attachment #557567 - Attachment is obsolete: true
Attachment #560904 - Flags: review+
Comment on attachment 560447 [details] [diff] [review]
(5/7) Add browser-chrome tests for HTML5 form validation UI

This patch is now attached to bug 687201. It should be checked in once 687194 is fixed.
Attachment #560447 - Attachment is obsolete: true
Requesting check-in of all 6 patches. HTML5 form validation tests will be checked-in once browser-chrome tests are working fine. These 6 patches do not break any existing tests.
Keywords: checkin-needed
Blocks: 687201
Target Milestone: --- → Firefox 9
Verified fixed on:
Mozilla/5.0 (Android;Linux armv7l;rv:9.0a1)Gecko/20110927
Firefox/9.0a1 Fennec/9.0a1
Device: Acer ICONIA A500
OS: Android 3.1
Status: RESOLVED → VERIFIED
Flags: in-litmus?(fennec)
The HTML 5 documentation as per the URL, https://developer.mozilla.org/en/HTML/Forms_in_HTML#Constraint_Validation, says:

=========
If the title attribute is set on the <input> element, that string is displayed in a tooltip when validation fails. If the title is set to the empty string, no tooltip is displayed. If the title attribute isn't set, the standard validation message (as specified by the x-moz-errormessage attribute or by calling the setCustomValidity() method) is displayed instead.
=========
But I tried with the following example, but I am still getting the standard error message, "Please fill out this field". Is this correct? Or did I miss anything in the code?

<!DOCTYPE html>
<html>
<head>
<title> new document </title>
</head>

<body>
<form>
<input type="text" title="Hello there" required="required">
<input type="submit">
</form>
</body>
</html>
(In reply to Satish from comment #78)
> The HTML 5 documentation as per the URL,
> https://developer.mozilla.org/en/HTML/Forms_in_HTML#Constraint_Validation,
> says:
> 
> =========
> If the title attribute is set on the <input> element, that string is
> displayed in a tooltip when validation fails. If the title is set to the
> empty string, no tooltip is displayed. If the title attribute isn't set, the
> standard validation message (as specified by the x-moz-errormessage
> attribute or by calling the setCustomValidity() method) is displayed instead.
> =========
> But I tried with the following example, but I am still getting the standard
> error message, "Please fill out this field". Is this correct? Or did I miss
> anything in the code?
> 
> <!DOCTYPE html>
> <html>
> <head>
> <title> new document </title>
> </head>
> 
> <body>
> <form>
> <input type="text" title="Hello there" required="required">
> <input type="submit">
> </form>
> </body>
> </html>

Thanks for reporting this.
This is not specific to Fennec, this also happens in Firefox, so it appears to be a platform bug if your assumption is correct.

Mounir has done the initial implementation of this feature for Firefox so he his the guy to ask when there is a specification point.
Thanks...

I have actually tested in Firefox Aurora 9.0a2. I came to this Bugzilla page via, http://lucasr.org/2011/09/20/html5-form-validation-in-firefox-mobile/ so I reported it here.
title only appears in the validation error message when coupled with the pattern attribute. In other words, you can write in plain english (or whatever language) what the pattern attribute means. The reason is that the pattern attribute isn't accessible as-is.

You can try this:
data:text/html,<form><input required pattern="foo" title="You have to type foo"></form>

If you focus the input field and press enter, you will have a message asking you to fill out the field, if you type let say "bar", you will have a message that will show the title string.

If you quote of MDC is correct, the article is wrong.
Blocks: 704879
No longer blocks: 704879
Do we have a test page?
I tested this using this page: http://www.bradshawenterprises.com/tests/formdemo.php.

The bug is still reproducible so I looked if there is bug filed for Native Fennec since this is fixed for Fennec XUL and I found bug 704879 which is still new.

I will remove the in-litmus flag from here and set it for the bug for Native Fennec.
Flags: in-litmus?(fennec) → in-litmus-
Depends on: 345624
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: