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

Update popover menu check to use isEmpty #6201

Merged
merged 1 commit into from
Nov 4, 2021
Merged

Conversation

Jag96
Copy link
Contributor

@Jag96 Jag96 commented Nov 4, 2021

cc @parasharrajat

Details

This PR fixes an issue where an empty string value used in a conditional render results in a mobile crash. The explanation of why this happens and why this is my recommended fix can be found in #6198 (comment).

Fixed Issues

$ #6198

Tests/QA

  1. Launch the app and login with expensifail account
  2. Click the FAB button and select Send money
  3. Enter an amount and tap Next button
  4. Select an attendee from the list
  5. Tap the payment selection button.
  6. Confirm options show up

Tested On

  • Web
  • Mobile Web
  • Desktop
  • iOS
  • Android

Screenshots

iOS Web
image image

@Jag96 Jag96 requested a review from a team as a code owner November 4, 2021 00:54
@Jag96 Jag96 self-assigned this Nov 4, 2021
@MelvinBot MelvinBot requested review from jasperhuangg and removed request for a team November 4, 2021 00:54
@Jag96
Copy link
Contributor Author

Jag96 commented Nov 4, 2021

Adding CP staging label to fix deploy blocker

@github-actions
Copy link
Contributor

github-actions bot commented Nov 4, 2021

⚠️ ⚠️ Heads up! This pull request has the CP Staging label. ⚠️ ⚠️
Merging it will cause it to be immediately deployed to staging, even if the open StagingDeployCash deploy checklist is locked.

@parasharrajat
Copy link
Member

parasharrajat commented Nov 4, 2021

Thanks, Jack Joe, I missed that. Looks like that component was not following guidelines 😄 .

@roryabraham roryabraham merged commit 9b9f694 into main Nov 4, 2021
@roryabraham roryabraham deleted the joe-fix-menu-crash branch November 4, 2021 17:58
github-actions bot pushed a commit that referenced this pull request Nov 4, 2021
@OSBotify
Copy link
Contributor

OSBotify commented Nov 4, 2021

🚀 Cherry-picked to staging by @roryabraham in version: 1.1.13-2 🚀

platform result
🤖 android 🤖 success ✅
🖥 desktop 🖥 success ✅
🍎 iOS 🍎 success ✅
🕸 web 🕸 success ✅

@Expensify/applauseleads please QA this PR and check it off on the deploy checklist if it passes.

@mvtglobally
Copy link

@roryabraham As long as page is not freezing and user is redirected to Onfido flow its a pass right? We do not see the "Pay with expensify" option in some cases.

Image.from.iOS.4.mp4

@roryabraham
Copy link
Contributor

@mvtglobally That looks like a pass to me 👍

@ogumen
Copy link

ogumen commented Nov 4, 2021

There are several accessibility issues with the Payment Options button and the overlay it opens:

  1. The button is announced with no role, state and with wrong label - failure of WCAG SC 4.1.2. The solution is to implement the element using native with aria-expanded attribute (to announce its expanded/collapsed state) and aria-label attribute (to announce its label, e.g. "Payment options"). Similar to [med] JAWS+Chrome: Many Pages: Multiple elements announced without button role #5446 and [med] JAWS+Chrome: Many Pages: Multiple buttons announced without labels #5519
  2. With the enabled screen reader the button opens the "Verify your identity" overlay instead of expanding the payment options - failure of WCAG SC 2.1.1. Keyboard event handlers should be checked, further investigation is required.
  3. The light grey borders of the Payment Options overlay fail minimum contrast requirements of 3.00:1 - failure of WCAG SC 1.4.11 (#ECECEC against the background #FEFEFE having a color contrast ratio of 1.17:1).
    https://user-images.githubusercontent.com/88733897/140419312-222a919a-decc-4095-9d2b-50f124da2818.mp4
    Expensify_Payment Options overlay boundaries contrast ratio
  4. An invisible "Pay with PayPal.me" element is focused and announced in the overlay additionally to the options displayed visually - failure of WCAG SC 2.4.3. Similar to [med] Keyboard Navigation: Many Pages: Invisible button is focused with Tab key #5452
    https://user-images.githubusercontent.com/88733897/140421072-399be122-cc96-43b3-b72a-53a50c1a844c.mp4
  5. The focus outline for the button does not meet minimum color contrast requirements of 3.00:1 - failure of WCAG SC 1.4.11. The blue outline (#0185FF) against the green button background (#03D47C) has a color contrast ratio of 1.84:1. Similar to [low] Color Contrast: Change Password: The focus outline does not meet the contrast ratio of 3:1 #5646
    Expensify_Blue outline againt green button fails color contrast requirements

@OSBotify
Copy link
Contributor

OSBotify commented Nov 4, 2021

🚀 Deployed to production by @yuwenmemon in version: 1.1.13-2 🚀

platform result
🤖 android 🤖 success ✅
🖥 desktop 🖥 success ✅
🍎 iOS 🍎 success ✅
🕸 web 🕸 success ✅

@OSBotify
Copy link
Contributor

OSBotify commented Nov 4, 2021

🚀 Deployed to staging by @roryabraham in version: 1.1.13-3 🚀

platform result
🤖 android 🤖 success ✅
🖥 desktop 🖥 success ✅
🍎 iOS 🍎 success ✅
🕸 web 🕸 success ✅

@OSBotify
Copy link
Contributor

🚀 Deployed to production by @Jag96 in version: 1.1.14-4 🚀

platform result
🤖 android 🤖 failure ❌
🖥 desktop 🖥 success ✅
🍎 iOS 🍎 success ✅
🕸 web 🕸 success ✅

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants