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

Implement focus-trap #15

Merged
merged 1 commit into from
Aug 16, 2017
Merged

Implement focus-trap #15

merged 1 commit into from
Aug 16, 2017

Conversation

limonte
Copy link
Contributor

@limonte limonte commented Aug 15, 2017

In #13 we decided to move focus-trap functionality (which will be required by vaadin-dialog) to vaadin-overlay.


This change is Reviewable

@limonte limonte force-pushed the feature/focus-trap branch 12 times, most recently from c1f150e to e96fc76 Compare August 16, 2017 05:17
@limonte
Copy link
Contributor Author

limonte commented Aug 16, 2017

Tests are finally working in all browsers, ready for review 🚀

@tomivirkki
Copy link
Member

Seems to be working well 👍


Review status: 0 of 5 files reviewed at latest revision, 2 unresolved discussions.


vaadin-overlay.html, line 302 at r2 (raw file):

      _getFocusableElements() {
        return Array.prototype.slice.call(

Prefer Array.from(...querySelectorAll...) with array-like objects in es6


vaadin-overlay.html, line 304 at r2 (raw file):

        return Array.prototype.slice.call(
          this.$.content.querySelectorAll(
            'button, input:not([type=hidden]), textarea, select, a, *[tabindex]:not([tabindex="-1"])'

The set is different from https://github.com/vaadin/vaadin-grid/blob/master/vaadin-grid-cell-click-behavior.html#L63 Probably both sets have some flaws


Comments from Reviewable

@limonte
Copy link
Contributor Author

limonte commented Aug 16, 2017

Review status: 0 of 5 files reviewed at latest revision, 2 unresolved discussions.


vaadin-overlay.html, line 302 at r2 (raw file):

Previously, tomivirkki (Tomi Virkki) wrote…

Prefer Array.from(...querySelectorAll...) with array-like objects in es6

Done, thx!


vaadin-overlay.html, line 304 at r2 (raw file):

Previously, tomivirkki (Tomi Virkki) wrote…

The set is different from https://github.com/vaadin/vaadin-grid/blob/master/vaadin-grid-cell-click-behavior.html#L63 Probably both sets have some flaws

Good point, it's good to be aligned in such cases. I copied the set from Grid with a small additional check for tabindex="-1"


Comments from Reviewable

@tomivirkki
Copy link
Member

:lgtm:


Reviewed 2 of 5 files at r2, 3 of 3 files at r3.
Review status: all files reviewed at latest revision, all discussions resolved.


Comments from Reviewable

@yuriy-fix
Copy link
Contributor

:lgtm:


Review status: :shipit: all files reviewed at latest revision, all discussions resolved, all commit checks successful.


Comments from Reviewable

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

Successfully merging this pull request may close these issues.

3 participants