fix: Overlap for long passwords and show/hide button in Password Auth Screen Issue #: 5614 #5621

KshitizSareen · 2022-12-20T05:45:47Z

In the Password Auth Screen, when you type a long password, it overlaps with the show/hide button.

I fixed this issue by creating a new stylesheet for the password text input field. I added a padding-right property of 15% to its style. This fix will ensure that the text input in the password field will never exceed a width of 85% of the text input field.

I tested this commit by viewing the password auth screen in horizontal and vertical views. The password input field is working fine in both instances.

Please let me know if you have any questions.

Thank you.

Fixes: #5614.

(added by Chris:)
Replaces: #4084

chrisbobbe · 2022-12-20T15:14:48Z

Hi @KshitizSareen, thank you!

Could you please post some screenshots of the proposed change?

KshitizSareen · 2022-12-21T00:58:06Z

Hi @chrisbobbe, Thank you for your message. Let me attach the screenshots.

KshitizSareen · 2022-12-21T01:08:53Z

Screen.Recording.2022-12-20.at.5.05.53.PM.mov

@chrisbobbe I have attached a small video showing the fix.

Please let me know if you have need any more details.

Regards,
Kshitiz.

chrisbobbe

Thanks! Comments below.

I think they're most easily addressed by giving up the design where the show/hide button appears inside the border (which is iOS only). Let's try removing the absolute positioning, and just let the button take up the horizontal space it needs, with the <Input /> stretching to fill what horizontal space is left.

Here's a sketch of a possible implementation:

flexDirection: 'row' on the parent <View />
flex: 1 on the <Input />
Remove the position, right, top, and bottom attributes of styles.showPasswordButton.

I think it makes sense for this change to be in just one clear, coherent commit.

chrisbobbe · 2022-12-27T17:14:15Z

src/common/PasswordInput.js

  },
  showPasswordButtonText: {
    margin: 8,
    color: BRAND_COLOR,
  },
+  passwordTextInput: {
+    paddingRight: '15%',


Hmm, I'd rather not use a percentage here. We should reserve exactly the right amount of space for the show/hide button, and this will cause that space to be too large or small depending on factors including:

How wide the input is (e.g., varies with screen width; will be large on a tablet)

How big the show/hide button's text is (varies with the user's chosen font size in system settings)

chrisbobbe · 2022-12-27T17:15:05Z

src/common/PasswordInput.js

+    borderTopWidth: 1,
+    borderColor: BORDER_COLOR,
+    borderBottomWidth: 1,
+    borderRightWidth: 1,


I expect this to look odd on Android, where the inputs aren't styled with a border (see use of Platform.select in Input.js).

KshitizSareen · 2022-12-28T23:29:00Z

Hi @chrisbobbe ,

Thank you so much for your comments. I tried to incorporate your changes.

I removed the absolute positioning and added flex-direction: 'row' to the view. Unfortunaltely, that wont look very good with borders. It adds a small break between the password text input and the show/hide button. It looks something like this:

We could do away with borders entirely, to make it look like this (Similar to Android):

Another issue is the margin of the show/hide button. Since show has a bigger width than height, its margin causes the password text input to shrink. It works something like this:

Screen.Recording.2022-12-28.at.3.13.56.PM.mov

To resolve this issue, I propose we could use some icons instead of the show/hide text. It looks better and more intuitive.
We could use icons like these instead of show/hide:

Since they always occupy a fixed size, their margin also wont cause a problem.

Please let me know if you want to discuss on this. I look forward to your response.

Thank you,
Regards,
Kshitiz.

chrisbobbe · 2023-01-03T19:25:59Z

To resolve this issue, I propose we could use some icons instead of the show/hide text. It looks better and more intuitive.
We could use icons like these instead of show/hide:

Sure, that seems like a fine thing to try. Do you have the development environment set up to test on Android too? We style the Input component differently on Android and iOS, and we'd like to make sure it looks reasonable on both. If it's difficult for you to set up, please let me know and I can test your revision and post screenshots.

KshitizSareen · 2023-01-05T10:41:12Z

Hi @chrisbobbe, Thanks for your comments. I have emulators for the platforms, and I will make sure to test on both Android and IOS. Let me code it up and show the proposed changes.

