Skip to content
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

Ensure blur event fires on prior focused element even if click target is not focusable #1033

Merged
merged 1 commit into from
May 30, 2021

Conversation

helgablazhkun
Copy link
Contributor

The root cause of the issue is that the blur event is not triggered on
the current active element in the click helper in case if the clicked
element is not focusable.

If the clicked element is focusable the previous active element gets
blur event trough the focus helper.

A call to blur was added for the case when the clicked element
is not focusable.

@helgablazhkun
Copy link
Contributor Author

Fixes #1032

Copy link
Member

@rwjblue rwjblue left a comment

Choose a reason for hiding this comment

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

If this is generally the correct behavior (hopefully the plain HTML demo will prove that to us), we should consider doing this inside focus itself. I suspect other of our interaction helpers would need this same thing (e.g. doubleClick, tap, etc).

@helgablazhkun
Copy link
Contributor Author

Moved 'isFocusable' check and blur into focus.
Updated click/double-click helpers according to this change.
Moved 'not focusable' exception from inner focus_ helper to focus as it looks like we should give ability to click/double-click/tap on non-focusable elements.

Copy link
Member

@rwjblue rwjblue left a comment

Choose a reason for hiding this comment

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

Overall seems great, thanks for working on this!

addon-test-support/@ember/test-helpers/dom/focus.ts Outdated Show resolved Hide resolved
The root cause of the issue is that the blur event is not triggered on
the current active element in the __focus__ helper in case if the
clicked (double-clicked, tapped) element is not focusable.

If the clicked (double-clicked, tapped) element is focusable the
previous active element gets
blur event trough the __focus__ helper.

A call to __blur__ was added to the __focus__ helper for the case
when the clicked (double-clicked, tapped) element element
is not focusable.
Copy link
Member

@rwjblue rwjblue left a comment

Choose a reason for hiding this comment

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

Great job, thank you!!

@rwjblue rwjblue added the bug label May 30, 2021
@rwjblue rwjblue changed the title Blur doesn't fire on click on non-focusable element Ensure blur event fires on prior focused element even if click target is not focusable May 30, 2021
@rwjblue rwjblue merged commit e47ea7b into emberjs:master May 30, 2021
@helgablazhkun helgablazhkun deleted the click-helper-blur-fix branch May 31, 2021 08:07
@bendemboski
Copy link
Contributor

Hey @rwjblue and @helgablazhkun, this is a great improvement, and definitely moves things in the right direction! However, it has turned out to be a pretty significantly breaking change for my test suite -- see #1126 and #1127. I guess it's already released, but maybe we can fast-track those PRs to minimize the time that the latest version of @ember/test-helpers has breaking changes relative to v2.2.5?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants