makes the PDF container focusable to provide accessibility #12391

mollfpr · 2022-11-02T18:14:01Z

Details

Adding focusable to the PDF container so the scroll with an arrow key is go through the container instead of depending on the body tag focusable.

Fixed Issues

$ #10824
$ #10824 (comment)

Tests

Test on Web/Desktop

Sign in to NewDot with any account
Select a chat with any user
Click the plus button to the left of the message input, and select "Add attachment"
Select a PDF
Click send
After the PDF uploads, click to open it
Close it (notice that tabIndex=-1 on the body HTML element)
Click the plus button to the left of the message input, and select "Add attachment"
Select a PDF
Click on it
Click the down arrow key to scroll the PDF down, and verify that it scrolls down
Click the up arrow key to scroll the PDF up, and verify that it scrolls up

Verify that no errors appear in the JS console

QA Steps

Test on Web/Desktop

Sign in to NewDot with any account
Select a chat with any user
Click the plus button to the left of the message input, and select "Add attachment"
Select a PDF
Click send
After the PDF uploads, click to open it
Close it (notice that tabIndex=-1 on the body HTML element)
Click the plus button to the left of the message input, and select "Add attachment"
Select a PDF
Click on it
Click the down arrow key to scroll the PDF down, and verify that it scrolls down
Click the up arrow key to scroll the PDF up, and verify that it scrolls up

Verify that no errors appear in the JS console

PR Author Checklist

PR Reviewer Checklist

The reviewer will copy/paste it into a new comment and complete it after the author checklist is completed

Screenshots

Web

Fix.PDF.Accessibility.on.Arrow.Key.Web.mov

Mobile Web - Chrome

Fix.PDF.Accessibility.on.Arrow.Key.mWeb-Chrome.mov

Mobile Web - Safari

Fix.PDF.Accessibility.on.Arrow.Key.mWeb-Safari.mov

Desktop

Fix.PDF.Accessibility.on.Arrow.Key.Desktop.mp4

iOS

Fix.PDF.Accessibility.on.Arrow.Key.iOS.mov

Android

Fix.PDF.Accessibility.on.Arrow.Key.Android.mov

Santhosh-Sellavel · 2022-11-06T22:37:29Z

@mollfpr
Can we update the testing steps, please remove the first two steps with the following.

Open New Expensify App & login(if not already)

Santhosh-Sellavel · 2022-11-06T23:29:41Z

Screen Recordings

Desktop & Web

Screen.Recording.2022-11-07.at.4.38.04.AM.mov

Android - iOS

Android_iOS.mov

Android - iOS mWeb

Screen.Recording.2022-11-07.at.4.54.30.AM.mov

Santhosh-Sellavel · 2022-11-06T23:30:06Z

PR Reviewer Checklist

The reviewer will copy/paste it into a new comment and complete it after the author checklist is completed