Thanks.

richab246 · 2023-01-07T14:42:49Z

Can I work on this issue?

chrisbobbe · 2023-01-07T16:32:41Z

@richab246, please find a different issue to work on; @KshitizSareen is working on this one.

KshitizSareen · 2023-01-09T11:06:29Z

Hi @chrisbobbe, I made the proposed changes. Please review them.

This is for Android:
https://user-images.githubusercontent.com/23134343/211293421-98e2d07e-6e6b-4ffa-be58-f6676f8ea2d9.mov

This is for IOS:
https://user-images.githubusercontent.com/23134343/211294324-dadbbbb0-d3e8-41f8-bdae-f6f16c13d947.mov

Thank you,
Regards,
Kshitiz.

chrisbobbe

Thanks, @KshitizSareen! See two small comments below. Also, in your next revision, please format your commit message according to our style (I linked to it upthread).

chrisbobbe · 2023-01-19T22:39:06Z

src/common/PasswordInput.js

      <Touchable style={styles.showPasswordButton} onPress={handleShow}>
-        <ZulipTextIntl style={styles.showPasswordButtonText} text={isHidden ? 'show' : 'hide'} />
+        <Icon name={isHidden ? 'eye-off' : 'eye'} size={20} style={styles.showPasswordButtonIcon} />


We default to using size 24 for icons; what does it look like with 24 instead of 20?

chrisbobbe · 2023-01-19T22:40:34Z

src/common/PasswordInput.js

@@ -3,24 +3,26 @@ import React, { useState, useCallback } from 'react';
 import type { Node } from 'react';
 import { View } from 'react-native';

+import Icon from 'react-native-vector-icons/Ionicons';


Suggested change

import Icon from 'react-native-vector-icons/Ionicons';

import { Icon } from '../common/Icons';

KshitizSareen · 2023-01-20T09:45:14Z

Hi @chrisbobbe,

Thanks for your comments. I changed the font-size to 24, and I imported Icon from Icons.js in the common folder.

It looks like this:

This is for tablet:
https://user-images.githubusercontent.com/23134343/213664807-cdd6c6ee-1adc-428f-8c1f-0268c1707c1a.mov

This is for Iphone:

Show.Hide.Password.Demo.for.IPhone.mov

I have pushed the changes.

Thank you.

chrisbobbe · 2023-01-20T19:12:02Z

Thanks! Please squash your fixup commits into the main, interesting commit, and give that commit a commit message according to our style.

KshitizSareen · 2023-01-25T10:07:13Z

Hi @chrisbobbe , I squashed all my commits.

Please review.

Thanks.

chrisbobbe · 2023-01-25T21:37:38Z

Thanks! Some commit-message nits before I hand this off to Greg to review:

Please make sure your summary line is 76 characters or less.
For the prefix, use the name of a subsystem instead of "fix". The name of a UI component often works; I think "PasswordInput" would be fine here.
Please end your commit message with Fixes: #5614 (no period, capital "F").

This is all covered in https://zulip.readthedocs.io/en/latest/contributing/commit-discipline.html. I highly recommend browsing the project's history to get a sense of our commit-message style, in addition to reading the docs.

KshitizSareen · 2023-01-26T04:31:01Z

Thank you @chrisbobbe.

I will change the commit message, and I will be more careful from now on.

Thank you.

KshitizSareen · 2023-01-26T05:45:36Z

Hi @chrisbobbe , I pushed some changes.

Please review.

Thanks.

chrisbobbe · 2023-01-26T18:10:46Z

Thanks! A few nits in the updated commit message:

PasswordInput: Fixed Overlap for long Passwords and show/hide button

Long Passwords and show/hide buttons overlap with each other.
I fixed this problem by doing the following:

- I changed the style of Password Text Input to take up as much space as needed. I did this by setting a flex of 1 to the password text input.

- I changed the positioning of the show/hide button from absolute to relative.

- I ensured Horizontal Alignment by setting the flex-direction of the parent view to 'row'.

- To fix the problem of the changing width of the text input, when the show/hide text changes, I replaced the show/hide text with a show/icon. The show/hide icon is an eye-open/eye-closed icon. This icon always has a fixed width and does not affect the text input.

