+
Skip to content

Removed hard-coded LTR dir attribute in inputs in configure OTP template #33652

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Oct 9, 2024
Merged

Removed hard-coded LTR dir attribute in inputs in configure OTP template #33652

merged 1 commit into from
Oct 9, 2024

Conversation

senyahnoj
Copy link
Contributor

This should be done in the html opening tag only (see issue #33642)

Closes #32844

Removed hard-coded LTR dir attribute in inputs in configure OTP template
4c1d0ce 
This should be done in the html opening tag only (see issue keycloak#33642)

Closes keycloak#32844

Signed-off-by: Jon Haynes <senyahnoj@gmail.com>
@senyahnoj senyahnoj requested review from a team as code owners October 7, 2024 16:19
@jonkoops
Copy link
Contributor

jonkoops commented Oct 7, 2024

Thanks for the PR @senyahnoj. I'll also mark the issue as something to backport for v26 so that we can put this fix in a patch release.

ahus1
ahus1 requested changes Oct 7, 2024
View reviewed changes
Copy link
Contributor

@ahus1 ahus1 left a comment

Choose a reason for hiding this comment

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

Thank you for the PR. The old theme had additional hard-coded dir attributes whenever there was a password with the show-password-toggle.

keycloak/themes/src/main/resources/theme/base/login/login-password.ftl

Lines 13 to 24 in 10fc2e9

<div class="${properties.kcInputGroup!}" dir="ltr">
<input tabindex="2" id="password" class="${properties.kcInputClass!}" name="password"
type="password" autocomplete="on" autofocus
aria-invalid="<#if messagesPerField.existsError('password')>true</#if>"
/>
<button class="${properties.kcFormPasswordVisibilityButtonClass!}" type="button" aria-label="${msg('showPassword')}"
aria-controls="password" data-password-toggle
data-icon-show="${properties.kcFormPasswordVisibilityIconShow!}" data-icon-hide="${properties.kcFormPasswordVisibilityIconHide!}"
data-label-show="${msg('showPassword')}" data-label-hide="${msg('hidePassword')}">
<i class="${properties.kcFormPasswordVisibilityIconShow!}" aria-hidden="true"></i>
</button>
</div>

Can you please comment if this should be restored? This was the original intent for this issue.

@keycloak-github-bot keycloak-github-bot bot mentioned this pull request Oct 7, 2024
Closed
@senyahnoj
Copy link
Contributor Author

Thanks @ahus1, I've had a look at the base theme as well and I see the hard-coded dir=ltr in a few places. Given that it's set in the HTML tag there is no need to override it.

I am new to contributing to this project but from the guidelines it looks like a single/rebased commit per issue is desirable. Therefore I'll close this PR and create a new one with the fix in both keycloak.v2 and base themes.

@senyahnoj senyahnoj closed this Oct 8, 2024
@senyahnoj senyahnoj deleted the keycloak.v2-login-hardcoded-dir branch October 8, 2024 07:37
@ahus1
Copy link
Contributor

ahus1 commented Oct 8, 2024

it looks like a single/rebased commit per issue is desirable

True, this is desired. If there is a good reason to keep two commits, that is possible when there is a justification. You can also agree with the reviewer to keep different commits for the review, and then the final approver will squash the changes when merging them to main.

@senyahnoj senyahnoj restored the keycloak.v2-login-hardcoded-dir branch October 8, 2024 15:47
@senyahnoj
Copy link
Contributor Author

Re-opening PR as we have decided to only make the change in keycloak.v2 theme

@senyahnoj senyahnoj reopened this Oct 8, 2024
@senyahnoj senyahnoj mentioned this pull request Oct 8, 2024
Closed
@jonkoops jonkoops requested a review from ahus1 October 9, 2024 15:38
ahus1
ahus1 approved these changes Oct 9, 2024
View reviewed changes
Copy link
Contributor

@ahus1 ahus1 left a comment

Choose a reason for hiding this comment

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

Thank you for everyone involved in this, and the discussions we had to find the right fix.

@ahus1 ahus1 merged commit df30358 into keycloak:main Oct 9, 2024
140 checks passed
@jonkoops
Copy link
Contributor

jonkoops commented Oct 9, 2024

Thanks @senyahnoj. I went ahead and created the backport as well: #33754

@senyahnoj senyahnoj deleted the keycloak.v2-login-hardcoded-dir branch October 9, 2024 15:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ahus1 ahus1 ahus1 approved these changes

+1 more reviewer

@jonkoops jonkoops jonkoops approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

flaky-test team/ui

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Login V2: Missing "dir" attributes

3 participants

@senyahnoj @jonkoops @ahus1
点击 这是indexloc提供的php浏览器服务，不要输入任何密码和下载