598092 - Autocomplete should not remember form values if the submit is stopped due to HTML5 validation

Status: RESOLVED FIXED

(Toolkit :: Form Manager, defect)

Description

[Nochum Sossonko [:Natch]]

As stated in the summary, currently if one enters an incorrect value for the input element, the submit is blocked per HTML5 Forms, but the entry is recorded by the Form Manager. This shouldn't happen.

Comment 1

[Justin Dolske [:Dolske]]

Hmm, tough call on blocking. I'm going to minus this, but it's definitely wanted.

Form Manager already has a bug (filed, I think) where if a page cancels the form submission, we will already have saved the form's data. So I think this is an existing problem, though HTML5 form validation will certainly make it more visible as it enters use.

I suspect the fix here shouldn't be too complex -- probably need to add a new |afterformsubmit| event, and save the form data there.

There's a chance this will cause some odd results -- the |earlyformsubmit| observer (which gets called before the normal |formsubmit| listeners) was added a long time ago because some sites were modifying form data during the submission, so the saved data != the entered data. I don't know if this is still an real problem on the web... If it is, we could probably make the |earlyformsubmit| observer just queue up the data -- as formSubmitListener.js already does due to E10S -- and only commit it to the DB upon |afterformsubmit|.

Comment 2

[Mounir Lamouri (:mounir)]

I wrote a patch moving the CheckValidity() method _before_ the form submission stuff. As a side effect, this bug should be fixed.

Can you confirm that?

Comment 3

[Mounir Lamouri (:mounir)]

When we try to submit an invalid form, nsHTMLFormElement::OnSubmitClickBegin is called which is calling nsIFormSubmitObserver::notify, making the form history to save the values.

Comment 4

[Mounir Lamouri (:mounir)]

Since we reached interface freeze, I'm wondering how far we can go here.

There would be a simple and dirty fix which would consist of checking that mInvalidElementsCount is null before calling NotifySubmitObservers in nsHTMLFormElement::OnSubmitClickBegin.

Other solutions would probably require interface changes or adding a new interface but calling methods from the new and the old one.

If we can't change the interfaces, I would prefer the dirty hack instead of adding a new interface and changing the interfaces after branching.

Dolske, what would you recommend?

Comment 5

[Mounir Lamouri (:mounir)]

Attachment: Patch v1

This is a fix that should be removed with bug 610402 when we will be authorized tho change interfaces.

Comment 6

[Mounir Lamouri (:mounir)]

Comment on attachment 488895 [details] [diff] [review]
Patch v1

I actually need a review from a content peer here.
Dolske, I still want to know if the way I'm solving this seems fine or if you would prefer the other way (which doesn't seem doable with the interface freeze).

Comment 7

[Benjamin Smedberg]

Comment on attachment 488895 [details] [diff] [review]
Patch v1

Please do not request approval until all reviews/feedback are complete.

Comment 8

[Mounir Lamouri (:mounir)]

Pushed:
http://hg.mozilla.org/mozilla-central/rev/60f75da9765b