Fix icon placement for two step login scenario where username is hidden#2960
Fix icon placement for two step login scenario where username is hidden#2960JanSvoboda wants to merge 3 commits intokeepassxreboot:developfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR fixes login-form icon placement for two-step login flows where the username field becomes hidden on the password step, ensuring the icon is placed on the visible password input instead.
Changes:
- Only place the default icon on the username field when it is visible.
- Fall back to placing the default icon on the password field when the username field is missing, read-only, or hidden (two-step login scenario).
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
This repository does not accept AI contributors, or AI-assisted commits. See https://github.com/keepassxreboot/keepassxc-browser/blob/develop/.github/CONTRIBUTING.md#using-ai. |
|
Hello, |
|
For me it showed that Copilot created this pull request. Now it doesn't show that? Strange. The Copilot review stuff is temporary enabled to see if it helps. |
|
ok, thank you, Copilot didn't do that PR, I did, I was afraid that I screwed something up |
|
might I have a question? copilot suggested logical change, if I were to implement it as suggested, how would it be evaluated? as AI assisted or not? |
It would be AI assisted :) I'd rather just take some hints from its suggestions, but not implement anything directly. I'm also thinking disabling Copilot here. Sometimes its suggestions are pretty random. |
| } | ||
|
|
||
| if (c.username && !c.username.readOnly) { | ||
| if (c.username && !c.username.readOnly && kpxcFields.isVisible(c.username)) { |
There was a problem hiding this comment.
Use:
const usernameFieldVisible = c.username && kpxcFields.isVisible(c.username);
| kpxcIcons.addIcon(c.username, kpxcIcons.iconTypes.DEFAULT); | ||
| } else if (c.password && (!c.username || (c.username && c.username.readOnly))) { | ||
| // Single password field | ||
| } else if (c.password && (!c.username || c.username.readOnly || !kpxcFields.isVisible(c.username))) { |
There was a problem hiding this comment.
Shouldn't this be:
} else if (c.password && (!c.username || (c.username && (c.username.readOnly || !usernameFieldVisible)))) {
So the single password field applies if:
- Password field is defined
- Usename field is not found OR
- Username field is found, but is readOnly or not visible
|
I would need some actual site where the error happens and I can confirm the fix works. |
Co-authored-by: Copilot <copilot@github.com>
|
I reflected your suggestions. I can provide you with some website, however I would rather avoid posting it here, if possible, is there some way how can I send it to you directly? |
Private message in Matrix, or via email. |
In some two-step login scenarios, where the username is filled during the first step and the password is provided in the next, the icon placement might not work properly if the username input is kept hidden.
This fix checks whether username input is visible, if not then icon is displayed on password input.
Screenshots or videos
Testing strategy
Manually tested on some corporate publicly inaccessible login screen. Currently I do not know of any other site that would have the same behaviour.
Type of change