- I removed hide text from messages_en.json since it is not in the app.

I tested this on Android and IOS devices with different screen sizes, and it works fine on both platforms.

Fixes: #5614

The summary line should say "Fix" instead of "Fixed", and there's no need to capitalize "overlap" or "passwords".
The paragraph content in the body should be wrapped to about 68 characters per line, but no more than 70.

https://zulip.readthedocs.io/en/latest/contributing/commit-discipline.html#commit-messages

KshitizSareen · 2023-02-09T20:56:22Z

Hi @chrisbobbe ,

Apologies for the delay.

I fixed up the commit message.

Please review.

Regards,
Kshitiz.

Long passwords and show/hide buttons overlap with each other. I fixed this problem by doing the following: - I changed the style of Password Input to take up as much space as needed. I did this by setting a flex of 1 to the Password Input. - I changed the positioning of the show/hide button from absolute to relative. - I ensured horizontal alignment by setting the flex-direction of the parent view to 'row'. - To fix the problem of the changing width of the text input, when the show/hide text changes, I replaced the show/hide text with a show/icon. The show/hide icon is an eye-open/eye-closed icon. This icon always has a fixed width and does not affect the text input. - I removed hide text from messages_en.json since it is not in the app. I tested this on Android and IOS devices with different screen sizes, and it works fine on both platforms. Fixes: zulip#5614

…tton The touchable-feedback area was right up against the right edge of the password input. Give some margin in between, for some breathing room.

…ight The default value for `alignItems` is 'stretch': https://reactnative.dev/docs/0.68/flexbox#align-items

This lets us have a more compact layout (e.g., the left edge of the icon is just 8px away from the right edge of the password input, instead of that 8px plus 12px of padding in the Touchable) while maintaining the 48px touch-target size with the help of `hitSlop`.

Now, after changing the show/hide button to be an icon instead of text, we still present user-facing text to screen reader software. Tested on iOS with VoiceOver, and implemented with help from this article: https://incl.ca/show-hide-password-accessibility-and-password-hints-tutorial/

chrisbobbe · 2023-02-10T20:08:13Z

Thanks! I've just pushed a revision with a few small tweaks to the layout; please take a look:

c0d475a PasswordInput: Fix overlap for long passwords and show/hide button
4b78875 PasswordInput: Add 8px margin between password input and show/hide button
3682bc3 PasswordInput: Don't let show/hide button stretch password input's height
322b330 PasswordInput: Give show/hide button a 48px touch target size
3f3324f PasswordInput: Use Pressable for show/hide icon button
9c473e1 PasswordInput: Add accessibility properties to show/hide button

Here's a video from after those small changes, to compare with the video of your revision that you posted (thanks again 🙂) at #5621 (comment):

And a screenshot with the touchable area highlighted ([shake] -> Show Inspector -> Touchables):

@gnprice, this is ready for your review. 🙂

chrisbobbe requested changes Dec 27, 2022

View reviewed changes

KshitizSareen force-pushed the zulip-mobile-branch branch 2 times, most recently from eec7e74 to 7652659 Compare January 9, 2023 11:23

chrisbobbe requested changes Jan 19, 2023

View reviewed changes

KshitizSareen force-pushed the zulip-mobile-branch branch from 0b8ee85 to e38a6b3 Compare January 25, 2023 09:52

KshitizSareen force-pushed the zulip-mobile-branch branch 3 times, most recently from d3cf64a to 9b1065c Compare January 26, 2023 05:25

KshitizSareen closed this Feb 9, 2023

KshitizSareen force-pushed the zulip-mobile-branch branch from 9b1065c to ff4e8a3 Compare February 9, 2023 19:29

KshitizSareen reopened this Feb 9, 2023

KshitizSareen closed this Feb 9, 2023

KshitizSareen force-pushed the zulip-mobile-branch branch from 5bbfcf2 to ff4e8a3 Compare February 9, 2023 20:25

KshitizSareen reopened this Feb 9, 2023

KshitizSareen and others added 6 commits February 10, 2023 10:30

PasswordInput: Add 8px margin between password input and show/hide bu…

4b78875

…tton The touchable-feedback area was right up against the right edge of the password input. Give some margin in between, for some breathing room.

PasswordInput: Don't let show/hide button stretch password input's he…

3682bc3

…ight The default value for `alignItems` is 'stretch': https://reactnative.dev/docs/0.68/flexbox#align-items

PasswordInput: Give show/hide button a 48px touch target size

322b330

chrisbobbe force-pushed the zulip-mobile-branch branch from 6a8246d to 9c473e1 Compare February 10, 2023 20:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: Overlap for long passwords and show/hide button in Password Auth Screen Issue #: 5614 #5621

fix: Overlap for long passwords and show/hide button in Password Auth Screen Issue #: 5614 #5621

KshitizSareen commented Dec 20, 2022 •

edited by chrisbobbe

Loading

chrisbobbe commented Dec 20, 2022

KshitizSareen commented Dec 21, 2022

KshitizSareen commented Dec 21, 2022

chrisbobbe left a comment

chrisbobbe Dec 27, 2022

chrisbobbe Dec 27, 2022

KshitizSareen commented Dec 28, 2022 •

edited

Loading

chrisbobbe commented Jan 3, 2023

KshitizSareen commented Jan 5, 2023

richab246 commented Jan 7, 2023

chrisbobbe commented Jan 7, 2023 •

edited

Loading

KshitizSareen commented Jan 9, 2023

chrisbobbe left a comment

chrisbobbe Jan 19, 2023

chrisbobbe Jan 19, 2023

KshitizSareen commented Jan 20, 2023 •

edited

Loading

chrisbobbe commented Jan 20, 2023

KshitizSareen commented Jan 25, 2023

chrisbobbe commented Jan 25, 2023

KshitizSareen commented Jan 26, 2023

KshitizSareen commented Jan 26, 2023

chrisbobbe commented Jan 26, 2023

KshitizSareen commented Feb 9, 2023

chrisbobbe commented Feb 10, 2023 •

edited

Loading

	import Icon from 'react-native-vector-icons/Ionicons';
	import { Icon } from '../common/Icons';

fix: Overlap for long passwords and show/hide button in Password Auth Screen Issue #: 5614 #5621

Are you sure you want to change the base?

fix: Overlap for long passwords and show/hide button in Password Auth Screen Issue #: 5614 #5621

Conversation

KshitizSareen commented Dec 20, 2022 • edited by chrisbobbe Loading

chrisbobbe commented Dec 20, 2022

KshitizSareen commented Dec 21, 2022

KshitizSareen commented Dec 21, 2022

chrisbobbe left a comment

Choose a reason for hiding this comment

chrisbobbe Dec 27, 2022

Choose a reason for hiding this comment

chrisbobbe Dec 27, 2022

Choose a reason for hiding this comment

KshitizSareen commented Dec 28, 2022 • edited Loading

chrisbobbe commented Jan 3, 2023

KshitizSareen commented Jan 5, 2023

richab246 commented Jan 7, 2023

chrisbobbe commented Jan 7, 2023 • edited Loading

KshitizSareen commented Jan 9, 2023

chrisbobbe left a comment

Choose a reason for hiding this comment

chrisbobbe Jan 19, 2023

Choose a reason for hiding this comment

chrisbobbe Jan 19, 2023

Choose a reason for hiding this comment

KshitizSareen commented Jan 20, 2023 • edited Loading

chrisbobbe commented Jan 20, 2023

KshitizSareen commented Jan 25, 2023

chrisbobbe commented Jan 25, 2023

KshitizSareen commented Jan 26, 2023

KshitizSareen commented Jan 26, 2023

chrisbobbe commented Jan 26, 2023

KshitizSareen commented Feb 9, 2023

chrisbobbe commented Feb 10, 2023 • edited Loading

KshitizSareen commented Dec 20, 2022 •

edited by chrisbobbe

Loading

KshitizSareen commented Dec 28, 2022 •

edited

Loading

chrisbobbe commented Jan 7, 2023 •

edited

Loading

KshitizSareen commented Jan 20, 2023 •

edited

Loading

chrisbobbe commented Feb 10, 2023 •

edited

Loading