Closed Bug 598092 Opened 9 years ago Closed 9 years ago

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


(Toolkit :: Form Manager, defect)

Not set



Tracking Status
blocking2.0 --- -


(Reporter: Natch, Assigned: mounir)




(Keywords: ux-consistency, ux-error-prevention)


(1 file)

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.
blocking2.0: --- → ?
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|.
blocking2.0: ? → -
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?
Assignee: nobody → mounir.lamouri
Whiteboard: [mounir-g2.0]
Whiteboard: [mounir-g2.0]
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.
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?
Attached patch Patch v1Splinter Review
This is a fix that should be removed with bug 610402 when we will be authorized tho change interfaces.
Attachment #488895 - Flags: review?(dolske)
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).
Attachment #488895 - Flags: review?(jonas)
Attachment #488895 - Flags: review?(dolske)
Attachment #488895 - Flags: feedback?(dolske)
Attachment #488895 - Flags: approval2.0?
Comment on attachment 488895 [details] [diff] [review]
Patch v1

Please do not request approval until all reviews/feedback are complete.
Attachment #488895 - Flags: approval2.0?
Whiteboard: [needs-review]
Attachment #488895 - Flags: feedback?(dolske)
Whiteboard: [needs-review] → [needs-landing]
Closed: 9 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [needs-landing]
Target Milestone: --- → mozilla2.0b9
You need to log in before you can comment on or make changes to this bug.