Closed Bug 827161 Opened 11 years ago Closed 10 years ago

Implement ValidityState.badInput

Categories

(Core :: DOM: Core & HTML, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla29

People

(Reporter: ehsan.akhgari, Assigned: jwatt)

References

Details

(Keywords: dev-doc-needed)

Attachments

(2 files, 5 obsolete files)

      No description provided.
I would recommend to wait until we need it before implementing it.
OS: Mac OS X → All
Hardware: x86 → All
I need this to fix bug 949117 and its dupes.
Assignee: nobody → jwatt
Blocks: 949117
Attached patch patch (obsolete) — Splinter Review
Attachment #8367896 - Flags: review?(bugs)
Attachment #8367967 - Flags: review?(bugs) → review+
Attached patch patch to add support for email (obsolete) — Splinter Review
Attachment #8368746 - Flags: review?(bugs)
Could you explain why SetValueInternal does different thing for email and number.
Right now for the IsSingleLineTextControl() path we do not call UpdateAllValidityStates(), but we do on the |else| path via OnValueChanged. The addition of UpdateAllValidityStates() for the NS_FORM_INPUT_EMAIL case is so that we'll update the .badInput state and notify (so that its appearance will change from invalid<->valid as appropriate.

The change to avoid calling SanitizeValue() in the case of NS_FORM_INPUT_NUMBER is because I need to keep the unsanitized value around so I can get it via GetValueInternal. As the comment states content is still protected from seeing an unsanitized value because GetValue will now sanitize the value before returning it.
Okay, this gets rid of the apparent inconsistency and uses the value stored in the frame to determine whether the element is suffering from "bad input". If the element's value is not the empty string then we know it isn't, since otherwise it would have been sanitized to the empty string. If it's the empty string then we check whether the frame's text field is also the empty string. If it is not, then it is suffering from bad input. Maybe this approach is more palatable?
Attachment #8367968 - Attachment is obsolete: true
Attachment #8368746 - Attachment is obsolete: true
Attachment #8367968 - Flags: review?(bugs)
Attachment #8368746 - Flags: review?(bugs)
Attachment #8369100 - Flags: review?(bugs)
More tests.
Attachment #8369100 - Attachment is obsolete: true
Attachment #8369100 - Flags: review?(bugs)
Attachment #8369127 - Flags: review?(bugs)
More hg add
Attachment #8369127 - Attachment is obsolete: true
Attachment #8369127 - Flags: review?(bugs)
Attachment #8369128 - Flags: review?(bugs)
Comment on attachment 8369128 [details] [diff] [review]
part 2 - Implement HTML 5's ValidityState.badInput for HTMLInputElement

>       if (IsSingleLineTextControl(false)) {
>         mInputData.mState->SetValue(value, aUserInput, aSetValueChanged);
>+        if (mType == NS_FORM_INPUT_EMAIL) {
>+          UpdateAllValidityStates(true);
>+        }
s/true/!mParserCreating/


>+  if (mType == NS_FORM_INPUT_EMAIL) {
>+    // Test if the value can be converted to punycode or not:
>+    nsAutoString value;
>+    nsAutoCString unused;
>+    uint32_t unused2;
>+    NS_ENSURE_SUCCESS(GetValueInternal(value), false);
>+    HTMLSplitOnSpacesTokenizer tokenizer(value, ',');
>+    while (tokenizer.hasMoreTokens()) {
>+      if (!PunycodeEncodeEmailAddress(tokenizer.nextToken(), unused, &unused2)) {
>+        return true;
>+      }
>+    }
Could you please add some comment here why PunycodeEncodeEmailAddress is used and not IsValidEmail.
(The spec is a bit odd here, IMO)
Attachment #8369128 - Flags: review?(bugs) → review+
Thank you, Olli!

https://hg.mozilla.org/mozilla-central/rev/771ed4be9dee
https://hg.mozilla.org/mozilla-central/rev/e67fbfeab5d5
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
Is there any chance we could backport this into earlier releases supporting HTML5 constraint validation? We depend on this in AngularJS a tiny bit, and you see weird behaviour in versions of FF prior to FF30, due to the value being set to null, but badInput not being set in the ValidityState.
Yes. And we won't be backporting this to v28, sorry. (Way too late.)
I was seeing weird behaviour with an FF29 Nightly, but it's possible that it was before the CL landed. Even so, as has been stated, you see very wrong behaviour with versions supporting HTML constraint validation, but without support for badInput. So I'm worried about there being weird behaviour once ESR releases start to catch up. That's going to cause problems which we can't do much about.
(In reply to Caitlin Potter (:caitp) from comment #17)
> I was seeing weird behaviour with an FF29 Nightly, but it's possible that it
> was before the CL landed. Even so, as has been stated, you see very wrong
> behaviour with versions supporting HTML constraint validation, but without
> support for badInput.

Can you be more specific? That is, please give a concrete example. Of the input types that we support the badInput state only really makes a difference to <input type=number>, and that will only ship with v29 at the earliest.
input[type=number] is, as far as I know, the only one that suffers from this at the moment, you're correct about that.

So, in Angular we have our own form validation stuff, which sort of acts as a polyfill (in a sense) for HTML5 constraint validation. With inputs "suffering from bad input", the original polyfilled validation logic doesn't really work (because the input value is reported as null or the empty string, which frankly was a terrible idea and should have never made it into any draft, but people seem to disagree with that opinion).

Relatively recently, we've started using the ValidityState to be able to differentiate between a "valid" null/empty string, and an invalid null/empty string (such as you would get when typing alphabet characters into input[type=number]). This works beautifully in browsers like Chromium which have handled the "inputs suffering from bad input" clauses of the spec for a few releases now. But it causes issues with Firefox where part of this is implemented, but not the very important "notify script contexts of an issue via the ValidityState object" bit.

So what this means is that number validation (stemming from user input) is broken in Firefox, for a stretch of releases up until FF29. People are using number inputs for things like zip codes (although, admittedly I'm not sure this is a good use case for a number input), so it's not really an uncommon thing for invalid inputs to be accidentally considered valid (at worst), or for people to display an incorrect validation message (at best).

This all kind of sucks, and it means that people using ESR releases of FF will probably get a pretty bad experience, at least for a while.
Depends on: 977310
(In reply to Caitlin Potter (:caitp) from comment #19)
> But it causes issues with Firefox where part of this is
> implemented, but not the very important "notify script contexts of an issue
> via the ValidityState object" bit.

To be clear you're talking about pre-v29, right?

> So what this means is that number validation (stemming from user input) is
> broken in Firefox, for a stretch of releases up until FF29.

That seems to be an artifact of your use of the ValidityState object/events to determine the validity. For versions that don't support number controls the control is treated as a text control, so you _will_ be able to see the "invalid" value using element.value to examine it. You'll need to do your own validation on that.

> People are using
> number inputs for things like zip codes (although, admittedly I'm not sure
> this is a good use case for a number input),

(It's not, and the spec specifically calls zip codes out as an inappropriate use.)

> This all kind of sucks, and it means that people using ESR releases of FF
> will probably get a pretty bad experience, at least for a while.

The validation code for number inputs is very much dependent on the number control implementation in general. For example, the step and overflow code is needed to validate against those steps. So I don't think there's much we can do here. Essentially we'd need to backport the number control implementation, and that isn't going to happen I'm afraid.
Depends on: 1064430
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: