fix(text-field): disable validation check in setRequired #2201

moog16 · 2018-02-06T18:27:07Z

resolves #2029

… required

codecov-io · 2018-02-06T18:41:15Z

Codecov Report

Merging #2201 into master will increase coverage by 0.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #2201      +/-   ##
==========================================
+ Coverage   98.84%   98.85%   +0.01%     
==========================================
  Files         100      100              
  Lines        4075     4115      +40     
  Branches      524      526       +2     
==========================================
+ Hits         4028     4068      +40     
  Misses         47       47

Impacted Files	Coverage Δ
packages/mdc-textfield/index.js	`94.8% <100%> (+1.41%)`	⬆️
packages/mdc-textfield/foundation.js	`98.85% <100%> (+0.04%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c14d244...fe82837. Read the comment docs.

kfranqueiro · 2018-02-08T20:14:23Z

demos/text-field.html

@@ -139,7 +139,7 @@ <h2>Password field with validation</h2>
        <h2>Outlined Text Field</h2>
        <div id="demo-tf-outlined-wrapper">
          <div id="tf-outlined-example" class="mdc-text-field mdc-text-field--outlined" data-demo-no-auto-js>
-            <input required pattern=".{8,}" type="text" id="tf-outlined-input" class="mdc-text-field__input"
+            <input pattern=".{8,}" type="text" id="tf-outlined-input" class="mdc-text-field__input"


This line is inconsistent with the JS code below (pattern would be .{8,} when required, but .* when not)

yes you're correct. I'll sift through the rest of the file and make sure that all works the same.

kfranqueiro · 2018-02-08T20:19:39Z

packages/mdc-textfield/foundation.js

@@ -301,9 +301,6 @@ class MDCTextFieldFoundation extends MDCFoundation {
   */
  setRequired(isRequired) {
    this.getNativeInput_().required = isRequired;
-    // Addition of the asterisk is automatic based on CSS, but validity checking


This solution is problematic, because it also leaves things as marked invalid if you change required from true to false... in which case they're no longer invalid if empty. At the very least I think we should still be calling styleValidity if isRequired is false.

Also, I think in the original issue you talked about distinguishing between whether the field was "dirty" or not. I'm not sure if we even have a check for that, though I think what you really mean is whether the input has ever received focus? This solution would just never update the state at all when turning required on.

I did notice your concern in the demo, and I did talk to Dave about that. In both cases the input is still marked as invalid (with or w/o styleValidity call). In the Text Field box example, this can be fixed by setting input.pattern before setting input.required. I thought having to do that was fragile.

This then led me to question why do we choose to manage required in foundation when HTML5 already does this for us? I see required as a just another piece of validation, meaning why aren't we managing pattern, minlength, etc. Having to manage all of those properties isn't reasonable, so why do we only choose required?

Huh... you're right, even now on the demo page in 0.30.0 it doesn't clear the invalid style when setting required back to false, which seems like a problem... and that particular example doesn't even use pattern at all.

HTML5 marks a field as invalid immediately if it is required and blank, which is user-unfriendly and something we're specifically avoiding doing, right? Though I suppose the same is true with pattern if the text input is initially populated, but AFAICT pattern isn't applying :invalid if the field is blank...

https://codepen.io/kfranqueiro/pen/yvgjGb

Interesting...I didn't realize the invalid attribute is added. We aren't using :invalid to style invalid state, which is why we don't see this in the demos. In @williamernest's (codepen)[https://codepen.io/williamernest/pen/ddPdxo] we can see the current behavior and the HTML5 behavior, which is what this fix is trying to achieve.

Do you have any thoughts as to just removing setRequired and get required?

In Will's demo, setting JS required from true to false properly clears the invalid state, reinforcing my original point that I think we should preserve the current logic specifically when setting required to false.

I made my codepen using pure HTML5 because I thought when you said "HTML5 already does this for us" you were referring to OOTB native behavior, not the behavior of our component WRT that attribute. I thought Will was originally only modifying the HTML5 attribute directly with our JS component as a workaround to avoid surfacing this bug in the demo anyway; I'm not sure it's something we should be actively comparing behavior to or seeking to match, since the JS component maintains state itself and setting the attribute directly skips that logic.

Ultimately I don't think we can remove the required setter/getter, given the problem regarding the input still being marked as invalid after it is no longer required. I also don't think we want to rely on the native HTML5 behavior directly rather than set our own class, since as I said above, generally users find that behavior undesirable because fields should not be highlighted as invalid before the user ever interacts with them.

I don't think Will's codepen is a common use case, but I suppose that doesn't mean we shouldn't design for it. Having the styleValidity call in setRequired is what is causing the original issue. Are you suggesting that developers use the HTML5 attr directly instead of our JS setter/getter if they see this issue?

I'm suggesting pretty much the opposite of that... We need to fix the programmatic API so that it works correctly in both directions (required -> not, not -> required) and developers should not ever have reason to programmatically toggle the HTML attribute directly on a JS text field. (It should work if the HTML attribute is initially set in markup though.)

Currently in our component on master, IIUC, required -> not works correctly programmatically, but not -> required doesn't. In the PR in its current state, the opposite is true. We want both to work.

If this needs further clarification, I will try to respond off-github.

kfranqueiro · 2018-02-08T20:20:15Z

test/unit/mdc-textfield/foundation.test.js

    setupValueTest('', /* isValid */ false);

  foundation.setRequired(true);
  assert.isOk(foundation.isRequired());
-  td.verify(mockAdapter.addClass(cssClasses.INVALID));


If we don't have a negative test for this (testing from required=true to required=false) we should probably add one (see my comment above).

BREAKING CHANGE: removed setRequired and isRequired from foundation. Removed get/set required from component.

moog16 · 2018-02-10T00:15:22Z

After much debate offline, the decision was made to remove set/get required from the component and remove isRequired and setRequired from foundation, and instead use a generic getValidationAttribute, setValidationAttribute and removeValidationAttribute. This will allow for other validation properties than just required. In our demo pages exists pattern and minlength which illustrate the benefit to this. This will also future proof the text-field if any new HTML validation attributes are added to the spec.

When either removeValidationAttribute or setValidationAttribute are invoked, it will set the validation styles to valid regardless of value. If any validation attributes are toggled on/off, the user should be given a chance to interact with the text-field before seeing errors.

kfranqueiro · 2018-02-13T18:09:49Z

packages/mdc-textfield/index.js

   */
-  get required() {
-    return this.foundation_.isRequired();
+  getValidationAttribute(attrName) {


I would still expect this to be exposed via required / pattern / etc. setters/getters on the component to make it match working with a native input element?

are you saying keep get/set required, but also add get/set pattern that proxy to foundation_.getValidationAttribute and foundation_.setValidationAttribute? Should we add others eg. minlength, maxlength?

Yeah, that's what I was thinking, and yes to min/maxlength too.

…com:material-components/material-components-web into fix/text-field/validation-behave-like-html5

BREAKING CHANGES: removed get/set required from MDCTextField. Should get/set from DOM input element.

BREAKING CHANGE: removed isRequired and setRequired from text-field foundation

kfranqueiro · 2018-02-23T19:56:12Z

package-lock.json

@@ -1368,9 +1368,9 @@
      "integrity": "sha1-+GzWzvT1MAyOY+B6TVEvZfv/RTE=",
      "dev": true,
      "requires": {
+        "JSONStream": "1.3.1",


Not sure how package-lock suddenly found its way into this PR but it doesn't need to be here

kfranqueiro · 2018-02-23T20:10:25Z

test/unit/mdc-textfield/mdc-text-field.test.js

+  assert.equal(component.maxLength, 10);
+  component.maxLength = -1;
+  // IE11 has a different value for no maxlength property
+  assert.isFalse(component.maxLength === 10);


Nit: assert.notEqual(component.maxLength, 10)

kfranqueiro · 2018-02-23T20:12:29Z

Also just noticed a bug: the helper text element below Text Field Box seems to initially be visible even though the checkbox is initially unchecked. This doesn't seem to be a problem with the first example on the page.

moog16 · 2018-02-23T20:43:37Z

Yes looks like it initially needs style='none'. I'll add it inline, since the checkbox toggles it on/off.

kfranqueiro

For some reason I'm not seeing helper text on the Text Field Box example when Use Helper Text is checked and I focus the input? This is inconsistent with the first example where helper text appears on focus. The "Make helper text persistent" seems to work though.

moog16 · 2018-02-27T03:38:36Z

It seems that Text Field Box is only used as a validation message, which is not given opacity: 1 when focused on. I'll add the the Use helper text as validation message checkbox to make it consistent with the top text field.

…p text-field

kfranqueiro · 2018-02-27T22:47:35Z

packages/mdc-textfield/adapter.js

+  /**
+   * Disconnect all validation attribute listeners on the input element.
+   */
+  deregisterValidationAttributeChangeHandler() {}


This should probably just disconnect a specific observer that it is handed, and we should loop within the component's destroy method instead of building the iteration logic into the adapter.

…o accept observer

…osure docs

…com:material-components/material-components-web into fix/text-field/validation-behave-like-html5

kfranqueiro · 2018-03-01T13:29:40Z

packages/mdc-textfield/adapter.js

+  /**
+   * Registers a validation attribute change listener on the input element.
+   * @return {!MutationObserver}
+   * @param {function(!Array): undefined} handler


Put @param before @return

kfranqueiro · 2018-03-01T13:30:43Z

packages/mdc-textfield/foundation.js

+   * Handles validation attribute changes
+   * @param {Array<MutationRecord>} mutationsList
+   */
+  handleValidationAttributeMutation(mutationsList) {


I'm inclined to think this should be a @private method with underscore suffix. No one should ever publicly need to call this, since we're hooking up the mutation observer via adapter APIs and that will call it.

kfranqueiro · 2018-03-01T14:30:37Z

test/unit/mdc-textfield/foundation.test.js

 });

 test('#destroy removes event listeners', () => {
  const {foundation, mockAdapter} = setupTest();
+  foundation.validationObserver_ = {test: 'test1'};


This can just be {}

@alternate

commit f17e0d3 Author: Will Ernest <[email protected]> Date: Thu Mar 8 13:20:48 2018 -0500 fix(text-field): Clicking label should focus textfield (#2353) commit 77b15f4 Author: Bonnie Zhou <[email protected]> Date: Wed Mar 7 16:29:57 2018 -0800 refactor(button): Remove compact variant (#2361) BREAKING CHANGE: The compact variant of MDC Button is removed. commit 35a5cfc Author: Will Ernest <[email protected]> Date: Tue Mar 6 19:54:23 2018 -0500 fix(toolbar): Fix colors for svg icons. Update custom-toolbar demo (#2331) SVG icons in the toolbar will now use the same color as the font icons. commit dc52201 Author: Andrew C. Dvorak <[email protected]> Date: Tue Mar 6 16:00:12 2018 -0800 fix(theme): Move @alternate annotations for Closure Stylesheets (#2355) `@alternate` annotations need to come before the _second_ property. Otherwise, Closure Compiler strips the first property (it does not output it at all). commit 3c04419 Author: aprigogin <[email protected]> Date: Tue Mar 6 14:29:12 2018 -0800 fix(button): icon in rtl should have margin right flipped. (#2346) commit b9000a4 Author: Cory Reed <[email protected]> Date: Tue Mar 6 07:37:40 2018 -0800 fix(demos): Correct RTL/LTR toggling in demos in Safari (#2348) commit ab85736 Author: Andrew C. Dvorak <[email protected]> Date: Mon Mar 5 19:00:11 2018 -0500 fix: Use `var` instead of `const` in menu demo (#2345) Safari 9.x and IE 10 do not support `const` commit dc3d69f Author: aprigogin <[email protected]> Date: Mon Mar 5 15:22:31 2018 -0800 fix(rtl): Adding noflip annotations to fix downstream rtl issues (#2344) * fix(rtl): Adding noflip annotations to fix downstream rtl issues * fix(rtl): remove changes to button, will be in separate PR * fix(rtl): remove changes to button, will be in separate PR - second attempt. * fix(rtl): removed extra whitespace commit eb4138e Author: Patrick RoDee <[email protected]> Date: Mon Mar 5 14:10:46 2018 -0800 docs: Update CHANGELOG.md commit f478610 Author: Patrick RoDee <[email protected]> Date: Mon Mar 5 14:10:30 2018 -0800 chore: Publish commit 78408bb Author: Andrew C. Dvorak <[email protected]> Date: Mon Mar 5 16:13:02 2018 -0500 fix: Use `var` instead of `const` in demos/ready.js (#2343) Safari 9.x and IE 10 do not support `const`, and `ready.js` is not transpiled to ES5. commit 49a9449 Author: Will Ernest <[email protected]> Date: Mon Mar 5 12:21:54 2018 -0500 chore(select): Fix JS example in Readme (#2332) commit bc17291 Author: Will Ernest <[email protected]> Date: Fri Mar 2 13:21:07 2018 -0500 feat(top app bar): Add short top app bar always collapsed feature (#2327) commit 0ba7d10 Author: Matty Goo <[email protected]> Date: Fri Mar 2 11:33:19 2018 -0500 fix(text-field): disable validation check in setRequired (#2201) BREAKING CHANGE: removed setRequired and isRequired from foundation. commit c14d244 Author: Will Ernest <[email protected]> Date: Fri Mar 2 10:37:18 2018 -0500 chore(select): Fix ID values in demo (#2330) Remove unused ID and changed duplicated ID values. Also fixed the parentheses in RTL. commit ecf4060 Author: Bonnie Zhou <[email protected]> Date: Thu Mar 1 16:38:41 2018 -0500 feat(chips): Change chip color when selected (#2329) BREAKING CHANGE: The `mdc-chip--activated` class, `mdc-chip-activated-ink-color` Sass mixin, and the `toggleActive` methods on `MDCChip`/`MDCChipSet` have been renamed to `mdc-chip--selected`, `mdc-chip-selected-ink-color`, and `toggleSelected`, respectively. commit 4b24b51 Author: Matty Goo <[email protected]> Date: Wed Feb 28 15:20:19 2018 -0500 chore(floating-label): separate label module from text-field (#2237) BREAKING CHANGE: must use `.mdc-floating-label` selector instead of `.mdc-text-field__label` commit fd8d8d9 Author: Will Ernest <[email protected]> Date: Wed Feb 28 14:21:55 2018 -0500 feat(top-app-bar): Implement short top app bar (#2290) Adds the short top app bar variant to the top app bar. commit a9f9bf2 Author: Kenneth G. Franqueiro <[email protected]> Date: Wed Feb 28 13:06:41 2018 -0500 docs(select): Fix front matter for label sub-component (#2323)

Matt Goo added 6 commits February 5, 2018 09:57

fix(text-field): demo pages on text-field using foundation api to set…

a997ada

… required

Merge branch 'master' into fix/text-field/validation-behave-like-html5

e17f21b

fix(text-field): removed check validity from setRequired in foundation

2a92713

fix(text-field): fixed foundation test #setRequired

3bc98ee

fix(text-field): undo demo var rename

09932f8

Merge branch 'master' into fix/text-field/validation-behave-like-html5

0246b7b

lynnmercier requested a review from kfranqueiro February 8, 2018 19:11

lynnmercier assigned kfranqueiro Feb 8, 2018

kfranqueiro suggested changes Feb 8, 2018

View reviewed changes

Matty Goo added 2 commits February 8, 2018 13:04

Merge branch 'master' into fix/text-field/validation-behave-like-html5

36f19d9

fix(text-field): added set/get/remove validationAttribute

10acedd

BREAKING CHANGE: removed setRequired and isRequired from foundation. Removed get/set required from component.

Matt Goo added 4 commits February 9, 2018 16:34

fix(text-field): update demo page code

411f0bf

fix(text-field): fixed up demo js

a05d689

fix(text-field): closure annotations update

f96970c

Merge branch 'master' into fix/text-field/validation-behave-like-html5

d123345

kfranqueiro reviewed Feb 13, 2018

View reviewed changes

Matty Goo added 12 commits February 13, 2018 10:56

Merge branch 'master' into fix/text-field/validation-behave-like-html5

9f0a3cb

Merge branch 'master' into fix/text-field/validation-behave-like-html5

b669e1b

WIP - closure compilier tests

a14c7a6

Merge branch 'fix/text-field/validation-behave-like-html5' of github.…

8e6691a

…com:material-components/material-components-web into fix/text-field/validation-behave-like-html5

fix(text-field): using mutation observer to style validity

228c92d

WIP - tests

7170f69

Merge branch 'master' into fix/text-field/validation-behave-like-html5

c3b01a8

fix(text-field): updated closure annotations

9c64e18

Merge branch 'fix/text-field/validation-behave-like-html5' of github.…

326bb15

…com:material-components/material-components-web into fix/text-field/validation-behave-like-html5

fix(text-field): test syntax updates

6080b21

BREAKING CHANGES: removed get/set required from MDCTextField. Should get/set from DOM input element.

refactor(text-field): inlined callback methods in tests

89f8dd0

Merge branch 'master' into fix/text-field/validation-behave-like-html5

9114639

Matt Goo added 2 commits February 23, 2018 11:27

fix(text-field): demo helper text and typos

094fae8

fix(text-field): remove extra line

f6035ea

BREAKING CHANGE: removed isRequired and setRequired from text-field foundation

kfranqueiro suggested changes Feb 23, 2018

View reviewed changes

fix(text-field): fixed demo page helper text hide/show

a79696d

kfranqueiro reviewed Feb 26, 2018

View reviewed changes

Merge branch 'master' into fix/text-field/validation-behave-like-html5

83f5158

Matt Goo added 2 commits February 26, 2018 22:44

fix(text-field): updated text-field box demo to be consistent with to…

28e25af

…p text-field

Merge branch 'master' into fix/text-field/validation-behave-like-html5

4f3e68e

kfranqueiro reviewed Feb 27, 2018

View reviewed changes

Matt Goo added 9 commits February 28, 2018 11:38

fix(text-field): changed deregisterValidationAttributeChangeHanlder t…

1d61de0

…o accept observer

Merge branch 'master' into fix/text-field/validation-behave-like-html5

f1b3b0e

Merge branch 'master' into fix/text-field/validation-behave-like-html5

af8536a

fix(text-field): update deregisterValidationAttributeChangeHandler cl…

849dbc9

…osure docs

fix(text-field): updated closure annotations

98917b5

Merge branch 'fix/text-field/validation-behave-like-html5' of github.…

a3328db

…com:material-components/material-components-web into fix/text-field/validation-behave-like-html5

Merge branch 'master' into fix/text-field/validation-behave-like-html5

986f27a

fix(text-field): updated closure annotations

84f25c5

Merge branch 'fix/text-field/validation-behave-like-html5' of github.…

d774e15

…com:material-components/material-components-web into fix/text-field/validation-behave-like-html5

kfranqueiro suggested changes Mar 1, 2018

View reviewed changes

Matt Goo added 2 commits March 1, 2018 15:48

Merge branch 'master' into fix/text-field/validation-behave-like-html5

5822c6b

fix(text-field): update foundation and adapter closure annotations

bc2b1eb

kfranqueiro approved these changes Mar 2, 2018

View reviewed changes

Merge branch 'master' into fix/text-field/validation-behave-like-html5

fe82837

moog16 merged commit 0ba7d10 into master Mar 2, 2018

moog16 deleted the fix/text-field/validation-behave-like-html5 branch March 2, 2018 16:33

kfranqueiro mentioned this pull request Sep 6, 2018

MDCTextfield: isRequired() and setRequired() do not exist anymore #3525

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(text-field): disable validation check in setRequired #2201

fix(text-field): disable validation check in setRequired #2201

moog16 commented Feb 6, 2018 •

edited

Loading

codecov-io commented Feb 6, 2018 •

edited

Loading

kfranqueiro Feb 8, 2018

moog16 Feb 8, 2018

kfranqueiro Feb 8, 2018

moog16 Feb 8, 2018

kfranqueiro Feb 8, 2018

moog16 Feb 9, 2018

kfranqueiro Feb 9, 2018

moog16 Feb 9, 2018 •

edited

Loading

kfranqueiro Feb 9, 2018

kfranqueiro Feb 8, 2018

moog16 commented Feb 10, 2018 •

edited

Loading

kfranqueiro Feb 13, 2018

moog16 Feb 13, 2018

kfranqueiro Feb 13, 2018

kfranqueiro Feb 23, 2018

kfranqueiro Feb 23, 2018

kfranqueiro commented Feb 23, 2018

moog16 commented Feb 23, 2018

kfranqueiro left a comment

moog16 commented Feb 27, 2018

kfranqueiro Feb 27, 2018

kfranqueiro Mar 1, 2018

kfranqueiro Mar 1, 2018

kfranqueiro Mar 1, 2018

fix(text-field): disable validation check in setRequired #2201

fix(text-field): disable validation check in setRequired #2201

Conversation

moog16 commented Feb 6, 2018 • edited Loading

codecov-io commented Feb 6, 2018 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

moog16 Feb 9, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

moog16 commented Feb 10, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kfranqueiro commented Feb 23, 2018

moog16 commented Feb 23, 2018

kfranqueiro left a comment

Choose a reason for hiding this comment

moog16 commented Feb 27, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

moog16 commented Feb 6, 2018 •

edited

Loading

codecov-io commented Feb 6, 2018 •

edited

Loading

moog16 Feb 9, 2018 •

edited

Loading

moog16 commented Feb 10, 2018 •

edited

Loading