Closed Bug 345512 Opened 18 years ago Closed 14 years ago

Implement pattern attribute for input elements

Categories

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

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla2.0b5

People

(Reporter: WeirdAl, Assigned: mounir)

References

(Depends on 1 open bug, )

Details

(Keywords: dev-doc-complete, html5)

Attachments

(1 file, 10 obsolete files)

The pattern attribute defines a way to validate HTML text inputs.  This will be interesting to implement, as our C++ code doesn't directly support regular expressions, and these input types are already implemented in C++ code.

So what I'd like to do is call into JS land with the regular expression.  If one of the content folks could give me an example of how to do that, I'd be really grateful.
Alex, is the bug releated with bug 273046?
Blocks: 273046
Depends on: 345624
helpwanted: I have no idea how I'm supposed to call on ECMAScript to run a regular expression against the value.  This is needed per the Web Forms 2.0 specification to do validation.
Keywords: helpwanted
Depends on: 106590
Depends on: 348642
No longer depends on: 106590
Component: DOM: HTML → DOM: Core & HTML
QA Contact: ian → general
This code compiles on Windows Vista x64.  I have not tried to link it with other libraries, nor have I tried to execute it for testing.  I'm posting this solely to get an idea whether the pattern match code uses the JSAPI correctly or not (including getting the runtime).

crowder: You offered to lend a hand in bug 348642 comment 7.  If you're still willing, I'd appreciate it.  Otherwise, please assign the review to whoever you feel is best.
Attachment #395535 - Flags: review?
Comment on attachment 395535 [details] [diff] [review]
Pattern match class, work-in-progress (not for check-in, only sanity check)

stupid bugzilla is too nice about accepting patches for review with multiple reviewer matches... see comment 3.

willitblend?
Attachment #395535 - Flags: review? → review?(crowder)
Comment on attachment 395535 [details] [diff] [review]
Pattern match class, work-in-progress (not for check-in, only sanity check)

I'm not a good reviewer for this, but it's essentially the right idea, assuming you must implement this in C++.  Are you quite sure that assumption is correct?  Can't this mutation observer be done in a JS component?
Attachment #395535 - Flags: review?(crowder)
(In reply to comment #5)
>  Can't this mutation observer be done in a JS component?

Not a chance - the interface isn't exposed to JavaScript:
http://mxr.mozilla.org/mozilla1.9.1/source/content/base/public/nsIMutationObserver.h

Now, I could use DOM mutation events, but those are hugely expensive (the event has to be dispatched along all the nodes from document to input element, and back), and I'm pretty sure that would not fly with the content peers.
No longer depends on: 348642
Note that contrary to the bug summary, pattern is not supposed to be implemented on textarea, only input:

http://www.whatwg.org/specs/web-apps/current-work/multipage/section-index.html#attributes-0
Work on this bug is proceeding on bug 345624.
Summary: Implement pattern attribute for <input type="text">, <textarea> → Implement pattern attribute for <input type="text">
No longer blocks: 273046
I'm taking this bug for the reasons I took bug 345624 and bug 345822.
Assignee: ajvincent → mounir.lamouri
Keywords: helpwanted
OS: Windows XP → All
Hardware: x86 → All
Keywords: html5
Good luck on this one... you'll be going where not that many developers want to go:  into the JavaScript engine itself.  Be prepared to write a JS_ExecuteRegExp public API function (or whatever it's supposed to be called).

