kratos icon indicating copy to clipboard operation
kratos copied to clipboard

Selfservice: change flow sort, oidc below password

Open afreakk opened this issue 3 years ago • 5 comments

Changed the order, to have password group above oidc, to fix so the top submit button is not the oidc. So enter = password, not oidc

There might be a reason for the previous order I dont know about.
But using nextjs or nodejs selfservice reference implementation with oidc enabled, it will render oidc on top which causes enter press inside a input to trigger the oidc action.
Which is very annoying :)
image image

Related issue(s)

Checklist

  • [ ] I have read the contributing guidelines.
  • [ ] I have referenced an issue containing the design document if my change introduces a new feature.
  • [ ] I am following the contributing code guidelines.
  • [ ] I have read the security policy.
  • [x] I confirm that this pull request does not address a security vulnerability. If this pull request addresses a security. vulnerability, I confirm that I got green light (please contact [email protected]) from the maintainers to push the changes.
  • [ ] I have added tests that prove my fix is effective or that my feature works.
  • [ ] I have added or changed the documentation.

Further Comments

I have not tested webauthn and the others, so, dont know if this is suboptimal in relation to them.

afreakk avatar Jun 17 '22 16:06 afreakk

Codecov Report

Merging #2529 (72c2abe) into master (7b966bd) will decrease coverage by 0.02%. The diff coverage is 100.00%.

:exclamation: Current head 72c2abe differs from pull request most recent head 79272d5. Consider uploading reports for the commit 79272d5 to get more accurate results

@@            Coverage Diff             @@
##           master    #2529      +/-   ##
==========================================
- Coverage   76.20%   76.18%   -0.03%     
==========================================
  Files         318      318              
  Lines       17728    17728              
==========================================
- Hits        13510    13506       -4     
- Misses       3280     3283       +3     
- Partials      938      939       +1     
Impacted Files Coverage Δ
selfservice/flow/login/sort.go 100.00% <100.00%> (ø)
selfservice/flow/registration/sort.go 100.00% <100.00%> (ø)
session/test/persistence.go 98.61% <0.00%> (-1.39%) :arrow_down:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update ac847bb...79272d5. Read the comment docs.

codecov[bot] avatar Jun 17 '22 17:06 codecov[bot]

Thank you, that is indeed annoying! But instead of re-ordering, could we use tabindex instead?

aeneasr avatar Jun 17 '22 17:06 aeneasr

yeah, that would require supporting code in the frontends though, and would still render the google login button above the rest.
I dont care much about that, but I think normally login with google is below the usr/pw input fields.

afreakk avatar Jun 18 '22 08:06 afreakk

yeah, that would require supporting code in the frontends though

That is relatively straight forward :) Basically just needs to be added:

https://github.com/ory/kratos-selfservice-ui-node/blob/7d5ac8831dc50708d4a0df4495ba6dd450595c21/views/partials/ui_node_input_default.hbs#L8-L14

and here:

https://github.com/ory/kratos-selfservice-ui-react-nextjs/blob/f3c98d927a2481688068c4e262298c05d70411e6/pkg/ui/NodeInputDefault.tsx#L27-L37

aeneasr avatar Jun 18 '22 09:06 aeneasr

This works: image

I tried changing tabindex, but tabindex doesnt seem to affect what action gets run when pressing enter in a form.
The negative thing about changing to type=button would be if you disable password login and only have webauthn signin, enter wouldnt do anything, and using enter for "submiting" email/first/last -name and use webauthn would make sense. In that way, if tabindex actually worked, it would be better, so it will always fallback to whatever button is remaining. button sorting-order would also work for that.

afreakk avatar Jun 20 '22 09:06 afreakk

As there hasn't been any progress on this PR, I'm closing it as we do not want to change the order globally. But you're of course always welcomed to contribute changes to tabindex as discussed :)

aeneasr avatar Oct 04 '22 12:10 aeneasr