Skip to content

Conversation

@UjjawalPrabhat
Copy link

@UjjawalPrabhat UjjawalPrabhat commented Nov 25, 2025

Requirements

  • This PR has a title that briefly describes the work done including the ticket number. Ensure your PR title includes a conventional commit label (such as feat, fix, or chore, among others). See existing PR titles for inspiration.

If applicable

  • My work is based on designs, which are linked or shown either in the Jira ticket or the description below.
  • My work includes tests or is validated by existing tests.
  • I have updated the esm-framework mock to reflect any API changes I have made.

Summary

Fixes browser autofill for username and password on the login page. This was previously working but regressed after earlier changes.

Related Issue

Fixes O3-5243

Other

Changes

  • Render both username and password inputs in DOM on initial load
  • Hide password field visually (CSS) instead of removing from DOM
  • Add proper name and autocomplete attributes for browser autofill
  • Maintain accessibility with aria-hidden and tabIndex management
  • Add tests verifying autofill functionality

Checklist

  • Tests pass locally (yarn test)
  • Manually tested autofill in Chrome and Firefox
  • No breaking changes to existing login flow
  • Accessibility attributes maintained
  • i18n support preserved

UjjawalPrabhat and others added 2 commits November 26, 2025 01:54
- Render both username and password inputs in DOM on initial load
- Hide password field visually using CSS (not display:none) until user continues
- Add proper name and autocomplete attributes for browser autofill
- Maintain accessibility with aria-hidden and tabIndex management
- Preserve existing two-step login UX
- Add tests verifying DOM presence and autofill attributes

Fixes regression where browser credential autofill stopped working after PR openmrs#104
@brandones
Copy link
Contributor

Hi @UjjawalPrabhat , please share a screen recording demonstrating the fixed behavior.

@UjjawalPrabhat
Copy link
Author

Hi @UjjawalPrabhat , please share a screen recording demonstrating the fixed behavior.

Screen.Recording.2025-12-02.180446.mp4

@brandones
Copy link
Contributor

Thanks @UjjawalPrabhat . Shame that we don't get the full credential chooser UI (see image) but based on my reading it seems like that's not possible with a two-step login.

image

@UjjawalPrabhat please fix the E2E tests, I think the breakage (at least for the esm-core ones) are caused by this PR.

UjjawalPrabhat and others added 2 commits December 3, 2025 00:45
- Update login-page.ts with proper waits for password field visibility
- Add waitFor({ state: 'visible' }) after clicking Continue
- Add waitForLoadState('networkidle') after login completes
- Handle session state clearing for fresh login tests
- Fixes timeout issues in login.spec.ts, logout.spec.ts, navbar.spec.ts

Note: navbar.spec.ts passes login flow but fails on unrelated button assertions
(buttons not present on OpenMRS instance configuration)
@UjjawalPrabhat
Copy link
Author

UjjawalPrabhat commented Dec 2, 2025

Thanks @UjjawalPrabhat . Shame that we don't get the full credential chooser UI (see image) but based on my reading it seems like that's not possible with a two-step login.

image @UjjawalPrabhat please fix the E2E tests, I think the breakage (at least for the esm-core ones) are caused by this PR.

Hey @brandones,
I've updated the PR with fixes for the E2E test failures.

Update:

  • Fixed login.spec.ts and logout.spec.ts by updating the login helper to wait for password field visibility.
  • navbar.spec.ts is now passing the login step successfully, but fails on unrelated assertions (missing buttons on the test instance), which seems pre-existing.

Regarding the UI, the full credential dropdown does appear after clicking "Continue", just not on the initial screen due to the two-step flow limitation.
Screenshot (412)

expect(continueButton).toBeInTheDocument();
expect(passwordInput).not.toBeInTheDocument();
// Password field is now always in DOM for autofill support, but hidden
expect(passwordInput).toBeInTheDocument();
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you try expect(passwordInput).not.toBeVisible()? Don't worry if it doesn't work, but if it does work it would be a nice way to express this.

Copy link
Author

Choose a reason for hiding this comment

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

I gave expect(passwordInput).not.toBeVisible() a try, but unfortunately, it didn't work as expected in this environment.

jest-dom considers the element "visible" because it's present in the DOM with visibility: hidden applied via a CSS class, rather than inline styles or the hidden attribute, which the matcher doesn't fully process in this test setup.

I've kept the explicit aria-hidden and tabIndex assertions to verify the behavior correctly.

Copy link
Contributor

@brandones brandones left a comment

Choose a reason for hiding this comment

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

See comments

@brandones
Copy link
Contributor

Also just for posterity: it is not possible at this time to E2E test autofill. See issue microsoft/playwright#26831

- Make location selection logic deterministic in E2E tests
- Delete unhelpful comments referencing ticket number in test files
- Remove incorrect CSS comment about pointer-events
- Delete implementation-detail test that doesn't test user behavior
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants