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
feat(cdk/a11y): Add a new InputModalityDetector service to detect the user's current input modality. #22371
Conversation
* The amount of time needed to pass after a touchstart event in order for a subsequent mousedown | ||
* event to be attributed as mouse and not touch. | ||
*/ | ||
export const TIME_AFTER_TOUCH_MS = 650; |
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.
This was grabbed from https://github.com/angular/components/blob/master/src/cdk/a11y/focus-monitor/focus-monitor.ts#L34 and from what I can tell they're both used for the same purpose? Should one of these usages import the other or should we move it into some shared file?
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.
Yeah, I think focus monitor should probably consume this. We should also preserve the comment about the origin of this value. However, we probably want to avoid adding this symbol to the public API.
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.
Done.
private _onKeydown = (event: KeyboardEvent) => { | ||
// If this is one of the keys we should ignore, then ignore it and don't update the input | ||
// modality to keyboard. | ||
if (this._options?.ignoreKeys?.some(key => key === event.key)) { return; } |
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.
I'd prefer to use event.key
instead of keyCode
, but if you're still targeting IE11 it looks like it has only partial support (https://caniuse.com/?search=KeyboardEvent.key), which may not be good enough (I'll need to test).
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.
Yeah, we unfortunately still use keyCode
everywhere and plan to switch to key
or code
once IE11 support is dropped (see #7374). We plan to deprecate IE11 support in v12 (this May) and fully remove it in either v13 or v14 based on community feedback.
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.
Gotcha. Switched to keyCode
.
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.
* The amount of time needed to pass after a touchstart event in order for a subsequent mousedown | ||
* event to be attributed as mouse and not touch. | ||
*/ | ||
export const TIME_AFTER_TOUCH_MS = 650; |
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.
Yeah, I think focus monitor should probably consume this. We should also preserve the comment about the origin of this value. However, we probably want to avoid adding this symbol to the public API.
* Event listener options that enable capturing and also mark the listener as passive if the browser | ||
* supports it. | ||
*/ | ||
const opts = normalizePassiveListenerOptions({ |
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.
const opts = normalizePassiveListenerOptions({ | |
const modalityEventListenerOptions = normalizePassiveListenerOptions({ |
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.
Done.
get inputModality(): InputModality { | ||
return this._inputModality; | ||
} |
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.
Any reason to make this a getter vs. making the actual property public?
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.
Only to prevent people from setting it themselves for whatever reason. Happy to change it to a public property, just LMK.
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.
Note that now the getter is more necessary because we're grabbing the value from the underlying BehaviorSubject
.
/** The underlying ReplaySubject that emits input modality changes. */ | ||
private readonly _change = new ReplaySubject<InputModality>(1); | ||
|
||
/** The current / last detected input modality by this service. */ |
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.
/** The current / last detected input modality by this service. */ | |
/** The most recently detected input modality by this service. */ |
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.
Done.
private _onKeydown = (event: KeyboardEvent) => { | ||
// If this is one of the keys we should ignore, then ignore it and don't update the input | ||
// modality to keyboard. | ||
if (this._options?.ignoreKeys?.some(key => key === event.key)) { return; } |
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.
Yeah, we unfortunately still use keyCode
everywhere and plan to switch to key
or code
once IE11 support is dropped (see #7374). We plan to deprecate IE11 support in v12 (this May) and fully remove it in either v13 or v14 based on community feedback.
* these keys so as to not update the input modality. | ||
*/ | ||
export const INPUT_MODALITY_DETECTOR_DEFAULT_OPTIONS: InputModalityDetectorOptions = { | ||
ignoreKeys: ['Alt', 'Control', 'Meta', 'Shift'], |
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.
These are all modifier keys so shouldn't we be looking at KeyboardEvent.altKey
etc.? We have some helpers for dealing with modifiers in @angular/cdk/keycodes
already.
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.
I considered that, but I wanted to give users the ability to configure which keys they'd like to ignore/not ignore, and it seemed easiest to support that via a single ignoreKeys
API.
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.
It could have a modifiers
array and ignoreKeys
. If I remember correctly, the keydown will dispatch Alt
if you just press the Alt
key once, but if you pressed Alt
+ DOWN_ARROW
, you'd get a DOWN_ARROW
event with the Alt
modifier.
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.
From my testing, if I press Alt
+ Down Arrow
, I get both an Alt
and a Down Arrow
event, so this should work fine (I tested with a number of modifier keys and on both Mac and Windows). Changing the API to both modifiers
and ignoreKeys
increases its surface error, and it's not clear to me what the benefit is (do we think it'll be clearer to users?).
} | ||
|
||
/** The underlying ReplaySubject that emits input modality changes. */ | ||
private readonly _change = new ReplaySubject<InputModality>(1); |
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.
You could replace the ReplaySubject
with a BehaviorSubject
which has identical behavior, but it also allows you to read the latest value which can then be used for the inputModality
getter.
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.
This was actually my previous implementation, but I ended switching to a ReplaySubject
because I didn't like how the BehaviorSubject
emitted null
at the start. My thinking was that inputModalityChange
should only emit when the input modality changes, and I didn't want subscribers to have to filter out the first null
value in their logic. WDYT?
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.
You could add .pipe(skip(1))
when exposing the subject publicly. It's not a bit deal to leave it as it is though.
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.
Okay, I don't feel strongly so I switched to the BehaviorSubject
.
this.inputModalityChange = this._change.pipe(distinctUntilChanged()); | ||
|
||
// Add the event listeners used to detect the user's input modality. | ||
document.addEventListener('keydown', this._onKeydown, opts); |
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.
All of these events should be bound outside of the Angular zone.
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.
Thanks, I made this change, but it's causing some problems in the dev-app demo and Stackblitz. If you take a look at the Stackblitz (I updated with the new implementation of binding these event listeners outside of the ngZone), you'll notice that the input modality no longer updates in the view (though of course the subscription still emits). Without knowing too much about zones my it seems like Angular isn't running CD because it doesn't think anything is changing. Is this expected behavior? If so, I'll need to update the dev-app demo accordingly.
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.
If the listener is bound outside of the zone, you'll have to use NgZone.run
when updating the view, otherwise change detection won't run. It's a little cumbersome, but we may end up triggering unnecessary change detections otherwise.
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.
Gotcha, done.
|
||
constructor( | ||
platform: Platform, | ||
@Inject(DOCUMENT) document: Document, |
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.
AFAIK there are some issues in ViewEngine with typing constructor parameters as Document
. The way we work around it in other places is to type the parameter as any
and then cast it to Document
afterwards.
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.
I did notice this when looking at other services. Do you recall how to reproduce these issues? I'd prefer to be able to reproduce the issue to confirm the any
+ cast is actually needed, but if you're confident, then I'll just go ahead and make the change.
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.
I think that we were running into it in our Angular Universal integration tests. There's a readme under src\universal-app\DEBUG.md
.
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.
I added the InputModalityDetector
to the universal kitchen sink app as-is, and ran bazel test //src/universal-app:server_test --test_output=all
. The target passed, and I didn't see any relevant console warnings/logs. Is there something in particular I should be looking for?
I think I responded to all open comments, but I still need to evaluate integration with |
It's possible to update Additionally, here's a Stackblitz demo-ing integration: https://stackblitz.com/edit/angular-ivy-ls9hgx?file=src%2Fapp%2Ffocus-monitor.ts |
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.
LGTM
@jelbourn do you want to put this straight into master or create a feature branch so that the focus monitor can be migrated to use this before merging to master?
I'd recommend a feature branch. Please see #22371 (comment) regarding integration with |
I've updated the PR to target a feature branch called |
/** | ||
* Service that detects the user's input modality. | ||
* | ||
* Note: This service will not update the input modality when a user is navigating with a screen |
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.
* Note: This service will not update the input modality when a user is navigating with a screen | |
* This service does not update the input modality when a user navigates with a screen |
nit: slightly more active voice, present tense
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.
Done.
@Injectable({ providedIn: 'root' }) | ||
export class InputModalityDetector implements OnDestroy { | ||
/** Emits when the input modality changes. */ | ||
readonly inputModalityChange: Observable<InputModality>; |
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.
Is it redundant to have inputModalityDetector.inputModalityChange
? Maybe we should shorten to just
modalityChanges
or changes
?
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.
I don't feel strongly, changed to modalityChanges
to align with mostRecentModality
.
/** Returns the most recently detected input modality. */ | ||
get inputModality(): InputModality { | ||
return this._inputModality.value; | ||
} |
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.
/** Returns the most recently detected input modality. */ | |
get inputModality(): InputModality { | |
return this._inputModality.value; | |
} | |
/** The most recently detected input modality. */ | |
get mostRecentModality(): InputModality { | |
return this._inputModality.value; | |
} |
What do you think about making this mostRecentModality
?
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.
Done.
constructor( | ||
private readonly _platform: Platform, | ||
ngZone: NgZone, | ||
@Inject(DOCUMENT) document: Document, |
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.
@Inject(DOCUMENT) document: Document, | |
@Inject(DOCUMENT) document: Document|Document, |
Using Document
here will break in SSR because TypeScript preserves type information on decorated params, and Document
doesn't existing in a nodejs environment. We can use this weird workaround to trick TS into not emitting the type as-is.
Can go in a follow-up, but you can test this by adding something to make this API run as part of our kitchen-sink.ts
component (which is our SSR test component).
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.
See the thread at #22371 (comment). The kitchen sink e2e test seems to pass - what should I be looking for?
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.
Hmm, maybe something changed in the tooling so that we don't have to worry about this any more ¯\(ツ)/¯
I synced input-modality up to master, it should go away if you rebase on input-modality |
Tried that (and also tried an empty commit to get the rebase picked up), to no avail. It's weird because comparing local with remote here (https://github.com/angular/components/compare/input-modality...zelliott:input-modality?expand=1) shows the commits I'd expect, whereas this PR has an extra one. |
Okay fixed by squashing. |
185ebb0
to
3ca1a94
Compare
3ca1a94
to
b172952
Compare
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.
lgtm - I also fixed the rebase issue
Adds a new InputModalityDetector service to detect a user's input modality. feat(cdk/a11y): Small fixes to InputModalityDetector feat(cdk/a11y): Add new API goldens feat(cdk/a11y): Respond to reviewer feedback feat(cdk/a11y): Respond to additional PR reviewer feedback feat(cdk/a11y): Respond to PR feedback feat(cdk/a11y): Accept new goldens feat(cdk/a11y): Empty commit to try to get a rebase picked up
b172952
to
0117c1b
Compare
… user's current input modality. (#22371)
… user's current input modality. (#22371)
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. |
Note: This is a first-stab at an input modality detection service and I expect there to be some churn regarding its exact behavior, so feel free to leave many comments.
Motivation
In our app, we conditionally render strong focus styles on elements if the element is determined to have been focused via keyboard. This is done by attaching a global
FocusMonitor
to thedocument.body
. One problem with this approach is that we can only detect input modality changes when focus also changes (see #21555 for why this is a problem witharia-activedescendant
elements). The purpose of theInputModalityDetector
service is to allow users to detect a broader array of input modality changes, regardless of whether focus changes.Approach
The
InputModalityDetector
attempts to detect input modality by listening tokeydown
,mousedown
, andtouchstart
events, and attributing each event to its corresponding input modality ('keyboard'
,'mouse'
,'touch'
). For example, when akeydown
event is fired, the service detects the'keyboard'
input modality. I chose this small subset of events as I thought they best represented legitimate, intentional, and committed user interaction.The service uses some additional heuristics under-the-hood to support scenarios when these events are fired but likely don't represent an actual change in input modality. For example:
mousedown
events that occur too closely after atouchstart
event (again, please see the comments in the source for why).Demo
https://stackblitz.com/edit/angular-ivy-9cxqav?file=src/app/input-modality-detector.ts
Behavior
The service essentially works as you would expect when interacting with a page. Interesting edge cases are as follows:
null
when the service is first instantiated, as we have no clue what the user's input modality is.<select>
or<input type="datepicker">
, this change is not detected. This is because no events are emitted.Testing
I'm planning on testing the service on the following platforms, browsers, and screen readers:
I've currently tested most of the combinations above, with the exception of Chrome OS (+ChromeVox), Android (+Android Accessibility Suite), and IE11.
Prior Art / Resources