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

fix(datepicker): simplify focus trap with observables #2419

Merged

Conversation

maxokorokov
Copy link
Member

Refactor existing implementation of the focus trap: handle only inside clicks and tab navigation. It doesn't steal outside focus anymore

Fixes #2406
Fixes #2390
Fixes #2372 (one more time)

Copy link
Member

@benouat benouat left a comment

Choose a reason for hiding this comment

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

For now this PR looks good to me.

Nonetheless, as soon as we have Protractor tests on the project, we should add a lot more test for both focustrap and any widget using it.

@benouat benouat mentioned this pull request Jun 1, 2018
@pkozlowski-opensource
Copy link
Member

@maxokorokov @benouat I've rebased this locally and can see 2 tests failing:

  • ngbFocusTrap should re-focus previously focused element inside the focus trap element FAILED
  • NgbInputDatepicker focus should re-focus elements inside the datepicker FAILED

@pkozlowski-opensource
Copy link
Member

I would vote for dropping unit tests that try to test focus() - this is just too fragile. IMO cost to benefit is just not there.

Refactor existing implementation of the focus trap: handle only inside clicks and tab navigation. It doesn't steal outside focus anymore

Fixes ng-bootstrap#2406
Fixes ng-bootstrap#2390
Fixes ng-bootstrap#2372 (one more time)
@maxokorokov
Copy link
Member Author

Rebased PR

@pkozlowski-opensource this is strange, as I haven't seen it locally and sauce labs didn't have any issues with these tests. Something went wrong during rebase, maybe?

I would prefer to keep at least these 2 tests before protractor is there. Will remove them if they're really unstable.

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

Successfully merging this pull request may close these issues.

3 participants