New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Apply unicode flag to pattern #20819

Closed
wants to merge 4 commits into
base: master
from

Conversation

@JLHwung
Contributor

JLHwung commented Dec 6, 2017

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

[x] Bugfix
[ ] Feature
[ ] Code style update (formatting, local variables)
[ ] Refactoring (no functional changes, no api changes)
[ ] Build related changes
[ ] CI related changes
[ ] Documentation content changes
[ ] angular.io application / infrastructure changes
[ ] Other... Please describe:

What is the current behavior?

The pattern validator does not apply unicode flag to string input

What is the new behavior?

The pattern validator applies unicode flag on string input

Does this PR introduce a breaking change?

[x] Yes
[ ] No

Rationale

If an input element has a pattern attribute specified, and the attribute's value, when compiled as a JavaScript regular expression with only the "u" flag specified, compiles successfully, then the resulting regular expression is the element's compiled pattern regular expression.

See the spec.

Both Firefox and Chrome have applied unicode flag to the pattern content attribute of INPUT element.

See https://www.chromestatus.com/feature/4753420745441280

@googlebot googlebot added the cla: yes label Dec 6, 2017

@@ -146,7 +146,7 @@ export class Validators {
let regexStr: string;
if (typeof pattern === 'string') {
regexStr = `^${pattern}$`;
regex = new RegExp(regexStr);
regex = new RegExp(regexStr, 'u');

This comment has been minimized.

@gkalpak

gkalpak Dec 6, 2017

Member

This will fail in browsers that do not support the unicode flag (e.g. IE).
Not sure if it is a good idea, but if one wanted to conditionally apply it, they could check whether RegExp.prototype.hasOwnProperty('unicode').

This comment has been minimized.

@JLHwung

JLHwung Dec 6, 2017

Contributor

Thank you. I agree that angular should be as consistent as possible to the browser behaviour. I've added conditional check and updated related test.

Where should I document the contingent behaviour of apply unicode flags?

This comment has been minimized.

@gkalpak

gkalpak Dec 6, 2017

Member

It seems that the validators are not part of the docs. Maybe you could document the behavior in the directive that adds the validator:

/**
* A Directive that adds the `pattern` validator to any controls marked with the
* `pattern` attribute, via the {@link NG_VALIDATORS} binding. Uses attribute value
* as the regex to validate Control value against. Follows pattern attribute
* semantics; i.e. regex must match entire Control value.
*
* ### Example
*
* ```
* <input [name]="fullName" pattern="[a-zA-Z ]*" ngModel>
* ```
* @stable
*/

cc @kara

This comment has been minimized.

@JLHwung

JLHwung Dec 8, 2017

Contributor

@gkalpak Documented and thanks.

@gkalpak gkalpak added the comp: forms label Dec 6, 2017

@JLHwung JLHwung force-pushed the JLHwung:apply-unicode-flag-to-pattern branch 6 times, most recently from 757f354 to 83a8f8a Dec 6, 2017

@JLHwung

This comment has been minimized.

Contributor

JLHwung commented Dec 11, 2017

ping @gkalpak.

The travis build errors seems to be irrelevant to this PR, any idea?

expect(Validators.pattern('[\u{10000}-\u{10001}]')(new FormControl('\u{10000}')))
.toBeNull();
} else {
expect(() => Validators.pattern('[\u{10000}-\u{10001}]')(new FormControl('\u{10000}')))

This comment has been minimized.

@gkalpak

gkalpak Dec 11, 2017

Member

I don't think there is any value in running this. I would change this to:

if (browserDetection.supportsRegExUnicodeFlag) {
  it('should apply unicode flag to pattern when supported', () => {
    expect(Validators.pattern('[\u{10000}-\u{10001}]')(new FormControl('\u{10000}'))).toBeNull();
  });
}

This comment has been minimized.

@JLHwung

JLHwung Dec 11, 2017

Contributor

👍 Good if we are not compelled to keep 100% code coverage.

@gkalpak

This comment has been minimized.

Member

gkalpak commented Dec 11, 2017

The curent Travis failures are indeed unrelated. We need to fix them on master.

This has to be reviewed/approved by forms and platform-browser owners.

@JLHwung JLHwung force-pushed the JLHwung:apply-unicode-flag-to-pattern branch 2 times, most recently from dc54167 to e08ab91 Dec 11, 2017

@JLHwung JLHwung force-pushed the JLHwung:apply-unicode-flag-to-pattern branch 3 times, most recently from f3d4a34 to f8d665a Jan 13, 2018

@ngbot

This comment has been minimized.

ngbot bot commented Jan 16, 2018

Hello? Don't want to hassle you. Sure you're busy. But this PR has some merge conflicts that you probably ought to resolve.
That is... if you want it to be merged someday...

@JLHwung JLHwung force-pushed the JLHwung:apply-unicode-flag-to-pattern branch from f8d665a to a14e841 Jan 17, 2018

@ngbot

This comment has been minimized.

ngbot bot commented Jan 24, 2018

Hi @JLHwung! This PR has merge conflicts due to recent upstream merges.
Please help to unblock it by resolving these conflicts. Thanks!

@JLHwung JLHwung force-pushed the JLHwung:apply-unicode-flag-to-pattern branch from a14e841 to 9d93193 Jan 24, 2018

@JLHwung

This comment has been minimized.

Contributor

JLHwung commented Jan 24, 2018

@gkalpak Changed according to your comments, any idea?

Also cc @alexeagle

* Note: if a string type attribute value is used, the regex will be applied with the
* unicode flag on supported browsers. If a unicode-regex is passed, it might break on
* unsupported browser. In this case, the user should be responsible to handle the
* browser compatibility

This comment has been minimized.

@gkalpak

gkalpak Jan 24, 2018

Member

Missing .

This comment has been minimized.

@JLHwung

JLHwung Jan 24, 2018

Contributor

Oops, fixup and rebased. Thanks.

@gkalpak

This comment has been minimized.

Member

gkalpak commented Jan 24, 2018

LGTM, but still needs approval from forms and platform-browser owners 😁
Restarted the failing e2e job (could be a flake).

@JLHwung JLHwung force-pushed the JLHwung:apply-unicode-flag-to-pattern branch from 9d93193 to d6836a9 Jan 24, 2018

@ngbot

This comment has been minimized.

ngbot bot commented Mar 20, 2018

Hi @JLHwung! This PR has merge conflicts due to recent upstream merges.
Please help to unblock it by resolving these conflicts. Thanks!

2 similar comments
@ngbot

This comment has been minimized.

ngbot bot commented Mar 20, 2018

Hi @JLHwung! This PR has merge conflicts due to recent upstream merges.
Please help to unblock it by resolving these conflicts. Thanks!

@ngbot

This comment has been minimized.

ngbot bot commented Mar 20, 2018

Hi @JLHwung! This PR has merge conflicts due to recent upstream merges.
Please help to unblock it by resolving these conflicts. Thanks!

@JLHwung JLHwung force-pushed the JLHwung:apply-unicode-flag-to-pattern branch from d6836a9 to 2339a37 Mar 22, 2018

@JLHwung

This comment has been minimized.

Contributor

JLHwung commented Mar 31, 2018

LGTM, but still needs approval from forms and platform-browser owners

cc @kara and @vicb for approval. Thanks.

@ngbot ngbot bot added this to the needsTriage milestone Nov 27, 2018

@kara

This comment has been minimized.

Contributor

kara commented Nov 27, 2018

@JLHwung Can you address @IgorMinar's comment? Then we should be good to go.

@JLHwung

This comment has been minimized.

Contributor

JLHwung commented Nov 28, 2018

@kara and @IgorMinar Thanks for writing suggestion.

@JLHwung JLHwung force-pushed the JLHwung:apply-unicode-flag-to-pattern branch from 2bcd86a to 64906b3 Nov 28, 2018

@kara

This comment has been minimized.

Contributor

kara commented Nov 28, 2018

@JLHwung I think if you rebase it will fix the errors in CI (it's red currently)

JLHwung and others added some commits Dec 6, 2017

fix(forms): apply unicode flag to pattern attribute when supported
Both Firefox and Chrome have applied unicode flag to the `pattern` content attribute of INPUT element.
See https://www.chromestatus.com/feature/4753420745441280
docs(forms): take words suggestion
Co-Authored-By: JLHwung <JLHwung@users.noreply.github.com>

@JLHwung JLHwung force-pushed the JLHwung:apply-unicode-flag-to-pattern branch from 64906b3 to 7efe1ae Nov 29, 2018

@JLHwung

This comment has been minimized.

Contributor

JLHwung commented Nov 29, 2018

@kara rebased and thanks.

@IgorMinar

thanks!

@IgorMinar

This comment has been minimized.

@IgorMinar IgorMinar closed this in 3c34b8b Nov 30, 2018

IgorMinar added a commit that referenced this pull request Nov 30, 2018

IgorMinar added a commit that referenced this pull request Nov 30, 2018

docs(forms): take words suggestion (#20819)
Co-Authored-By: JLHwung <JLHwung@users.noreply.github.com>

PR Close #20819

@JLHwung JLHwung deleted the JLHwung:apply-unicode-flag-to-pattern branch Nov 30, 2018

IgorMinar added a commit to IgorMinar/angular that referenced this pull request Nov 30, 2018

Revert "docs(forms): take words suggestion (angular#20819)"
This reverts commit e024f2f.

breaks google3 see http://cl/223526995

IgorMinar added a commit to IgorMinar/angular that referenced this pull request Nov 30, 2018

Revert "test(forms): add test on applying unicode flag (angular#20819)"
This reverts commit 619b8ab.

breaks google3 see http://cl/223526995

IgorMinar added a commit to IgorMinar/angular that referenced this pull request Nov 30, 2018

Revert "fix(forms): apply unicode flag to pattern attribute when supp…
…orted (angular#20819)"

This reverts commit 3c34b8b.

breaks google3 see http://cl/223526995

pull bot pushed a commit to sfeir-open-source/angular that referenced this pull request Nov 30, 2018

Revert "docs(forms): take words suggestion (angular#20819)" (angular#…
…27375)

This reverts commit e024f2f.

breaks google3 see http://cl/223526995

PR Close angular#27375

pull bot pushed a commit to sfeir-open-source/angular that referenced this pull request Nov 30, 2018

Revert "test(forms): add test on applying unicode flag (angular#20819)…
…" (angular#27375)

This reverts commit 619b8ab.

breaks google3 see http://cl/223526995

PR Close angular#27375

pull bot pushed a commit to sfeir-open-source/angular that referenced this pull request Nov 30, 2018

Revert "fix(forms): apply unicode flag to pattern attribute when supp…
…orted (angular#20819)" (angular#27375)

This reverts commit 3c34b8b.

breaks google3 see http://cl/223526995

PR Close angular#27375
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment