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

[kavimuru]

If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!

---

Action Performed:

1. Go to any conversation
2. Send Protected PDF (if required)
3. Open Protected PDF
4. Press TAB key
5. Notice it will directly focus on enter the password instead of download/close button

Expected Result:

On pressing TAB should first focus on Download icon, pressing TAB again should focus on Close Icon (just like chrome)

Actual Result:

On pressing TAB it directly focus on enter the password (makes modal un-usable while app is on accessibility)

Workaround:

unknown

Platform:

Where is this issue occurring?

• Web : Safari

Version Number: 1.2.18-1 Reproducible in staging?: y Reproducible in production?: y Email or phone of affected tester (no customers): Logs: https://stackoverflow.com/c/expensify/questions/4856 Notes/Photos/Videos:

https://user-images.githubusercontent.com/43996225/196806708-c5e35438-7c4a-416d-9ecf-af760a83ecc6.mov

https://user-images.githubusercontent.com/43996225/196807039-11ff5ab8-8f0f-4508-9234-34cf64068c45.mp4

Expensify/Expensify Issue URL: Issue reported by: @dhairyasenjaliya Slack conversation: https://expensify.slack.com/archives/C01GTK53T8Q/p1666150625899149

View all open jobs on GitHub

[melvin-bot[bot]]

Triggered auto assignment to @MitchExpensify (AutoAssignerTriage), see https://stackoverflow.com/c/expensify/questions/4749 for more details.

[MitchExpensify]

How do you send a protected PDF exactly @kavimuru ?

[kavimuru]

@MitchExpensify Added using add attachment from the compose box.

[JmillsExpensify]

@MitchExpensify Do you need help with a password protected PDF? I can create one for you.

[MitchExpensify]

Thanks @JmillsExpensify , I figured it out!

I am able to reproduce this on Safari - Labeling external as this is a contained front end issue

[melvin-bot[bot]]

Current assignee @MitchExpensify is eligible for the External assigner, not assigning anyone new.

[melvin-bot[bot]]

Triggered auto assignment to Contributor-plus team member for initial proposal review - @sobitneupane (External)

[melvin-bot[bot]]

Triggered auto assignment to @deetergp (External), see https://stackoverflow.com/c/expensify/questions/7972 for more details.

[MitchExpensify]

Waiting for proposals

[melvin-bot[bot]]

@deetergp, @sobitneupane, @MitchExpensify Whoops! This issue is 2 days overdue. Let's get this updated quick!

[melvin-bot[bot]]

@deetergp, @sobitneupane, @MitchExpensify Whoops! This issue is 2 days overdue. Let's get this updated quick!

[sobitneupane]

Waiting for proposals

[MitchExpensify]

Upwork Job

[MitchExpensify]

Doubled!

[melvin-bot[bot]]

@deetergp, @sobitneupane, @MitchExpensify Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

[sobitneupane]

Waiting for proposals

[MitchExpensify]

Doubled!

[MitchExpensify]

Waiting on proposals! Bumped thread here

[s77rt]

Proposal

diff --git a/src/components/PDFView/PDFInfoMessage.js b/src/components/PDFView/PDFInfoMessage.js
index 5d583db40..898983f13 100644
--- a/src/components/PDFView/PDFInfoMessage.js
+++ b/src/components/PDFView/PDFInfoMessage.js
@@ -1,6 +1,6 @@
 import React from 'react';
 import PropTypes from 'prop-types';
-import {View} from 'react-native';
+import {View, Pressable} from 'react-native';
 import Text from '../Text';
 import TextLink from '../TextLink';
 import Icon from '../Icon';
