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

Login-page a11y improvements #27193

Merged
merged 8 commits into from
Feb 28, 2024
Merged

Login-page a11y improvements #27193

merged 8 commits into from
Feb 28, 2024

Conversation

pkeuter
Copy link
Contributor

@pkeuter pkeuter commented Feb 21, 2024

Closes #27190

Changes:

@pkeuter
Copy link
Contributor Author

pkeuter commented Feb 23, 2024

Thanks for the review @jonkoops. Like I said in the PR-description, this is a script from W3C, which is now placed in this repo without any modifications. If you want all these changes, I think it is better to rewrite from scratch. But to be honest, treating this as a library-file is not so bad, is it?

It's your choice obviously but I am not sure if I have the time currently to rewrite this file. Is there another way to incorporate this as a library file?

@jonkoops
Copy link
Contributor

If we are going to check something in it will have to be up to our code standards. If it is a library available on NPM it can be included as a 'common' package, which can be installed here.

@pkeuter
Copy link
Contributor Author

pkeuter commented Feb 26, 2024

Thanks for your reply @jonkoops. Other scripts, like https://github.com/keycloak/keycloak/blob/main/themes/src/main/resources/theme/base/login/resources/js/base64url.js are also committed using functions that do not use the standards of Keycloak since they are just used as a library (I assume):

// for embedded scripts, quoted and modified from https://github.com/swansontec/rfc4648.js by William Swanson.

Can't we commit this file in the same fashion?

@jonkoops
Copy link
Contributor

Other scripts, like base64url.js are also committed using functions that do not use the standards of Keycloak since they are just used as a library.

Yes, this is true as in this case this is being refactored in another PR (#27239) to match our code conventions. The convention generally is:

  • If it is on NPM, and distributed in the ECMAScript module format, install it through NPM and include it on the page.
  • If it does not meet these conditions, relevant parts may be included as files in source, refactored to meet Keycloak standards.

@pkeuter
Copy link
Contributor Author

pkeuter commented Feb 26, 2024

Thanks, I will try to find a moment to refactor then. I think this PR brings some useful accessibility fixes to the login-page.

@jonkoops
Copy link
Contributor

I think this PR brings some useful accessibility fixes to the login-page.

For sure, would love to land this!

@pkeuter
Copy link
Contributor Author

pkeuter commented Feb 27, 2024

@jonkoops I think I have now implemented all your suggested changes. Please let me know if everything is okay now.

Copy link
Contributor

@jonkoops jonkoops left a comment

Choose a reason for hiding this comment

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

One small nit, otherwise LGTM

@jonkoops
Copy link
Contributor

@pkeuter Just FYI, we're developing a new login theme based on PatternFly 5, you might want to backport your efforts there as well.

@pkeuter
Copy link
Contributor Author

pkeuter commented Feb 28, 2024

@pkeuter Just FYI, we're developing a new login theme based on PatternFly 5, you might want to backport your efforts there as well.

That's good to know. I'll look into that later.

@jonkoops jonkoops enabled auto-merge (squash) February 28, 2024 11:14
@ahus1 ahus1 self-assigned this Feb 28, 2024
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.

approving as of @jonkoops's review.

@jonkoops
Copy link
Contributor

The FIPS test failures are unrelated, we'll have to a rebase after those have been fixed under #27346

Signed-off-by: Peter Keuter <github@peterkeuter.nl>
Signed-off-by: Peter Keuter <github@peterkeuter.nl>
Signed-off-by: Peter Keuter <github@peterkeuter.nl>
Signed-off-by: Peter Keuter <github@peterkeuter.nl>
Signed-off-by: Peter Keuter <github@peterkeuter.nl>
Signed-off-by: Peter Keuter <github@peterkeuter.nl>
Signed-off-by: Peter Keuter <github@peterkeuter.nl>
Signed-off-by: Peter Keuter <github@peterkeuter.nl>
@ahus1 ahus1 merged commit 9cced05 into keycloak:main Feb 28, 2024
65 checks passed
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.

a11y improvements on login page
3 participants