Conversation
☁️ Nx Cloud ReportCI is running/has finished running commands for commit 5e8f50e. As they complete they will appear below. Click to see the status, the terminal output, and the build insights. 📂 See all runs for this branch ✅ Successfully ran 7 targets
Sent with 💌 from NxCloud. |
| this.metrics.flush(); | ||
|
|
||
| // Sync service requires a password to be set before it can be used. | ||
| if (this.relier.isSync()) { |
There was a problem hiding this comment.
Should we also check if the account is missing a password?
There was a problem hiding this comment.
Ah yea we should, updating
There was a problem hiding this comment.
I tested this a few more times, a user won't be able to do social login for Sync. They are almost prompted to enter password if they have it.
| </div> | ||
|
|
||
| <div class="tooltip-container mb-5"> | ||
| <input id="vpassword" type="password" class="input-text tooltip-below" placeholder="{{#t}}Repeat password{{/t}}" pattern=".{8,}" required data-synchronize-show="true" /> |
There was a problem hiding this comment.
Does this need name="vpassword" also to match L20?
|
|
||
| <div class="input-balloon hidden" id="vpassword-balloon"> | ||
| <div class="before:content-key flex before:w-8"> | ||
| <p class="ltr:pl-2 rtl:pr-2"> |
There was a problem hiding this comment.
@LZoog / @vpomerleau can we use ps-2 here instead of bi-di specific props?
| <p class="ltr:pl-2 rtl:pr-2"> | |
| <p class="ps-2"> |
| {{#t}}Choose what to sync{{/t}} | ||
| </h2> | ||
| </header> | ||
| <div class="flex flex-wrap text-start ltr:mobileLandscape:ml-6 rtl:mobileLandscape:mr-6 mb-1"> |
There was a problem hiding this comment.
Same Q here re: ltr:mobileLandscape:ml-6 rtl:mobileLandscape:mr-6 versus ms-6
| <div class="flex flex-wrap text-start ltr:mobileLandscape:ml-6 rtl:mobileLandscape:mr-6 mb-1"> | |
| <div class="flex flex-wrap text-start mobileLandscape:ms-6 mb-1"> |
| </header> | ||
| <div class="flex flex-wrap text-start ltr:mobileLandscape:ml-6 rtl:mobileLandscape:mr-6 mb-1"> | ||
| {{#engines}} | ||
| <div class="mb-4 relative flex-50% rtl:mobileLandscape:pr-6 ltr:mobileLandscape:pl-6 rtl:pr-3 ltr:pl-3 flex items-center"> |
There was a problem hiding this comment.
… and here:
| <div class="mb-4 relative flex-50% rtl:mobileLandscape:pr-6 ltr:mobileLandscape:pl-6 rtl:pr-3 ltr:pl-3 flex items-center"> | |
| <div class="mb-4 relative flex-50% mobileLandscape:ps-6 ps-3 flex items-center"> |
Disclaimer: I'm very possibly not correct, so I'll defer to Tailwind aficionados @vpomerleau and @LZoog.
| </p> | ||
|
|
||
| <div class="tooltip-container mb-5"> | ||
| <input name="password" id="password" type="password" class="input-text tooltip-below" placeholder="{{#t}}Password{{/t}}" value="{{ password }}" pattern=".{8,}" required autofocus data-form-prefill="password" data-synchronize-show="true" /> |
There was a problem hiding this comment.
HHmm, I should probably know this, but do these <input> fields need a <label> for a11y reasons? Or maybe a screen-reader-only label field isn't great for a11y-ing.
/summon my a11y gurus @sardesam and @xlisachan
There was a problem hiding this comment.
@pdehaan I'm gonna defer some of these changes for now, they were taken from other pages.
There was a problem hiding this comment.
Yes. There are some exceptions (like a submit button for example), but in general they all should. Placeholders do not replace labels and while seen by sighted customers, they are not heard by screenreaders or other ATs.
As suggested, they can be hidden visually, but they should be included for a11y.
packages/fxa-content-server/app/scripts/views/post_verify/third_party_auth/set_password.js
Outdated
Show resolved
Hide resolved
| const PASSWORD_INPUT_SELECTOR = '#password'; | ||
| const VPASSWORD_INPUT_SELECTOR = '#vpassword'; |
There was a problem hiding this comment.
Q: should these be exported?
Looks like we have similar consts in
| const PASSWORD_INPUT_SELECTOR = '#password'; | ||
| const VPASSWORD_INPUT_SELECTOR = '#vpassword'; |
There was a problem hiding this comment.
I left a note above about import/exporting these from
But maybe they're far enough apart (maybe ../../../../scripts/views/post_verify/third_party_auth/set_password.js) that hardcoding them in each file is better. 🤷
dschom
left a comment
dschom
left a comment
There was a problem hiding this comment.
Just got done with some manual testing. It looks great and works as expected. 👍
I noticed that after supplying the password, I am no longer given the option to to sign in with google when signing in from sync. Is this expected? Regardless, it doesn't seem directly related to these changes.
Correct that is expected. Sync requires a password so if we showed the social options, they would still need to enter password after third party auth. |
5 participants
Because
This pull request
Issue that this pull request solves
Closes: https://mozilla-hub.atlassian.net/browse/FXA-8033
Checklist
Screenshots
Please attach the screenshots of the changes made in case of change in user interface.
Notes
To test, login via
http://localhost:8080using "Sign-in (Google)" link. After logging in, log into Sync via hamburger menu. You should be prompted to set a password after completing the Google auth flow.