@@ -29,9 +29,11 @@ const PDFInfoMessage = props => (
         <Text>{props.translate('attachmentView.pdfPasswordForm.infoText')}</Text>
         <Text>
             {props.translate('attachmentView.pdfPasswordForm.beforeLinkText')}
-            <TextLink onPress={props.onShowForm}>
-                {` ${props.translate('attachmentView.pdfPasswordForm.linkText')} `}
-            </TextLink>
+            <Pressable onPress={props.onShowForm}>
+                <Text style={[styles.link]}>
+                    {` ${props.translate('attachmentView.pdfPasswordForm.linkText')} `}
+                </Text>
+            </Pressable>
             {props.translate('attachmentView.pdfPasswordForm.afterLinkText')}
         </Text>
     </View>
diff --git a/src/pages/home/report/ReportActionItem.js b/src/pages/home/report/ReportActionItem.js
index 27302710b..80af66ea8 100644
--- a/src/pages/home/report/ReportActionItem.js
+++ b/src/pages/home/report/ReportActionItem.js
@@ -168,10 +168,6 @@ class ReportActionItem extends Component {
                 onPressOut={() => ControlSelection.unblock()}
                 onSecondaryInteraction={this.showPopover}
                 preventDefaultContentMenu={!this.props.draftMessage}
-                onKeyDown={(event) => {
-                    // Blur the input after a key is pressed to keep the blue focus border from appearing
-                    event.target.blur();
-                }}
             >
                 <Hoverable resetsOnClickOutside>
                     {hovered => (

Details

There are two issues:

1. The main issue is introduced by #8443 to fix #7853 However the fix is not that effective, you can still get the focus border using TAB key. Besides it causes more issues specially on Safari. That's why I removed the onKeydown event listener
2. The second is more related to how Safari handles TAB and focus. After the first fix, you will be able to focus on Download and Close buttons or "Enter the password", but not in a sequence (either get stuck on the two buttons, or the text), unless you press Options(alt) + TAB then you can traverses all the three elements in sequence. To fix this we can simply wrap the "Enter the password" in a div (with tabindex attr) There is a related issue so I applied the fix suggested on #12058

This fixes this issue, #12058 (thanks to @huzaifa-99), #12554, and possibly other Safari related issues.

PS: This issue is not only related to the PDF modal, but to every popover, modal that get's called from inside the PressableWithSecondaryInteraction component

https://user-images.githubusercontent.com/16493223/201357798-af678e62-0064-4228-b808-03bb2705cf99.mp4

[sobitneupane]

@s77rt Thanks for your proposal.

Few Questions: -> Does your proposal recreate https://github.com/Expensify/App/issues/7853 issue? -> Does this proposal on the other issue handle Safari problem as well?

[s77rt]

@sobitneupane

Does your proposal recreate https://github.com/Expensify/App/issues/7853 issue?

Yes, the proposed fix does more harm than good. And as i said it's not effective, you can bypass the fix simply by pressing TAB.

Does https://github.com/Expensify/App/issues/12058#issuecomment-1307500529 on https://github.com/Expensify/App/issues/12058 handle Safari problem as well?

Yes, the second issue is caused by how Safari works, this is a very known behaviour on Safari. Possible fixes:

1. (require user interaction) simply use Option+TAB instead of TAB
2. (require use interaction) https://ebay.gitbook.io/oatmeal/help/fix-safari-tabbing
3. (does not require use interaction - chosen) use a div with tabindex attr instead of an anchor tag

[sobitneupane]

And as i said it's not effective, you can bypass the fix simply by pressing TAB.

@s77rt It is an expected behavior https://github.com/Expensify/App/issues/7853#issuecomment-1054398902:

Yeah I think from an accessibility perspective, we want the blue outline when a message is "selected" to show a visual cue that the message item is in focus - but that being said, I would think the only way to select a message is to tab through the items. So I think maybe we should just get rid of this select + click behavior. Thoughts?

Fix of this issue should not recreate the other issue.

[s77rt]

The discussion on #7853 is conflicting. You either use both TAB and SHIFT or you don't. SHIFT is also used select a message, example TAB+SHIFT to select the previous message.

• Use TAB and SHIFT (buttons works and with border): remove the onKeyDown event listener

• Use TAB and SHIFT (buttons works, but without border): apply a style to PressableWithSecondaryInteraction style={styles.noSelect}

• Don't use TAB and/or SHIFT discouraged. The current implemented code makes TAB/SHIFT effects cancelled and lead to a weird behaviour

IMHO, solutions that are based on blurring the event.target or event.relatedTarget are bad and will likely cause a lof of trouble

Not every issue is a valid issue, and not every fix is a valid fix.

[sobitneupane]

Yeah. I totally agree with you that the onKeyDown event listener is causing the issue. But your proposal should also include fix to the issue for which onKeyDown event listener was introduced.

[s77rt]

Proposal (Updated)

diff --git a/src/components/PDFView/PDFInfoMessage.js b/src/components/PDFView/PDFInfoMessage.js
index 5d583db40..898983f13 100644
--- a/src/components/PDFView/PDFInfoMessage.js
+++ b/src/components/PDFView/PDFInfoMessage.js
@@ -1,6 +1,6 @@
 import React from 'react';
 import PropTypes from 'prop-types';
-import {View} from 'react-native';
+import {View, Pressable} from 'react-native';
 import Text from '../Text';
 import TextLink from '../TextLink';
 import Icon from '../Icon';
@@ -29,9 +29,11 @@ const PDFInfoMessage = props => (
         <Text>{props.translate('attachmentView.pdfPasswordForm.infoText')}</Text>
         <Text>
             {props.translate('attachmentView.pdfPasswordForm.beforeLinkText')}
-            <TextLink onPress={props.onShowForm}>
-                {` ${props.translate('attachmentView.pdfPasswordForm.linkText')} `}
-            </TextLink>
+            <Pressable onPress={props.onShowForm}>
+                <Text style={[styles.link]}>
+                    {` ${props.translate('attachmentView.pdfPasswordForm.linkText')} `}
+                </Text>
+            </Pressable>
             {props.translate('attachmentView.pdfPasswordForm.afterLinkText')}
         </Text>
     </View>
diff --git a/src/pages/home/report/ReportActionItem.js b/src/pages/home/report/ReportActionItem.js
index 27302710b..84febd28d 100644
--- a/src/pages/home/report/ReportActionItem.js
+++ b/src/pages/home/report/ReportActionItem.js
@@ -73,6 +73,7 @@ class ReportActionItem extends Component {
         this.popoverAnchor = undefined;
         this.state = {
             isContextMenuActive: ReportActionContextMenu.isActiveReportAction(props.action.reportActionID),
+            isFocusableVisible: false,
         };
         this.checkIfContextMenuActive = this.checkIfContextMenuActive.bind(this);
         this.showPopover = this.showPopover.bind(this);
@@ -85,7 +86,8 @@ class ReportActionItem extends Component {
             || this.props.hasOutstandingIOU !== nextProps.hasOutstandingIOU
             || this.props.shouldDisplayNewIndicator !== nextProps.shouldDisplayNewIndicator
             || !_.isEqual(this.props.action, nextProps.action)
-            || this.state.isContextMenuActive !== nextState.isContextMenuActive;
+            || this.state.isContextMenuActive !== nextState.isContextMenuActive
+            || this.state.isFocusableVisible !== nextState.isFocusableVisible;
     }
 
     componentDidUpdate(prevProps) {
@@ -168,10 +170,15 @@ class ReportActionItem extends Component {
                 onPressOut={() => ControlSelection.unblock()}
                 onSecondaryInteraction={this.showPopover}
                 preventDefaultContentMenu={!this.props.draftMessage}
-                onKeyDown={(event) => {
-                    // Blur the input after a key is pressed to keep the blue focus border from appearing
-                    event.target.blur();
+                onKeyDown={(e) => {
+                    if (e.key === "Tab") {
+                        this.setState({isFocusableVisible: true});
+                    } else {
+                        this.setState({isFocusableVisible: false});
+                    }
                 }}
+                onFocus={() => this.setState({isFocusableVisible: true})}
+                style={this.state.isFocusableVisible ? {} : styles.noSelect}
             >
                 <Hoverable resetsOnClickOutside>
                     {hovered => (

Details (Updated)

The issue #7853 addresses only SHIFT key but it actually applies to every key. I would say #7853 is a valid issue after all. However the proposed fix on #8443 is problematic. In my updated proposal, instead of doing anything that affects accessibility, my solution is based on CSS. I have added a new state isFocusableVisible, if it's true then we let the outline/boxShadow to be shown just normally, if not we use the noSelect style which applies "none" to both the outline and boxShadow and thus fix the issue #7853 The value of isFocusableVisible is set to true only if we are pressing the TAB key or focusing on the element.

This fixes this issue, #7853, #12058, #12554, and possibly other Safari related issues

[parasharrajat]

Heads up, we are fixing the Tab key focus on Enter the password link in https://github.com/Expensify/App/issues/12058#issuecomment-1312066971.

cc: @sobitneupane

[sobitneupane]

Thanks @parasharrajat.

[sobitneupane]

@s77rt Your proposal does not completely solve the issue. On tabbing, the focus must rotate among download, close and enter the password.

Expected -> Download -> Close -> Enter the password -> Download ... Actual -> Download -> Close -> Focus Lost -> Enter the password -> Focus Lost -> Download

https://user-images.githubusercontent.com/25876548/201663736-e1ae9ccb-7856-4379-9a77-f3d2b7e92360.mov

Is there any way to get rid of onKeyDown event listener. It is causing many issues.

[s77rt]

@sobitneupane The expected behaviour is: Download -> Close -> Enter the password -> Focus Lost -> Download ...

And that's the behaviour I'm getting on Safari 16.0 and on other browsers too. App Version: 1.2.25-0 + Patch

Is there any way to get rid of onKeyDown event listener. It is causing many issues.

What issues? The current implemented onKeyDown is causing a lot of issues due to blur and messing with accessibility. In my proposal i changed the onKeyDown functionality and still providing a fix for #7853

Please retest

[sobitneupane]

I am testing it on v1.2.27-3. Can you please test it on latest version.