Migrate kc themes to vaadin - #33
Conversation
7b2e9fe to
733a4af
Compare
733a4af to
49f1d39
Compare
49f1d39 to
0343e75
Compare
Vaadin's InputControlMixin manages the slotted <input>: it replaces the id and silently drops name, value, required and autocomplete, re-delegating its own from the host element. Setting them on the input rendered a correct-looking but unnamed field, so every form posted nothing but its submit button (login=). Move them onto the component in field() and on the two hand-written checkboxes, and hide Lumo's required indicator, which the design does not show.
wborn
left a comment
wborn
left a comment
There was a problem hiding this comment.
This review was AI-assisted. The implementation and relevant Keycloak/OpenRemote templates were compared to identify behavioral and compatibility differences.
Thanks for the work on this migration. The overall approach looks promising: using the OpenRemote Vaadin design system, keeping the page implementations typed and relatively small, retrieving branding from the manager configuration, and falling back to Keycloak's parent theme for pages OpenRemote does not customize should make the theme considerably easier to maintain.
The implementation was also compared with the old OpenRemote FreeMarker theme and the Keycloak 26.7 templates. The OTP/TOTP implementation in particular looks good and preserves the important behavior from the old theme.
There are, however, a few compatibility gaps that should be addressed before merging:
- The login page no longer handles Keycloak's hidden/preselected username state and loses the selected credential state.
- Some shared authentication controls are missing, including the locale selector and "try another way".
- Registration uses the Keycloak user profile, but currently treats essentially every custom attribute as a text field and doesn't handle terms acceptance or the full reCAPTCHA behavior.
- Global message suppression only knows about a small hard-coded set of fields and can therefore duplicate validation errors.
- The old login page's self-registration link has intentionally been removed. If that is a product decision, that is fine, but it does mean the new theme is not fully feature-equivalent to the old one.
These compatibility issues should therefore either be addressed or explicitly documented as intended changes before merging.
| <form id="kc-form-login" action=${url.loginAction} method="post"> | ||
| ${field({ | ||
| kcContext, | ||
| name: "username", |
There was a problem hiding this comment.
Could we preserve Keycloak's username-hidden flow here? The old OpenRemote template already handled the equivalent usernameEditDisabled state, and Keycloak 26.7 now uses usernameHidden: it omits the username field and Remember me in that state. The stock template also carries auth.selectedCredential as credentialId.
As written, a flow where Keycloak has already established the username will render and submit an editable username again, and selected-credential state is lost. Could we mirror the relevant Keycloak 26.7 login semantics here?
| ` | ||
| : null} | ||
| ` | ||
| // No "New user? Register" footer: the design's login card ends at "Forgot password?" and |
There was a problem hiding this comment.
The old OpenRemote theme exposed self-registration here when the realm allowed it, and Keycloak 26.7 still does the same. Removing the link means a realm with self-registration enabled no longer exposes that capability from its login page.
If dropping this is an intentional product decision that's fine, but otherwise the conditional registration link should probably be retained.
| <span>${message.summary}</span> | ||
| </div>` | ||
| : null} | ||
| ${content} |
There was a problem hiding this comment.
The shared layout should preserve the common Keycloak flow controls rather than only the page body. At minimum the old OpenRemote theme had the locale selector, and Keycloak 26.7's shared template also renders auth.showTryAnotherWayLink() and the organization-switch action when enabled.
Without tryAnotherWay, a user can lose the UI for switching to another configured authentication method. Could we add these shared controls to the new layout, including the locale selector when multiple languages are available?
| and free text for anything a realm has added; advancedMsgStr handles both, | ||
| and falls back to the attribute name when there is neither. */ | ||
| label: advancedMsgStr(attribute.displayName ?? attribute.name), | ||
| type: attribute.name === "email" ? "email" : "text", |
There was a problem hiding this comment.
Since this now drives registration from profile.attributesByName, the profile field metadata should also be honored rather than treating every custom attribute as text. Keycloak supports selects/multiselects, radio/checkbox options, textareas, multivalued/read-only fields and HTML5 annotations here.
There are also registration controls outside the profile, notably termsAcceptanceRequired and the visible/invisible reCAPTCHA variants. With terms required, for example, this form currently has no termsAccepted control at all, so the user cannot complete registration.
Could we either implement the supported Keycloak registration semantics or deliberately fall back to the parent registration page for configurations we don't support?
| */ | ||
| export function layout(options: LayoutOptions): TemplateResult { | ||
| const { kcContext, heading, intro, back, suppressWarning, content } = options; | ||
| const hasFieldError = kcContext.messagesPerField.existsError( |
There was a problem hiding this comment.
This global field list looks fragile now that registration is profile-driven. Errors for firstName, lastName, password-confirm, userLabel, termsAccepted, or any custom profile attribute aren't included, so field() can render the field error while the same validation failure is also shown in the global alert.
Could the page tell layout() which fields it owns, or could we follow Keycloak's distinction between global and field messages instead of maintaining a hard-coded list here?
2 participants
Description
Checklist