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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Prevent tabbing into closed navigation #3114

Merged
merged 1 commit into from Jun 6, 2023

Conversation

PVince81
Copy link
Contributor

@PVince81 PVince81 commented Aug 29, 2022

When the navigation is closed, add "inert" attribute to prevent tabbing
into the sidebar.

  • FIXME: inert="false" does NOT disable inert, need to remove the attribute completely 馃槖

Note: I didn't test this but tied the attribute to the open value. I verified in the Talk app that the open attribute is properly updated when swiping, resizing the window, etc. So setting inert under the same conditions should work.

@PVince81 PVince81 added 2. developing Work in progress accessibility Making sure we design for the widest range of people possible, including those who have disabilities labels Aug 29, 2022
@PVince81 PVince81 self-assigned this Aug 29, 2022
When the navigation is closed, add "inert" attribute to prevent tabbing
into the sidebar.

Signed-off-by: Vincent Petry <vincent@nextcloud.com>
@PVince81 PVince81 force-pushed the a11y/noid/inert-closed-navigation branch from fc30204 to e94868a Compare August 29, 2022 21:38
@PVince81
Copy link
Contributor Author

according to https://stackoverflow.com/a/64598898 just setting the attribute to null should make Vue remove it

@PVince81
Copy link
Contributor Author

would be good to do some tests with an app that has the sidebar

sadly the style guide cannot test the app navigation

@PVince81 PVince81 added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Aug 29, 2022
@PVince81
Copy link
Contributor Author

does anyone has a working npm link setup to quickly test this ?

Copy link
Contributor

@vinicius73 vinicius73 left a comment

Choose a reason for hiding this comment

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

The change seems ok, Vue will remove this prop.
But I did not test it properly.

Maybe a full example of this component will help in future changes.

@skjnldsv
Copy link
Contributor

Inert seems not supported enough yet :(
image

@nickvergessen
Copy link
Contributor

Inert seems not supported enough yet :(

Well sounds like we can fix most of the browsers now and the last one will catch up eventually.

@skjnldsv
Copy link
Contributor

Inert seems not supported enough yet :(

Well sounds like we can fix most of the browsers now and the last one will catch up eventually.

Not really, the browser versions are all very new. Chrome 102 is just from last May.
We should consider another approach too imho

Copy link
Contributor

@skjnldsv skjnldsv left a comment

Choose a reason for hiding this comment

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

See comments

@PVince81
Copy link
Contributor Author

We should consider another approach too imho

we could look into polyfills for inert

I don't think we should invest time trying to hack this around on our own, unless quick, easy and reliable

in caniuse it said Firefox 106 supports it now: https://caniuse.com/?search=inert

@skjnldsv
Copy link
Contributor

skjnldsv commented Aug 30, 2022

in caniuse it said Firefox 106 supports it now: caniuse.com/?search=inert

Scheduled to be released on 2022-10-18 馃槙

What about just tabindex=-"1"
EDIT: Ah, I guess it won't solve the issue with accessibility tools ?

@skjnldsv
Copy link
Contributor

skjnldsv commented Aug 30, 2022

Also:
image

@PVince81
Copy link
Contributor Author

tabindex="-1" on the container doesn't fix sub-items as far as I remember.
we'd need to set this on every link and field within the sidebar and remember its original value, if any, and then reset it back
quite an involved hack

@PVince81
Copy link
Contributor Author

idea: would it be possible to unmount the navigation when it's closed after a small delay ? then mount it again when opened ?

might cause delays though when mounting again while the user is swiping

@skjnldsv
Copy link
Contributor

idea: would it be possible to unmount the navigation when it's closed after a small delay ? then mount it again when opened ?

might cause delays though when mounting again while the user is swiping

Would be a good idea, debounce show/hide could be worth it
You would need two props one for v-show and another for v-if

@PVince81 PVince81 assigned Pytal and unassigned PVince81 Jan 30, 2023
@susnux
Copy link
Contributor

susnux commented May 31, 2023

Currently 88% of global users are supported, the remaining missing browsers from our browserslist config are currently Safari 15.4 and Firefox ESR.

I think this approach:

Well sounds like we can fix most of the browsers now and the last one will catch up eventually.

is now good enough for version 8 / NC28 (meaning I think this is fine for current master).

@skjnldsv skjnldsv merged commit cf90618 into master Jun 6, 2023
@skjnldsv skjnldsv deleted the a11y/noid/inert-closed-navigation branch June 6, 2023 13:16
@skjnldsv skjnldsv mentioned this pull request Jun 28, 2023
@raimund-schluessler raimund-schluessler added this to the 8.0.0 milestone Aug 25, 2023
@raimund-schluessler
Copy link
Contributor

This seems to break opening the sidebar when it is closed, see #4433

@susnux
Copy link
Contributor

susnux commented Aug 25, 2023

I think we need to move the button outside the navigation element

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. to review Waiting for reviews accessibility Making sure we design for the widest range of people possible, including those who have disabilities
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants