Bug 1815564 - Modify password field text to using Roboto monospace and Added Asterisk over Bullet on password fields. #864

gitstart · 2023-02-17T09:43:31Z

Modify password field text to using Roboto monospace && add password reveal toggle on Add Login Screen.
Screens need refactor:

AddLoginFragment
LoginDetailsFragment
EditLoginFragment

Enhancement Task

Added Asterisk over Bullet

Issue Screenshots

fnxoss-40.webm

Fix Screenshots

fnxoss-40-fix.webm

Pull Request checklist

Tests: This PR includes thorough tests or an explanation of why it does not
Screenshots: This PR includes screenshots or GIFs of the changes made or an explanation of why it does not
Accessibility: The code in this PR follows accessibility best practices or does not include any user facing features. In addition, it includes a screenshot of a successful accessibility scan to ensure no new defects are added to the product.

QA

QA Needed

GitHub Automation
Fixes mozilla-mobile/fenix#28609
https://bugzilla.mozilla.org/show_bug.cgi?id=1815564
https://bugzilla.mozilla.org/show_bug.cgi?id=1815564
https://bugzilla.mozilla.org/show_bug.cgi?id=1815564
https://bugzilla.mozilla.org/show_bug.cgi?id=1815564
https://bugzilla.mozilla.org/show_bug.cgi?id=1815564
https://bugzilla.mozilla.org/show_bug.cgi?id=1815564

mergify · 2023-04-17T17:40:46Z

This pull request has conflicts when rebasing. Could you fix it @gitstart? 🙏

gabrielluong · 2023-07-10T16:54:58Z

fenix/app/src/main/java/org/mozilla/fenix/settings/logins/fragment/AddLoginFragment.kt

@@ -80,11 +82,9 @@ class AddLoginFragment : Fragment(R.layout.fragment_add_login), MenuProvider {
        )

        initEditableValues()
-


nits: We should avoid removing these new lines in the final revision of this PR. New lines are used to separate logical blocks in a way so that things are more readable.

mergify · 2023-07-10T22:20:38Z

This pull request has conflicts when rebasing. Could you fix it @gitstart? 🙏

titooan · 2023-07-17T10:03:20Z

Hey @debojyoti452,
Thanks a lot for this PR!

I saw the TODO (extend PasswordTransformationMethod() to change bullets to asterisks) was 3 years old so I checked with the UX team if we still want this.
The answer is: we currently use bullets on desktop and want to keep using them on mobile too, so we can remove this TODO. Can you please update this PR to remove the code that fixed this TODO?

I'm sorry for that, you did a good job anyways!

fenix/app/src/main/java/org/mozilla/fenix/settings/logins/fragment/AddLoginFragment.kt

fenix/app/src/main/res/layout/fragment_add_login.xml

titooan · 2023-07-10T10:57:04Z

fenix/app/src/main/java/org/mozilla/fenix/settings/logins/PasswordRevealHelper.kt

        revealPasswordButton.contentDescription =
            context.getString(R.string.saved_login_reveal_password)
    }
    // We need to reset to take effect
    passwordText.text = passwordText.text
+    passwordText.typeface = Typeface.MONOSPACE


This doesn't seem to be directly related to toggling the password "reveal state". I think it could be more coherent to move it to where the text field is configured instead.

github-actions · 2023-08-09T14:56:47Z

🚧 Commit message is using the wrong format: Import translations from android-l10n

The comment message should look like:

    Bug xxxx - Short description of your change
    Optionally, a longer description of the change.

github-actions · 2023-08-09T14:56:48Z

🚧 Commit message is using the wrong format: Import translations from android-l10n

The comment message should look like:

    Bug xxxx - Short description of your change
    Optionally, a longer description of the change.

gabrielluong · 2023-08-09T21:00:16Z

fenix/app/src/main/res/layout/fragment_add_login.xml

+    <ImageButton
+        android:id="@+id/revealPasswordButton"
+        android:layout_width="48dp"
+        android:layout_height="30dp"
+        android:layout_marginTop="3dp"
+        android:background="@null"
+        android:contentDescription="@string/saved_login_reveal_password"
+        app:tint="?attr/textPrimary"
+        app:layout_constraintEnd_toStartOf="@id/clearPasswordTextButton"
+        app:layout_constraintTop_toTopOf="@id/inputLayoutPassword"
+        app:srcCompat="@drawable/mozac_ic_eye_24" />


I am wondering if I missing something. I don't see anything about adding a reveal password functionality in the original requirements.

@cwzilla Could you clarify your intention from mozilla-mobile/fenix#28609? Were we suppose to add a button to reveal the password when adding a login?

Yes, correct, would like to add the eye icon to reveal/hide password on the password field on the Add a Login screen.

titooan

Sorry for the delay for reviewing the last changes.
Thanks for adressing all the issues we pointed out!

When testing it I realised that clicking the "reveal/unreveal password" button resets the field state, putting the carret at the beggining of the field. It doesn't sound ideal to me.
One way to solve this would be to use the passwordToggleEnabled attribute (see StackOverflow link for more info).

The problem is: it doesn't seem to be easy to place the visibility button where we want on the field, meaning I think it will need to lie at the end of the field.
We still need to position the "clear" button. It could be:

outside of the field, at its end
before the visibility button, in the field.

@cwzilla, do you know which option would be the best?

cwzilla · 2023-09-05T20:26:44Z

Sorry for the delay for reviewing the last changes. Thanks for adressing all the issues we pointed out!

