[$1000] 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:

Screen.Recording.2022-10-19.at.8.56.29.AM.mov

Recording.30.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]

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]

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

[melvin-bot]

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

[melvin-bot]

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

[MitchExpensify]

Waiting for proposals

[melvin-bot]

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

[melvin-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!

[sobitneupane]

@s77rt Thanks for your proposal.

Few Questions:
-> Does your proposal recreate #7853 issue?
-> Does this proposal on the other issue handle Safari problem as well?

[s77rt]

@sobitneupane

Does your proposal recreate #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 #12058 (comment) on #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 #7853 (comment):

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 #12058 (comment).

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

Screen.Recording.2022-11-14.at.18.27.07.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.

[sobitneupane]

Why is this expected behavior?

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

This should be the expected behavior

Expected : Download -> Close -> Enter the password -> Download ...

[sobitneupane]

Also please update your proposal based on #12010 (comment)

Heads up, we are fixing the Tab key focus on Enter the password link in #12058 (comment).

Your proposal has some conflict with the accepted proposal in the othe issue.

[s77rt]

@sobitneupane
That's the behaviour I was getting on other browsers. I had to match Safari with the rest.

By the way, the lost focus issue is a regression and it's effecting all the browsers

[s77rt]

regression from #12391
adding focusable is causing the issue

[sobitneupane]

By the way, the lost focus issue is a regression and it's effecting all the browsers.

Then we should get rid of this regression first. Reported here

[sobitneupane]

#12718 might solve the issue. Will test once it is merged.

[s77rt]

@sobitneupane I don't know if this is still a valid issue
The revert is not enough to fix the issue
The tracking got messy quickly. This issue is caused by two issues:

1. The fix of the first issue related to keydown event may no longer exists (due to a revert)
2. The fix of the second issue related to non-focusable "Enter the password" was based on @huzaifa-99 solution

I don't know if my proposals are eligible anymore, but I think finding the root cause and deducting that issue is caused by two separate issue have a value

[melvin-bot]

@deetergp, @sobitneupane, @MitchExpensify Eep! 4 days overdue now. Issues have feelings too...

[MitchExpensify]

Based on an internal discussion I am closing this issue because it relates to tab navigation is more accurately a new feature versus being a bug. I've place it on the accessibility milestone so it can be picked up down the road!

[melvin-bot]

Looks like something related to react-navigation may have been mentioned in this issue discussion.

As a reminder, please make sure that all proposals are not workarounds and that any and all attempt to fix the issue holistically have been made before proceeding with a solution. Proposals to change our DeprecatedCustomActions.js files should not be accepted.

Feel free to drop a note in #expensify-open-source with any questions.