Repository navigation
Make the OTP device picker in the keycloak.v2 login theme keyboard accessible - #54079
Rishavbhattarai wants to merge 3 commits into
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
Visible selection is lost in the inherited OTP reset picker and becomes stale during login with JavaScript disabled.
2 open issues
What changed in this PR
Improves keyboard and screen-reader access to the keycloak.v2 OTP device picker, addressing #51208.
Changes:
- Replaces clickable tiles with labeled native radios and an accessible group name.
- Adds default selection styling and visible keyboard focus.
- Updates page objects and adds keyboard-selection coverage.
| File | Description |
|---|---|
| themes/src/main/resources/theme/keycloak.v2/login/theme.properties | Adds visually hidden radio styling. |
| themes/src/main/resources/theme/keycloak.v2/login/resources/css/styles.css | Outlines the keyboard-focused tile. |
| themes/src/main/resources/theme/keycloak.v2/login/login-otp.ftl | Adds radios, labels, and selection synchronization. |
| themes/src/main/resources/theme/base/login/messages/messages_en.properties | Names the device-selection group. |
| testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/forms/BrowserFlowTest.java | Tests default and keyboard selection. |
| testsuite/integration-arquillian/tests/base/src/main/java/org/keycloak/testsuite/pages/LoginTotpPage.java | Updates tile lookup and adds keyboard interaction. |
| test-framework/ui/src/main/java/org/keycloak/testframework/ui/page/LoginTotpPage.java | Updates tile lookup to labels. |
Review effort: Balanced
3d3968a to
3890c39
Compare
3890c39 to
8597860
Compare
8597860 to
6f49dc6
Compare
6f49dc6 to
ec7217f
Compare
There was a problem hiding this comment.
Thanks @Rishavbhattarai! LGTM in general! Just one comment, WDYT about using a class and the theme properties instead of assigning directly to the name? Do you think it makes sense?
|
@mabartos Do you want to take a look to this one too? |
There was a problem hiding this comment.
Thanks @Rishavbhattarai for the changes! Another nit-pick that I realized, maybe putting the css to the label instead of the class is also better (because the class is also in a property). I have not tested it, so please check that I'm not doing something wrong.
And please rebase your branch to pick up the last changes because there was a CI error in JS that was fixed yesterday.
For me with this, the PR is OK if the tests pass. Thanks again!
| } No newline at end of file | ||
| } | ||
| /* OTP device pickers (login and reset): the radio is visually hidden, so the tile shows its checked state and keyboard focus */ | ||
| .kc-login-otp-radio:checked + .pf-v5-c-tile { |
There was a problem hiding this comment.
| .kc-login-otp-radio:checked + .pf-v5-c-tile { | |
| .kc-login-otp-radio:checked + label { |
There was a problem hiding this comment.
Good point, applied both in f989536. I checked in a browser that the checked tile and the keyboard focus outline still show. I'll rebase on main next to pick up the JS CI fix. Thanks for the reviews!
| --pf-v5-c-tile--after--ScaleY: var(--pf-v5-c-tile--m-selected--after--ScaleY); | ||
| } | ||
|
|
||
| .kc-login-otp-radio:focus-visible + .pf-v5-c-tile { |
There was a problem hiding this comment.
| .kc-login-otp-radio:focus-visible + .pf-v5-c-tile { | |
| .kc-login-otp-radio:focus-visible + label { |
…cessible - render each OTP credential as a radio input with the tile as its label, like the legacy keycloak theme - give the group an accessible name and a visible keyboard focus outline - style the selected tile from the checked radio, so the default credential is shown as selected on page load and the selection works without JavaScript - apply the same styling to the inherited reset OTP picker, whose radios are now hidden too Closes keycloak#51208 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LP6NzY6hpzUKArjJCC4Mv1 Signed-off-by: Rishavbhattarai <bhattarai.rishav@gmail.com>
Select the visually hidden radios by the kc-login-otp-radio class, set through kcLoginOTPListInputClass, instead of by the input name. Closes keycloak#51208 Signed-off-by: Rishavbhattarai <bhattarai.rishav@gmail.com>
Closes keycloak#51208 Signed-off-by: Rishavbhattarai <bhattarai.rishav@gmail.com>
f989536 to
9dbfeda
Compare
There was a problem hiding this comment.
Thanks a lot @Rishavbhattarai! But I saw that there was another bigger PR that was pending approval in #52724 (bigger refactor to move keycloak.v2 to base). I saw that one also fixed this issue indirectly and using another approach (that one changed the JS). I rather prefer this one as we are getting rid of any JS and using plain html. @mabartos WDYT? Do we continue with this but now in the moved files? @Rishavbhattarai wait a bit until Martin comments about this, I don't want to make you work again for nothing.
Closes #51208
The OTP device picker in the
keycloak.v2login theme rendered each credential as a<div onclick>. Keyboard users couldn't reach it or change the selected device, screen readers got no role or state, and it did nothing with JavaScript disabled.Changes:
login-otp.ftl: each credential is now an<input type="radio" name="selectedCredentialId">with the PatternFly tile as its<label>, the same structure as the legacykeycloaktheme. The group hasrole="radiogroup"and an accessible name from a new message,loginOtpCredentialSelection. The inline script is gone.kcLoginOTPListInputClass=pf-v5-screen-reader).styles.cssstyles the tile frominput:checked + labelwith PatternFly's own selected-tile values, and draws a focus outline while the radio has keyboard focus. The default credential shows as selected on page load (before this, no tile was highlighted), and selecting works the same with JavaScript disabled.keycloak.v2inheritslogin-reset-otp.ftlfrombase, which uses the same input class, so the reset picker gets the same hidden radios and checked styling.kcLoginOTPListSelectedClassis removed from the v2theme.propertiesbecause nothing uses it anymore.selectedCredentialIdinput is unchanged.Tests:
BrowserFlowTest#testUserWithTwoAdditionalFactorsSelectedWithKeyboard: checks the default device is pre-selected, presses Shift+Tab from the one-time code field and asserts that focus lands on the checked radio, then presses the down arrow to select the second device and logs in with its code. With the radios taken out of the tab order, it fails (focus lands on a button). It's skipped on HtmlUnit, which doesn't move focus or selection with these keys, so it runs in the Chrome and Firefox Forms IT jobs.testUserWithTwoAdditionalFactorsnow also asserts the default device is pre-selected, so HtmlUnit covers that part.LoginTotpPagepage objects look uplabeltiles instead ofdivtiles and read the selection from the checked radio.ResetOtpPage.selectOtpclicks the tile instead of the hidden radio.Locally:
BrowserFlowTest30/30 andResetOtpTe 10000 st4/4 in headless Chrome; on HtmlUnitBrowserFlowTest29/30 with 1 skipped andResetOtpTest4/4.MultiFactorAuthenticationTestpasses.I used Claude Code (an AI agent) to help write the change, the tests and this description. I reviewed every line and can explain and revise it.