Skip to content
This repository has been archived by the owner on Sep 5, 2024. It is now read-only.

feat(interaction): added service to detect last interaction #7965

Merged
merged 1 commit into from
Oct 12, 2016

Conversation

devversion
Copy link
Member

This is a re-submit / rework of my old PR (#5589), should start from clean again.

Sample use-case:

  • Sidenav want's to restore focus after the pane gets closed, this should only happen for keyboard interactions.

Fixes #5563 Fixes #5434 Closes #5583

// the `onInput` function multiple times.
timer = $timeout(function() {
buffer = false;
}, 650);
Copy link
Member

Choose a reason for hiding this comment

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

  1. is this timeout need to happen only if touchstart is being listened
  2. have we already talked about the 650 timeout? why 650? there's no way to avoid that?

Copy link
Member Author

Choose a reason for hiding this comment

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

  1. Yes this is only needed for the touch event.
  2. Yes, we already talked about that.

    The 650ms are coming from Patrick H. Lauke (W3C)

300ms is causing iOS (which has default delay of ~350ms) to trigger touch, then mouse (once mouse compatibility events fire). through trial and error (on iPhone 6S) a value of around 650ms seems more appropriate (and still snappier than the original 1s from previous version)

  No there is no way, to avoid that.

Copy link
Member

Choose a reason for hiding this comment

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

I don't think this timeout needs to trigger a digest.

Copy link
Member Author

Choose a reason for hiding this comment

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

That is a good catch @crisbeto Thanks!

}, 650);
}

body.on('keydown', onInput);
Copy link
Member

Choose a reason for hiding this comment

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

This is pretty nitpicky and probably down to preference, but you can combine the calls like body.on('keydown mouseenter ' + mouseEvent, onInput);

Copy link
Member Author

Choose a reason for hiding this comment

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

That works too, but listing those events in order is more clear here.

@crisbeto
Copy link
Member

Apart from that digest on the $timeout, LGTM.

@ThomasBurleson ThomasBurleson added this to the 1.2.0 milestone Apr 19, 2016
@ThomasBurleson ThomasBurleson added the in progress Mainly for in progress PRs, but may be used for issues that require multiple PRs label Apr 19, 2016
@ThomasBurleson ThomasBurleson modified the milestones: 1.2.0, Backlog Apr 21, 2016
@devversion
Copy link
Member Author

@crisbeto Thanks for the review. @ThomasBurleson @EladBezalel PTAL.

@devversion devversion removed the in progress Mainly for in progress PRs, but may be used for issues that require multiple PRs label Apr 24, 2016
@FreakTheMighty
Copy link

FreakTheMighty commented May 11, 2016

Do ya'll expect this will resolve #7869 (comment) ?

@topherfangio
Copy link
Contributor

@bradrich and @topherfangio to review.

@devversion devversion added the needs: rebase This PR needs to be rebased on the latest commits from master and conflicts need to be resolved label Sep 10, 2016
@devversion devversion removed the needs: rebase This PR needs to be rebased on the latest commits from master and conflicts need to be resolved label Sep 19, 2016
@devversion
Copy link
Member Author

Ping @bradrich and @topherfangio for review.

@topherfangio
Copy link
Contributor

This looks good to me. Quick question though (and this may be totally irrelevant), but your input map only handles keydown.

Does this mean that if someone holds down a key, then presses a button with the mouse, then lets up the key getLastInteractionType() will incorrectly return the mouse event?

Like I said, this may be totally irrelevant/probably not a case we need to worry about; just curious more than anything :-)

@devversion
Copy link
Member Author

devversion commented Sep 23, 2016

@topherfangio Yeah this is valid.

For example if you hold the mouse and quickly press a button while holding the mouse, it will incorrectly display use recognize a keyboard.

But I don't think it's worth addressing this kind of edge-case, because the intention is just to provide proper accessibility -> If he doesn't trigger the element with a keyboard, the focus will be not restored.

The only thing (the worst) which could happen is, that the focus will be restored incorrectly, due to a click while holding a keyboard press - I do not consider this as a big deal.

I think the reason why we don't listen for the up events, is that we don't want to register too many listeners on the body element, because this is potentially performance damaging.

Here is a package, which does the same

https://github.com/ten1seven/what-input/blob/master/src/what-input.js#L197

@topherfangio
Copy link
Contributor

Cool; that's what I thought.

LGTM 👍

@ThomasBurleson ThomasBurleson added needs: presubmit and removed needs: review This PR is waiting on review from the team labels Oct 5, 2016
@ThomasBurleson ThomasBurleson modified the milestones: - Backlog, 1.1.2 Oct 5, 2016
@ThomasBurleson
Copy link
Contributor

ThomasBurleson commented Oct 5, 2016

Fixes #8749
Fixes #7963

@ThomasBurleson ThomasBurleson added the P0: critical Critical issues that must be addressed immediately. label Oct 9, 2016
@kara kara added pr: merge ready This PR is ready for a caretaker to review and removed needs: presubmit labels Oct 12, 2016
@kara kara merged commit 24370e7 into angular:master Oct 12, 2016
@devversion devversion deleted the fix/interaction-detection branch October 12, 2016 16:59
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
P0: critical Critical issues that must be addressed immediately. pr: merge ready This PR is ready for a caretaker to review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

md-toolbar and sidenav focus problems Ripple 'sticks' if using UI Router in the app under certain conditions
8 participants