I have verified the author checklist is complete (all boxes are checked off).
I verified the correct issue is linked in the ### Fixed Issues section above
I verified testing steps are clear and they cover the changes made in this PR
- I verified the steps for local testing are in the Tests section
- I verified the steps for Staging and/or Production testing are in the QA steps section
- I verified the steps cover any possible failure scenarios (i.e. verify an input displays the correct error message if the entered data is not correct)
- I turned off my network connection and tested it while offline to ensure it matches the expected behavior (i.e. verify the default avatar icon is displayed if app is offline)
I checked that screenshots or videos are included for tests on all platforms
I included screenshots or videos for tests on all platforms
I verified tests pass on all platforms & I tested again on:
- iOS / native
- Android / native
- iOS / Safari
- Android / Chrome
- MacOS / Chrome
- MacOS / Desktop
If there are any errors in the console that are unrelated to this PR, I either fixed them (preferred) or linked to where I reported them in Slack
I verified proper code patterns were followed (see Reviewing the code)
- I verified that any callback methods that were added or modified are named for what the method does and never what callback they handle (i.e. toggleReport and not onIconClick).
- I verified that comments were added to code that is not self explanatory
- I verified that any new or modified comments were clear, correct English, and explained "why" the code was doing something instead of only explaining "what" the code was doing.
- I verified any copy / text shown in the product was added in all src/languages/* files
- I verified any copy / text that was added to the app is correct English and approved by marketing by adding the Waiting for Copy label for a copy review on the original GH to get the correct copy.
- I verified proper file naming conventions were followed for any new files or renamed files. All non-platform specific files are named after what they export and are not named "index.js". All platform-specific files are named for the platform the code supports as outlined in the README.
- I verified the JSDocs style guidelines (in STYLE.md) were followed
If a new code pattern is added I verified it was agreed to be used by multiple Expensify engineers
I verified that this PR follows the guidelines as stated in the Review Guidelines
I verified other components that can be impacted by these changes have been tested, and I retested again (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using Avatar have been tested & I retested again)
I verified all code is DRY (the PR doesn't include any logic written more than once, with the exception of tests)
I verified any variables that can be defined as constants (ie. in CONST.js or at the top of the file that uses the constant) are defined as such
If a new component is created I verified that:
- A similar component doesn't exist in the codebase
- All props are defined accurately and each prop has a /** comment above it */
- The file is named correctly
- The component has a clear name that is non-ambiguous and the purpose of the component can be inferred from the name alone
- The only data being stored in the state is data necessary for rendering and nothing else
- For Class Components, any internal methods passed to components event handlers are bound to this properly so there are no scoping issues (i.e. for onClick={this.submit} the method this.submit should be bound to this in the constructor)
- Any internal methods bound to this are necessary to be bound (i.e. avoid this.submit = this.submit.bind(this); if this.submit is never passed to a component event handler like onClick)
- All JSX used for rendering exists in the render method
- The component has the minimum amount of code necessary for its purpose, and it is broken down into smaller components in order to separate concerns and functions
If a new CSS style is added I verified that:
- A similar style doesn't already exist
- The style can't be created with an existing StyleUtils function (i.e. StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)
If the PR modifies a generic component, I tested and verified that those changes do not break usages of that component in the rest of the App (i.e. if a shared library or component like Avatar is modified, I verified that Avatar is working as expected in all cases)
If the PR modifies a component related to any of the existing Storybook stories, I tested and verified all stories for that component are still working as expected.
I have checked off every checkbox in the PR reviewer checklist, including those that don't apply to this PR.

Santhosh-Sellavel · 2022-11-06T23:34:30Z

@neil-marcellini
While Testing offline, I found this error. Should I log a bug for this? As this is send attachment flow and I expect a preview here

Santhosh-Sellavel

@neil-marcellini
Just waiting for @mollfpr to update the test steps & clarification on the above bug!

mollfpr · 2022-11-07T00:39:01Z

@Santhosh-Sellavel updated testing step!

How do you repro the bug? it’s not happen to me

Santhosh-Sellavel · 2022-11-07T00:51:55Z

Screen.Recording.2022-11-07.at.6.21.22.AM.mov

Santhosh-Sellavel · 2022-11-07T01:02:50Z

@neil-marcellini While Testing offline, I found this error. Should I log a bug for this? As this is send attachment flow and I expect a preview here

As this is not introduced here, I reported it in slack here

Santhosh-Sellavel

Recordings: #12391 (comment)
Checklist: #12391 (comment)
C+ Reviewed
🎀 👀 🎀

Looks good, tests well!

All you @neil-marcellini

neil-marcellini

Test on Web/Desktop

Open New Expensify App & login(if not already)

Login with an Expensify account

Select any user

Click the plus button to the left of the message input, and select "Add attachment"

Select a password-protected PDF

Verify that the password form appears

Enter the password and Click Confirm

Click on the PDF document and use the up and down keys for scrolling

Verify the scroll is working with the arrow key

I will approve this PR once we update these test steps. The problem is that the tests pass on staging already. See the video below.

password-pdf-staging.mov

In the video posted in the issue description we can see that at step 7 the user types the password and then hits the Enter key. In the next step they expect the up and down arrow keys to scroll the document. I don't think that's actually a problem, but it's also not what you described with these test steps.

Sorry for the confusion. I think these are the test / QA steps that should actually be used, since we agreed that this is the real issue here. @Santhosh-Sellavel unfortunately I think you will also have to test on all platforms again.

Sign into NewDot with any account
Select a chat with any user
Click the plus button to the left of the message input, and select "Add attachment"
Select a PDF
Click send
After the PDF uploads, click to open it
Close it (notice that tabIndex=-1 on the body html element)
Click the plus button to the left of the message input, and select "Add attachment"
Select a PDF
Click on it
Click the down arrow key to scroll the PDF down, verify that it scrolls down
Click the up arrow key to scroll the PDF up, verify that it scrolls up

Santhosh-Sellavel · 2022-11-07T23:34:54Z

