-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
fix(text-field): disable validation check in setRequired #2201
Changes from 12 commits
a997ada
e17f21b
2a92713
3bc98ee
09932f8
0246b7b
36f19d9
10acedd
411f0bf
a05d689
f96970c
d123345
9f0a3cb
b669e1b
a14c7a6
8e6691a
228c92d
7170f69
c3b01a8
9c64e18
326bb15
6080b21
89f8dd0
9114639
fec58f1
8dff2fe
479a875
ede9868
02a0ab4
400d7d2
dcada20
83adc37
58e1f6f
218cc3c
9d5c10c
c22f731
d177047
05e8c3f
52f282d
143c34b
094fae8
f6035ea
a79696d
83f5158
28e25af
4f3e68e
1d61de0
f1b3b0e
af8536a
849dbc9
98917b5
a3328db
986f27a
84f25c5
d774e15
5822c6b
bc2b1eb
fe82837
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 type="text" id="tf-outlined-input" class="mdc-text-field__input" | ||
aria-controls="name-validation-message"> | ||
<label for="tf-outlined-input" class="mdc-text-field__label">Your Name</label> | ||
<div class="mdc-text-field__outline"> | ||
|
@@ -149,8 +149,8 @@ <h2>Outlined Text Field</h2> | |
</div> | ||
<div class="mdc-text-field__idle-outline"></div> | ||
</div> | ||
<p class="mdc-text-field-helper-text mdc-text-field-helper-text--validation-msg"> | ||
Must be at least 8 characters | ||
<p class="mdc-text-field-helper-text mdc-text-field-helper-text--validation-msg" id="tf-outlined-validation-msg"> | ||
Helper Text (possibly validation message) | ||
</p> | ||
</div> | ||
<div> | ||
|
@@ -169,6 +169,14 @@ <h2>Outlined Text Field</h2> | |
<input id="outlined-dense" type="checkbox"> | ||
<label for="outlined-dense">Dense</label> | ||
</div> | ||
<div> | ||
<input id="outlined-required" type="checkbox"> | ||
<label for="outlined-required">Required</label> | ||
</div> | ||
<div> | ||
<input id="outlined-pattern" type="checkbox"> | ||
<label for="outlined-pattern">Must be at least 8 characters</label> | ||
</div> | ||
</section> | ||
|
||
<section class="example" id="text-field-box-example"> | ||
|
@@ -203,7 +211,11 @@ <h2>Text Field box</h2> | |
</div> | ||
<div> | ||
<input id="box-required" type="checkbox"> | ||
<label for="box-required">Required and must be at least 8 characters</label> | ||
<label for="box-required">Required</label> | ||
</div> | ||
<div> | ||
<input id="box-pattern" type="checkbox"> | ||
<label for="box-pattern">Must be at least 8 characters</label> | ||
</div> | ||
<div> | ||
<input id="box-use-helper-text" type="checkbox"> | ||
|
@@ -285,7 +297,11 @@ <h2>Text Field - Leading/Trailing icons</h2> | |
</div> | ||
<div> | ||
<input id="required-leading-trailing" type="checkbox"> | ||
<label for="required-leading-trailing">Required and must be at least 8 characters</label> | ||
<label for="required-leading-trailing">Required</label> | ||
</div> | ||
<div> | ||
<input id="pattern-leading-trailing" type="checkbox"> | ||
<label for="pattern-leading-trailing">Must be at least 8 characters</label> | ||
</div> | ||
</section> | ||
|
||
|
@@ -359,8 +375,17 @@ <h2>Full-Width Text Field and Textarea</h2> | |
|
||
<script src="/assets/material-components-web.js" async></script> | ||
<script> | ||
function toggleRequired(textfield, isRequired) { | ||
if (isRequired) { | ||
textfield.setValidationAttribute('required', 'true'); | ||
} else { | ||
textfield.removeValidationAttribute('required'); | ||
} | ||
} | ||
|
||
demoReady(function() { | ||
var tfEl = document.getElementById('tf-outlined-example'); | ||
var tfHelperText = document.getElementById('tf-outlined-validation-msg'); | ||
var tf = new mdc.textField.MDCTextField(tfEl); | ||
var wrapper = document.getElementById('demo-tf-outlined-wrapper'); | ||
document.getElementById('outlined-disable').addEventListener('change', function(evt) { | ||
|
@@ -382,6 +407,21 @@ <h2>Full-Width Text Field and Textarea</h2> | |
tfEl.classList[evt.target.checked ? 'add' : 'remove']('mdc-text-field--dense'); | ||
tf.layout(); | ||
}); | ||
document.getElementById('outlined-required').addEventListener('change', function(evt) { | ||
toggleRequired(tf, evt.target.checked); | ||
}); | ||
document.getElementById('outlined-pattern').addEventListener('change', function(evt) { | ||
var checked = evt.target.checked; | ||
var plainHelperText = 'Helper Text (possibly validation message)'; | ||
var requiredHelperText = 'Must be at least 8 characters';requiredHelperText | ||
if (checked) { | ||
tf.setValidationAttribute('minlength', 8); | ||
tfHelperText.innerHTML = requiredHelperText; | ||
} else { | ||
tf.removeValidationAttribute('minlength'); | ||
tfHelperText.innerHTML = plainHelperText; | ||
} | ||
}); | ||
}); | ||
|
||
demoReady(function() { | ||
|
@@ -415,13 +455,18 @@ <h2>Full-Width Text Field and Textarea</h2> | |
}); | ||
|
||
document.getElementById('box-required').addEventListener('change', function(evt) { | ||
var target = evt.target; | ||
var input = tfEl.querySelector('.mdc-text-field__input'); | ||
toggleRequired(tf, evt.target.checked); | ||
}); | ||
document.getElementById('box-pattern').addEventListener('change', function(evt) { | ||
var checked = evt.target.checked; | ||
var requiredHelperText = 'Must be at least 8 characters'; | ||
var plainHelperText = 'Helper Text (possibly validation message)'; | ||
input.required = target.checked; | ||
input.pattern = evt.target.checked ? '.{8,}' : '.*'; | ||
helperText.innerHTML = target.checked ? requiredHelperText : plainHelperText; | ||
if (checked) { | ||
tf.setValidationAttribute('pattern', '.{8,}'); | ||
} else { | ||
tf.setValidationAttribute('pattern', '.*'); | ||
} | ||
helperText.innerHTML = checked ? requiredHelperText : plainHelperText; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Again, use the |
||
}); | ||
|
||
document.getElementById('box-use-helper-text').addEventListener('change', function(evt) { | ||
|
@@ -512,10 +557,26 @@ <h2>Full-Width Text Field and Textarea</h2> | |
}); | ||
|
||
document.getElementById('required-leading-trailing').addEventListener('change', function(evt) { | ||
[].slice.call(tfInputs).forEach(function(input) { | ||
input.required = evt.target.checked; | ||
input.pattern = evt.target.checked ? '.{8,}' : '.*'; | ||
}); | ||
var checked = evt.target.checked; | ||
toggleRequired(tfBoxLeading, checked); | ||
toggleRequired(tfBoxTrailing, checked); | ||
toggleRequired(tfOutlinedLeading, checked); | ||
toggleRequired(tfOutlinedTrailing, checked); | ||
}); | ||
|
||
document.getElementById('pattern-leading-trailing').addEventListener('change', function(evt) { | ||
var checked = evt.target.checked; | ||
if (checked) { | ||
tfBoxLeading.setValidationAttribute('pattern', '.{8,}'); | ||
tfBoxTrailing.setValidationAttribute('pattern', '.{8,}'); | ||
tfOutlinedLeading.setValidationAttribute('pattern', '.{8,}'); | ||
tfOutlinedTrailing.setValidationAttribute('pattern', '.{8,}'); | ||
} else { | ||
tfBoxLeading.setValidationAttribute('pattern', '.*'); | ||
tfBoxTrailing.setValidationAttribute('pattern', '.*'); | ||
tfOutlinedLeading.setValidationAttribute('pattern', '.*'); | ||
tfOutlinedTrailing.setValidationAttribute('pattern', '.*'); | ||
} | ||
}); | ||
|
||
document.getElementById('leading-trailing-alternate-colors').addEventListener('change', function(evt) { | ||
|
@@ -557,8 +618,7 @@ <h2>Full-Width Text Field and Textarea</h2> | |
tfRoot.classList[target.checked ? 'add' : 'remove']('mdc-text-field--dense'); | ||
}); | ||
document.getElementById('required').addEventListener('change', function(evt) { | ||
var target = evt.target; | ||
tfRoot.querySelector('.mdc-text-field__input').required = target.checked; | ||
toggleRequired(tf, evt.target.checked); | ||
}); | ||
document.getElementById('alternate-colors').addEventListener('change', function (evt) { | ||
var target = evt.target; | ||
|
@@ -606,13 +666,8 @@ <h2>Full-Width Text Field and Textarea</h2> | |
var target = evt.target; | ||
tfRoot.classList[target.checked ? 'add' : 'remove']('demo-textarea'); | ||
}); | ||
|
||
document.getElementById('textarea-required').addEventListener('change', function(evt) { | ||
var target = evt.target; | ||
[].slice.call(tfRoot.querySelectorAll('.mdc-text-field__input')) | ||
.forEach(function(input) { | ||
input.required = target.checked; | ||
}) | ||
toggleRequired(tf, evt.target.checked); | ||
}); | ||
}); | ||
|
||
|
@@ -636,13 +691,10 @@ <h2>Full-Width Text Field and Textarea</h2> | |
el.classList[target.checked ? 'add' : 'remove']('mdc-text-field--dense'); | ||
}); | ||
}); | ||
|
||
document.getElementById('fullwidth-required').addEventListener('change', function(evt) { | ||
var target = evt.target; | ||
[].slice.call(section.querySelectorAll('.mdc-text-field__input')) | ||
.forEach(function(input) { | ||
input.required = target.checked; | ||
}) | ||
var checked = evt.target.checked; | ||
toggleRequired(tf, evt.target.checked); | ||
toggleRequired(tfMulti, evt.target.checked); | ||
}); | ||
|
||
document.getElementById('fullwidth-alternate-colors').addEventListener('change', function (evt) { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -290,20 +290,32 @@ class MDCTextFieldFoundation extends MDCFoundation { | |
} | ||
|
||
/** | ||
* @return {boolean} True if the Text Field is required. | ||
* @return {string} the attribute value from the input element | ||
* @param {string} attrName is the name of the attribute on the input element | ||
*/ | ||
isRequired() { | ||
return this.getNativeInput_().required; | ||
getValidationAttribute(attrName) { | ||
return this.getNativeInput_().getAttribute(attrName); | ||
} | ||
|
||
/** | ||
* @param {boolean} isRequired Sets the text-field required or not. | ||
* @param {string} attrName is the name of the attribute to be set | ||
* @param {string} val is the value of attribute to be set | ||
*/ | ||
setRequired(isRequired) { | ||
this.getNativeInput_().required = isRequired; | ||
// Addition of the asterisk is automatic based on CSS, but validity checking | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
// needs to be manually run. | ||
this.styleValidity_(this.isValid()); | ||
setValidationAttribute(attrName, val) { | ||
this.getNativeInput_().setAttribute(attrName, val); | ||
// Setting validation to true even if input is incorrect. | ||
// If developer toggles a validation property (required, minlength, etc) | ||
// the user should have a chance interact with the input before it being | ||
// invalid again. | ||
this.styleValidity_(true); | ||
} | ||
|
||
/** | ||
* @param {string} attrName is the name of the attribute to remove from input element | ||
*/ | ||
removeValidationAttribute(attrName) { | ||
this.getNativeInput_().removeAttribute(attrName); | ||
this.styleValidity_(true); | ||
} | ||
|
||
/** | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -203,17 +203,26 @@ class MDCTextField extends MDCComponent { | |
} | ||
|
||
/** | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think we should remove these, but rather make them directly proxy to the DOM node which the instance already has a reference to. Ditto for pattern / minlength / maxlength. Then we can simply reference the components in the demo page; we should never have to dig into the component's DOM structure there. |
||
* @return {boolean} True if the Text Field is required. | ||
* @return {string} the attribute value from the input element | ||
* @param {string} attrName is the name of the attribute on the input element | ||
*/ | ||
get required() { | ||
return this.foundation_.isRequired(); | ||
getValidationAttribute(attrName) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would still expect this to be exposed via There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. are you saying keep There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, that's what I was thinking, and yes to min/maxlength too. |
||
return this.foundation_.getValidationAttribute(attrName); | ||
} | ||
|
||
/** | ||
* @param {boolean} required Sets the Text Field to required. | ||
* @param {string} attrName is the attribute name to be set | ||
* @param {string} val is the value of attribute to be set | ||
*/ | ||
set required(required) { | ||
this.foundation_.setRequired(required); | ||
setValidationAttribute(attrName, val) { | ||
this.foundation_.setValidationAttribute(attrName, val); | ||
} | ||
|
||
/** | ||
* @param {string} attrName is the name of the attribute to remove from input element | ||
*/ | ||
removeValidationAttribute(attrName) { | ||
this.foundation_.removeValidationAttribute(attrName); | ||
} | ||
|
||
/** | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -202,29 +202,47 @@ test('#setValid updates classes', () => { | |
td.verify(mockAdapter.removeClass(cssClasses.DISABLED), {times: 0}); | ||
}); | ||
|
||
test('#setRequired updates CSS classes', () => { | ||
test('#setValidationAttribute required to true updates CSS classes', () => { | ||
// Native validity checking does not apply in unittests, so manually mark as valid or invalid. | ||
const {foundation, mockAdapter, nativeInput, helperText} = | ||
const {foundation, mockAdapter, helperText} = | ||
setupValueTest('', /* isValid */ false); | ||
|
||
foundation.setRequired(true); | ||
assert.isOk(foundation.isRequired()); | ||
td.verify(mockAdapter.addClass(cssClasses.INVALID)); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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). |
||
td.verify(helperText.setValidity(false)); | ||
td.when(mockAdapter.getNativeInput()).thenReturn({setAttribute: () => null}); | ||
foundation.setValidationAttribute('required', 'true'); | ||
td.verify(mockAdapter.removeClass(cssClasses.INVALID)); | ||
td.verify(helperText.setValidity(true)); | ||
|
||
nativeInput.validity.valid = true; | ||
foundation.setRequired(false); | ||
assert.isNotOk(foundation.isRequired()); | ||
// None of these is affected by setValidationAttribute. | ||
td.verify(mockAdapter.addClass(cssClasses.FOCUSED), {times: 0}); | ||
td.verify(mockAdapter.removeClass(cssClasses.FOCUSED), {times: 0}); | ||
td.verify(mockAdapter.addClass(cssClasses.DISABLED), {times: 0}); | ||
td.verify(mockAdapter.removeClass(cssClasses.DISABLED), {times: 0}); | ||
}); | ||
|
||
test('#removeValidationAttribute required updates CSS classes', () => { | ||
const {foundation, mockAdapter, helperText} = | ||
setupValueTest('', /* isValid */ false); | ||
|
||
td.when(mockAdapter.getNativeInput()).thenReturn({removeAttribute: () => null}); | ||
foundation.removeValidationAttribute('required'); | ||
td.verify(mockAdapter.removeClass(cssClasses.INVALID)); | ||
td.verify(helperText.setValidity(true)); | ||
|
||
// None of these is affected by setRequired. | ||
// None of these is affected by setValidationAttribute. | ||
td.verify(mockAdapter.addClass(cssClasses.FOCUSED), {times: 0}); | ||
td.verify(mockAdapter.removeClass(cssClasses.FOCUSED), {times: 0}); | ||
td.verify(mockAdapter.addClass(cssClasses.DISABLED), {times: 0}); | ||
td.verify(mockAdapter.removeClass(cssClasses.DISABLED), {times: 0}); | ||
}); | ||
|
||
test('#getValidationAttribute required returns correct value', () => { | ||
const {foundation, mockAdapter} = | ||
setupValueTest('', /* isValid */ false); | ||
|
||
td.when(mockAdapter.getNativeInput()).thenReturn({getAttribute: () => true}); | ||
assert.equal(foundation.getValidationAttribute('required'), true); | ||
}); | ||
|
||
test('#setDisabled flips disabled when a native input is given', () => { | ||
const {foundation, mockAdapter} = setupTest(); | ||
const nativeInput = {disabled: false}; | ||
|
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.
Since you're always passing both arguments, shorten these 5 lines to
textfield.required = isRequired
?