Repository navigation
Unify login theme actions and styling hooks - #52724
Conversation
There was a problem hiding this comment.
🟢 Approval recommended
No unresolved review comments were supplied, and the changes are limited to consistent styling and layout updates.
Pull request overview
This pull request standardizes button styling and action spacing across Keycloak login themes.
Changes:
- Adds shared form, button, and spacing properties.
- Normalizes action placement in v2 login templates.
- Applies consistent styling to inherited base templates.
File summaries
| File | Description |
|---|---|
themes/src/main/resources/theme/keycloak.v2/login/update-email.ftl |
Relocates the action group to form level. |
themes/src/main/resources/theme/keycloak.v2/login/theme.properties |
Adds shared button and spacing mappings. |
themes/src/main/resources/theme/keycloak.v2/login/terms.ftl |
Places terms content and actions within the form. |
themes/src/main/resources/theme/keycloak.v2/login/login-recovery-authn-code-config.ftl |
Adds shared form and button action styling. |
themes/src/main/resources/theme/keycloak.v2/login/link-idp-action.ftl |
Aligns message and actions within the form. |
themes/src/main/resources/theme/keycloak.v2/login/delete-credential.ftl |
Aligns message and actions within the form. |
themes/src/main/resources/theme/base/login/saml-post-form.ftl |
Styles the no-JavaScript submit control. |
themes/src/main/resources/theme/base/login/logout-confirm.ftl |
Applies form, action, and link spacing styling. |
themes/src/main/resources/theme/base/login/login-x509-info.ftl |
Enables secondary button styling for Ignore. |
themes/src/main/resources/theme/base/login/login-idp-link-confirm-override.ftl |
Adds shared form and action-container styling. |
Review details
- Files reviewed: 10/10 changed files
- Comments generated: 0
- Review effort level: Lite (auto)
Note
Copilot is running an experiment and ran this review at Lite.
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Unreported flaky test detectedIf the flaky tests below are affected by the changes, please review and update the changes accordingly. Otherwise, a maintainer should report the flaky tests prior to merging the PR. org.keycloak.testsuite.adapter.servlet.SAMLServletAdapterTest#salesMetadataTestKeycloak CI - Adapter IT Strict Cookies |
There was a problem hiding this comment.
@kathari00 Thanks for the PR!
I see that for the base theme, we use the <button> elements that causes so many unstable styling with the login v2.
What if we migrate the buttons macro in the base theme and reuse it as much as possible?
| <div class="${properties.kcFormButtonsClass!}"> | ||
| <#if isAppInitiatedAction??> | ||
| <input type="submit" | ||
| class="${properties.kcButtonClass!} ${properties.kcButtonPrimaryClass!} ${properties.kcButtonLargeClass!}" |
There was a problem hiding this comment.
Would it be possible to reuse the buttons macro?
There was a problem hiding this comment.
Would it make sense to move all reusable components into base generally? I can propose something. This would make custom theme styling a lot easier (I tried to do it with Bootstrap and Carbon).
There was a problem hiding this comment.
To make my earlier suggestion more concrete i would start with divider.ftl and qr-code.ftl, since both already use theme properties for styling. Another candidate is shared checkbox rendering eg in logoutOtherSessions. Does that scope make sense and would you prefer a separate PR?
There was a problem hiding this comment.
@kathari00 We can provide additional macros in the follow-ups, but I agree.
280910a to
9032cc0
Compare
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Critical test-resource and logout-selector failures remain, along with three theme compatibility regressions.
Get a fresh assessment by requesting another Copilot review.
Review effort: Balanced
Findings: 2
9032cc0 to
3bbe5e7
Compare
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Several test-framework/ui page objects still target submit inputs and will fail after controls migrate to buttons.
Get a fresh assessment by requesting another Copilot review.
Review effort: Balanced
Findings: 2
3bbe5e7 to
1a436d8
Compare
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Three moderate styling and theme-inheritance compatibility issues remain unresolved.
Get a fresh assessment by requesting another Copilot review.
Review effort: Balanced
Findings: 2
Open (6)
1a436d8 to
c94355d
Compare
7e6e618 to
743fdfa
Compare
|
As discussed offline, we're not going to introduce the |
| <form id="kc-select-credential-form" class="${properties.kcFormClass!}" action="${url.loginAction}" method="post"> | ||
| <input type="hidden" name="authenticationExecution" value="${authenticationSelection.authExecId}"> | ||
| </form> | ||
| <div role="button" class="${properties.kcSelectAuthListItemClass!}" tabindex="0" |
There was a problem hiding this comment.
wouldn't it be nicer to have button type="submit" then we don't need javascript for this
There was a problem hiding this comment.
@edewit Yep, I think there might be more stuff that we can polish here. We can do it in a follow-up.
There was a problem hiding this comment.
Thanks a lot @kathari00! It's a big change. I have just tested a bit and I see no issues in the login pages. So I will approve based on the previous reviews. But there is a conflict in the upgrading note (I suppose there is a new note added recently, just move your note after the new one).
Move all login pages, macros and the layout (template.ftl) from keycloak.v2 into the Base theme. Base reads every framework class from theme properties, so keycloak.v2 now consists only of theme.properties and resources, and other themes can restyle the login pages through properties or by overriding single macros instead of whole pages. Closes keycloak#52723 Signed-off-by: kathari00 <katharina.eiserfey1@gmail.com> Co-authored-by: Martin Bartoš <mabartos@redhat.com>
743fdfa to
15c9510
Compare
|
@rmartinc Ahh, I've just fixed the conflict, but you were faster :) |
|
👍 No problem @mabartos! I will approve when CI passes. |
|
@rmartinc Ok, thanks! Tip: you can use the auto-merge that merges the PR automatically if the CI passes. |
There was a problem hiding this comment.
🔵 Needs a closer look
Broad authentication-template and theme changes lack fresh mobile/desktop rendering comparisons and browser-level flow validation needed to assess regressions.
No open issues 🎉
1 resolved since last review
Review effort: Balanced
Stian commented only about the legacy base user and we discussed about it offline, and we're on the same page now. Right now, the existing reviews are sufficient.
Closes #52723
Closes #54547
Summary
keycloak.v2now supplies theme properties and resources and inherits its FreeMarker templates from Base.Migration
Custom login themes with
parent=basewill inherit the new Base markup. Review and update their template overrides and CSS to match the new structure. The upgrade guide documents this change.Validation
A fresh rendered-state comparison and manual authentication-flow check have not yet been completed for the current revision.
Follow-ups
Additional alert, form, and header customization hooks can be handled in follow-up changes.
Editor-specific padding and bottom-alignment CSS are outside this PR.