@neil-marcellini Web/Desktop alone will have to be enough!

mollfpr · 2022-11-08T02:08:09Z

Thanks @neil-marcellini for the correction, now the test steps is updated.

neil-marcellini · 2022-11-09T17:44:38Z

@Santhosh-Sellavel can you test today please?

Santhosh-Sellavel · 2022-11-10T00:13:27Z

Yeah, will be testing it shortly!

Santhosh-Sellavel · 2022-11-10T00:33:47Z

@neil-marcellini this is the recording. the selection outline is not visible clearly on top or bottom while scrolling through pages, only visible when we reach the end at the top/bottom!

Web

Screen.Recording.2022-11-10.at.6.02.01.AM.mov

Desktop

Desktop.mov

mollfpr · 2022-11-10T11:34:47Z

src/components/PDFView/index.js

+            ? [styles.PDFView, styles.noSelect, this.props.style, styles.invisible]
+            : [styles.PDFView, styles.noSelect, this.props.style];


@Santhosh-Sellavel There should be no outline showing when the div is focused. I can't reproduce that outline and neither your first video test.

neil-marcellini · 2022-11-10T19:40:30Z

I'll test now myself.

neil-marcellini · 2022-11-10T20:26:48Z

@Santhosh-Sellavel you must have had some kind of glitch when testing this branch. It works fine for me!

Web

web.mov

Desktop

desktop.mov

melvin-bot · 2022-11-10T20:28:00Z

@neil-marcellini looks like this was merged without the checklist test passing. Please add a note explaining why this was done and remove the Emergency label if this is not an emergency.

neil-marcellini · 2022-11-10T20:29:08Z

All checks passed. This is a bug that will be fixed soon.

OSBotify · 2022-11-10T20:31:46Z

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

Santhosh-Sellavel · 2022-11-11T01:28:02Z

Seems something is fishy It's not working for me properly sometimes!

Santhosh-Sellavel · 2022-11-11T02:22:23Z

Seems something is fishy It's not working for me properly sometimes!

NVM, I was testing on the wrong build, all looks good!

OSBotify · 2022-11-11T03:39:15Z

🚀 Deployed to staging by @neil-marcellini in version: 1.2.27-0 🚀

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

JmillsExpensify · 2022-11-14T15:24:40Z

Echoing @sobitneupane in the main issue, this PR has caused a regression. Per the BZ checklist, is there anything we can add to TestRail or the PR reviewer checklist to prevent such a regression for happening again?

OSBotify · 2022-11-15T21:09:59Z

🚀 Deployed to production by @roryabraham in version: 1.2.27-4 🚀

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

OSBotify · 2022-11-16T00:17:52Z

🚀 Deployed to production by @roryabraham in version: 1.2.27-4 🚀

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

makes the PDF container focusable to provide accessibility

b6cfe40

mollfpr requested a review from a team as a code owner November 2, 2022 18:14

melvin-bot bot requested review from neil-marcellini and Santhosh-Sellavel and removed request for a team November 2, 2022 18:14

Santhosh-Sellavel reviewed Nov 6, 2022

View reviewed changes

Santhosh-Sellavel approved these changes Nov 7, 2022

View reviewed changes

neil-marcellini requested changes Nov 7, 2022

View reviewed changes

mollfpr commented Nov 10, 2022

View reviewed changes

neil-marcellini approved these changes Nov 10, 2022

View reviewed changes

neil-marcellini merged commit 5401816 into Expensify:main Nov 10, 2022

melvin-bot bot added the Emergency label Nov 10, 2022

neil-marcellini removed the Emergency label Nov 10, 2022

OSBotify mentioned this pull request Nov 11, 2022

Deploy Checklist: New Expensify 2022-11-11 #12662

Closed

27 tasks

s77rt mentioned this pull request Nov 14, 2022

[$1000] BUG: Web/Safari - Opening the protect PDF modal and pressing TAB doesn’t focus on download/close button reported by @dhairyasenjaliya #12010

Closed

sobitneupane mentioned this pull request Nov 14, 2022

[PAID] [$1000] Web- Attachment - Up and down arrow key does not work for scrolling PDF file. #10824

Closed

laurenreidexpensify mentioned this pull request Nov 16, 2022

PAID [$250] [Bug] - error message out of screen reported by @gadhiyamanan #11488

Closed

aimane-chnaif mentioned this pull request Jan 12, 2023

[HOLD for payment 2023-02-15] [$2000] not able to select text of keyboard shortcuts when you open popup second time #13968

