fix(text-field): disable validation check in setRequired #2201
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -27,6 +27,12 @@ import MDCTextFieldOutlineFoundation from './outline/foundation'; | |
| import {cssClasses, strings, numbers} from './constants'; | ||
|
|
||
|
|
||
| // whitelist based off of https://developer.mozilla.org/en-US/docs/Web/Guide/HTML/HTML5/Constraint_validation | ||
| // under section: `Validation-related attributes` | ||
| const VALIDATION_ATTR_WHITELIST = [ | ||
| 'pattern', 'min', 'max', 'required', 'step', 'minlength', 'maxlength', | ||
| ]; | ||
|
|
||
| /** | ||
| * @extends {MDCFoundation<!MDCTextFieldAdapter>} | ||
| * @final | ||
|
|
@@ -61,6 +67,8 @@ class MDCTextFieldFoundation extends MDCFoundation { | |
| deregisterTextFieldInteractionHandler: () => {}, | ||
| registerInputInteractionHandler: () => {}, | ||
| deregisterInputInteractionHandler: () => {}, | ||
| registerValidationAttributeChangeHandler: () => {}, | ||
| deregisterValidationAttributeChangeHandler: () => {}, | ||
| getNativeInput: () => {}, | ||
| isFocused: () => {}, | ||
| isRtl: () => {}, | ||
|
|
@@ -104,6 +112,10 @@ class MDCTextFieldFoundation extends MDCFoundation { | |
| this.setPointerXOffset_ = (evt) => this.setTransformOrigin(evt); | ||
| /** @private {function(!Event): undefined} */ | ||
| this.textFieldInteractionHandler_ = () => this.handleTextFieldInteraction(); | ||
| /** @private {function(!Array): undefined} */ | ||
| this.validationAttributeChangeHandler_ = (mutations) => this.handleValidationAttributeMutation(mutations); | ||
| /** @private {!MutationObserver} */ | ||
| this.validationObserver_; | ||
| } | ||
|
|
||
| init() { | ||
|
|
@@ -127,6 +139,8 @@ class MDCTextFieldFoundation extends MDCFoundation { | |
| ['click', 'keydown'].forEach((evtType) => { | ||
| this.adapter_.registerTextFieldInteractionHandler(evtType, this.textFieldInteractionHandler_); | ||
| }); | ||
| this.validationObserver_ = this.adapter_.registerValidationAttributeChangeHandler( | ||
| this.validationAttributeChangeHandler_); | ||
| } | ||
|
|
||
| destroy() { | ||
|
|
@@ -140,6 +154,7 @@ class MDCTextFieldFoundation extends MDCFoundation { | |
| ['click', 'keydown'].forEach((evtType) => { | ||
| this.adapter_.deregisterTextFieldInteractionHandler(evtType, this.textFieldInteractionHandler_); | ||
| }); | ||
| this.adapter_.deregisterValidationAttributeChangeHandler(this.validationObserver_); | ||
| } | ||
|
|
||
| /** | ||
|
|
@@ -152,6 +167,19 @@ class MDCTextFieldFoundation extends MDCFoundation { | |
| this.receivedUserInput_ = true; | ||
| } | ||
|
|
||
| /** | ||
| * Handles validation attribute changes | ||
| * @param {Array<MutationRecord>} mutationsList | ||
| */ | ||
| handleValidationAttributeMutation(mutationsList) { | ||
|
There was a problem hiding this comment. I'm inclined to think this should be a |
||
| mutationsList.some((mutation) => { | ||
| if (VALIDATION_ATTR_WHITELIST.indexOf(mutation.attributeName) > -1) { | ||
| this.styleValidity_(true); | ||
| return true; | ||
| } | ||
| }); | ||
| } | ||
|
|
||
| /** | ||
| * Updates the focus outline for outlined text fields. | ||
| */ | ||
|
|
@@ -289,23 +317,6 @@ class MDCTextFieldFoundation extends MDCFoundation { | |
| this.styleDisabled_(disabled); | ||
| } | ||
|
|
||
|
There was a problem hiding this comment. 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. There was a problem hiding this comment. 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 This then led me to question why do we choose to manage There was a problem hiding this comment. 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 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 There was a problem hiding this comment. 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 There was a problem hiding this comment. 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. There was a problem hiding this comment. 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 There was a problem hiding this comment. 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. |
||
| /** | ||
| * @param {string} content Sets the content of the helper text. | ||
| */ | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Put
@parambefore@return