FYI, peers have already rejected a JS-based regular expression service, and constructing a regexp from a literal.  I think my approach in bug 345624 comment 17 is the closest we've gotten, but bug 345624 comments 23 through 28 should be considered too.
(In reply to comment #10)
> Good luck on this one... you'll be going where not that many developers want to
> go:  into the JavaScript engine itself.  Be prepared to write a
> JS_ExecuteRegExp public API function (or whatever it's supposed to be called).
> 
> FYI, peers have already rejected a JS-based regular expression service, and
> constructing a regexp from a literal.  I think my approach in bug 345624
> comment 17 is the closest we've gotten, but bug 345624 comments 23 through 28
> should be considered too.

Thanks for the hints :) I'm going to have a look of the regexp-related parts of your patch in bug 345624.
Attached patch Tests - WIP (obsolete) — Splinter Review
This is WIP. I will tests more complex regexps and for every types.
Attached patch Patch - WIP (obsolete) — Splinter Review
This is nearly finished. I will finalize that tonight or tomorrow.
:jorendorff, could you give me a feedback on JS-related changes so I will be able to apply your modifications in the final patch ?
Attachment #440537 - Flags: feedback?(jorendorff)
Status: NEW → ASSIGNED
Wow.  You make it look so simple.  I'm shocked.

About the only thing I notice is that the pattern processing is built into the HTMLInputElement class.  Which is fine, but I had hoped for something more generic, which other C++ code (XForms being the most obvious example on my mind, but other input types might need regexp support for field formatting, such as dates & times) could use.
(In reply to comment #14)
> Wow.  You make it look so simple.  I'm shocked.
> 
> About the only thing I notice is that the pattern processing is built into the
> HTMLInputElement class.  Which is fine, but I had hoped for something more
> generic, which other C++ code (XForms being the most obvious example on my
> mind, but other input types might need regexp support for field formatting,
> such as dates & times) could use.

Actually, I've think about adding a method in ContentUtils but I'm not sure using the javascript regexp is really how we want to handle regexps. There are probably easier ways to do that. If this function is added to ContentUtils and used in the code, moving to a simple regexp handling would be harder.
In addition, don't think dates and times will need any regexp. A validation method like the one I've wrote for emails should be enough.
The current specification says the pattern attribute can be used for constraint validation if pattern="". This will make the element invalid if the value isn't the empty string. I don't think that's a correct behavior because authors may use pattern="" to disable the check.

I've open a bug to w3c:
http://www.w3.org/Bugs/Public/show_bug.cgi?id=9579

The coming patch will follow the current spec and a follow-up will come if the change is accepted.
Attached patch Tests v1 (obsolete) — Splinter Review
Attachment #440536 - Attachment is obsolete: true
Attachment #440748 - Flags: review?(jonas)
Attached patch Patch v1 (obsolete) — Splinter Review
Jonas, could you review the content part of this patch ?
Attachment #440537 - Attachment is obsolete: true
Attachment #440749 - Flags: review?(jonas)
Attachment #440537 - Flags: feedback?(jorendorff)
Comment on attachment 440749 [details] [diff] [review]
Patch v1

Jason, could you review the JS part of this patch ?
Attachment #440749 - Flags: review?(jorendorff)
Attachment #440749 - Flags: feedback?(ajvincent)
Comment on attachment 440749 [details] [diff] [review]
Patch v1

For once, I just don't see anything that I can offer suggestions on.
Attachment #440749 - Flags: feedback?(ajvincent) → feedback+
Blocks: 561623
Comment on attachment 440749 [details] [diff] [review]
Patch v1

>+nsHTMLInputElement::DoesPatternApply() const
>+{
>+  switch (mType)
>+  {
>+    case NS_FORM_INPUT_TEXT:
>+    case NS_FORM_INPUT_SEARCH:
>+    case NS_FORM_INPUT_EMAIL:
>+    case NS_FORM_INPUT_URL:
>+    case NS_FORM_INPUT_TEL:
>+    case NS_FORM_INPUT_PASSWORD:
>+      return PR_TRUE;

Nit: IMHO it'd be cleaner to just do

return mType == NS_FORM_INPUT_TEXT ||
       mType == NS_FORM_INPUT_SEARCH ||
...
       mType == NS_FORM_INPUT_PASSWORD;


>+nsHTMLInputElement::HasPatternMismatch()
>+{
>+  if (!DoesPatternApply() || !HasAttr(kNameSpaceID_None, nsGkAtoms::pattern)) {
>+    return PR_FALSE;
>+  }
>+
>+  nsAutoString value;
>+  NS_ENSURE_SUCCESS(GetValue(value), PR_FALSE);
>+
>+  if (value.IsEmpty()) {
>+    return PR_FALSE;
>+  }
>+
>+  nsAutoString pattern;
>+  GetAttr(kNameSpaceID_None, nsGkAtoms::pattern, pattern);
>+
>+  if (!GetOwnerDoc()) {
>+    return PR_FALSE;
>+  }
>+
>+  return !IsPatternMatching(value, pattern, GetOwnerDoc());
>+}

Please cache the result of the call to GetOwnerDoc.

>@@ -3587,16 +3676,35 @@ nsHTMLInputElement::GetValidationMessage
>       } else {
>         return NS_ERROR_UNEXPECTED;
>       }
>       rv = nsContentUtils::GetLocalizedString(nsContentUtils::eDOM_PROPERTIES,
>                                               key.get(), message);
>       aValidationMessage = message;
>       break;
>     }
>+    case VALIDATION_MESSAGE_PATTERN_MISMATCH:
>+    {
>+      nsXPIDLString message;
>+      nsAutoString title;
>+      GetAttr(kNameSpaceID_None, nsGkAtoms::title, title);
>+      if (title.IsEmpty()) {
>+        rv = nsContentUtils::GetLocalizedString(nsContentUtils::eDOM_PROPERTIES,
>+                                                "ElementSuffersFromPatternMismatch",
>+                                                message);
>+      } else {
>+        const PRUnichar* params[] = { title.get() };
>+        printf(" === title: %s\n", NS_ConvertUTF16toUTF8(title).get());
>+        rv = nsContentUtils::FormatLocalizedString(nsContentUtils::eDOM_PROPERTIES,
>+                                                   "ElementSuffersFromPatternMismatchWithTitle",
>+                                                   params, 1, message);
>+      }
>+      aValidationMessage = message;

Using the title seems a bit weird. I'm not sure that will be very useful to users (which likely are the ones that ultimately will be the consumers of this string). Instead might be better to include the actual typed value.

Also, there's a printf here which I suspect you didn't intend to leave :)

>+nsHTMLInputElement::IsPatternMatching(nsAString& aValue, nsAString& aPattern,
>+                                      nsIDocument* aDocument)
>+{
>+  NS_ASSERTION(aDocument, "aDocument should be a valid pointer (not null)");
>+  NS_ENSURE_TRUE(aDocument->GetScriptGlobalObject(), PR_TRUE);
>+
>+  JSContext* ctx = (JSContext*) aDocument->GetScriptGlobalObject()
>+                                  ->GetContext()->GetNativeContext();
>+  NS_ENSURE_TRUE(ctx, PR_TRUE);
>+
>+  JSAutoRequest ar(ctx);
>+
>+  // The pattern has to match the entire value.
>+  aPattern.Insert(NS_LITERAL_STRING("^(?:"), 0);
>+  aPattern.Append(NS_LITERAL_STRING(")$"));

This won't do the right thing if the pattern contains a stray ')', right?

http://xkcd.com/327/ :)

Looks good otherwise assuming jorendorff or someone else from the JS team signs off on the jseng changes.

Would like to see a new patch though.
Attachment #440749 - Flags: review?(jonas) → review+
Comment on attachment 440748 [details] [diff] [review]
Tests v1

>+function completeValidityCheck(element, alwaysValid)
>+{
>+  if (element.type == 'file') {
>+    // Need privileges to set a filename with .value.
>+    netscape.security.PrivilegeManager.enablePrivilege("UniversalXPConnect");
>+  }
>+
>+  // Check when pattern matches.
>+  if (element.type == 'email') {
>+    element.pattern = ".*@bar.com";
>+    element.value = "foo@bar.com";
>+  } else if (element.type == 'url') {
>+    element.pattern = "http://.*\\.com$";
>+    element.value = "http://mozilla.com";
>+  } else {
>+    element.pattern = "foo";
>+    element.value = "foo";
>+  }

Per http://www.whatwg.org/specs/web-apps/current-work/?slow-browser#file-upload-state

the pattern attribute doesn't apply to <input type=file> so remove that from the tests (including the enablePrivilege stuff)

r=me with that.
Attachment #440748 - Flags: review?(jonas) → review+
(In reply to comment #22)
> (From update of attachment 440748 [details] [diff] [review])
> >+function completeValidityCheck(element, alwaysValid)
> >+{
> >+  if (element.type == 'file') {
> >+    // Need privileges to set a filename with .value.
> >+    netscape.security.PrivilegeManager.enablePrivilege("UniversalXPConnect");
> >+  }
> >+
> >+  // Check when pattern matches.
> >+  if (element.type == 'email') {
> >+    element.pattern = ".*@bar.com";
> >+    element.value = "foo@bar.com";
> >+  } else if (element.type == 'url') {
> >+    element.pattern = "http://.*\\.com$";
> >+    element.value = "http://mozilla.com";
> >+  } else {
> >+    element.pattern = "foo";
> >+    element.value = "foo";
> >+  }
> 
> Per
> http://www.whatwg.org/specs/web-apps/current-work/?slow-browser#file-upload-state
> 
> the pattern attribute doesn't apply to <input type=file> so remove that from
> the tests (including the enablePrivilege stuff)

That's exactly what this test is checking: the pattern attribute doesn't apply to <input type='file'>. This function is called with type='file' to check when @pattern is set, the element doesn't suffer from patter mismatch.
(In reply to comment #21)
> (From update of attachment 440749 [details] [diff] [review])
> >+nsHTMLInputElement::DoesPatternApply() const
> >+{
> >+  switch (mType)
> >+  {
> >+    case NS_FORM_INPUT_TEXT:
> >+    case NS_FORM_INPUT_SEARCH:
> >+    case NS_FORM_INPUT_EMAIL:
> >+    case NS_FORM_INPUT_URL:
> >+    case NS_FORM_INPUT_TEL:
> >+    case NS_FORM_INPUT_PASSWORD:
> >+      return PR_TRUE;
> 
> Nit: IMHO it'd be cleaner to just do
> 
> return mType == NS_FORM_INPUT_TEXT ||
>        mType == NS_FORM_INPUT_SEARCH ||
> ...
>        mType == NS_FORM_INPUT_PASSWORD;

Blame me for that one, Jonas.  See bug 345822 comment 52, which smaug implied acceptance for in bug 345822 comment 58.  I felt a debug assertion would be a good idea for future-proofing.
Attached patch Tests v1.1 (obsolete) — Splinter Review
r=sicking

I've added tests for '(' and ')'. In a regexp, they need to be escaped and that is working correctly with the current patch.
Attachment #440748 - Attachment is obsolete: true
Attached patch Patch v1.1 (obsolete) — Splinter Review
r=sicking

I've changed |DoesPatternApply| to something much more simple.
For @title, that is what the specifications require. See http://dev.w3.org/html5/spec/forms.html#attr-input-pattern and http://dev.w3.org/html5/spec/dom.html#the-title-attribute
Attachment #441773 - Flags: superreview?(Olli.Pettay)
Attachment #441773 - Flags: review?(jorendorff)
Attachment #440749 - Attachment is obsolete: true
Attachment #440749 - Flags: review?(jorendorff)
I'm sorry I didn't get to this earlier. The new API looks great.
A few minor changes are needed in the implementation.

>+    if (!obj->isRegExp()) {
>+        return JS_FALSE;
>+    }

This must report an error before returning JS_FALSE.

You can do that by adding a line to the end of js/src/js.msg:

  MSG_DEF(JSMSG_NOT_EXPECTED_TYPE,      249, 1, JSEXN_TYPEERR, "{0}: expected {1}, got {2}")

Then in JS_ExecuteRegExp:
  if (!obj->isRegExp) {
      JS_ReportErrorNumber(cx, js_GetErrorMessage, NULL, JSMSG_NOT_EXPECTED_TYPE,
                           "JS_ExecuteRegExp", "RegExp", obj->getClass()->name);
      return JS_FALSE;
  }

(It may not fit on 2 lines here but it will in your editor -- in js/src we use longish lines, up to 99 columns.)

>+    JSString *str = js_NewStringCopyN(cx, chars, length);

Add a check for NULL:

    if (!str)
        return JS_FALSE;

There is no need to report an error here because js_NewStringCopyN does it for you if needed.

Also, the string must be rooted against GC, like so:

    AutoValueRooter v(cx, str);

Add that line just after the null check.

If copying the whole string ever turns out to be a performance
bottleneck, we can modify js_ExecuteRegExp to operate directly on
the characters rather than requiring a JSString. I don't think we
need to do it right now.

Looks great. Thanks.
Attachment #441773 - Flags: review?(jorendorff)
Attached patch Patch v1.2 (obsolete) — Splinter Review
r=jonas

This patch is applying the changes requested by Jason.
Attachment #441773 - Attachment is obsolete: true
Attachment #442556 - Flags: superreview?(Olli.Pettay)
Attachment #442556 - Flags: review?(jorendorff)
Attachment #441773 - Flags: superreview?(Olli.Pettay)
Keywords: dev-doc-needed
Comment on attachment 442556 [details] [diff] [review]
Patch v1.2

>+  JSObject* re = JS_NewUCRegExpObject(ctx, reinterpret_cast<jschar*>
>+                                             (aPattern.BeginWriting()),
>+                                      aPattern.Length(), 0);

After this I think you need

  js::AutoValueRooter re_root(cx, re);

to be quite safe.
Attachment #442556 - Flags: review?(jorendorff) → review+
Sorry I didn't notice that before.

r=me for the JS parts, with that change.
Attached patch Patch v1.3 (obsolete) — Splinter Review
r=jonas,jorendorff
Attachment #442556 - Attachment is obsolete: true
Attachment #442952 - Flags: superreview?(Olli.Pettay)
Attachment #442556 - Flags: superreview?(Olli.Pettay)
(In reply to comment #30)
> Sorry I didn't notice that before.
> 
> r=me for the JS parts, with that change.

Thank you for your review Jason :)
The last patch is adding the line you've requested.
Comment on attachment 442952 [details] [diff] [review]
Patch v1.3


> PRBool
>+nsHTMLInputElement::HasPatternMismatch()
>+{
>+  if (!DoesPatternApply() || !HasAttr(kNameSpaceID_None, nsGkAtoms::pattern)) {
>+    return PR_FALSE;
>+  }

Could you write this
  nsAutoString pattern;
  if (!DoesPatternApply() || !GetAttr(kNameSpaceID_None, nsGkAtoms::pattern, pattern)) {
    return PR_FALSE;
  }
And then remove GetAttr call few lines below.

>+PRBool
>+nsHTMLInputElement::IsPatternMatching(nsAString& aValue, nsAString& aPattern,
>+                                      nsIDocument* aDocument)
>+{
>+  NS_ASSERTION(aDocument, "aDocument should be a valid pointer (not null)");
>+  NS_ENSURE_TRUE(aDocument->GetScriptGlobalObject(), PR_TRUE);
>+
>+  JSContext* ctx = (JSContext*) aDocument->GetScriptGlobalObject()
>+                                  ->GetContext()->GetNativeContext();
Nit, -> should be in the previous line.
Attachment #442952 - Flags: superreview?(Olli.Pettay) → superreview+
Attached patch Patch v1.4 (obsolete) — Splinter Review
r=jonas,jorendorff sr=smaug
Attachment #442952 - Attachment is obsolete: true
This piggybacks on the main JS engine behavior for superlinear regular expressions -- which means it's pretty easy to hang the browser here if you want.  I have no idea why we haven't flipped javascript.options.relimit yet to address the issue.  This may be something worth considering in the context of this patch (or it may just be worth considering as followup), just throwing it out there as something that probably needs to be kept in mind...
(In reply to comment #35)
> This piggybacks on the main JS engine behavior for superlinear regular
> expressions -- which means it's pretty easy to hang the browser here if you
> want.  I have no idea why we haven't flipped javascript.options.relimit yet to
> address the issue.  This may be something worth considering in the context of
> this patch (or it may just be worth considering as followup), just throwing it
> out there as something that probably needs to be kept in mind...

I'm not sure I get your comment. Do you want javascript.options.relimit to be true by default ?
Feel free to open a follow-up and add me in CC.
I would indeed like that, but not specifically due to this particular bug.  What I'm saying is that, since you're implementing this, you may care about this particular failure mode more than I do, enough to push on that change specifically due to this particular case .  :-)  It's bug 452451, by the way -- requested blocking193 on it while I had it open.
(In reply to comment #37)
> I would indeed like that, but not specifically due to this particular bug. 
> What I'm saying is that, since you're implementing this, you may care about
> this particular failure mode more than I do, enough to push on that change
> specifically due to this particular case .  :-)  It's bug 452451, by the way --
> requested blocking193 on it while I had it open.

I've attached a patch on bug 452451.
See Also: → 452451
Blocks: 566348
Summary: Implement pattern attribute for <input type="text"> → Implement pattern attribute for input elements
Blocks: 558788
Blocks: 561635
Depends on: 581021
Attached patch Patch v1.5Splinter Review
Updated patch to apply against current tip.
Merged with test patch.
Removing changes that have been landed with bug 581021.
Attachment #395535 - Attachment is obsolete: true
Attachment #441771 - Attachment is obsolete: true
Attachment #443247 - Attachment is obsolete: true
It doesn't seem that turning off javascript affects this feature, although in essence this feature is running content through the javascript engine.

Am I wrong in assuming it should affect this feature? Should I file a follow-up? Don't bother?
I don't think the JS-enabled preference should affect this at all.  JS-enabled controls whether scripts in pages can execute, but this isn't script -- it's just pattern-matching that happens to be performed by a script engine.

That said, it occurs to me that exploitable regular expression bugs in many cases, after this change, no longer have workarounds other than upgrading.  It seems worthwhile to file a new bug to create such a preference (although it's hard to say exactly what should happen with pattern support flipped to false, since the browser would probably still appear via feature-detection to support it).  I don't think I could say what priority adding it would have, tho, or whether it should be done (presumably for 4.0) in lieu of other work.
Depends on: 588945
This is now in the tree.
rev: http://hg.mozilla.org/mozilla-central/rev/283ec5de5ee2
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.0b5
You need to log in before you can comment on or make changes to this bug.