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

Help browser account saving with accounts-ui login/signup forms #9442

Merged
merged 3 commits into from Dec 13, 2017

Conversation

@hwillson
Copy link
Member

@hwillson hwillson commented Dec 5, 2017

accounts-ui-unstyled currently uses <div />'s to hold its login/signup forms, as well as <div />'s to represent the login/signup buttons in the form. By not using proper <form /> and <button /> elements, certain browser's do not notice incoming login/signup requests, and therefore do not trigger their built in "would you like to save your user/password" functionality. This commit adjusts the accounts-ui-unstyled login/signup form to use proper <form /> and <button /> elements, allowing most browsers (Chrome, Firefox, IE - Safari will recognize the request when a user attempts to leave the page) to recognize incoming login/signup requests.

Fixes #1746.

`accounts-ui-unstyled` currently uses `<div />`'s to hold its
login/signup forms, as well as `<div />`'s to represent the
login/signup buttons in the form. By not using proper
`<form />` and `<button />` elements, certain browser's do not
notice incoming login/signup requests, and therefore do not
trigger their built in "would you like to save your user/password"
functionality. This commit adjusts the `accounts-ui-unstyled`
login/signup form to use proper `<form />` and `<button />`
elements, allowing most (Chrome, Firefox, IE - Safari will
recognize the request when a user attempts to leave the page)
browsers to recognize incoming login/signup requests.

Fixes #1746.
@benjamn
benjamn approved these changes Dec 6, 2017
Copy link
Member

@benjamn benjamn left a comment

This should be a patch update for the accounts-ui-unstyled package, right?

@hwillson
Copy link
Member Author

@hwillson hwillson commented Dec 6, 2017

@benjamn Normally I would consider this a package patch, but I just thought of something. Since we're changing some of the HTML elements used by the login/signup form, if anyone is styling based on explicit HTML element names instead of using the class/id selectors, this could be a backwards incompatible change. So if someone is adding custom styles like div.login-form { ... } for example, their styles will break now that div has been replaced with form. In the name of progress however, we should still consider moving ahead with these changes, but I'll add a History.md entry explaining the potential backwards compatibility issues. Because of this, maybe we should tie these changes to release 1.6.1 (giving people a better chance of seeing this potential issue in the change log)?

@abernix
Copy link
Member

@abernix abernix commented Dec 12, 2017

That's a good point @hwillson. It sounds reasonable to me to tie this to 1.6.1 — which means it would earn a minor version bump, in addition to a minor version bump on accounts-ui which depends on it, correct?

(Otherwise this LGTM.)

@hwillson
Copy link
Member Author

@hwillson hwillson commented Dec 13, 2017

Minor versions bumped - thanks @abernix!

@benjamn benjamn merged commit 56a86bf into meteor:devel Dec 13, 2017
12 checks passed
12 checks passed
CLA Author has signed the Meteor CLA.
Details
ci/circleci: Get Ready Your tests passed on CircleCI!
Details
ci/circleci: Group 0 Your tests passed on CircleCI!
Details
ci/circleci: Group 1 Your tests passed on CircleCI!
Details
ci/circleci: Group 2 Your tests passed on CircleCI!
Details
ci/circleci: Group 3 Your tests passed on CircleCI!
Details
ci/circleci: Group 4 Your tests passed on CircleCI!
Details
ci/circleci: Group 5 Your tests passed on CircleCI!
Details
ci/circleci: Group 6 Your tests passed on CircleCI!
Details
ci/circleci: Group 7 Your tests passed on CircleCI!
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants