Add directive to determine how elements were focused. #2646

mmalerba · 2017-01-14T01:39:41Z

No description provided.

jelbourn

+ @devversion who worked on something like this for material1. I suspect there may be some edge cases this doesn't capture

jelbourn · 2017-01-14T02:35:05Z

src/lib/core/style/add-focus-classes.ts

+
+/** Singleton that allows all instances of CdkAddFocusClasses to share document event listeners. */
+@Injectable()
+export class CdkFocusCauseDetector {


How about FocusOriginMonitor? I would like to avoid prefixing any classes with "cdk".

jelbourn · 2017-01-14T02:36:17Z

src/lib/core/style/add-focus-classes.ts

+export class CdkFocusCauseDetector {
+  /** Whether a keydown event has just occurred. */
+  get keydownOccurred() { return this._keydownOccurred; }
+  private _keydownOccurred = false;


Can you just do

private readonly keydownOccurred = false;

This doesn't do the same thing does it? I do want this to be writable inside the class, just don't want other people setting it.

jelbourn · 2017-01-14T02:38:52Z

src/lib/core/style/add-focus-classes.ts

+  constructor() {
+    document.addEventListener('keydown', () => {
+      this._keydownOccurred = true;
+      setTimeout(() => this._keydownOccurred = false, 0);


Instead of setTimeout, can you do

Promise.resolve().then(() => this.keydownOccured = false);

(same elsewhere)

jelbourn · 2017-01-14T02:41:37Z

src/lib/core/style/add-focus-classes.ts

+  private _mousedownOccurred = false;
+
+  constructor() {
+    document.addEventListener('keydown', () => {


Add comment explaining why useCapture is appropriate here.

jelbourn · 2017-01-14T02:44:05Z

src/lib/core/style/add-focus-classes.ts

+  constructor() {
+    document.addEventListener('keydown', () => {
+      this._keydownOccurred = true;
+      setTimeout(() => this._keydownOccurred = false, 0);


I'm slightly concerned about the performance implications of scheduling a asynchronous action on every keydown in the application. What can we do to conclusively say the cost for this would be minor?

In Material 1 we only had a timeout for touch events (for the buffering), but for normal mousedown events we didn't need one.

We always keep track of the timestamp for the last interaction and then compare it based on the time difference.

https://github.com/angular/material/blob/master/src/core/services/interaction/interaction.js#L144
https://github.com/angular/material/blob/master/src/components/button/button.js#L169

@devversion Why did you guys choose 15ms, that looks kind of race-condition-y.

@jelbourn I could do something similar to the ng1 version I suppose. The scheduled function is about as lightweight as possible, I wouldn't expect it to cost us much in performance. I'll make a little experiment to verify

Added my performance test code & results, doesn't look like it adds any noticeable latency vs the timestamp approach (at least on my machine)

jelbourn · 2017-01-14T02:46:06Z

src/lib/core/style/add-focus-classes.ts

+ * Directive that determines how a particular element was focused (via keyboard, mouse, or
+ * programmatically) and adds corresponding classes to the element.
+ */
+@Directive({


How would you add this directive to something like MdButton, where the host element itself is the focused element. It's not something the user should need to apply to the material components.

good question... maybe I can move most of the functionality into the injectable and just have the directive pass its element to the injectable?

jelbourn · 2017-01-14T02:48:32Z

src/lib/core/style/add-focus-classes.ts

+      setTimeout(() => this._keydownOccurred = false, 0);
+    }, true);
+
+    document.addEventListener('mousedown', () => {


Will this cover touch events?

A lot of touch devices actually emit a mousedown event for touch clicks, but I don't think that this really reliable.

Test for mousedown and touchstart events: http://codepen.io/DevVersion/pen/qRNgOZ

In the Material 1 interaction service we had to buffer the touch events: see here

Also interesting would be how that handles IE11 and eventually IE mobile browsers (special treatment in M1)

mmalerba · 2017-01-25T01:32:01Z

PTAL, restructured the code so it can be easily used by MdButton and similar components and did a performance experiment. Still need to add touch event support, just want to make sure we're on the same page with this approach first.

devversion · 2017-01-30T19:05:11Z

src/lib/core/style/add-focus-classes.spec.ts

+
+    it('should detect focus via keyboard', async(() => {
+      // Simulate focus via keyboard.
+      dispatchKeydownEvent(document, 9 /* tab */);


You should be able to use the TAB constant from here

devversion · 2017-01-30T19:14:10Z

src/lib/core/style/add-focus-classes.ts

+  focusVia(element: Node, renderer: Renderer, focusOrigin: FocusOrigin) {
+    this._fakeOrigin = focusOrigin;
+    document.addEventListener('focus', this._clearFakeOrigin, true);
+    renderer.invokeElementMethod(element, 'focus');


How about moving the focus listener (above) to its own function, so you can just invoke it with the given fakeOrigin.

export type FocusOriginType = 'keyboard' | ' mouse' | 'manual'; /* Somewhere in the code */ renderer.listen(element, 'focus', () => this._onElementFocus(element, origin)); /* Focus event listener */ private _onElementFocus(element: HTMLElement, target: FocusOriginType) { /* Handle the target */ } /* Fake Focusing*/ private focusVia(XXX) { this._onElementFocus(element, focusOrigin); }

This would also mean that you get rid of the _keydownOccurred properties and just assign the given string from FocusOriginType / or you use an enum.

I'm not sure this does the same thing. We need to actually focus the element in addition to adding the styles, so we want to invoke the focus method. We need to record the fakeOrigin first though, so that when we later wind up in the focus listener we know that we still want to fake the origin.

Good point- Although I think that you would simplify a lot of things by only keeping the focusOrigin as type of FocusOriginType instead of having booleans like _keydownOccured.

document.addEventListener('keydown', () => { this.focusOrigin = 'keyboard'; setTimeout(() => this.focusOrigin = null, 0); }, true); private focusVia(element: HTMLElement, focusOrigin: FocusOrigin) { this.focusOrigin = focusOrigin; element.focus(); // Or with renderer } private _onElementFocus(element: HTMLElement) { /* Handle the this.focusOrigin */ this.focusOrigin = null; }

This way you would not need to listen for another focus event when faking the origin.

In this example above, null would be treated as programmatic.

Also this could be just an idea. Let me know what you think.

Ah I see, yeah I think that will be much simpler, thanks!

… the FocusOrigin.

mmalerba · 2017-02-01T00:07:03Z

@devversion So it seems like the focus & blur event listeners are not triggering in the tests on Firefox. They do work fine in the demo app though. It may have something to do with the test driver starting multiple windows, I've seen a couple mentions around the web of Firefox not bothering to fire these events if the window isn't active. I just disabled these tests for Firefox.

mmalerba · 2017-02-01T19:06:18Z

+ @tinayuangao since jeremy is out

devversion

LGTM! - Chatted a lot with @mmalerba about the API and implementation. I'm still not a fan of program as FocusOrigin, but it is definitely very precise.

In the future we should definitely revisit the Firefox tests.

tinayuangao · 2017-02-01T20:17:59Z

LGTM

angular-automatic-lock-bot · 2019-09-06T07:03:32Z

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

_{This action has been performed automatically by a bot.}

mmalerba added the pr: needs review label Jan 14, 2017

mmalerba assigned jelbourn Jan 14, 2017

mmalerba requested a review from jelbourn January 14, 2017 01:39

googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Jan 14, 2017

jelbourn reviewed Jan 14, 2017

View reviewed changes

jelbourn requested a review from devversion January 14, 2017 02:49

mmalerba force-pushed the fc branch from 179933e to 350479e Compare January 24, 2017 23:31

devversion reviewed Jan 30, 2017

View reviewed changes

mmalerba added 9 commits January 31, 2017 08:17

Add directive to determine how elements were focused.

3363c86

addressed some comments

fec6cbb

move all functionality to FocusOriginMonitor and add a method to fake…

a46e5ac

… the FocusOrigin.

small fix

be94341

re-did perf test w/ setTimeout just to be sure

01d4f66

s/cdkAddFocusClasses/cdkFocusClasses

626c9d8

rename files & use TAB constant

f654887

fix imports

0a8e0ed

addressed comments and added more tests

46d8cab

mmalerba force-pushed the fc branch 9 times, most recently from 053854b to 1b97f7a Compare January 31, 2017 19:53

addressed comments

d14ac91

mmalerba force-pushed the fc branch 2 times, most recently from 33a5ac4 to 731c0dd Compare January 31, 2017 21:02

mmalerba force-pushed the fc branch 6 times, most recently from 6ffbca2 to 2e70169 Compare January 31, 2017 23:35

skip tests on ff

6e06f60

mmalerba force-pushed the fc branch from 2e70169 to 6e06f60 Compare January 31, 2017 23:56

mmalerba requested a review from tinayuangao February 1, 2017 19:05

mmalerba assigned tinayuangao Feb 1, 2017

devversion approved these changes Feb 1, 2017

View reviewed changes

mmalerba added pr: lgtm action: merge The PR is ready for merge by the caretaker and removed pr: needs review labels Feb 1, 2017

improve explanation of ff issues

9b37044

mmalerba mentioned this pull request Feb 2, 2017

(tests): firefox doesn't fire focus/blur events in tests #2903

Closed

kara merged commit 8a6d902 into angular:master Feb 4, 2017

angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 6, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add directive to determine how elements were focused. #2646

Add directive to determine how elements were focused. #2646

mmalerba commented Jan 14, 2017

jelbourn left a comment •

edited

Loading

jelbourn Jan 14, 2017

jelbourn Jan 14, 2017

mmalerba Jan 24, 2017

jelbourn Jan 14, 2017

jelbourn Jan 14, 2017

jelbourn Jan 14, 2017

devversion Jan 14, 2017

mmalerba Jan 24, 2017

mmalerba Jan 24, 2017

jelbourn Jan 14, 2017

mmalerba Jan 24, 2017

jelbourn Jan 14, 2017

devversion Jan 14, 2017 •

edited

Loading

mmalerba commented Jan 25, 2017

devversion Jan 30, 2017

devversion Jan 30, 2017

mmalerba Jan 30, 2017

devversion Jan 31, 2017

mmalerba Jan 31, 2017

mmalerba commented Feb 1, 2017

mmalerba commented Feb 1, 2017

devversion left a comment •

edited

Loading

tinayuangao commented Feb 1, 2017

angular-automatic-lock-bot bot commented Sep 6, 2019

Add directive to determine how elements were focused. #2646

Add directive to determine how elements were focused. #2646

Conversation

mmalerba commented Jan 14, 2017

jelbourn left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

devversion Jan 14, 2017 • edited Loading

Choose a reason for hiding this comment

mmalerba commented Jan 25, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mmalerba commented Feb 1, 2017

mmalerba commented Feb 1, 2017

devversion left a comment • edited Loading

Choose a reason for hiding this comment

tinayuangao commented Feb 1, 2017

angular-automatic-lock-bot bot commented Sep 6, 2019

jelbourn left a comment •

edited

Loading

devversion Jan 14, 2017 •

edited

Loading

devversion left a comment •

edited

Loading