Skip to content
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

Add a .user-valid and .user-invalid to inputs, fieldsets and forms #3823

Merged
merged 8 commits into from Aug 2, 2016

Conversation

mkhatib
Copy link
Contributor

@mkhatib mkhatib commented Jun 29, 2016

This would allow publishers to better handling and direct users attention when the fields they're interacting with become valid/invalid. As opposed to the :valid and :invalid pseudo classes which get applied on the document load.

See :user-invalid spec and :-moz-ui-invalid - I don't try to match Mozilla prefixed. Instead I apply these classes when user interacts with the inputs.

ITI: #3343

Play around with the demo here to see how this works. I over-did the styling to showcase the different hooks at each level. Note that you might have to wait 2 minutes for the heroku instance to start.

@@ -95,6 +110,11 @@ export class AmpForm {
/** @private */
installSubmitHandler_() {
this.form_.addEventListener('submit', e => this.handleSubmit_(e));
// Add 'focus' event on all inputs.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

drop comment.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@mkhatib
Copy link
Contributor Author

mkhatib commented Jun 29, 2016

Play around with the demo here to see how this works. I over-did the styling to showcase the different hooks at each level. Note that you might have to wait 2 minutes for the heroku instance to start.

const ancestors = [];
for (let ancestor = child.parentElement; ancestor;
ancestor = ancestor.parentElement) {
if (callback(ancestor)) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

per previous discussions, please call this predicate

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

@mkhatib
Copy link
Contributor Author

mkhatib commented Jun 29, 2016

PTAL

element.classList.add('user-valid');
element.classList.remove('user-invalid');
// Don't propagate user-valid if this was unmarked before.
return previousValidityState != UserValidityState.NONE;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We already know it's not USER_VALID, and we're testing that it's not NONE, so the only option left is USER_INVALID. Let's use == USER_INVALID instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

@mkhatib mkhatib changed the title WIP: Add a .user-valid and .user-invalid to inputs, fieldsets and forms Add a .user-valid and .user-invalid to inputs, fieldsets and forms Jul 2, 2016
@mkhatib
Copy link
Contributor Author

mkhatib commented Jul 2, 2016

Thanks @jridgewell for the reviews! Added tests and addressed the comments. PTAL.

@mkhatib
Copy link
Contributor Author

mkhatib commented Jul 7, 2016

Let me know if there are other comments on this PR.

*/
function isReportValiditySupported() {
if (reportValiditySupported === undefined) {
reportValiditySupported = !!document.createElement('form').reportValidity;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can't reference the global document.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

@@ -15,7 +15,12 @@
*/

import {createIframePromise} from '../../../../testing/iframe';
import {AmpForm, installAmpForm} from '../amp-form';
import {
AmpForm,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

-2sp

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

@mkhatib
Copy link
Contributor Author

mkhatib commented Aug 1, 2016

Addressed comments. PTAL

@jridgewell
Copy link
Contributor

LGTM.

@mkhatib mkhatib merged commit c340fb3 into ampproject:master Aug 2, 2016
@mkhatib mkhatib deleted the input-user-invalid branch August 2, 2016 20:30
@devvercer
Copy link
Contributor

This does not seem to update the fieldset with user-invalid in my test page (or my real pages)

https://codepen.io/vercer/pen/aLRKpd

This sets up a simple form > fieldset > input. When the single input is invalid, the input and form are marked user-invalid but the fieldset is marked user-valid.

@jridgewell
Copy link
Contributor

Hi @devvercer: Can you file a new issue?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants