minlength and tooShort (form constraint validation API)

RESOLVED FIXED in Firefox 51

Status

()

Core
DOM: Core & HTML
RESOLVED FIXED
4 years ago
10 months ago

People

(Reporter: Alexander Farkas, Assigned: Thomas Wisniewski)

Tracking

(Blocks: 1 bug, {html5})

unspecified
mozilla51
html5
Points:
---

Firefox Tracking Flags

(firefox51 fixed)

Details

(Whiteboard: [parity-chrome][parity-blink])

Attachments

(1 attachment, 5 obsolete attachments)

(Reporter)

Description

4 years ago
User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:25.0) Gecko/20100101 Firefox/25.0 (Beta/Release)
Build ID: 20131025150754

Steps to reproduce:

HTML (5.1) now specifies a minlength Attribute and validityState.tooShort property. Would be nice, if this is implemented:
http://www.whatwg.org/specs/web-apps/current-work/multipage/association-of-controls-and-forms.html#attr-fe-minlength
http://www.whatwg.org/specs/web-apps/current-work/multipage/association-of-controls-and-forms.html#suffering-from-being-too-short
Assignee: nobody → MattN+bmo
Blocks: 344614
Status: UNCONFIRMED → ASSIGNED
Component: Untriaged → DOM: Core & HTML
Ever confirmed: true
OS: Mac OS X → All
Product: Firefox → Core
Hardware: x86 → All
Version: 25 Branch → unspecified
Created attachment 8411408 [details] [diff] [review]
WIP 1 - Implementation

This seems like a reasonable request to me (as someone not on the DOM team) and I ran into the want for this on username and password fields on the weekend so I decided to take a stab at it. The WhatWG bug also indicates there is demand for this and how it's better than just using @pattern.

This seems to work but I'm having to fix the W3C web platform tests on this feature to work on Gecko[1].

Shall I email an intent to implement email at this point or should I gather consensus from peers first?

See also:
* Spec addition: http://html5.org/tools/web-apps-tracker?from=8205&to=8206
* WhatWG bug: https://www.w3.org/Bugs/Public/show_bug.cgi?id=10053

[1] https://github.com/w3c/web-platform-tests/issues/923
Flags: needinfo?(overholt)
Flags: needinfo?(annevk)

Comment 2

3 years ago
Feel free to email intent to implement. That's the best way to get feedback from peers.
Flags: needinfo?(overholt)
Flags: needinfo?(annevk)
Did you email the intent to implement? I can't it on dev-platform...
Flags: needinfo?(MattN+bmo)
Comment hidden (advocacy)

Comment 5

a year ago
PS: minlength & maxlength SHOULD work together so as to form a character length range (as per spec)!

See the "Name of Event" example here: https://html.spec.whatwg.org/multipage/forms.html#attr-fe-minlength

Updated

a year ago
Keywords: html5
Whiteboard: [parity-chrome][parity-blink]
(Assignee)

Comment 6

11 months ago
Matt, given that you haven't worked on this in a couple of years, I'm guessing that you wouldn't mind if I complete your patch? 

This feature is in the WhatWG spec, we already support maxLength, *and* Chrome supports both min and maxLength, so I doubt it would be controversial, but I'll definitely send an intent to implement and ship just in case (assuming you don't want to tackle this one yourself anytime soon).
(Assignee)

Comment 7

11 months ago
Created attachment 8776240 [details] [diff] [review]
932755-add_support_for_input_and_textarea_minLength_and_tooShort.diff

In the interest of getting this done, here's a rebased version of the work-in-progress patch that also accounts for the note in comment 5 (ie, trying to a set maxLength less than the current minLength throws, and vice-versa).

This patch needs to be applied on top on the patch I'm writing in bug 613019.

It should be ready for review once some mochitests are written for it.

Sorry if I'm stepping on your toes, :MattN!
Attachment #8411408 - Attachment is obsolete: true
(Assignee)

Comment 8

11 months ago
Created attachment 8780789 [details] [diff] [review]
932755-add_support_for_input_and_textarea_minLength_and_tooShort.diff

It doesn't seem like :MattN has the time to deal with this right now, so I've gone ahead and finished the patch by adding minlength tests where maxlength tests are in the tree. This makes for a large file, but the non-test stuff is still the same as it was in the patch I'm obsoleting (if that helps speed up review).

Some potentially useful notes about the test changes:
- I updated the maxlength tests to account for the fact that user-interaction is now needed in the spec before the state can ever become invalid. This required moving the reftests into test_reftests_with_caret.html.
- I also changed the "todo" :-moz-ui-valid/invalid reftests while I was at it (so they test the same way the :valid/invalid tests do).
- There are some maxlength-related tests that don't analogously apply to minlength (because browsers currently don't let users type more text in a maxlength field than it can hold, yet don't restrict minlength fields the same way).

A try run seems fine so far: https://treeherder.mozilla.org/#/jobs?repo=try&revision=12454725dbcb
Assignee: MattN+bmo → wisniewskit
Attachment #8776240 - Attachment is obsolete: true
Flags: needinfo?(MattN+bmo)
Attachment #8780789 - Flags: review?(mrbkap)
(Assignee)

Comment 9

11 months ago
Created attachment 8780848 [details] [diff] [review]
932755-add_support_for_input_and_textarea_minLength_and_tooShort.diff

Turns out that I had missed marking a couple tests as passing, so here's a revised patch. Try otherwise seemed fine, just unrelated issues. Here's a fresh run with the new patch: https://treeherder.mozilla.org/#/jobs?repo=try&revision=1f6a5b2e5d7e
Attachment #8780789 - Attachment is obsolete: true
Attachment #8780789 - Flags: review?(mrbkap)
Attachment #8780848 - Flags: review?(mrbkap)

Updated

10 months ago
Attachment #8780848 - Flags: review?(mrbkap) → review+
(Assignee)

Updated

10 months ago
Keywords: checkin-needed
Needs rebasing.
Flags: needinfo?(wisniewskit)
Keywords: checkin-needed
(Assignee)

Comment 11

10 months ago
Created attachment 8781821 [details] [diff] [review]
932755-add_support_for_input_and_textarea_minLength_and_tooShort.diff

Alright, let's try again. Here's a rebased patch. Carrying over r+.
Attachment #8780848 - Attachment is obsolete: true
Flags: needinfo?(wisniewskit)
(Assignee)

Updated

10 months ago
Keywords: checkin-needed
patching file dom/tests/mochitest/ajax/jquery/test/unit/core.js
Hunk #1 FAILED at 314
Hunk #2 FAILED at 388
Hunk #3 FAILED at 415
3 out of 3 hunks FAILED -- saving rejects to file dom/tests/mochitest/ajax/jquery/test/unit/core.js.rej

This is on top of inbound in case that matters.
Keywords: checkin-needed
(Assignee)

Comment 13

10 months ago
Created attachment 8781829 [details] [diff] [review]
932755-add_support_for_input_and_textarea_minLength_and_tooShort.diff

Ah, sorry, I didn't remember to rebase against inbound. Here's a fresh patch rebased against inbound.
Attachment #8781821 - Attachment is obsolete: true
(Assignee)

Updated

10 months ago
Keywords: checkin-needed

Comment 14

10 months ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f4d0e045901e
Add support for input/textarea minLength and tooShort. r=mrbkap
Keywords: checkin-needed

Comment 15

10 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/f4d0e045901e
Status: ASSIGNED → RESOLVED
Last Resolved: 10 months ago
status-firefox51: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
You need to log in before you can comment on or make changes to this bug.