When testing it I realised that clicking the "reveal/unreveal password" button resets the field state, putting the carret at the beggining of the field. It doesn't sound ideal to me. One way to solve this would be to use the passwordToggleEnabled attribute (see StackOverflow link for more info).

The problem is: it doesn't seem to be easy to place the visibility button where we want on the field, meaning I think it will need to lie at the end of the field. We still need to position the "clear" button. It could be:
* outside of the field, at its end

* before the visibility button, in the field.
@cwzilla, do you know which option would be the best?

Hi, I think in the field would look better!

mergify · 2024-02-28T22:39:08Z

This pull request has conflicts when rebasing. Could you fix it @gitstart? 🙏

mergify · 2024-03-04T18:38:15Z

This pull request has conflicts when rebasing. Could you fix it @gitstart? 🙏

mergify · 2024-03-15T07:47:11Z

This pull request has conflicts when rebasing. Could you fix it @gitstart? 🙏

debojyoti452 · 2024-03-17T20:07:01Z

hello @gabrielluong,
Is the issue still persists? do you think I can push the fix as the PR was raised by me

gabrielluong · 2024-04-03T06:30:27Z

Hello, Thank you so much for your contribution. We have moved our firefox-android repo over to mozilla-central. Please feel free to reopen a new PR against the new repo. You will have to clone the new repository and reapply your changes. Thank you again, and sorry for the inconvenience.

For more information, please see https://github.com/mozilla-mobile/firefox-android/wiki#upcoming-migration-to-mozilla-central.

gitstart marked this pull request as ready for review February 22, 2023 07:51

debojyoti452 force-pushed the FNXOSS-40 branch from 4bd132f to c5e95d2 Compare February 27, 2023 10:56

gitstart force-pushed the FNXOSS-40 branch from c5e95d2 to 19b8927 Compare March 1, 2023 06:57

titooan self-assigned this Jun 16, 2023

gabrielluong reviewed Jul 10, 2023

View reviewed changes

titooan reviewed Jul 17, 2023

View reviewed changes

github-actions bot added the 🕵️‍♀️ needs review PRs that need to be reviewed label Aug 9, 2023

Bug 1815564 - Modify password field text to using Roboto monospace

c47307d

gitstart force-pushed the FNXOSS-40 branch from e14697a to c47307d Compare August 9, 2023 15:07

gabrielluong reviewed Aug 9, 2023

View reviewed changes

gabrielluong requested review from gabrielluong and titooan August 10, 2023 17:44

github-actions bot assigned gabrielluong Aug 10, 2023

Merge branch 'main' into FNXOSS-40

32294a6

titooan suggested changes Sep 4, 2023

View reviewed changes

debojyoti452 added 2 commits September 14, 2023 02:36

Merge branch 'main' into FNXOSS-40

53f336d

Merge branch 'main' into FNXOSS-40

14d0dae

gabrielluong added the followup PR that needs to be followed up label Mar 15, 2024

Merge branch 'main' into FNXOSS-40

51a8d10

gabrielluong closed this Apr 3, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bug 1815564 - Modify password field text to using Roboto monospace and Added Asterisk over Bullet on password fields. #864

Bug 1815564 - Modify password field text to using Roboto monospace and Added Asterisk over Bullet on password fields. #864

gitstart commented Feb 17, 2023 •

edited by github-actions bot

Loading

mergify bot commented Apr 17, 2023

gabrielluong Jul 10, 2023

mergify bot commented Jul 10, 2023

titooan commented Jul 17, 2023

titooan Jul 10, 2023

github-actions bot commented Aug 9, 2023

github-actions bot commented Aug 9, 2023

gabrielluong Aug 9, 2023

gabrielluong Aug 9, 2023

cwzilla Aug 10, 2023

titooan left a comment

cwzilla commented Sep 5, 2023

mergify bot commented Feb 28, 2024

mergify bot commented Mar 4, 2024

mergify bot commented Mar 15, 2024

debojyoti452 commented Mar 17, 2024

gabrielluong commented Apr 3, 2024

		@@ -80,11 +82,9 @@ class AddLoginFragment : Fragment(R.layout.fragment_add_login), MenuProvider {
		)

		initEditableValues()

Bug 1815564 - Modify password field text to using Roboto monospace and Added Asterisk over Bullet on password fields. #864

Bug 1815564 - Modify password field text to using Roboto monospace and Added Asterisk over Bullet on password fields. #864

Conversation

gitstart commented Feb 17, 2023 • edited by github-actions bot Loading

Enhancement Task

Issue Screenshots

Fix Screenshots

mergify bot commented Apr 17, 2023

gabrielluong Jul 10, 2023

Choose a reason for hiding this comment

mergify bot commented Jul 10, 2023

titooan commented Jul 17, 2023

titooan Jul 10, 2023

Choose a reason for hiding this comment

github-actions bot commented Aug 9, 2023

github-actions bot commented Aug 9, 2023

gabrielluong Aug 9, 2023

Choose a reason for hiding this comment

gabrielluong Aug 9, 2023

Choose a reason for hiding this comment

cwzilla Aug 10, 2023

Choose a reason for hiding this comment

titooan left a comment

Choose a reason for hiding this comment

cwzilla commented Sep 5, 2023

mergify bot commented Feb 28, 2024

mergify bot commented Mar 4, 2024

mergify bot commented Mar 15, 2024

debojyoti452 commented Mar 17, 2024

gabrielluong commented Apr 3, 2024

gitstart commented Feb 17, 2023 •

edited by github-actions bot

Loading