fix(cdk/a11y): Fix the touch/program origin regression introduced in the recent FocusMonitor refactor. #22754
Conversation
…the recent FocusMonitor refactor.
google-cla
bot
added
the
cla: yes
| @@ -493,7 +520,7 @@ export class FocusMonitor implements OnDestroy { | |||
| } | |||
|
|
|||
| /** Gets the target of an event, accounting for Shadow DOM. */ | |||
| function getTarget(event: Event): HTMLElement|null { | |||
| export function getTarget(event: Event): HTMLElement|null { | |||
There was a problem hiding this comment.
With this change, this file and the input-modality-detector.ts file both import from one another. I'd like to avoid this if possible. Should I move this small helper to a separate file? If so, where's a good place? @crisbeto
There was a problem hiding this comment.
I ended up moving getTarget to input-modality-detector.ts to resolve the circular dependencies (apparently there's some circular deps golden check), but it still doesn't feel like the right place for this small helper to live.
| @@ -100,6 +101,12 @@ export class InputModalityDetector implements OnDestroy { | |||
| return this._modality.value; | |||
| } | |||
|
|
|||
| /** | |||
| * The most recently detected input modality event target. Is null if no input modality has been | |||
| * detected or if the associated event target is null for some unknown reason. | |||
There was a problem hiding this comment.
I'm not sure in what case the event target would be null, but apparently it's possible (at least per the types).
| * @param focusEventTarget The target of the focus event under examination. | ||
| */ | ||
| private _shouldBeAttributedToTouch(focusEventTarget: HTMLElement | null): boolean { | ||
| // Please note that this check is not perfect. Consider the following edge case: |
There was a problem hiding this comment.
Essentially copied from
.
jelbourn
added
merge safe
target: feature
…the recent FocusMonitor refactor. (#22754) * fix(cdk/a11y): Fix the touch/program origin regression introduced in the recent FocusMonitor refactor.
…the recent FocusMonitor refactor. (#22754) * fix(cdk/a11y): Fix the touch/program origin regression introduced in the recent FocusMonitor refactor.
|
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
#22489 purposefully introduced a regression into
FocusMonitorwhen touch events are quickly followed by programmatic focus events (read the PR for more details). It was thought this regression was fine because (1) it allowed us to remove a bunch of specialized touch logic fromFocusMonitor, (2) it allows us to easily integrateInputModalityDetector, and (3) we didn't believe that mixing up touch/program attribution was that big of a deal, as not many consumers would style things differently between the two cases.It turns out that many components display focus overlays on program focus and not on touch focus (these are styles that are built-in to AM). This regression thus led to failures when testing in g3 where tests would open popups via touch that programmatically focus their first focusable element. Given this, I decided to just go ahead and fix the regression. In reviewing this PR, it may help to review how the pre-
InputModalityDetectorFocusMonitorhandled touch events (this implementation is largely similar but has some minor differences).This PR also fixes a subtle bug in how the pre-
InputModalityDetectorimplementation handled programmatic focus events that quickly follow touch events in EVENTUAL mode. This Stackblitz demonstrates and describes the bug: https://stackblitz.com/edit/angular-ivy-syzr9m. Additionally, it fixes an even subtler bug with touch-triggered programmatic focus events again in EVENTUAL mode, but I won't bother spinning up a demo unless people are curious.Note: I also already ran a TPG for this PR on top of the rest of the input modality work and all looks good.