melvin-bot[bot]
commented
1 year ago Triggered auto assignment to @joekaufmanexpensify (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.
Closed kavimuru closed 1 year ago
melvin-bot[bot]
commented
1 year ago Triggered auto assignment to @joekaufmanexpensify (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.
jasperhuangg
commented
1 year ago Hey! Don't really think this needs to be fixed, @joekaufmanexpensify do you agree?
sketchydroide
commented
1 year ago this is a rather specific BUG, but one nonetheless, and acording to WAQ, if it't reproducable it's a Bug... Also I can see reasons a user would like to copy paste keyboard shortcuts, namely to send it to another user so 🤷🏼 Oh and I can reproduce it to.
If you don't take it I will @jasperhuangg
joekaufmanexpensify
commented
1 year ago Yes, agreed. My understanding is if it's a reproducible bug, we will fix it. I actually raised this yesterday, and this is where we landed.
joekaufmanexpensify
commented
1 year ago Triaging:
From this SO: https://stackoverflow.com/c/expensify/questions/14418
joekaufmanexpensify
commented
1 year ago I can reproduce the fact that the text is not selectable on the second try here. Whether we want the text to be selectable, I am less sure. I am going to bring to Slack.
joekaufmanexpensify
commented
1 year ago Posted about this here.
joekaufmanexpensify
commented
1 year ago Bumped slack discussion.
joekaufmanexpensify
commented
1 year ago Still waiting for more discussion in Slack.
joekaufmanexpensify
commented
1 year ago Not overdue.
jasperhuangg
commented
1 year ago Hey sorry @sketchydroide I missed your message, feel free to take this on!
joekaufmanexpensify
commented
1 year ago Bumped slack thread. I think we should get crisp on what the expected behavior is here before we fix this. Either way, I think this is a bug, but just want to make sure we know whether this text should be copiable in all situations, or none (thinking the former).
joekaufmanexpensify
commented
1 year ago Okay, we agreed in the slack thread that the expected behavior here is that you can copy the text any time this page is opened. So we're all set to proceed. It sounds like you want to take this on @sketchydroide and keep this internal. Is that right?
joekaufmanexpensify
commented
1 year ago waiting for confirmation
joekaufmanexpensify
commented
1 year ago Chatted with Andre, and making this external.
melvin-bot[bot]
commented
1 year ago Job added to Upwork: https://www.upwork.com/jobs/~0142c09828b956ad3b
melvin-bot[bot]
commented
1 year ago Current assignee @joekaufmanexpensify is eligible for the External assigner, not assigning anyone new.
melvin-bot[bot]
commented
1 year ago Triggered auto assignment to Contributor-plus team member for initial proposal review - @aimane-chnaif (External)
melvin-bot[bot]
commented
1 year ago Triggered auto assignment to @pecanoro (External), see https://stackoverflow.com/c/expensify/questions/7972 for more details.
getusha
commented
1 year ago index 323df19c77..11b4f50854 100644
--- a/src/components/Modal/BaseModal.js
+++ b/src/components/Modal/BaseModal.js
@@ -116,6 +116,7 @@ class BaseModal extends PureComponent {
animationInTiming={this.props.animationInTiming}
animationOutTiming={this.props.animationOutTiming}
statusBarTranslucent={this.props.statusBarTranslucent}
+ focusable
>
<SafeAreaInsetsContext.Consumer>
{(insets) => {OR we can apply the focus here if we don't want additional focus on the modal border
index 323df19c77..b5452e7985 100644
--- a/src/components/Modal/BaseModal.js
+++ b/src/components/Modal/BaseModal.js
@@ -149,6 +149,7 @@ class BaseModal extends PureComponent {
...modalPaddingStyles,
}}
ref={this.props.forwardedRef}
+ focusable
>
{this.props.children}
</View>Because the modal was not focusable, the text inside the modal cannot be selected. This is because text selection is typically enabled on focusable elements that contain textual content.
This solution applies to all instances of the issue, which occurs in every modal.
syedsaroshfarrukhdot
commented
1 year ago Proposal :-
Issue occurs because modal is not focused when closed and reopened so can use focusable props.
To avoid any regression on modals where we don't need focus such as data pickers modal etc we should set focusable using props and use it in modals where issue is being occurred.
diff --git a/src/components/KeyboardShortcutsModal.js b/src/components/KeyboardShortcutsModal.js
index c45ced82f..3fc9df81c 100644
--- a/src/components/KeyboardShortcutsModal.js
+++ b/src/components/KeyboardShortcutsModal.js
@@ -81,6 +81,7 @@ class KeyboardShortcutsModal extends React.Component {
type={modalType}
containerStyle={{...styles.keyboardShortcutModalContainer, ...StyleUtils.getKeyboardShortcutsModalWidth(this.props.isSmallScreenWidth)}}
onClose={KeyboardShortcutsActions.hideKeyboardShortcutModal}
+ focusable
>
<HeaderWithCloseButton title={this.props.translate('keyboardShortcutModal.title')} onCloseButtonPress={KeyboardShortcutsActions.hideKeyboardShortcutModal} />
<View style={[styles.p5, styles.pt0]}>
diff --git a/src/components/Modal/BaseModal.js b/src/components/Modal/BaseModal.js
index 323df19c7..90f8db626 100644
--- a/src/components/Modal/BaseModal.js
+++ b/src/components/Modal/BaseModal.js
@@ -34,7 +34,6 @@ class BaseModal extends PureComponent {
if (prevProps.isVisible === this.props.isVisible) {
return;
}
-
Modal.willAlertModalBecomeVisible(this.props.isVisible);
}
@@ -116,6 +115,7 @@ class BaseModal extends PureComponent {
animationInTiming={this.props.animationInTiming}
animationOutTiming={this.props.animationOutTiming}
statusBarTranslucent={this.props.statusBarTranslucent}
+ focusable={this.props.focusable}
>
<SafeAreaInsetsContext.Consumer>
{(insets) => {OR
We need to remove the focus from body when modal opens and add focus back to body when modal is closed like below.
diff --git a/src/components/Modal/index.web.js b/src/components/Modal/index.web.js
index 3cb6f3561..9d17b6584 100644
--- a/src/components/Modal/index.web.js
+++ b/src/components/Modal/index.web.js
@@ -18,11 +18,13 @@ const Modal = (props) => {
const hideModal = () => {
setStatusBarColor();
props.onModalHide();
+ document.body.setAttribute('tabindex', '1');
};
const showModal = () => {
setStatusBarColor(StyleUtils.getThemeBackgroundColor());
props.onModalShow();
+ document.body.removeAttribute('tabindex');
};OR
We could do something like this
diff --git a/src/components/Modal/index.web.js b/src/components/Modal/index.web.js
index 3cb6f3561..7b491f05d 100644
--- a/src/components/Modal/index.web.js
+++ b/src/components/Modal/index.web.js
@@ -18,11 +18,13 @@ const Modal = (props) => {
const hideModal = () => {
setStatusBarColor();
props.onModalHide();
+ document.body.focus()
};
const showModal = () => {
setStatusBarColor(StyleUtils.getThemeBackgroundColor());
props.onModalShow();
+ document.body.blur()
};
aimane-chnaif
commented
1 year ago To avoid any regression on modals where we don't need focus such as data pickers modal etc we should set focusable using props and use it in modals where issue is being occurred.
@syedsaroshfarrukhdot do you have any instances of regression when focusable applied to Modal always? please share video if possible.
aimane-chnaif
commented
1 year ago focusable on individual screen.In this GH, it's ideal to apply this fix to all modals that need focused (text selection, scroll, etc).
getusha
commented
1 year ago @aimane-chnaif there is no regression as of my tests you can confirm that by adding
onBlur={()=>{
console.log("Modal Blurred")
}}the blur happens every time we close the modal
syedsaroshfarrukhdot
commented
1 year ago @aimane-chnaif Such As In Send Attachment Modal Text In Not Selectable From Default.
You can see in video below.
After Adding Focusable It Make Text Selectable In The Send Attachment Modal Where We Don't Need It So It Is A Regression I Believe.
I proposed solution to add focusable only in KeyboardShortcutModal only because the scope of this issue was to make text selectable always only on this modal.
If you want I can identify any other instances if there is need to add focus on any other modals where are we facing text selectable issues where we need it.
After My Fix :-
Also I Checked WhatsApp Doesn't Allow Text Selectable On Keyboard ShortCut Modal
Should We Also Disable Selection On Keyboard Shortcut Modal Completely ?
If We Agree Upon It We Can Achieve It By Following Code.
diff --git a/src/libs/actions/KeyboardShortcuts.js b/src/libs/actions/KeyboardShortcuts.js
index d619e9c8f..27f46a43a 100644
--- a/src/libs/actions/KeyboardShortcuts.js
+++ b/src/libs/actions/KeyboardShortcuts.js
@@ -1,5 +1,6 @@
import Onyx from 'react-native-onyx';
import ONYXKEYS from '../../ONYXKEYS';
+import ControlSelection from '../ControlSelection';
let isShortcutsModalOpen;
Onyx.connect({
@@ -14,6 +15,7 @@ function showKeyboardShortcutModal() {
if (isShortcutsModalOpen) {
return;
}
+ ControlSelection.block();
Onyx.set(ONYXKEYS.IS_SHORTCUTS_MODAL_OPEN, true);
}
@@ -24,6 +26,7 @@ function hideKeyboardShortcutModal() {
if (!isShortcutsModalOpen) {
return;
}
+ ControlSelection.unblock();
Onyx.set(ONYXKEYS.IS_SHORTCUTS_MODAL_OPEN, false);
}
this issue is if the cursor is I beam the text should be selectable ref: https://expensify.slack.com/archives/C049HHMV9SM/p1672770003595509?thread_ts=1672381522.931449&cid=C049HHMV9SM
syedsaroshfarrukhdot
commented
1 year ago I don't agree with it as above in my comment you can see reference is WhatsApp Keyboard Shotcut Modal where cursor is I-Beam but they have disable selection on the keyboard shortcut modal. So it is totally up to the development team how to handle text selection.
getusha
commented
1 year ago The text is not blocked by default actually, it's an issue which should be addressed and my proposal it fixes it all
getusha
commented
1 year ago And IMO since the proposal is fixing the issue, it's another thing that we want to apply it on a specific component or not
s77rt
commented
1 year ago In node_modules/@expensify/react-native-web/dist/exports/UIManager modify both index.js and index.js.flow by making this change:
@@ -00,0 +00,0 @@ var focusableElements = {
A: true,
INPUT: true,
SELECT: true,
- TEXTAREA: true
+ TEXTAREA: true,
+ BODY: true // Adding tabIndex to BODY messes up modal accessibility. Browsers do focus the body without the need of tabIndex
};
var UIManager = {
blur(node) {PS: not a valid diff, you need to make the changes manually and not using git apply
Getting to the RCA is pretty easy as you can see we can select just fine at first and the issue only happens after closing the modal.
That's because at first you can see that the body tag is something like that <body style="background-color: rgb(6, 27, 9);"> However after you close the modal it will become <body style="background-color: rgb(6, 27, 9);" tabindex="-1"> We can see that this is related to tabIndex and that's why the proposals above that are focus related seems to work.
The proper fix is to understand why tabindex="-1" is being added and is it necessary?
Okay, so there is a known behaviour on RN modals that once you close the modal, it will reset focus to the previously focused element.
and in our case the previously focused element is <body>.
In RNW the focus functionality is handled by UIManager in which we have a function that takes a node and focus it, and if that node is not focusable it adds tabIndex="-1" to make it focusable.
The thing is <body> is a special case where it can be focused without the need of tabIndex. You can verify that by document.body.focus() followed by document.activeElement you will get <body>.
Now we know why tabIndex is being added and we know that it is unnecessary to add and it's the root cause of this issue (and possibly others) so we can fix the issue by simply excluding <body> from getting that attribute.
The proposal makes changes to node_modules and those are not handled by webpack so after you apply the patch you need to clear the cache and rebuild: rm -rf node_modules/.cache && npm run web
I think we should raise a PR upstream and in our fork instead of applying a patch.
joekaufmanexpensify
commented
1 year ago Reviews of solutions still pending.
aimane-chnaif
commented
1 year ago After Adding Focusable It Make Text Selectable In The Send Attachment Modal Where We Don't Need It So It Is A Regression I Believe.
@syedsaroshfarrukhdot this is not a regression but fix. When first open attachment modal, texts are selectable. But when close it and open again, not selectable. This is issue and should be fixed in this GH.
pecanoro
commented
1 year ago Is there a case in which we wouldn't want the text to be focusable?
aimane-chnaif
commented
1 year ago Is there a case in which we wouldn't want the text to be focusable?
No case I found. But I'd like to hear @syedsaroshfarrukhdot's thoughts which stops us progressing with @getusha's proposal
s77rt
commented
1 year ago @pecanoro @aimane-chnaif Would you please take a look at my proposal when you get a chance? I think we are looking to fix the issue upstream.
getusha
commented
1 year ago @aimane-chnaif there is no case i found either. and i explained his concern here https://github.com/Expensify/App/issues/13968#issuecomment-1380290144
aimane-chnaif
commented
1 year ago I don't think anyone found the correct root cause which explains why tabindex="-1" is being added to body when close modal.
Okay, so there is a known behaviour on RN modals that once you close the modal, it will reset focus to the previously focused element.
@s77rt this is correct but I don't think previously focused element is expected to be body as default. There should be hidden issue why previously focused element is not set correctly when close modal.
s77rt
commented
1 year ago @aimane-chnaif
I don't think previously focused element is expected to be
bodyas default
Why do you think so? The body is supposed to be the default, if no element is focused then the body is set to be focused. You can test that on any website that's the expected behaviour.
s77rt
commented
1 year ago @aimane-chnaif If you are referring that the focused element should be the button, then yes usually it is but we blur the buttons on click and moving the focus to body. https://github.com/Expensify/App/blob/7f276b6d02a6e596abe314ff44a6197b8f2397d4/src/components/MenuItem.js#L70-L72
Why we blur the buttons on click? This was introduced by https://github.com/Expensify/App/pull/9348 to fix https://github.com/Expensify/App/issues/9248
aimane-chnaif
commented
1 year ago @aimane-chnaif If you are referring that the focused element should be the button, then yes usually it is but we blur the buttons on click and moving the focus to body.
Why we blur the buttons on click? This was introduced by #9348 to fix #9248
yes I expected this answer
s77rt
commented
1 year ago So technically this is a regression from https://github.com/Expensify/App/pull/9348
aimane-chnaif
commented
1 year ago s77rt
commented
1 year ago @aimane-chnaif Yes, that's both a regression from our code and hidden bug in RNW.
Fixing that on RNW will fix the issue once and for all (for modals (<BaseModal />) and for similar popups that we use or may use in the future)
getusha
commented
1 year ago after the blur we need to focus the modal that's why adding focusable solves all these issue.
s77rt
commented
1 year ago I have raised a PR on RNW https://github.com/necolas/react-native-web/pull/2468 Following our upstream fix procedure I think I should create a PR on our forked repo as well (if the solution is acceptable ofc)
joekaufmanexpensify
commented
1 year ago Still discussing proposals
joekaufmanexpensify
commented
1 year ago Same.
joekaufmanexpensify
commented
1 year ago @aimane-chnaif when you have a chance, could you share an update on where we are this one/whether we're close to being able to proceed with any of the proposals? TY!
aimane-chnaif
commented
1 year ago I expect to get updates from upstream repo. If @s77rt's PR is progressing, I am more inclined to this.
If no news from them soon, I think just applying focusable to Modal is fine and simple. (I don't find any impact on this approach yet). I don't suggest to apply @s77rt's solution to our forked RNW repo which necolas or their team doesn't accept or is not familiar with.
s77rt
commented
1 year ago @aimane-chnaif Even if the PR ended up not being accepted we would still be better merging it in our repo. Some changes are not necessary to be applied upstream that's why we have forks. Applying focusable to <Modal /> is only a temporarily solution as we will have remove it later once we start focusing on accessibility again.
If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!
Action Performed:
Expected Result:
not able to select text when you open popup second time
Actual Result:
user should be able to select text
Workaround:
unknown
Platforms:
Which of our officially supported platforms is this issue occurring on?
Version Number: 1.2.47-0 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/210454215-e770a2da-afdf-4dbe-ab23-2b6b7e45c4bb.mov
https://user-images.githubusercontent.com/43996225/210454442-330d11d9-8dde-4e24-bdcd-ec5a5c6d5a30.mp4
Expensify/Expensify Issue URL: Issue reported by: @gadhiyamanan Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1672381522931449
View all open jobs on GitHub
Upwork Automation - Do Not Edit