Closed

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

makes the PDF container focusable to provide accessibility #12391

makes the PDF container focusable to provide accessibility #12391

mollfpr commented Nov 2, 2022 •

edited

Loading

Santhosh-Sellavel commented Nov 6, 2022 •

edited

Loading

Santhosh-Sellavel commented Nov 6, 2022

Screen Recordings

Desktop & Web

Android - iOS

Android - iOS mWeb

Santhosh-Sellavel commented Nov 6, 2022 •

edited

Loading

PR Reviewer Checklist

Santhosh-Sellavel commented Nov 6, 2022

Santhosh-Sellavel left a comment •

edited

Loading

mollfpr commented Nov 7, 2022

Santhosh-Sellavel commented Nov 7, 2022

Santhosh-Sellavel commented Nov 7, 2022

Santhosh-Sellavel left a comment •

edited

Loading

neil-marcellini left a comment

Santhosh-Sellavel commented Nov 7, 2022

mollfpr commented Nov 8, 2022

neil-marcellini commented Nov 9, 2022

Santhosh-Sellavel commented Nov 10, 2022

Santhosh-Sellavel commented Nov 10, 2022 •

edited

Loading

mollfpr Nov 10, 2022

neil-marcellini commented Nov 10, 2022

neil-marcellini commented Nov 10, 2022

melvin-bot bot commented Nov 10, 2022

neil-marcellini commented Nov 10, 2022

OSBotify commented Nov 10, 2022

Santhosh-Sellavel commented Nov 11, 2022

Santhosh-Sellavel commented Nov 11, 2022

OSBotify commented Nov 11, 2022

JmillsExpensify commented Nov 14, 2022

OSBotify commented Nov 15, 2022

OSBotify commented Nov 16, 2022

		? [styles.PDFView, styles.noSelect, this.props.style, styles.invisible]
		: [styles.PDFView, styles.noSelect, this.props.style];

makes the PDF container focusable to provide accessibility #12391

makes the PDF container focusable to provide accessibility #12391

Conversation

mollfpr commented Nov 2, 2022 • edited Loading

Details

Fixed Issues

Tests

QA Steps

PR Author Checklist

PR Reviewer Checklist

Screenshots

Web

Mobile Web - Chrome

Mobile Web - Safari

Desktop

iOS

Android

Santhosh-Sellavel commented Nov 6, 2022 • edited Loading

Santhosh-Sellavel commented Nov 6, 2022

Screen Recordings

Desktop & Web

Android - iOS

Android - iOS mWeb

Santhosh-Sellavel commented Nov 6, 2022 • edited Loading

PR Reviewer Checklist

Santhosh-Sellavel commented Nov 6, 2022

Santhosh-Sellavel left a comment • edited Loading

Choose a reason for hiding this comment

mollfpr commented Nov 7, 2022

Santhosh-Sellavel commented Nov 7, 2022

Santhosh-Sellavel commented Nov 7, 2022

Santhosh-Sellavel left a comment • edited Loading

Choose a reason for hiding this comment

neil-marcellini left a comment

Choose a reason for hiding this comment

Santhosh-Sellavel commented Nov 7, 2022

mollfpr commented Nov 8, 2022

neil-marcellini commented Nov 9, 2022

Santhosh-Sellavel commented Nov 10, 2022

Santhosh-Sellavel commented Nov 10, 2022 • edited Loading

Web

Desktop

mollfpr Nov 10, 2022

Choose a reason for hiding this comment

neil-marcellini commented Nov 10, 2022

neil-marcellini commented Nov 10, 2022

melvin-bot bot commented Nov 10, 2022

neil-marcellini commented Nov 10, 2022

OSBotify commented Nov 10, 2022

Santhosh-Sellavel commented Nov 11, 2022

Santhosh-Sellavel commented Nov 11, 2022

OSBotify commented Nov 11, 2022

JmillsExpensify commented Nov 14, 2022

OSBotify commented Nov 15, 2022

OSBotify commented Nov 16, 2022

mollfpr commented Nov 2, 2022 •

edited

Loading

Santhosh-Sellavel commented Nov 6, 2022 •

edited

Loading

Santhosh-Sellavel commented Nov 6, 2022 •

edited

Loading

Santhosh-Sellavel left a comment •

edited

Loading

Santhosh-Sellavel left a comment •

edited

Loading

Santhosh-Sellavel commented Nov 10, 2022 •

edited

Loading