345512 - Implement pattern attribute for input elements

Status: RESOLVED FIXED

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

Description

[Alex Vincent [:WeirdAl]]

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.

Comment 1

[alexander :surkov (:asurkov)]

Alex, is the bug releated with bug 273046?

Comment 2

[Alex Vincent [:WeirdAl]]

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.

Comment 3

[Alex Vincent [:WeirdAl]]

Attachment: Pattern match class, work-in-progress (not for check-in, only sanity check)

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.

Comment 4

[Alex Vincent [:WeirdAl]]

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?

Comment 5

[Brian 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?

Comment 6

[Alex Vincent [:WeirdAl]]

(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.

Comment 7

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

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

Comment 8

[Alex Vincent [:WeirdAl]]

Work on this bug is proceeding on bug 345624.

Comment 9

[Mounir Lamouri (:mounir)]

I'm taking this bug for the reasons I took bug 345624 and bug 345822.

Comment 10

[Alex Vincent [:WeirdAl]]

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.

Comment 11

[Mounir Lamouri (:mounir)]

(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.

Comment 12

[Mounir Lamouri (:mounir)]

Attachment: Tests - WIP

This is WIP. I will tests more complex regexps and for every types.

Comment 13

[Mounir Lamouri (:mounir)]

Attachment: Patch - WIP

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 ?

Comment 14

[Alex Vincent [:WeirdAl]]

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.

Comment 15

[Mounir Lamouri (:mounir)]

(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.

Comment 16

[Mounir Lamouri (:mounir)]

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.

Comment 17

[Mounir Lamouri (:mounir)]

Attachment: Tests v1

Comment 18

[Mounir Lamouri (:mounir)]

Attachment: Patch v1

Jonas, could you review the content part of this patch ?

Comment 19

[Mounir Lamouri (:mounir)]

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

Jason, could you review the JS part of this patch ?

Comment 20

[Alex Vincent [:WeirdAl]]

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

For once, I just don't see anything that I can offer suggestions on.

Comment 21

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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.

Comment 22

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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 = "[e-mail]";
>+  } 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.

Comment 23

[Mounir Lamouri (:mounir)]

(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 = "[e-mail]";
> >+  } 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.

Comment 24

[Alex Vincent [:WeirdAl]]

(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.

Comment 25

[Mounir Lamouri (:mounir)]

Attachment: Tests v1.1

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.

Comment 26

[Mounir Lamouri (:mounir)]

Attachment: Patch v1.1

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

Comment 27

[Jason Orendorff [: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.

Comment 28

[Mounir Lamouri (:mounir)]

Attachment: Patch v1.2

r=jonas

This patch is applying the changes requested by Jason.

Comment 29

[Jason Orendorff [:jorendorff]]

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.

Comment 30

[Jason Orendorff [:jorendorff]]

Sorry I didn't notice that before.

r=me for the JS parts, with that change.

Comment 31

[Mounir Lamouri (:mounir)]

Attachment: Patch v1.3

r=jonas,jorendorff

Comment 32

[Mounir Lamouri (:mounir)]

(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 33

[Olli Pettay [:smaug][[e-mail]]]

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.

Comment 34

[Mounir Lamouri (:mounir)]

Attachment: Patch v1.4

r=jonas,jorendorff sr=smaug

Comment 35

[Jeff Walden [:Waldo]]

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...

Comment 36

[Mounir Lamouri (:mounir)]

(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.

Comment 37

[Jeff Walden [:Waldo]]

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.

Comment 38

[Mounir Lamouri (:mounir)]

(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.

Comment 39

[Mounir Lamouri (:mounir)]

Attachment: Patch v1.5

Updated patch to apply against current tip.
Merged with test patch.
Removing changes that have been landed with bug 581021.

Comment 40

[Nochum Sossonko [:Natch]]

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?

Comment 41

[Jeff Walden [:Waldo]]

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.

Comment 42

[Nochum Sossonko [:Natch]]

Bug 588945.

Comment 43

[Mounir Lamouri (:mounir)]

This is now in the tree.
rev: http://hg.mozilla.org/mozilla-central/rev/283ec5de5ee2