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

Make possible to focus grid/list view toggle via keyboard #16154

Merged
merged 1 commit into from
Aug 15, 2019

Conversation

jancborchardt
Copy link
Member

Needs help from @nextcloud/javascript to actually make the toggle also work on pressing enter when focused. :)

@jancborchardt
Copy link
Member Author

Needs help from @nextcloud/javascript to actually make the toggle also work on pressing enter when focused. :)

@MorrisJobke MorrisJobke mentioned this pull request Jul 15, 2019
28 tasks
Copy link
Member

@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.

Hum, actually I think this is the wrong way to go.
The label should not receive the focus on keyboard nav.
The checkbox should. That way you don't loose the element indicator on screen readers and you get the default behaviour everywhere :)

@jancborchardt
Copy link
Member Author

@skjnldsv tried giving the checkbox the tab focus, but it seems to conflict with our custom checkboxes. What would your approach be?

@skjnldsv
Copy link
Member

tried giving the checkbox the tab focus

But the checkbox should already have the focus, right? Since this is a hidden and not display:none element?
If you're talking about the focus visual feedback of the label, then I guess there is a css issue? 🤔

@jancborchardt
Copy link
Member Author

tried giving the checkbox the tab focus

But the checkbox should already have the focus, right? Since this is a hidden and not display:none element?
If you're talking about the focus visual feedback of the label, then I guess there is a css issue? thinking

Please test. :) It does not receive the focus at all at the moment.

@skjnldsv
Copy link
Member

Nice catch!
This needs to be removed indeed:

#showgridview {
display: none;
}

@rullzer
Copy link
Member

rullzer commented Aug 11, 2019

So 17 or 18?

@jancborchardt
Copy link
Member Author

I would say 17 since it fixes the accessibility of an essential part of the software. As said, needs JS help. @skjnldsv?

@skjnldsv
Copy link
Member

As said, needs JS help

No, only what is mentioned on my comment.
Nothing related to js here :)

Signed-off-by: John Molakvoæ (skjnldsv) <skjnldsv@protonmail.com>
@skjnldsv skjnldsv force-pushed the fix/noid/accessibility-focusable-viewtoggle branch from 900440e to cbc3862 Compare August 14, 2019 14:46
@skjnldsv
Copy link
Member

Here, can you review @jancborchardt :)

@skjnldsv skjnldsv added 3. to review Waiting for reviews and removed 2. developing Work in progress help wanted labels Aug 14, 2019
@jancborchardt
Copy link
Member Author

jancborchardt commented Aug 15, 2019

Nice, this works! :)

The only thing is that the toggle is focused before the home and upload icons in the controls bar? This seems strange and it should rather be between the upload button and the Recommendations.

Otherwise 👍

@skjnldsv
Copy link
Member

The only thing is that the toggle is focused before the home and upload icons in the controls bar? This seems strange and it should rather be between the upload button and the Recommendations.

then we should move the input and label after the home button (in the html order)

@rullzer
Copy link
Member

rullzer commented Aug 15, 2019

Lets fix that separatly so this at least is in

@rullzer rullzer merged commit 465e919 into master Aug 15, 2019
@rullzer rullzer deleted the fix/noid/accessibility-focusable-viewtoggle branch August 15, 2019 08:56
@skjnldsv
Copy link
Member

/backport to stable16

@backportbot-nextcloud
Copy link

backport to stable16 in #16745

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.

None yet

4 participants