Closed Bug 561636 Opened 14 years ago Closed 14 years ago

When an invalid form is submitted, error messages should be displayed

Categories

(Firefox :: General, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 4.0b7
Tracking Status
blocking2.0 --- beta7+

People

(Reporter: mounir, Assigned: mounir)

References

(Depends on 2 open bugs, Blocks 3 open bugs, )

Details

(Keywords: dev-doc-complete, html5)

Attachments

(9 files, 12 obsolete files)

31.02 KB, image/png
Details
112.05 KB, image/png
Details
35.82 KB, image/png
Details
4.87 KB, patch
smaug
: review+
Details | Diff | Splinter Review
9.32 KB, patch
Dolske
: review+
Details | Diff | Splinter Review
13.83 KB, patch
Dolske
: review+
Details | Diff | Splinter Review
14.53 KB, patch
smaug
: review+
Details | Diff | Splinter Review
551 bytes, text/html
Details
13.91 KB, patch
Details | Diff | Splinter Review
With the constraint validation in HTML5 Forms, when an element is invalid, the user should be informed. At least, when the user tries to submit the form because the form submission should be canceled if an element is invalid. Maybe also when the user stop interact with an element (unfocus) which is now in an invalid state.

We could probably use tooltip to show the error message.
Adding David and Alex for UI and a11y.
Having something similar to the Mac OS X field/textarea glow but in the colors red and green, instead of the regular blue, would look nice in my opinion. The glow would not be visible on page load, but as soon as any input is made, it either glows red or green depending on the field type and input. Of course, the user should also be notified about what actually is wrong, by, for example a tooltip, as you said.
I do not think we should have a default style when an element is valid or invalid. Authors will be able to use :valid and :invalid CSS pseudo-class for that. Setting a default style could be a bit too intrusive in the page and will probably be sometimes not desired.
(In reply to comment #1)
> Adding David and Alex for UI and a11y.

When element is marked as invalid then invalid state should be exposed on accesible object and state change should be fired (see bug 555728). I think no any special UI is necessary to emphasize this for AT clients.

(In reply to comment #3)
> I do not think we should have a default style when an element is valid or
> invalid. Authors will be able to use :valid and :invalid CSS pseudo-class for
> that. Setting a default style could be a bit too intrusive in the page and will
> probably be sometimes not desired.

It might be nice to have. Web developers might find a default styling handy. If they do not like it then they should be able to override it.

(In reply to comment #0)

> We could probably use tooltip to show the error message.

What element should tooltip be attached to? What should it contain?
(In reply to comment #4)
> What element should tooltip be attached to? What should it contain?

I think the tooltip should be attached to the invalid element and contain |element.validationMessage| string.
(In reply to comment #5)
> (In reply to comment #4)
> > What element should tooltip be attached to? What should it contain?
> 
> I think the tooltip should be attached to the invalid element and contain
> |element.validationMessage| string.

Ok, that makes sense. Does the spec defines (or give a hint for) UI of validationMessage property?
(In reply to comment #6)
> Ok, that makes sense. Does the spec defines (or give a hint for) UI of
> validationMessage property?
AFAICT, there is only mentioned the UA should inform the user.

I'm wondering if we could use the new notification API (bug 398776) for this ?
CCing Gavin to know his opinion.
During the form submission [1], the elements constraints have to be checked [2]. If the element is invalid and the invalid event isn't canceled [3], we have to inform the user at least one element is not valid.
Recommendations are: "User agents may focus one of those elements in the process, by running the focusing steps  for that element, and may change the scrolling position of the document, or perform some other action that brings the element to the user's attention. User agents may report more than one constraint violation. User agents may coalesce related constraint violation reports if appropriate (e.g. if multiple radio buttons in a group are marked as required, only one error need be reported)."

Opera is the only UA implementing that at the moment. When the form is submitted, if there is at least one invalid element, the first one is blinking and a tooltip is showing the error message.
I will attach a screenshot of what opera is doing.

I think we should only show a tooltip for the first invalid element too because it can be really messy if there are a lot of invalid elements in the same page. The element should be focused too (Opera isn't focusing the element).
I will try to attach a mock-up.

[1] http://dev.w3.org/html5/spec/forms.html#form-submission
[2] http://dev.w3.org/html5/spec/forms.html#interactively-validate-the-constraints
[3] http://dev.w3.org/html5/spec/forms.html#statically-validate-the-constraints
Component: Form Manager → General
Product: Toolkit → Firefox
QA Contact: form.manager → general
Version: unspecified → Trunk
Mounir, the problem with a tooltip is that it can easily be dismissed (times out) or even be missed by several groups of users. We have the notification bar in Firefox (the one that also shows the "do you want to save this password?" prompt, and I think that should be used to notify the user of the problem in the form. The advantage: This one is already fully accessible, screen readers will pick it upautomatically as it appears, and it will stick, allowing people with low-vision to also be able to see it without it timing out.
(In reply to comment #10)
> Mounir, the problem with a tooltip is that it can easily be dismissed (times
> out) or even be missed by several groups of users. We have the notification bar
> in Firefox (the one that also shows the "do you want to save this password?"
> prompt, and I think that should be used to notify the user of the problem in
> the form. The advantage: This one is already fully accessible, screen readers
> will pick it upautomatically as it appears, and it will stick, allowing people
> with low-vision to also be able to see it without it timing out.

The idea is interesting but it may be quite difficult to associate the message with the invalid element. I mean, if I have the massage in the notification bar, how could I know which element is invalid ?
You can still blink the element, or also add a button to the notification that would allow to focus the offending element.
I've done 3 screens:
http://oldworld.fr/mozilla/mockups/form-validation/form-validation-1.png
http://oldworld.fr/mozilla/mockups/form-validation/form-validation-2.png
http://oldworld.fr/mozilla/mockups/form-validation/form-validation-3.png

This is using Marco's idea: a message is shown in the navigation bar if the form is not valid. The first invalid element is shown just below the navigation bar and the user can still click on "Show element" to have it blinking (in case of it's not obvious). When the user has changed the element's value, he/she can re-submit the element which will re-try validating the form so it will show another error for the next invalid element if any. In some way, it could help navigating in the invalid form controls.

I'm wondering if i would be better to show a first message in the navigation bar like "The form is not valid, click on the button to go to the next invalid element" then, the navigation bar could only show the error from the element with "Next" button if there is another invalid element or "Submit" if there is no more invalid elements. This is more clear in case of there are a lot of invalid elements but maybe we can assume regular users are going to have only one or two invalid elements (a missing required or a non-matching password for example).

We could also imagine using the current form manager on Fennec to show the invalid elements.

I will try to create some mockups with a notification instead of the navigation bar.
Assignee: nobody → mounir.lamouri
Status: NEW → ASSIGNED
(In reply to comment #14)
> (In reply to comment #13)
> > I've done 3 screens:
> > http://oldworld.fr/mozilla/mockups/form-validation/form-validation-1.png
> > http://oldworld.fr/mozilla/mockups/form-validation/form-validation-2.png
> > http://oldworld.fr/mozilla/mockups/form-validation/form-validation-3.png
> 
> Does the white box at the right side of these screenshots come from Firefox, or
> page content?

I've added this box to explain the screenshot. Do you mean identi.ca design is as bad as my Gimp skills ? ;)
(In reply to comment #15)
> (In reply to comment #14)
> > (In reply to comment #13)
> > > I've done 3 screens:
> > > http://oldworld.fr/mozilla/mockups/form-validation/form-validation-1.png
> > > http://oldworld.fr/mozilla/mockups/form-validation/form-validation-2.png
> > > http://oldworld.fr/mozilla/mockups/form-validation/form-validation-3.png
> > 
> > Does the white box at the right side of these screenshots come from Firefox, or
> > page content?
> 
> I've added this box to explain the screenshot. Do you mean identi.ca design is
> as bad as my Gimp skills ? ;)

Oh, no, I just wanted to make sure that it's not something that we add to the page content, because of l10n concerns.

Please note that we have to be able to handle bidirectional text here.  I think we're safe for the most part, but this is something which we should be keeping in mind.  I'm CCing Simon to keep him in the loop on the bidi stuff.
Renaming the bug to focus on the form validation error messages.
Summary: When an element is invalid, the user should be informed → When an invalid form is submitted, error messages should be displayed
Depends on: 561634
Blocks: 566348
(In reply to comment #17)
> http://oldworld.fr/mozilla/mockups/form-validation/form-validation-notification-1.png
> http://oldworld.fr/mozilla/mockups/form-validation/form-validation-notification-2.png
> http://oldworld.fr/mozilla/mockups/form-validation/form-validation-notification-3.png
> 
> Other mockups using UI from the new notification API.
> 
> Alexander, what's your opinion about these mockups ?

I think they are a little bit too complex. Putting previous/next navigation in the pop-ups is confusing, and inserting a submit button when the last missing value is filled in is also a bit over the top. 

I'll add my suggestion for how it should look/work.
- When a field does not satisfy the constraints, we glow the field red + make the border red when you leave the field without a valid value.

- The submit button has this same glow if you aren't allowed to submit the form because of this. If you try to click it anyway, you get moved to the missing/invalid form element with the error text visible.

The guiding principle is that we do not want to get in the way while you're filling out the form — which is why we don't show the error as you exit the field, just the glow — but we want to be more verbose when you try to submit an invalid form.

The styling of the help/error text doesn't have to look like the above, and slight transparency might be better to show that there's content underneath, in the cases where we overlap other form fields like shown above.
Do we also want to show the error message if the user explicitly focuses a field with a red glow? To aid them if they go back to correct an error before actually hitting the submit button.
Thank you for the mockups Alexander! :)

I'm wondering if the red glow (+ red border) wouldn't conflict with :required, :optional, :valid and :invalid ? I suppose the basic usage of :invalid could be:
:invalid { border: 1px red solid; }
However, if we use :invalid with a default to show the glow, we may have some sites with nothing and it would be hard to have the same appearance for the "submit" button.

(In reply to comment #21)
> Do we also want to show the error message if the user explicitly focuses a
> field with a red glow? To aid them if they go back to correct an error before
> actually hitting the submit button.

I think it's a good idea.
(In reply to comment #22)
> Thank you for the mockups Alexander! :)
> 
> I'm wondering if the red glow (+ red border) wouldn't conflict with :required,
> :optional, :valid and :invalid ? I suppose the basic usage of :invalid could
> be:
> :invalid { border: 1px red solid; }
> However, if we use :invalid with a default to show the glow, we may have some
> sites with nothing and it would be hard to have the same appearance for the
> "submit" button.

I'm not fully following your concern here. Is the concern that sites will have CSS like:

input.foo { box-shadow: none }

and thus inadvertently turn off the glow? Or are you concerned that they will have

input:invalid { border: 1px solid blue }

and thus hide the glow?
(In reply to comment #23)
> (In reply to comment #22)
> > Thank you for the mockups Alexander! :)
> > 
> > I'm wondering if the red glow (+ red border) wouldn't conflict with :required,
> > :optional, :valid and :invalid ? I suppose the basic usage of :invalid could
> > be:
> > :invalid { border: 1px red solid; }
> > However, if we use :invalid with a default to show the glow, we may have some
> > sites with nothing and it would be hard to have the same appearance for the
> > "submit" button.
> 
> I'm not fully following your concern here. Is the concern that sites will have
> CSS like:
> 
> input.foo { box-shadow: none }
> 
> and thus inadvertently turn off the glow? Or are you concerned that they will
> have
> 
> input:invalid { border: 1px solid blue }
> 
> and thus hide the glow?

I'm wondering if the glow Alexander want to show isn't conflicting with :invalid/:valid/:optional/:required. It may be turned-off or just make the website really ugly. Otherwise, it should be something customizable by the website but in that case, we may want to directly use :invalid I suppose.
I'm still not sure I understand your concern. What I think we should do is add something like the following to our html.css:

input:invalid, select:invalid,
input:-moz-submit-invalid, button:-moz-submit-invalid {
  box-shadow: 0 0 2px red;
}

where -moz-submit-invalid is a pseudo class that only matches submitting elements whose forms contain invalid controls. It would also never match elements with the novalidate attribute set.

Would this satisfy your concerns? (I personally have some concerns with this implementation, but I think it's good enough to get to people to see how well it works).
CCing David and Marco for a11y.
sicking's comments in 23/25 wrt to CSS make sense to me, I don't think it's any different than the default style we give to, say, table borders or buttons. Sites that want to mange every detail of their appearance can just override them. I'd just clone the existing focus-ring styles from other places. [Though I'd also probably give different colors for :invalid and :invalid:focused, the focused case should be a bright saturated color like the normal focus-ring color, the unfocused color less saturated. But that's a refinement...]

One way to implement the on-hover (?) tooltip-like thing would be to, well, just set that as the default tooltip text (unless content overrides it), and literally use tooltips. That might be good enough for a first-pass implementation of this? Implementing the pointy-panel ala the mockup would be nice, but is a bit more work... Maybe XBL to bind a <panel> of appropriate style to the input, with onhover activation?

Where's the tooltip text come from? Something in content, or is it static text per input type? What's HTML5 say, if anything, about how content can control its display?
The text comes from various sources, but can be retrieved through a DOM attribute (element.validationMessage)

The "tooltip" in the mockup isn't displayed on hover, but rather when the element is in invalid state, and the element has focus, so I don't think we can use normal tooltips.

HTML5 does not say how the error message should be displayed.
(In reply to comment #28)
> The "tooltip" in the mockup isn't displayed on hover, but rather when the
> element is in invalid state, and the element has focus, so I don't think we can
> use normal tooltips.

As I understand limi's mockup, the tooltip should be shown when trying to submit an invalid form. The first invalid element should be focused and the tooltip should be shown. The tooltip should not be shown each time the user focuses an element.
If I got it correctly, the idea is to not disturb the user until he really tries to do something wrong. I guess the user should know what are the constraints with the page context so the error message should not be needed in most cases.
Blocks: 344614
Depends on: 580575
You are correct. My point was that simply using a tooltip won't work. We need something else where we have more control over when and where the popup is displayed.
Depends on: 566045
Ok, talked with limi about the mockup. One of the concerns I had was that since we don't know anything about the layout of the page, we need to be careful about when/how we overlay error text, so that it doesn't obscure something important on the page.

There's a few different issues at play, here's the updated proposal:

Input style
-----------

* The red-ring "invalid" style is very much desired here, should figure out a way to do it without causing textfields to look non-native.
* The red-ring should have a different style (desaturated) for unfocused fields, compared to focused invalid fields. Since it replaces the normal focus ring, the user should be able to tell which invalid input is the one focused.
* Avoid showing the invalid style while the user is typing, because it's distracting. Don't update the state unless X milliseconds have elapsed since the last input. [First thought was to only change style when focus changes to another field, but that leads to awkward interactions when you're trying to figure out how to make the input valid. We use a delay mechanism for the urlbar and search box.]
* Don't show invalid style when the page initially loads (to avoid sea of red)
* Don't show invalid style unless the user has modified the input's value (to avoid tabbing through a blank form triggering a bunch of red)
* Do ignore the last two "don't" points if the user has attempts to submit the invalid form -- upon submit, all invalid inputs should be styled as such.


Error message
-------------
* Invalid inputs default to not showing any form of error-message UI
* When hovering over an invalid input, show the error message. Dismiss upon a click, keypress, or mousemove.
* When attempting to submit an invalid form, the error-message UI should be shown for the first invalid field (only). Dismiss upon a click or keypress.
* Enn is working on a <panel> type (bug 554937) for the kind of style as in limi's mockup, that's probably what we should use.


Invalid Submission
------------------
* Submit button, if any, should have red-ring when invalid (but not required?). Button should remain enabled and clickable.
* When submission is attempted, the page should scroll -- NOT jump -- so that the first invalid input is visible on the screen. (Jumping to that position may make the use think they've navigated to the submitted page).
* As noted above, upon submission all invalid fields will be styled as such, and the error message UI will be shown for the first invalid field.

Might be possible to push a few of these things to followup bugs, but this is the desired overall UI.
(In reply to comment #31)
> Ok, talked with limi about the mockup. One of the concerns I had was that since
> we don't know anything about the layout of the page, we need to be careful
> about when/how we overlay error text, so that it doesn't obscure something
> important on the page.
> 
> There's a few different issues at play, here's the updated proposal:

Thank you for taking the time to speak to limi and report here :)

> Input style
> -----------
> 
> * The red-ring "invalid" style is very much desired here, should figure out a
> way to do it without causing textfields to look non-native.

We are trying to do that, see bug 566045.

> * Avoid showing the invalid style while the user is typing, because it's
> distracting. Don't update the state unless X milliseconds have elapsed since
> the last input. [First thought was to only change style when focus changes to
> another field, but that leads to awkward interactions when you're trying to
> figure out how to make the input valid. We use a delay mechanism for the urlbar
> and search box.]

I think this is reducing the "explorability". I mean, for example, if I have to write something which needs to follow a pattern (with @pattern), with a dynamic invalid style, I will quite easily know when I type an invalid character.
In addition, to be honest, for some users that may be interpreted like "Oh, Firefox is so slow it has to compute during 2 seconds to know if my field is now valid!".

> * Don't show invalid style when the page initially loads (to avoid sea of red)
> * Don't show invalid style unless the user has modified the input's value (to
> avoid tabbing through a blank form triggering a bunch of red)
> * Do ignore the last two "don't" points if the user has attempts to submit the
> invalid form -- upon submit, all invalid inputs should be styled as such.

I do not completely agree with these points. First, you have to know only elements marked with @required will be invalid 'by default'. All other validity constraints consider no value as valid. So, you will not really see a sea of red. However, I understand that may be weird for the user to see something marked as invalid even if he/she didn't interact with it.
We could consider not showing invalid style if the element is suffering from being missing and the element value hasn't been changed for <input> and <textarea>. However, for <input type='checkbox|radio'> it would not be doable.
Another idea would be to use :invalid:required. I think, authors should be able to play with that to be sure the style is correct. We could also provide a default style in that case.
Finally, I agree that in a UX POV not showing invalid required elements before the first form submission attempt is a good idea, I'm not sure it's the best thing to force this behavior for all the web. I would prefer letting the choice to the authors.

I would really appreciate your feedback on that Jonas.

> Error message
> -------------
> * Invalid inputs default to not showing any form of error-message UI
> * When hovering over an invalid input, show the error message. Dismiss upon a
> click, keypress, or mousemove.

I'm wondering if that tooltip couldn't be conflicting with another one? In that case, what should we do? On top of my head I see @pattern tooltip which is describing the pattern with @title but I guess we could show @title when the form is valid and the validation message otherwise. Maybe we could _always_ show the validation message when the element isn't valid.

> * When attempting to submit an invalid form, the error-message UI should be
> shown for the first invalid field (only). Dismiss upon a click or keypress.
> * Enn is working on a <panel> type (bug 554937) for the kind of style as in
> limi's mockup, that's probably what we should use.

Great! Should we use that only for the error-message when trying to submit the form and native tooltip for :hover?

> Invalid Submission
> ------------------
> * Submit button, if any, should have red-ring when invalid (but not required?).
> Button should remain enabled and clickable.

Maybe we could style it differently to show to the user that's not an invalid field but the form is not valid? I was thinking of opacity (in addition of the red-ring maybe?). Just an idea, not a strong feeling.

> * When submission is attempted, the page should scroll -- NOT jump -- so that
> the first invalid input is visible on the screen. (Jumping to that position may
> make the use think they've navigated to the submitted page).

I completely agree but I've no idea if that is currently doable while calling focus on an element. So, it may be a very good candidate for a follow-up.

> * As noted above, upon submission all invalid fields will be styled as such,
> and the error message UI will be shown for the first invalid field.
> 
> Might be possible to push a few of these things to followup bugs, but this is
> the desired overall UI.

Again, thank you a lot for this very exhaustive list :)
Depends on: 554937
Depends on: 581947
(In reply to comment #31) 
> Error message
> -------------
> * Invalid inputs default to not showing any form of error-message UI
> * When hovering over an invalid input, show the error message. Dismiss upon a
> click, keypress, or mousemove

There is a patch for that in bug 581947.
(In reply to comment #32)

> > * Avoid showing the invalid style while the user is typing
> 
> I think this is reducing the "explorability".

The problem is that this is _extremely_ distracting if the field is blinking between valid and invalid states as you're typing a value. (For example, typing in a long number where the constraint is that the number is odd. You could get is flashing back-and-forth on every digit).

Similarly, when you know what you're going to type (say, a phone number), having the UI distract you as you're entering the value isn't really helpful.

> > * Don't show invalid style when the page initially loads
> > * Don't show invalid style unless the user has modified the input's value

> All other validity constraints consider no value as valid.

Ah, I should probably read the spec again.

> Finally, I agree that in a UX POV not showing invalid required elements before
> the first form submission attempt is a good idea, I'm not sure it's the best
> thing to force this behavior for all the web.

That's a good point. There could also be some tricky interactions with having the UI visually lag the actual CSS state, especially if we want to also give page authors to control the immediacy of feedback. Hmm.


> > Error message
> > -------------
> > * Invalid inputs default to not showing any form of error-message UI
> > * When hovering over an invalid input, show the error message. Dismiss upon a
> > click, keypress, or mousemove.
> 
> I'm wondering if that tooltip couldn't be conflicting with another one?

I'm not sure I understand.

Think of this as being a refinement of submitting and getting a modal error dialog... Instead of a dialog, you get an in-content overlay that dismisses itself as soon as you attempt to interact with the page.

> Maybe we could _always_
> show the validation message when the element isn't valid.

I don't think so, we don't know how the page is designed, so have to assume that whatever we show is likely to overlap something important (eg, text explaining something about allowed format that isn't reflected in the error message we show).

> > * Enn is working on a <panel> type (bug 554937) for the kind of style as in
> > limi's mockup, that's probably what we should use.
> 
> Great! Should we use that only for the error-message when trying to submit the
> form and native tooltip for :hover?

Ideally, yes. Though I don't see a patch in that bug yet. :)

> > Invalid Submission
> > ------------------
> > * Submit button, if any, should have red-ring when invalid (but not required?).
> > Button should remain enabled and clickable.
> 
> Maybe we could style it differently to show to the user that's not an invalid
> field but the form is not valid? I was thinking of opacity (in addition of the
> red-ring maybe?). Just an idea, not a strong feeling.

Yeah, we didn't talk about this extensively. This could be something to handle in a followup bug...

> > * When submission is attempted, the page should scroll -- NOT jump
> 
> I completely agree but I've no idea if that is currently doable while calling
> focus on an element. So, it may be a very good candidate for a follow-up.

Yeah, this is probably something that might be better to handle in the front end code... Content could just fire a notification, and the FE could handle the scrolling.
(In reply to comment #32)
> > * Don't show invalid style when the page initially loads (to avoid sea of red)
> > * Don't show invalid style unless the user has modified the input's value (to
> > avoid tabbing through a blank form triggering a bunch of red)
> > * Do ignore the last two "don't" points if the user has attempts to submit the
> > invalid form -- upon submit, all invalid inputs should be styled as such.
> 
> I do not completely agree with these points. First, you have to know only
> elements marked with @required will be invalid 'by default'. All other validity
> constraints consider no value as valid. So, you will not really see a sea of
> red. However, I understand that may be weird for the user to see something
> marked as invalid even if he/she didn't interact with it.
> We could consider not showing invalid style if the element is suffering from
> being missing and the element value hasn't been changed for <input> and
> <textarea>. However, for <input type='checkbox|radio'> it would not be doable.
> Another idea would be to use :invalid:required. I think, authors should be able
> to play with that to be sure the style is correct. We could also provide a
> default style in that case.
> Finally, I agree that in a UX POV not showing invalid required elements before
> the first form submission attempt is a good idea, I'm not sure it's the best
> thing to force this behavior for all the web. I would prefer letting the choice
> to the authors.

I agree with Limi/Dolske that we need something other than immediately updating redness. Even if empty controls always count as valid, I think we want to avoid the control immediately turning red as soon as you start typing. That easily gives the impression that the user did something wrong, whereas the user might just not have finished typing yet.

However, given that empty non-required fields aren't invalid, how about this strategy:

For non-required fields:
* If the control does not have focus, always color invalid fields red.
* If the control is focused, and it was valid (including empty) when it originally gained focus, don't color the field invalid.
* If the control is focused, and it was invalid when it originally gained focus, update the control immediately on every keystroke.

This means that if the control was valid when the user started interacting with it, it only turns red once the user moves on to edit another field (and thus usually considers him/herself done editing the control). However if the user is trying to correct a previously error-marked control, we aid by immediately showing when the value changes between valid and invalid.

For required fields, we do the above only if the user has done any edits in the control, or has attempted to submit the form.

> > Error message
> > -------------
> > * Invalid inputs default to not showing any form of error-message UI
> > * When hovering over an invalid input, show the error message. Dismiss upon a
> > click, keypress, or mousemove.
> 
> I'm wondering if that tooltip couldn't be conflicting with another one? In that
> case, what should we do? On top of my head I see @pattern tooltip which is
> describing the pattern with @title but I guess we could show @title when the
> form is valid and the validation message otherwise. Maybe we could _always_
> show the validation message when the element isn't valid.

Tooltips generally don't get in the way of things as the user can easily dismiss them by moving around the mouse.

I think it might be useful though to be able to see the error message while editing the form control. I think I liked the initial suggestion better.

> > Invalid Submission
> > ------------------
> > * Submit button, if any, should have red-ring when invalid (but not required?).
> > Button should remain enabled and clickable.

Doesn't this result in a lake-of-red since many submit buttons would be red by default.

How about we make the submit button glow red if any of the form controls inside it (according to the rules above) are glowing red.
blocking2.0: --- → ?
Depends on: 582277
I think we should keep in mind that the red glow on the form elements is part of :invalid behavior so that's not a Firefox-only behavior. I think we should land everything with the full respect of the current specifications (:invalid/:valid is updated on each keystrokes). Then, depending on the feedback we have, we will modify the behavior.

By the way, I think Jonas' proposition is good and that sounds to be a good candidate for a follow-up. What's your opinion Justin?

> I think it might be useful though to be able to see the error message while
> editing the form control. I think I liked the initial suggestion better.

Having .validationMessage in the tooltip of the element isn't removing the big tooltip that was in the initial suggestion. This will be implemented with <panel>. I think being able to see the error message in the tooltip is quite good to understand "why this textfield has a red glow?!"

I've open bug 582277 for :-moz-submit-invalid default style. For :-moz-submit-invalid behavior, I think we should do like for :invalid : land somethnig that shows the default style if at least one element is invalid in a first time. Then, we could think about a better behavior in a follow-up.
Actually, I think we can prevent the sea-of-red here by having a quite discrete default style.
Tried today's tryserver build and noticed something that should probably be considered.

<label for="part" >Part number:</label>
<input pattern="[0-9][A-Z]{3}" name="part" required="required" placeholder="9AAA"
        title="A part number is a digit followed by three uppercase letters." /> 

The tooltip for the above example shows the "required" message:
"This field is mandatory. You have to fill it."

Shouldn't it also be showing the "title" since there's a "pattern"? 
Perhaps as in, e.g.:
"A part number is a digit followed by three uppercase letters.
This field is mandatory. You have to fill it."

which looks like an example in the spec.
http://www.whatwg.org/specs/web-apps/current-work/multipage/common-input-element-attributes.html#attr-input-pattern
(unfortunately the example doesn't include the "required" attribute as it probably should - I'll ping hixie about that). 

FYI, Opera currently handles this way:
Tooltip always has (before or after submit): 
"A part number is a digit followed by three uppercase letters."

After submit, the "big tooltip"/error message popup has:
"You have to specify a value."
(In reply to comment #37)
> Tried today's tryserver build and noticed something that should probably be
> considered.
> 
> <label for="part" >Part number:</label>
> <input pattern="[0-9][A-Z]{3}" name="part" required="required"
> placeholder="9AAA"
>         title="A part number is a digit followed by three uppercase letters."
> /> 
> 
> The tooltip for the above example shows the "required" message:
> "This field is mandatory. You have to fill it."
> 
> Shouldn't it also be showing the "title" since there's a "pattern"? 
> Perhaps as in, e.g.:
> "A part number is a digit followed by three uppercase letters.
> This field is mandatory. You have to fill it."

That's part of my comment (comment 32): what should we do if we have more than one tooltip to show? For the moment, with the patch from bug 581947, as soon as an element is invalid, the tooltip will show the validation message error. So, indeed, if an element is suffering from being missing, it will show the validation message without showing the pattern description.
There is something we can change: if an element is invalid it can be invalid for more than one reason but only one message will be shown. We could make the value missing error message less important than other ones so the pattern mismatch error message would be shown in your example.

> which looks like an example in the spec.
> http://www.whatwg.org/specs/web-apps/current-work/multipage/common-input-element-attributes.html#attr-input-pattern
> (unfortunately the example doesn't include the "required" attribute as it
> probably should - I'll ping hixie about that). 

The examples are non-normative and showing the tooltip describing the pattern isn't a requirement.

> FYI, Opera currently handles this way:
> Tooltip always has (before or after submit): 
> "A part number is a digit followed by three uppercase letters."
> 
> After submit, the "big tooltip"/error message popup has:
> "You have to specify a value."

Actually, this make me think we really should re-order the validation messages: it may be better to say to the user the field doesn't follow the required pattern than the field should be filed: the former implying the later.
That maybe be a good candidate for a follow-up.
I am going to create a new method in nsIFormSubmitObserver (NotifyInvalidSubmitAttempt) so I need to rename Notify to NotifySubmit to make the name less confusing. It's not in the main patch to be less confusing

There would be another way to implement that: have an observer listening to the form submission and let it check the form validity (instead of being done by the form). The observer could be able to cancel the form submission with aCancel but I think it is better to check everything in the form and only send the event if needed.
Attachment #465280 - Flags: review?(dolske)
This is only WIP. Some improvements and fixes are needed but the design of the implementation is there.
Dolske, could I have your feedback?

Quickly, what needs to be improved:
- following the specifications: I am getting the first invalid form element but I should get the first invalid form element with invalid event not canceled (that means an author can disable our interface by canceling all invalid events) ;
- popup design: I would like to have a max-width but it doesn't breaks the label so it hides most of the text. I guess it's a basic XUL issue but I'm not a XUL person ;)
- popup behavior: I should make the popup disappear on a keypress. I should also check that the popup isn't already showed when I try to show it (it looks like it behaves like a toggle so openPopup() called when a popup is already open hides it...). I've seen some really annoying bug with this popup which should be investigating: 1. it blocks my desktop change shortcuts (I hope it will disappear with the keypress handling) 2. it still visible when I change tab (I hope it will disappear with the keypress handling too)
- crash: I have a nice crash when exiting Firefox.
- cleaning
Attachment #465284 - Flags: feedback?
Attachment #465284 - Flags: feedback? → feedback?(dolske)
By the way, shouldn't this bug be blocking beta5+ instead of betaN+? I guess we are not going to land all HTML5 Forms related patches after feature freeze, right?
Attached image UI Preview v1 (obsolete) —
A simple UI preview of the current WIP patch.
It shows how important max-width would be ;)
blocking2.0: betaN+ → beta5+
Comment on attachment 465280 [details] [diff] [review]
Part 1 - Rename nsIFormSubmitObserver::Notify

I don't think we should rename the API; that's basically needlessly breaking any addons already using it for a very tiny naming improvemnt. I'd leave the existing API name as-is, and just add your new |notifyInvalidSubmit| alongside it.

Though I do think that it would be worthy to -- at some point -- just kill nsIFormSubmitObserver entirely and just use plain old nsIObservers (the arguments could be items in a nsIPropertyBag passed through the observer's |subject| arg).
Attachment #465280 - Flags: review?(dolske) → review-
Attachment #465295 - Attachment is patch: false
Attachment #465295 - Attachment mime type: text/plain → image/png
This patch create a new kind of event and blocks the form submission if there is at least one observer for this event.

There is no tests at the moment. They will follow.
Attachment #465280 - Attachment is obsolete: true
Attachment #465284 - Attachment is obsolete: true
Attachment #466221 - Flags: review?(Olli.Pettay)
Attachment #465284 - Flags: feedback?(dolske)
Attached patch Patch Part2 WIP (obsolete) — Splinter Review
Attachment #466222 - Flags: feedback?(dolske)
What still need to be improved with this popup:
- transparency: I can't set transparency. At least, with GTK, it behaves like the background is black..
- box-shadow: limi's mockup shows a box-shadow but it doesn't look to work on popup's.
- colors: maybe the colors could be improved?
- blocking some shortcuts: I use CTRL+ALT+<arrows> to change desktop and when there is a popup, this shortcut can't be used. That's not specific to this popup (happens with geolocation's too) but this one may appear quite often so this bug may be more annoying.
- showed between tabs: if the popup appears when you are in another tab (not so frequent) or if you change tabs when the popup is showed (may be frequent), the popup stays. I should see what geolocation does because the popup disappear when changing tabs and reappears after (we don't want to have this one reappearing actually).
Attached image UI Preview v2
Attachment #465295 - Attachment is obsolete: true
Comment on attachment 466221 [details] [diff] [review]
Patch Part1 v1 - Block invalid form submission if there is an observer


>+    // If the invalid form submission has been handled, we can block it.
>+    // Otherwise, we show a warning and we continue the submission.
>+    if (handled) {
>+      return NS_OK;
>+    }
Why this? Shouldn't we always block it if it is invalid?
Blocks: 587671
Now there is a check for observers before checking for form validity. If there is no observer, form validity isn't checked (so no 'invalid' events are sent). I've open a follow-up to remove this check when needed.
Attachment #466221 - Attachment is obsolete: true
Attachment #466318 - Flags: review?(Olli.Pettay)
Attachment #466221 - Flags: review?(Olli.Pettay)
Comment on attachment 466318 [details] [diff] [review]
Patch Part1 v2 - Block invalid form submission if there is an observer


>+  /**
>+   * Check for form validity: do not submit a form if there are unhandled
>+   * invalid controls in the form.
>+   *
>+   * NOTE: for the moment, we are also checking that there is an observer for
>+   * NS_INVALIDFORMSUBMIT_SUBJECT so it will prevent blocking form submission
>+   * if the browser does not have implemented a UI yet.
>+   *
>+   * TODO: the check for observer should be removed later when HTML5 Forms will
>+   * be spread enough and authors will assume forms can't be submitted when
>+   * invalid. See bug 587671.
>+   */
Make sure Fennec developers know about this.


>+  nsresult rv = service->EnumerateObservers(NS_INVALIDFORMSUBMIT_SUBJECT,
>+                                            getter_AddRefs(theEnum));
>+  NS_ENSURE_SUCCESS(rv, rv);
>+
>+  PRBool hasObserver = PR_FALSE;
>+  rv = theEnum->HasMoreElements(&hasObserver);
>+
>+  // Do not check form validity if there is no observer for
>+  // NS_INVALIDFORMSUBMIT_SUBJECT.
>+  if (NS_SUCCEEDED(rv) && hasObserver) {
>+    nsCOMPtr<nsIMutableArray> invalidElements =
>+      do_CreateInstance(NS_ARRAY_CONTRACTID, &rv);
>+    NS_ENSURE_SUCCESS(rv, rv);
>+
>+    if (!CheckFormValidity(invalidElements.get())) {
>+      nsCOMPtr<nsISupports> inst;
>+      nsCOMPtr<nsIFormSubmitObserver> observer;
>+      for (PRBool more = PR_TRUE;
>+           theEnum && NS_SUCCEEDED(theEnum->HasMoreElements(&more)) && more; ) {
Might look better if written using while.
And why the null check for theEnum?
Attachment #466318 - Flags: review?(Olli.Pettay) → review+
Vivien and Mark: a UI is going to be needed for the form validation on Fennec.
r=smaug
Attachment #466318 - Attachment is obsolete: true
Fixes since last WIP patch:
- the popup doesn't show if it's called by a tab in background ;
- the popup doesn't stay when changing tabs ;
- the popup hides when blurring the element or typing in it.

So, I think it's ready to be reviewed considering some polish will be done in follow ups. I'm already setting up a Mac environment to have transparency working (it looks like transparency on popup with GTK doesn't work).
Attachment #466222 - Attachment is obsolete: true
Attachment #466869 - Flags: review?(dolske)
Attachment #466222 - Flags: feedback?(dolske)
(In reply to comment #51)
> Vivien and Mark: a UI is going to be needed for the form validation on Fennec.

Does this patch will make it for the FF4.0 release or is it designed for later?
(In reply to comment #54)
> (In reply to comment #51)
> > Vivien and Mark: a UI is going to be needed for the form validation on Fennec.
> 
> Does this patch will make it for the FF4.0 release or is it designed for later?

We want this patch for ff4b5. So, we are going to do our best to have it landed in the next two weeks. Fennec will not be impacted (form submission will not be blocked if the form is invalid because there will be no observer) but I guess you will want to have a UI for Fennec 2.0.
Blocks: 589696, 556013
Comment on attachment 466869 [details] [diff] [review]
Patch Part2 v1 - Invalid form submission Firefox UI

A few issues from just reviewing code, might have some more after you update the patch and I actually play with it. (Do you have a demo page to try this out with?)

General question: if a site author wants to use HTML5 form validation, but does not want the browser's UI added by this patch (ie, they will roll their own), can they do that, and how?


>+++ b/browser/base/content/browser.js
...
>+  notifyInvalidSubmit : function (aFormElement, aInvalidElements)
>+  {
...
>+    if (!aInvalidElements.length) {
>+      return;
>+    }

Shouldn't this always have at least 1 value?

>+    // Don't show the popup if the current tab doesn't contain the invalid form.
>+    let found = false;
>+    for each(let f in gBrowser.selectedTab.linkedBrowser.contentDocument.forms) {
>+      if (f == aFormElement) {
>+        found = true;
>+        break;
>+      }
>+    }
>+    if (!found) {
>+      return;
>+    }

Iterating through the forms is inefficient, and seems like it could be slow in absurd cases.

Should be sufficient to check for:

  1) aFormElement.ownerDocument == gBrowser.selectedTab.linkedBrowser.contentDocument
  2) aInvalidElements[0].form == aFormElement

That should ensure the form is in the currently active tab, and the invalid element is in that form. If the form/element is removed from the document, or the element moved to a different form, we'll just do nothing (because that's weird and we don't know what's going on).

>+    if (element instanceof HTMLInputElement ||
>+        element instanceof HTMLTextAreaElement ||
>+        element instanceof HTMLSelectElement ||
>+        element instanceof HTMLButtonElement) {
>+      panel.firstChild.nodeValue = element.validationMessage;
>+    }

Why the element-instanceof checks? Seems like the observer should only be handing us an array of expected invalid elements? Oh, I bet this is just so they don't taste like generic nsISupports objects. Would a QI to nsIDOMHTMLInputElement work instead? Bah, looks like at least textareas don't implement that.

In any case, if we can't find a useful interface from what's given, this should just explicitly return and do nothing.

This also needs to handle the case of the validation message being empty.

And we probably want a (brief) blurb of (localized) text setting context for the content's string. UX probably has an opinion here, but I'm thinking, very roughly:

  ---/\-----------------------------
 | This form value needs corrected. |
  ----------------------------------


  ---/\--------------------------------------
 | This form value needs corrected. The page |
 | says:                                     |
 |                                           |
 | We can't ship to zipcodes that are in     |
 | another galaxy.                           |
  -------------------------------------------

[Needs some wordsmithing, I imagine existing site forms would serve as examples to follow. Shouldn't be too harsh ("UNACCEPTABLE") or jargony.]

Finally, there should also be appropriate controls to make abusive content not be able to cause enormous panels (in width or height), eg by setting the validation message to a huge value. See bug 244273 for related issues that were fixed for the HTTP Auth prompt.

>+    panel.openPopup(element, "after_start", 0, 0);

Shouldn't there be some code here to start scrolling the page if the element is out of view?

>   onLocationChange: function (aWebProgress, aRequest, aLocationURI) {
>     var location = aLocationURI ? aLocationURI.spec : "";
>     this._hostChanged = true;
> 
>+    // Hide the form invalid popup.
>+    document.getElementById('invalid-form-popup').hidePopup();

Hmm, I think you want to avoid doing all this work for every location change (especially since most won't need to actually do anything!).

Instead, something like:
  let p = gFormSubmitObserver.fooPanel; // use better names :)
  if (!p || (p.state != "hiding" && p.state != "closed"))
    p.hidePopup()

With some code to set p to document.getElementById('invalid-form-popup') the first time the popup is shown.


>+++ b/browser/base/content/browser.xul
...
>+    <!-- for invalid form error message -->
>+    <panel id="invalid-form-popup" noautofocus="true" hidden="true">foo</panel>

No "foo". Other changes above will require more changes in the panel anyway.

Also, add level="parent" (eh, if a form gets autosubmitted while the browser is behind another app's window, the popup shouldn't be shown on top of the other app). See https://developer.mozilla.org/en/XUL/panel#Attributes

>+++ b/browser/themes/gnomestripe/browser/browser.css
...
>+/* Invalid form popup */
>+#invalid-form-popup {
>+  -moz-appearance: none;
>+  background-color: #fffcd6;
>+  border: 1px solid #dad8b6;
>+  padding: 5px 5px 5px 5px;
>+  max-width: 280px;
>+  font-weight: bold;
>+}

How were these colors chosen? max-width should probably be in browser/base/content/browser.css, so that 3rd party themes don't need to worry about it for abusive content.


Finally, seems like it would be worthwhile to have a simple test here, which at least checks to see that the panel gets shown at about the right place (relative to the element on the page), and with the right content.
Attachment #466869 - Flags: review?(dolske) → review-
(In reply to comment #56)
> Comment on attachment 466869 [details] [diff] [review]
> Patch Part2 v1 - Invalid form submission Firefox UI
> 
> A few issues from just reviewing code, might have some more after you update
> the patch and I actually play with it. (Do you have a demo page to try this out
> with?)

This patch is the last patch of a long patch queue. I will try to have a build tomorrow so you can test it.

> General question: if a site author wants to use HTML5 form validation, but does
> not want the browser's UI added by this patch (ie, they will roll their own),
> can they do that, and how?

Yes, the simple way would be to set the "novalidate" attribute on the form element. This should prevent the UA to do any form of form validation when submitting. It will not remove the other features. There is also a "formnovalidate" attribute that can be set on the submit button. These two attributes are beta6 blocker.

> >+++ b/browser/base/content/browser.js
> ...
> >+  notifyInvalidSubmit : function (aFormElement, aInvalidElements)
> >+  {
> ...
> >+    if (!aInvalidElements.length) {
> >+      return;
> >+    }
> 
> Shouldn't this always have at least 1 value?

No. This is the list of "unhandled invalid elements". That means invalid elements which didn't canceled the invalid event. If all elements cancel this events, the form submission will be canceled but there would be no unhandled invalid elements. I guess it might be still a good idea to inform the observers in case of they want to do something even if no element is in the list.

> >+    // Don't show the popup if the current tab doesn't contain the invalid form.
> >+    let found = false;
> >+    for each(let f in gBrowser.selectedTab.linkedBrowser.contentDocument.forms) {
> >+      if (f == aFormElement) {
> >+        found = true;
> >+        break;
> >+      }
> >+    }
> >+    if (!found) {
> >+      return;
> >+    }
> 
> Iterating through the forms is inefficient, and seems like it could be slow in
> absurd cases.
> 
> Should be sufficient to check for:
> 
>   1) aFormElement.ownerDocument ==
> gBrowser.selectedTab.linkedBrowser.contentDocument
>   2) aInvalidElements[0].form == aFormElement
> 
> That should ensure the form is in the currently active tab, and the invalid
> element is in that form. If the form/element is removed from the document, or
> the element moved to a different form, we'll just do nothing (because that's
> weird and we don't know what's going on).

Thanks :) Actually, I knew this was completely stupid but I didn't know how to do that and #developers didn't really helped...

> >+    if (element instanceof HTMLInputElement ||
> >+        element instanceof HTMLTextAreaElement ||
> >+        element instanceof HTMLSelectElement ||
> >+        element instanceof HTMLButtonElement) {
> >+      panel.firstChild.nodeValue = element.validationMessage;
> >+    }
> 
> Why the element-instanceof checks? Seems like the observer should only be
> handing us an array of expected invalid elements? Oh, I bet this is just so
> they don't taste like generic nsISupports objects. Would a QI to
> nsIDOMHTMLInputElement work instead? Bah, looks like at least textareas don't
> implement that.
> 
> In any case, if we can't find a useful interface from what's given, this should
> just explicitly return and do nothing.

I think we can't do that another way. There is nsIConstraintValidation but it has no IDL.

> This also needs to handle the case of the validation message being empty.

This can't happen. validationMessage will always provide a message if the element is invalid, per HTML specifications.

> And we probably want a (brief) blurb of (localized) text setting context for
> the content's string. UX probably has an opinion here, but I'm thinking, very
> roughly:
> 
>   ---/\-----------------------------
>  | This form value needs corrected. |
>   ----------------------------------
> 
> 
>   ---/\--------------------------------------
>  | This form value needs corrected. The page |
>  | says:                                     |
>  |                                           |
>  | We can't ship to zipcodes that are in     |
>  | another galaxy.                           |
>   -------------------------------------------
> 
> [Needs some wordsmithing, I imagine existing site forms would serve as examples
> to follow. Shouldn't be too harsh ("UNACCEPTABLE") or jargony.]

AFAIU limi's mockup, he wants only the validation message with no context. The author can set a custom error and, in that case, the validation message will be custom. In all other cases (most common), the validation messages are set by the UA. The english ones are here: dom/locales/en-US/chrome/dom/dom.properties (they may need some tweaking)
If limi want a sort of introduction message before the validation message, it's fine but I guess these messages can be set without it.

> Finally, there should also be appropriate controls to make abusive content not
> be able to cause enormous panels (in width or height), eg by setting the
> validation message to a huge value. See bug 244273 for related issues that were
> fixed for the HTTP Auth prompt.

Indeed! I guess I have to found a max string length. The one used for bug 244273 sounds a bit small (150). I will see.

> >+    panel.openPopup(element, "after_start", 0, 0);
> 
> Shouldn't there be some code here to start scrolling the page if the element is
> out of view?

element.focus() should make sure the element is in the "viewport" (or whatever it is called in Firefox).

> >   onLocationChange: function (aWebProgress, aRequest, aLocationURI) {
> >     var location = aLocationURI ? aLocationURI.spec : "";
> >     this._hostChanged = true;
> > 
> >+    // Hide the form invalid popup.
> >+    document.getElementById('invalid-form-popup').hidePopup();
> 
> Hmm, I think you want to avoid doing all this work for every location change
> (especially since most won't need to actually do anything!).
> 
> Instead, something like:
>   let p = gFormSubmitObserver.fooPanel; // use better names :)
>   if (!p || (p.state != "hiding" && p.state != "closed"))
>     p.hidePopup()
> 
> With some code to set p to document.getElementById('invalid-form-popup') the
> first time the popup is shown.

Ok.

> >+++ b/browser/base/content/browser.xul
> ...
> >+    <!-- for invalid form error message -->
> >+    <panel id="invalid-form-popup" noautofocus="true" hidden="true">foo</panel>
> 
> No "foo". Other changes above will require more changes in the panel anyway.

Actually, I set foo to have a text node. Actually, I could probably initialize it somewhere.

> Also, add level="parent" (eh, if a form gets autosubmitted while the browser is
> behind another app's window, the popup shouldn't be shown on top of the other
> app). See https://developer.mozilla.org/en/XUL/panel#Attributes

Ok.

> >+++ b/browser/themes/gnomestripe/browser/browser.css
> ...
> >+/* Invalid form popup */
> >+#invalid-form-popup {
> >+  -moz-appearance: none;
> >+  background-color: #fffcd6;
> >+  border: 1px solid #dad8b6;
> >+  padding: 5px 5px 5px 5px;
> >+  max-width: 280px;
> >+  font-weight: bold;
> >+}
> 
> How were these colors chosen?

With limi's mockup.

> max-width should probably be in
> browser/base/content/browser.css, so that 3rd party themes don't need to worry
> about it for abusive content.

Ok.

> Finally, seems like it would be worthwhile to have a simple test here, which at
> least checks to see that the panel gets shown at about the right place
> (relative to the element on the page), and with the right content.

They are a lot of stuff that could be tested here actually. I guess a browser-chrome mochitest should be fine or you were expecting a reftest for the panel rendering?
--> beta6, as it's clearly not going to make beta5. Do we think we need to block on this?
blocking2.0: beta5+ → beta6+
Attachment #470633 - Flags: review?(Olli.Pettay)
Olli, could you review the changes? There are two majors changes:
- do not check for form validation if the form submission has been called with .submit();
- do not send the submit event if the form isn't valid. The way it's done is quite ugly but that's because of the way the submit event is sent. I hope I will be able to fix that after Firefox 4 (see bug 592124).

Let me know if you want me to attach a diff of the two patches.
Attachment #466866 - Attachment is obsolete: true
Attachment #470637 - Flags: review?(Olli.Pettay)
Attachment #470637 - Attachment description: Patch Part1 v2.1 - Block invalid form submission if there is an observer → Patch Part 2 - Block invalid form submission if there is an observer
Attachment #470637 - Attachment description: Patch Part 2 - Block invalid form submission if there is an observer → Part 2 - Block invalid form submission if there is an observer
Attachment #470633 - Attachment is obsolete: true
Attachment #470984 - Flags: review?(Olli.Pettay)
Attachment #470633 - Flags: review?(Olli.Pettay)
Attachment #470637 - Attachment is obsolete: true
Attachment #470985 - Flags: review?(Olli.Pettay)
Attachment #470637 - Flags: review?(Olli.Pettay)
Attached patch Part 3 - Tests for browser (obsolete) — Splinter Review
Attachment #471350 - Flags: review?(dolske)
This is fixing the review comments.
Attachment #466869 - Attachment is obsolete: true
Attachment #471351 - Flags: review?(dolske)
Attachment #471350 - Attachment is obsolete: true
Attachment #471353 - Flags: review?(dolske)
Attachment #471350 - Flags: review?(dolske)
Builds with the patches included:
http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/mlamouri@mozilla.com-6d77d5ea2af5/

Limi, let me know if there is anything that should be changed.
Comment on attachment 470984 [details] [diff] [review]
Part 1 - Test for form submission

+
>+// The following test should not be done if there is no observer for
>+// "invalidformsubmit" because the form submission will not be canceled in that
>+// case.
>+if (observers.hasMoreElements()) {
If there aren't any observers, this test fails, right?
Add some dummy todo() or ok() to 'else'
Attachment #470984 - Flags: review?(Olli.Pettay) → review+
Comment on attachment 470985 [details] [diff] [review]
Part 2 - Block invalid form submission if there is an observer


>@@ -498,17 +498,19 @@ nsHTMLButtonElement::PostHandleEvent(nsE
>         nsCOMPtr<nsIPresShell> presShell =
>           aVisitor.mPresContext->GetPresShell();
>         // If |nsIPresShell::Destroy| has been called due to
>         // handling the event, the pres context will return
>         // a null pres shell.  See bug 125624.
>         //
>         // Using presShell to dispatch the event. It makes sure that
>         // event is not handled if the window is being destroyed.
>-        if (presShell) {
>+        if (presShell && mForm->CheckValidFormSubmission()) {
Do we really need to validate even if the event is reset?


>@@ -2385,17 +2387,19 @@ nsHTMLInputElement::PostHandleEvent(nsEv
>             nsEventStatus status  = nsEventStatus_eIgnore;
> 
>             nsCOMPtr<nsIPresShell> presShell =
>               aVisitor.mPresContext->GetPresShell();
> 
>             // If |nsIPresShell::Destroy| has been called due to
>             // handling the event the pres context will return a null
>             // pres shell.  See bug 125624.
>-            if (presShell) {
>+            // TODO: removing this code and have the submit event sent by the
>+            // form, see bug 592124.
>+            if (presShell && mForm->CheckValidFormSubmission()) {
Same here

And where do we call the validation when formelement.submit() is called?
Attachment #470985 - Flags: review?(Olli.Pettay) → review-
(In reply to comment #68)
> And where do we call the validation when formelement.submit() is called?
Or does HTML5 not require that?
(In reply to comment #67)
> >+// The following test should not be done if there is no observer for
> >+// "invalidformsubmit" because the form submission will not be canceled in that
> >+// case.
> >+if (observers.hasMoreElements()) {
> If there aren't any observers, this test fails, right?
> Add some dummy todo() or ok() to 'else'

If there is no observers, this test doesn't test anything. At least, when running alone, this produces a todo, not a failure.

(In reply to comment #69)
> (In reply to comment #68)
> > And where do we call the validation when formelement.submit() is called?
> Or does HTML5 not require that?

Exactly. When .submit() is called, the form validity shouldn't be checked.

I will fix the the other points.
Olli, This should be fixing the issues you point out.
Attachment #470985 - Attachment is obsolete: true
Attachment #472775 - Flags: review?(Olli.Pettay)
Attachment #472775 - Flags: review?(Olli.Pettay) → review+
(In reply to comment #70) 
> If there is no observers, this test doesn't test anything. At least, when
> running alone, this produces a todo, not a failure.
OK. Perhaps things have changed then.
At some point tests without any checks caused failure, IIRC.
(In reply to comment #57)

> > (Do you have a demo page to try this out with?)
> 
> This patch is the last patch of a long patch queue. I will try to have a build
> tomorrow so you can test it.

Is this still the case? I'd like to try it as part of the final review, if it's not easy to build this yet it would be nice to have a tryserver build. Or I could just walk over and try your build. :)
(In reply to comment #73)
> Is this still the case? I'd like to try it as part of the final review, if it's
> not easy to build this yet it would be nice to have a tryserver build. Or I
> could just walk over and try your build. :)

just me talkin', but that last thing sounds like a swell idea :)
Whiteboard: [has patch][needs review dolske]
This try to be similar to limi's UI preview and can be used as a test case.
Small fixes in the browser tests.
Comment on attachment 471351 [details] [diff] [review]
Part 4 - Invalid form submission Firefox UI

>+  init: function()
>+  {
>+    this.panel = document.getElementById('invalid-form-popup');
>+    this.panel.appendChild(document.createTextNode(""));
>+  },

You could just use a <description> and set it's .value...

>   onLocationChange: function (aWebProgress, aRequest, aLocationURI) {
...
>+    // Hide the form invalid popup.
>+    let invalidFormPanel = gFormSubmitObserver.panel;
>+    if (invalidFormPanel &&
>+        (invalidFormPanel.state != "hiding" &&
>+         invalidFormPanel.state != "closed")) {
>+      invalidFormPanel.hidePopup();
>+    }

This is a somewhat warm path, so it would be nice to minimize work as much as possible (for the common case) so that this could just be:

  if (gFormSubmitObserver.panelIsOpen)
    gFormSubmitObserver.panel.hidePopup();

But what you have doesn't seem like it's significantly more expensive, so doesn't really matter.


>+++ b/browser/base/content/browser.xul
>@@ -162,16 +162,19 @@
>     <tooltip id="aHTMLTooltip" onpopupshowing="return FillInHTMLTooltip(document.tooltipNode);"/>
> 
>     <!-- for search and content formfill/pw manager -->
>     <panel type="autocomplete" id="PopupAutoComplete" noautofocus="true" hidden="true"/>
> 
>     <!-- for url bar autocomplete -->
>     <panel type="autocomplete-richlistbox" id="PopupAutoCompleteRichResult" noautofocus="true" hidden="true"/>
> 
>+    <!-- for invalid form error message -->
>+    <panel id="invalid-form-popup" noautofocus="true" hidden="true" level="parent"></panel>

Nit: Follow surrounding style, use <panel .../> instead of a separate close tag.

Also, can you file a followup bug so we remember to update this panel once arrow panels (bug 554937) land?

>+++ b/browser/themes/*/browser/browser.css
...
>+#invalid-form-popup {

Dunno if UX will want to do some further style tweaking before ship, but this should be fine for landing.
Attachment #471351 - Flags: review?(dolske) → review+
Comment on attachment 471353 [details] [diff] [review]
Part 3 - Tests for browser

>+++ b/browser/base/content/test/browser_bug561636.js
...
>+function test()
>+{
>+  waitForExplicitFinish();
>+
>+  test10();
>+}

Uhh. There's no test10() in this patch. Are you sure these tests were actually running? :)


>+  gObserver.notifyInvalidSubmit = function() {
>+    executeSoon(function() {
...
>+      checkPopupShow();

Hmm, I'm not sure if executeSoon() will always be sufficient for ensuring that the popup is open by the time it runs. Driving this by via a popupshown even would be safer and help prevent (potential) intermittent orange.

(This is fine for when you're expecting the popup not to be shown, since there obviously isn't a popupnotshown event to listen for. :-)
Attachment #471353 - Flags: review?(dolske) → review+
(In reply to comment #78)

> >+  test10();
> 
> Uhh. There's no test10() in this patch.

Oh, I see you attached a fixed patch but I reviewed the other one. Was that the only change? Bugzilla's intradiff says _nothing_ changed, which is lies lies lies.
(In reply to comment #79)
> (In reply to comment #78)
> 
> > >+  test10();
> > 
> > Uhh. There's no test10() in this patch.
> 
> Oh, I see you attached a fixed patch but I reviewed the other one. Was that the
> only change? Bugzilla's intradiff says _nothing_ changed, which is lies lies
> lies.

I fixed the max size message test. String(300, 'a') isn't actually a String constructor and spaces were needed for wrapping.
Blocks: 595432
No longer depends on: 554937
Blocks: 595451
Pushed:
http://hg.mozilla.org/mozilla-central/rev/92ea14236f46
http://hg.mozilla.org/mozilla-central/rev/c8685734392d
http://hg.mozilla.org/mozilla-central/rev/8e70f3fc0d49
http://hg.mozilla.org/mozilla-central/rev/e3f194205a3b

Yeah, we have a real form validation support \o/
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Firefox 4.0b6
Depends on: 595507
Depends on: 596273
Keywords: dev-doc-needed
Whiteboard: [has patch][needs review dolske]
So what happens if web page calls stopPropagation() on capture phase
in some ancestor element or document or window when handling input or blur?
http://hg.mozilla.org/mozilla-central/annotate/b439e73f33b7/browser/base/content/browser.js
I think the event listener should be added to chrome and to capture phase,
or to system event group, in which case the phase doesn't really matter in this case.
Comment on attachment 471351 [details] [diff] [review]
Part 4 - Invalid form submission Firefox UI

>diff --git a/browser/base/content/browser.js b/browser/base/content/browser.js

>+  notifyInvalidSubmit : function (aFormElement, aInvalidElements)

>+    // Don't show the popup if the current tab doesn't contain the invalid form.
>+    if (gBrowser.selectedTab.linkedBrowser.contentDocument !=
>+        aFormElement.ownerDocument) {
>+      return;
>+    }

This check will fail for forms in subframes. Is that expected?

(Also this could just use gBrowser.contentDocument)
Mounir pointed out on IRC that bug 595507 was already filed. Do we need a followup for comment 82, though?
(In reply to comment #82)
> So what happens if web page calls stopPropagation() on capture phase
> in some ancestor element or document or window when handling input or blur?
> http://hg.mozilla.org/mozilla-central/annotate/b439e73f33b7/browser/base/content/browser.js
> I think the event listener should be added to chrome and to capture phase,
> or to system event group, in which case the phase doesn't really matter in this
> case.

Olli, could you submit a bug?
Depends on: 599660
Blocks: 599745
Depends on: 600154
Depends on: 606663
Depends on: 606817
Looking at the HTML preview, looks like the input element's title attribute is used as the text for the tooltip (with the addition of a browser-generated error message at the beginning). Correct?
(In reply to comment #86)
> Looking at the HTML preview, looks like the input element's title attribute is
> used as the text for the tooltip (with the addition of a browser-generated
> error message at the beginning). Correct?

Actually not. That's only the case for <input pattern='something' title='some text'>. Otherwise, we only show the error message in the tooltip when an element is invalid. We are thinking of only doing that if the title attribute isn't set (see bug 605277).
Depends on: 638826
Blocks: 779720
No longer blocks: 779720
Depends on: 1035116
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: