Skip to content

Commit

Permalink
fix: forms correctly focus the first invalid control instead of last
Browse files Browse the repository at this point in the history
Previously all text fields would focus themselves when the form reports validity, meaning the last one got focus. In reality, reportValidity is supposed to focus the first invalid control.

I added a "call" method wrapper around the `onReportValidity` callback that handles focusing logic.

PiperOrigin-RevId: 597904790
  • Loading branch information
asyncLiz authored and copybara-github committed Jan 12, 2024
1 parent 3151fd8 commit 7dd7a68
Show file tree
Hide file tree
Showing 4 changed files with 163 additions and 26 deletions.
83 changes: 77 additions & 6 deletions labs/behaviors/on-report-validity.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
import {LitElement, isServer} from 'lit';

import {ConstraintValidation} from './constraint-validation.js';
import {WithElementInternals, internals} from './element-internals.js';
import {MixinBase, MixinReturn} from './mixin.js';

/**
Expand Down Expand Up @@ -47,6 +48,8 @@ export const onReportValidity = Symbol('onReportValidity');
// Private symbol members, used to avoid name clashing.
const privateCleanupFormListeners = Symbol('privateCleanupFormListeners');
const privateDoNotReportInvalid = Symbol('privateDoNotReportInvalid');
const privateIsSelfReportingValidity = Symbol('privateIsSelfReportingValidity');
const privateCallOnReportValidity = Symbol('privateCallOnReportValidity');

/**
* Mixes in a callback for constraint validation when validity should be
Expand Down Expand Up @@ -81,7 +84,7 @@ const privateDoNotReportInvalid = Symbol('privateDoNotReportInvalid');
* @return The provided class with `OnReportValidity` mixed in.
*/
export function mixinOnReportValidity<
T extends MixinBase<LitElement & ConstraintValidation>,
T extends MixinBase<LitElement & ConstraintValidation & WithElementInternals>,
>(base: T): MixinReturn<T, OnReportValidity> {
abstract class OnReportValidityElement
extends base
Expand All @@ -98,6 +101,13 @@ export function mixinOnReportValidity<
*/
[privateDoNotReportInvalid] = false;

/**
* Used to determine if the control is reporting validity from itself, or
* if a `<form>` is causing the validity report. Forms have different
* control focusing behavior.
*/
[privateIsSelfReportingValidity] = false;

// Mixins must have a constructor with `...args: any[]`
// tslint:disable-next-line:no-any
constructor(...args: any[]) {
Expand All @@ -124,9 +134,7 @@ export function mixinOnReportValidity<
// A normal bubbling phase event listener. By adding it here, we
// ensure it's the last event listener that is called during the
// bubbling phase.
if (!invalidEvent.defaultPrevented) {
this[onReportValidity](invalidEvent);
}
this[privateCallOnReportValidity](invalidEvent);
},
{once: true},
);
Expand All @@ -149,15 +157,50 @@ export function mixinOnReportValidity<
}

override reportValidity() {
this[privateIsSelfReportingValidity] = true;
const valid = super.reportValidity();
// Constructor's invalid listener will handle reporting invalid events.
if (valid) {
this[onReportValidity](null);
this[privateCallOnReportValidity](null);
}

this[privateIsSelfReportingValidity] = false;
return valid;
}

[privateCallOnReportValidity](invalidEvent: Event | null) {
// Since invalid events do not bubble to parent listeners, and because
// our invalid listeners are added lazily after other listeners, we can
// reliably read `defaultPrevented` synchronously without worrying
// about waiting for another listener that could cancel it.
const wasCanceled = invalidEvent?.defaultPrevented;
if (wasCanceled) {
return;
}

this[onReportValidity](invalidEvent);

// If an implementation calls invalidEvent.preventDefault() to stop the
// platform popup from displaying, focusing is also prevented, so we need
// to manually focus.
const implementationCanceledFocus =
!wasCanceled && invalidEvent?.defaultPrevented;
if (!implementationCanceledFocus) {
return;
}

// The control should be focused when:
// - `control.reportValidity()` is called (self-reporting).
// - a form is reporting validity for its controls and this is the first
// invalid control.
if (
this[privateIsSelfReportingValidity] ||
isFirstInvalidControlInForm(this[internals].form, this)
) {
this.focus();
}
}

[onReportValidity](invalidEvent: Event | null) {
throw new Error('Implement [onReportValidity]');
}
Expand Down Expand Up @@ -185,7 +228,7 @@ export function mixinOnReportValidity<
this,
form,
() => {
this[onReportValidity](null);
this[privateCallOnReportValidity](null);
},
this[privateCleanupFormListeners].signal,
);
Expand Down Expand Up @@ -328,3 +371,31 @@ function getFormValidateHooks(form: HTMLFormElement) {

return FORM_VALIDATE_HOOKS.get(form)!;
}

/**
* Checks if a control is the first invalid control in a form.
*
* @param form The control's form. When `null`, the control doesn't have a form
* and the method returns true.
* @param control The control to check.
* @return True if there is no form or if the control is the form's first
* invalid control.
*/
function isFirstInvalidControlInForm(
form: HTMLFormElement | null,
control: HTMLElement,
) {
if (!form) {
return true;
}

let firstInvalidControl: Element | undefined;
for (const element of form.elements) {
if (element.matches(':invalid')) {
firstInvalidControl = element;
break;
}
}

return firstInvalidControl === control;
}
82 changes: 82 additions & 0 deletions labs/behaviors/on-report-validity_test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -453,5 +453,87 @@ describe('mixinOnReportValidity()', () => {
.toHaveBeenCalledTimes(1);
});
});

describe('focusing after preventing platform popup', () => {
it('should focus the control when calling reportValidity()', () => {
const control = new TestOnReportValidity();
control[onReportValidity] = (invalidEvent: Event | null) => {
invalidEvent?.preventDefault();
};

spyOn(control, 'focus');

control.required = true;
control.reportValidity();
expect(control.focus)
.withContext('is focused')
.toHaveBeenCalledTimes(1);
});

it('should only focus the first invalid control of a form', () => {
const firstControl = new TestOnReportValidity();
firstControl[onReportValidity] = (invalidEvent: Event | null) => {
invalidEvent?.preventDefault();
};

const secondControl = new TestOnReportValidity();
secondControl[onReportValidity] = (invalidEvent: Event | null) => {
invalidEvent?.preventDefault();
};

spyOn(firstControl, 'focus');
spyOn(secondControl, 'focus');

const form = document.createElement('form');
form.appendChild(firstControl);
form.appendChild(secondControl);
document.body.appendChild(form);

firstControl.required = true;
secondControl.required = true;
form.reportValidity();
form.remove();

expect(firstControl.focus)
.withContext('first control (invalid) is focused')
.toHaveBeenCalledTimes(1);
expect(secondControl.focus)
.withContext('second control (invalid) is not focused')
.not.toHaveBeenCalled();
});

it('should focus the control when calling control.reportValidity(), even if not the first invalid control of a form', () => {
const firstControl = new TestOnReportValidity();
firstControl[onReportValidity] = (invalidEvent: Event | null) => {
invalidEvent?.preventDefault();
};

const secondControl = new TestOnReportValidity();
secondControl[onReportValidity] = (invalidEvent: Event | null) => {
invalidEvent?.preventDefault();
};

spyOn(firstControl, 'focus');
spyOn(secondControl, 'focus');

const form = document.createElement('form');
form.appendChild(firstControl);
form.appendChild(secondControl);
document.body.appendChild(form);

firstControl.required = true;
secondControl.required = true;
secondControl.reportValidity();

expect(firstControl.focus)
.withContext('first control (invalid) is not focused')
.not.toHaveBeenCalled();
expect(secondControl.focus)
.withContext(
'second control (invalid, called reportValidity()) is focused',
)
.toHaveBeenCalledTimes(1);
});
});
});
});
12 changes: 2 additions & 10 deletions select/internal/select.ts
Original file line number Diff line number Diff line change
Expand Up @@ -308,16 +308,8 @@ export abstract class Select extends selectBaseClass {
}

[onReportValidity](invalidEvent: Event | null) {
if (invalidEvent?.defaultPrevented) {
return;
}

if (invalidEvent) {
// Prevent default pop-up behavior. This also prevents focusing, so we
// manually focus.
invalidEvent.preventDefault();
this.focus();
}
// Prevent default pop-up behavior.
invalidEvent?.preventDefault();

const prevMessage = this.getErrorText();
this.nativeError = !!invalidEvent;
Expand Down
12 changes: 2 additions & 10 deletions textfield/internal/text-field.ts
Original file line number Diff line number Diff line change
Expand Up @@ -772,16 +772,8 @@ export abstract class TextField extends textFieldBaseClass {
}

[onReportValidity](invalidEvent: Event | null) {
if (invalidEvent?.defaultPrevented) {
return;
}

if (invalidEvent) {
// Prevent default pop-up behavior. This also prevents focusing, so we
// manually focus.
invalidEvent.preventDefault();
this.focus();
}
// Prevent default pop-up behavior.
invalidEvent?.preventDefault();

const prevMessage = this.getErrorText();
this.nativeError = !!invalidEvent;
Expand Down

0 comments on commit 7dd7a68

Please sign in to comment.