Skip to content

LG-9710: Add hidden field to associate password with email#8372

Merged
aduth merged 1 commit intomainfrom
lg-9710-save-password
May 11, 2023
Merged

LG-9710: Add hidden field to associate password with email#8372
aduth merged 1 commit intomainfrom
lg-9710-save-password

Conversation

@aduth
Copy link
Contributor

@aduth aduth commented May 10, 2023

🎫 Ticket

LG-9710

🛠 Summary of changes

Adds a hidden field to the password creation screen, so that browsers or password managers can associate the newly-created password as a full credential including the user's email address.

📜 Testing Plan

  1. Go to http://localhost:3000
  2. Click "Create an account"
  3. Continue account creation to the password entry screen
  4. Enter and confirm a password
  5. Submit
  6. Observe if/how the browser prompts you to save the password you just created

👀 Screenshots

Device Before After
Chrome (Desktop) chrome-before chrome-after
Chrome (Mobile) chrome-mobile-before chrome-mobile-after
Firefox firefox-before firefox-after
Safari (Desktop) safari-before safari-aftersafari-after-passwords
Safari (Mobile) safari-mobile-before safari-mobile-after

changelog: User-Facing Improvements, Account Creation, Improve support for browsers to prompt user to save password for new account
expect(rendered).to have_css('noscript link') do |node|
node[:href] == no_js_detect_css_path
end
expect(rendered).to have_css("noscript link[href='#{no_js_detect_css_path}']")
Copy link
Contributor Author

@aduth aduth May 10, 2023

Choose a reason for hiding this comment

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

This seemingly unrelated change is because I was trying to incorporate a similar use of the optional_block_filter feature of have_field in the spec that I did add, something like...

expect(rendered).to have_field('username', type: :text, with: user.email, visible: false) do |node|
  node[:autocomplete] == 'username'
end

But I discovered that apparently it doesn't actually filter at all? So the node[:href] check wasn't doing anything here previously (you could change == to != and it would still pass).

input_html: { aria: { describedby: 'password-description' } },
},
) %>
<%= text_field_tag('username', @email_address.email, hidden: true, autocomplete: 'username') %>
Copy link
Contributor Author

@aduth aduth May 11, 2023

Choose a reason for hiding this comment

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

Testing this in the real-world, it works well, and Chromium's guidance suggests that a visually-hidden [type=text] is the recommended approach, but I'm a bit concerned if a browser may try to auto-fill this field, even if visually hidden with the [hidden] attribute. I could imagine some strange scenarios if the user already had a saved credential with the site, if autofill could cause the user to be prompted to save that password for the wrong email address.

In reviewing the relevant specification, it describes how the behavior we want should be applied through [type=hidden]:

There are two ways this [autocomplete] attribute is used. When wearing the autofill expectation mantle, the autocomplete attribute describes what input is expected from users. When wearing the autofill anchor mantle, the autocomplete attribute describes the meaning of the given value.

On an input element whose type attribute is in the Hidden state, the autocomplete attribute wears the autofill anchor mantle. In all other cases, it wears the autofill expectation mantle.

tl;dr: [type=hidden] = "autofill anchor mantle" = "describes the meaning of the given value" (as opposed to "describes what input is expected from users")

I might try this out locally to see if it works the same to change [type=text] to [type=hidden], since it seems a little safer / closer to the spec..

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changing from [type=text] to [type=hidden], it still worked as expected in Safari, but it regressed to the original behavior in Firefox and Chrome.

I think this is the implementation we'll want to keep. For extra assurance, I tested autofill with a different credential's password in Chrome, Safari, and Firefox with [type=text][hidden], and none of them manipulated the value.

@aduth aduth merged commit 803eb17 into main May 11, 2023
@aduth aduth deleted the lg-9710-save-password branch May 11, 2023 13:55
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.

3 participants