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

Can't get out of a verification dialog #4025

Closed
kittykat opened this issue Sep 16, 2021 · 11 comments · Fixed by #7950
Closed

Can't get out of a verification dialog #4025

kittykat opened this issue Sep 16, 2021 · 11 comments · Fixed by #7950
Assignees
Labels
A-E2EE help wanted Extra attention is needed O-Occasional Affects or can be seen by some users regularly or most users rarely S-Major Severely degrades major functionality or product features, with no satisfactory workaround T-Defect Something isn't working: bugs, crashes, hangs and other reported problems Z-Ready This issue is ready for development Z-WTF WTF moment: High Impact, Low Effort

Comments

@kittykat
Copy link
Contributor

kittykat commented Sep 16, 2021

This issue is: Ready to build

This screen should act as a bottom sheet. When the user clicks outside the area of the bottom sheet it should close. We should implement the appropriate close actions suggested by google. Found here:

Other ways to close the bottom sheet should be:
https://material.io/components/sheets-bottom#modal-bottom-sheet

Control
Modal bottom sheets appear when triggered by a user action, such as tapping a button or an overflow icon. They can be dismissed by:

  • Tapping a menu item or action within the bottom sheet
  • Tapping the scrim
  • Swiping the sheet down

Original issue

I accidentally clicked on a new session toast as it popped up while I was doing something else.

Now I have this and can't dismiss it:
Screenshot_20210916-085451~2.png

I want to be able to dismiss that dialog by clicking away from it.

@kittykat kittykat added the T-Defect Something isn't working: bugs, crashes, hangs and other reported problems label Sep 16, 2021
@bmarty
Copy link
Member

bmarty commented Sep 28, 2021

It was a design decision to use BottomSheet for this purpose (I was against it), and to make BottomSheet non dismissable (which is also bad practice).

That said, clicking on "Back" should propose the user to cancel the process.

@kittykat kittykat added the X-Needs-Product Issue needs input from Product team label Oct 21, 2021
@kittykat kittykat added this to Incoming in Issue triage Oct 21, 2021
@ouchadam
Copy link
Contributor

It does seem a little unfriendly to enforce a non dismissable flow

other parts of this flow go to the are you sure screen, feels like we could do the same here?

CANCEL VERIFICATION
Screenshot_20211025_114239

@ouchadam ouchadam added O-Occasional Affects or can be seen by some users regularly or most users rarely S-Minor Impairs non-critical functionality or suitable workarounds exist A-Verification labels Oct 25, 2021
@ouchadam ouchadam moved this from Incoming to Triaged in Issue triage Oct 25, 2021
@ouchadam ouchadam added S-Major Severely degrades major functionality or product features, with no satisfactory workaround and removed S-Minor Impairs non-critical functionality or suitable workarounds exist labels Nov 26, 2021
@bmarty
Copy link
Member

bmarty commented Dec 1, 2021

About "Are you sure?":

image

From https://material.io/components/dialogs

@daniellekirkwood
Copy link
Contributor

daniellekirkwood commented Dec 2, 2021

Bottom sheets don't have "close" button unless they're in their expanded state (full screen) but I don't think this bottom sheet expands?

Other ways to close the bottom sheet should be:
https://material.io/components/sheets-bottom#modal-bottom-sheet

Control
Modal bottom sheets appear when triggered by a user action, such as tapping a button or an overflow icon. They can be dismissed by:

  • Tapping a menu item or action within the bottom sheet
  • Tapping the scrim
  • Swiping the sheet down
  • Using a close affordance within the bottom sheet’s top app bar, if available

It doesn't look like a Bottom sheet is the correct material.io component here so I'm tagging @amshakal and labelling Needs-Design for 2 UX decisions:

  • Do we want to keep the bottom sheet for this instance. If no, which component instead?
  • If yes... Do we add a Close button to the bottom sheet or implement the Google recommended close actions (see above) or something else?

EDIT
Met with Amsha today and we will keep the bottom sheet component as it's a pattern across the app that it well used and familiar to users.
Amsha will discuss our further options with others on the team and let us know the final decision 👍

@daniellekirkwood
Copy link
Contributor

Removing the Needs-Product label.

I think we should implement the bottom sheet action where selecting not-the-bottom-sheet closes the bottom sheet.

Design to make final decision here.

@daniellekirkwood daniellekirkwood removed the X-Needs-Product Issue needs input from Product team label Jan 7, 2022
@amshakal
Copy link

Hello, I agree with Daneille. The reason is that we are using the bottom sheet pattern for all things verification already.

@daniellekirkwood
Copy link
Contributor

Thanks, @amshakal

I will remove the Needs Design label and update the description of the issue

@daniellekirkwood daniellekirkwood removed the X-Needs-Design May require input from the design team label Jan 10, 2022
@daniellekirkwood daniellekirkwood added this to Next Sprint in Android App Team Jan 10, 2022
@daniellekirkwood daniellekirkwood added the Z-WTF WTF moment: High Impact, Low Effort label Jan 10, 2022
@daniellekirkwood daniellekirkwood moved this from Next Sprint to WTFs & Papercuts in Android App Team Jan 10, 2022
@daniellekirkwood daniellekirkwood added the Z-Ready This issue is ready for development label Jan 26, 2022
@bmarty bmarty added this to the WTF ready list milestone Jan 31, 2022
@bmarty
Copy link
Member

bmarty commented Jan 31, 2022

As a general remark, there are too many bottom sheets in this app and I did not success in the past to change the designer mind. Having text input area in bottom sheet is also quite weird :/. According to material.io bottom sheets can be used to: provide additional room for content, iconography, and actions. and it's great for instance when long clicking on a Matrix room or on a message. But for a flow of several screens, it's quite bad especially when using dark pattern to block user from dismissing it like reported in this issue.

The verification flow should be a full screen flow IMO. In this case it is possible to display a dialog when the user when to exit before it's finished, like we are doing for instance during account creation.

@bmarty
Copy link
Member

bmarty commented Jan 31, 2022

(oh I am repeating myself, sorry about that)

@daniellekirkwood
Copy link
Contributor

That's great feedback @bmarty
As the scope of this issue is to fix the defect (where the user can't exit the flow) we will stick to the agreed approach. However, we can raise an Enhancement issue to address your comments about the usage of the Bottom Sheet throughout the app and I'm sure the design team would be happy to review old decisions.

@ahmed-radhouane ahmed-radhouane self-assigned this Feb 2, 2022
@ahmed-radhouane ahmed-radhouane moved this from WTFs & Papercuts to In progress in Android App Team Feb 2, 2022
@bmarty
Copy link
Member

bmarty commented Feb 2, 2022

OK, thanks.

So as far as I understand, we want this bottom sheet to be dismissable, and when dismissed, it should cancel the verification process (if there is something to do).

CC @ahmed-radhouane

@ahmed-radhouane ahmed-radhouane moved this from In progress to In Code Review in Android App Team Feb 8, 2022
ahmed-radhouane added a commit that referenced this issue Feb 11, 2022
 - update changelog file.

Signed-off-by: Ahmed Radhouane Belkilani <arbelkilani@gmail.com>
ahmed-radhouane added a commit that referenced this issue Feb 22, 2022
 - update changelog file.

Signed-off-by: Ahmed Radhouane Belkilani <arbelkilani@gmail.com>
ahmed-radhouane added a commit that referenced this issue Feb 23, 2022
- Add annotation @callsuper to ensure calling super method.
- Remove unused fragment class VerificationCancelFragment.

Signed-off-by: Ahmed Radhouane Belkilani <arbelkilani@gmail.com>
ahmed-radhouane added a commit that referenced this issue Feb 23, 2022
 - update changelog file.

Signed-off-by: Ahmed Radhouane Belkilani <arbelkilani@gmail.com>
ahmed-radhouane added a commit that referenced this issue Feb 23, 2022
- Add annotation @callsuper to ensure calling super method.
- Remove unused fragment class VerificationCancelFragment.

Signed-off-by: Ahmed Radhouane Belkilani <arbelkilani@gmail.com>
ahmed-radhouane added a commit that referenced this issue Feb 23, 2022
- code quality line length limit.

Signed-off-by: Ahmed Radhouane Belkilani <arbelkilani@gmail.com>
@daniellekirkwood daniellekirkwood removed their assignment Aug 10, 2022
@kittykat kittykat added the help wanted Extra attention is needed label Aug 31, 2022
@onurays onurays self-assigned this Jan 13, 2023
Android App Team automation moved this from In Code Review to Merged Jan 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-E2EE help wanted Extra attention is needed O-Occasional Affects or can be seen by some users regularly or most users rarely S-Major Severely degrades major functionality or product features, with no satisfactory workaround T-Defect Something isn't working: bugs, crashes, hangs and other reported problems Z-Ready This issue is ready for development Z-WTF WTF moment: High Impact, Low Effort
Projects
Development

Successfully merging a pull request may close this issue.

7 participants