[$250] Firefox - Chat - Highlighted text disappears when selecting a part of the text and opening context menu reported by @daraksha-dk

[kavimuru]

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:

1. Go to any chat
2. Select a part of any message
3. Open the context menu for any message
4. Highlighting disappears for the selected part

Expected Result:

Highlighting should remain as it as until an action is taken

Actual Result :

It gets disappear as soon as the menu appears (working fine for other devices)

Workaround:

unknown

Platform:

Where is this issue occurring?

• Web - Firefox

Version Number: 1.2.24-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:

Recording.864.mp4

hightlighting.issue.mp4

Expensify/Expensify Issue URL:

Issue reported by: @daraksha-dk

Slack conversation: https://expensify.slack.com/archives/C01GTK53T8Q/p1667833686198119

View all open jobs on GitHub

[melvin-bot]

Triggered auto assignment to @johncschuster (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.

[johncschuster]

This issue looks unique and I can reproduce this by doing the following:

1. Go to any chat
2. Highlight any text already in the chat window
3. right click the text to bring up the context menu (highlighting remains on the text)
4. select an option in the context menu (highlighting goes away immediately after clicking an option in the context menu, but before the menu is dismissed)

I tested this in Safari and the NewDot app on Staging, so I don't think this is specific to Firefox.

@kavimuru, did you test this on all platforms? Did you get different results across platforms?

[johncschuster]

I chatted this over with @hax, and we really dialed in the nuance of the behavior in Slack.

In general, my expectation of highlighted text works something like this:

• I select some text
• I right-click on the text to bring up a context menu (text stays highlighted in the background)
• I select an option from the context menu (text stays highlighted in the background)
• To deselect the text, I would need to click somewhere else on the page.

On Firefox, it seems the highlighted text is dismissed right as the context menu is summoned. On other platforms (Safari, for example), the highlighted text is dismissed when an option from the context menu is selected.

Let's achieve parity on all platforms, either by fixing the Firefox-specific behavior to match the behavior in Safari and others or by updating all highlight-dismissing behavior to match what I would consider "system-level" behavior.

[melvin-bot]

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

[melvin-bot]

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

[melvin-bot]

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

[johncschuster]

Upwork job: https://www.upwork.com/jobs/~01f721fddf8bdf3016

[s77rt]

Proposal

fixing the Firefox-specific behavior to match the behavior in Safari and others

--- a/src/components/PopoverWithMeasuredContent.js
+++ b/src/components/PopoverWithMeasuredContent.js
@@ -72,9 +72,66 @@ class PopoverWithMeasuredContent extends Component {
         this.state = {
             isContentMeasured: this.popoverWidth > 0 && this.popoverHeight > 0,
             isVisible: false,
+
+            prevSelection: null,
         };
 
         this.measurePopover = this.measurePopover.bind(this);
+
+        this.onSelectionChange = this.onSelectionChange.bind(this);
+    }
+
+    getSnapshotBeforeUpdate(prevProps, prevState) {
+        // Save current selection while the popover is not visible yet
+        if (prevState.isVisible === false) {
+            const selection = window.getSelection();
+            if (selection.rangeCount > 0) {
+                this.setState({
+                    prevSelection: {
+                        range: selection.getRangeAt(0)
+                    }
+                });
+            }
+        }
+
+        return null;
+    }
+
+    componentDidUpdate(prevProps, prevState, snapshot) {
+        /*
+            In FF, the change in document.activeElement will cause a change in the window current selection
+            Using the event "selectionchange" we can reset the selection
+
+            In other browsers, the window current selection will remain unchanged; "selectionchange" event will not be fired
+            for the sake of consistency and to make the behavior identical across all browsers, we trigger that event manually
+        */
+        if (this.state.prevSelection !== null && this.state.isVisible === true) {
+            if (navigator.userAgent.search("Firefox") === -1) { // Not Firefox
+                document.dispatchEvent(new Event("selectionchange"));
+            }
+        }
+    }
+
+    onSelectionChange(e) {
+        // If the selection changed and we have a saved selection, revert to that saved selection
+        if (this.state.prevSelection !== null && this.state.isVisible === true) {
+            const selection = window.getSelection();
+            const range = this.state.prevSelection.range
+            selection.removeAllRanges();
+            selection.addRange(range);
+
+            this.setState({
+                prevSelection: null
+            });
+        }
+    }
+
+    componentDidMount() {
+        document.addEventListener('selectionchange', this.onSelectionChange);
+    }
+
+    componentWillUnmount() {
+        document.removeEventListener('selectionchange', this.onSelectionChange);
     }
 
     /**

Selection and input focus (indicated by Document.activeElement) have a complex relationship that varies by browser. In cross-browser compatible code, it's better to handle them separately.
Safari and Chrome (unlike Firefox) currently focus the element containing selection when modifying the selection programmatically; it's possible that this may change in the future (see W3C bug 14383 and WebKit bug 38696).

https://developer.mozilla.org/en-US/docs/Web/API/Selection#selection_and_input_focus

Explantion

The workaround it to save the current selection just before the context menu (popover) is displayed
Then once it's displayed, FF will change the current selection since the active element has changed, this change will trigger the event "selectionchange" which we will be licensing for so we can revert the change and get the selection back. Others browsers will have a side-effect where context menu won't be closed until you click twice (the first click will trigger "selectionchange" and will reselect the text that's already selected -- because other browsers won't change the selection and won't trigger this event), to fix this side-effect I have added a condition to check if the browser is not FF, then trigger the "selectionchange" event manually so the behaviour is all the same.
Note: in FF the selected text may flicker due to the lost focus and the re-selection (see attached video, first time flicked, second didn't). Not sure how to fix this completely, that's how far I got

Kooha-2022-11-08-15-58-55.mp4

[johncschuster]

@stitesExpensify what are your thoughts on the proposal?

[stitesExpensify]

I'll have @thesahindia take a look first 😄

[thesahindia]

Sorry for the delay but I don't have the bandwidth. Please re-assign.

[melvin-bot]

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

[melvin-bot]

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

[melvin-bot]

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

[stitesExpensify]

Hi @Santhosh-Sellavel can you please check out @s77rt 's proposal?

[s77rt]

Proposal (Updated)

diff --git a/src/components/Modal/BaseModal.js b/src/components/Modal/BaseModal.js
index 91b1867a6..f7300cc09 100644
--- a/src/components/Modal/BaseModal.js
+++ b/src/components/Modal/BaseModal.js
@@ -103,8 +103,8 @@ class BaseModal extends PureComponent {
                 backdropColor={themeColors.modalBackdrop}
                 backdropOpacity={hideBackdrop ? 0 : 0.5}
                 backdropTransitionOutTiming={0}
-                hasBackdrop={this.props.fullscreen}
-                coverScreen={this.props.fullscreen}
+                hasBackdrop={typeof(this.props.hasBackdrop) === "boolean" ? this.props.hasBackdrop : this.props.fullscreen}
+                coverScreen={typeof(this.props.coverScreen) === "boolean" ? this.props.coverScreen : this.props.fullscreen}
                 style={modalStyle}
                 deviceHeight={this.props.windowHeight}
                 deviceWidth={this.props.windowWidth}
diff --git a/src/components/Modal/index.js b/src/components/Modal/index.js
index add8feef0..d7b7a1603 100644
--- a/src/components/Modal/index.js
+++ b/src/components/Modal/index.js
@@ -6,29 +6,52 @@ import {propTypes, defaultProps} from './modalPropTypes';
 class Modal extends Component {
     constructor(props) {
         super(props);
+        this.focus = this.focus.bind(this);
+        this.trapFocus = this.trapFocus.bind(this);
         this.closeOnOutsideClick = this.closeOnOutsideClick.bind(this);
     }
 
     componentDidMount() {
-        if (!this.props.shouldCloseOnOutsideClick) {
-            return;
+        if (this.props.shouldTrapFocus) {
+            document.addEventListener('focusin', this.trapFocus);
+        }
+        if (this.props.shouldCloseOnOutsideClick) {
+            document.addEventListener('mousedown', this.closeOnOutsideClick);
         }
-
-        document.addEventListener('mousedown', this.closeOnOutsideClick);
     }
 
     componentWillUnmount() {
-        if (!this.props.shouldCloseOnOutsideClick) {
+        if (this.props.shouldTrapFocus) {
+            document.removeEventListener('focusin', this.trapFocus);
+        }
+        if (this.props.shouldCloseOnOutsideClick) {
+            document.removeEventListener('mousedown', this.closeOnOutsideClick);;
+        }
+    }
+
+    focus() {
+        if (!this.props.isVisible || !this.baseModalRef) {
             return;
         }
+        this.baseModalRef.focus();
+    }
 
-        document.removeEventListener('mousedown', this.closeOnOutsideClick);
+    trapFocus(event) {
+        if (!this.props.isVisible
+            || !this.baseModalRef
+            || (this.baseModalRef && this.baseModalRef.contains(event.target))
+            || !this.props.shouldTrapFocus) {
+            return;
+        }
+
+        event.preventDefault();
+        this.baseModalRef.focus();
     }
 
     closeOnOutsideClick(event) {
         if (!this.props.isVisible
             || !this.baseModalRef
-            || this.baseModalRef.contains(event.target)
+            || (this.baseModalRef && this.baseModalRef.contains(event.target))
             || !this.props.shouldCloseOnOutsideClick) {
             return;
         }
@@ -39,9 +62,13 @@ class Modal extends Component {
     render() {
         return (
             <BaseModal
-                ref={el => this.baseModalRef = el}
                 // eslint-disable-next-line react/jsx-props-no-spreading
                 {...this.props}
+                ref={el => this.baseModalRef = el}
+                onModalShow={() => {
+                    this.baseModalRef.setAttribute("tabindex", "-1");
+                    this.props.onModalShow();
+                }}
             >
                 {this.props.children}
             </BaseModal>
diff --git a/src/components/Modal/modalPropTypes.js b/src/components/Modal/modalPropTypes.js
index 8f09588e3..14b20b78b 100644
--- a/src/components/Modal/modalPropTypes.js
+++ b/src/components/Modal/modalPropTypes.js
@@ -5,9 +5,18 @@ import {windowDimensionsPropTypes} from '../withWindowDimensions';
 import stylePropTypes from '../../styles/stylePropTypes';
 
 const propTypes = {
-    /** Decides whether the modal should cover fullscreen. FullScreen modal has backdrop */
+    /** Decides whether the modal should cover the screen (can be overridden by specifying coverScreen explicitly). FullScreen modal has backdrop (can be overridden by specifying hasBackdrop explicitly) */
     fullscreen: PropTypes.bool,
 
+    /** Decides whether the modal should cover screen */
+    coverScreen: PropTypes.bool,
+
+    /** Decides whether the modal should have a backdrop */
+    hasBackdrop: PropTypes.bool,
+
+    /** Decides whether the modal should trap focus */
+    shouldTrapFocus: PropTypes.bool,
+
     /** Should we close modal on outside click */
     shouldCloseOnOutsideClick: PropTypes.bool,
 
@@ -66,7 +75,8 @@ const propTypes = {
 
 const defaultProps = {
     fullscreen: true,
-    shouldCloseOnOutsideClick: false,
+    shouldTrapFocus: true,
+    shouldCloseOnOutsideClick: true,
     shouldSetModalVisibility: true,
     onSubmit: null,
     type: '',
diff --git a/src/components/Popover/index.js b/src/components/Popover/index.js
index 9a2e20ed3..354275282 100644
--- a/src/components/Popover/index.js
+++ b/src/components/Popover/index.js
@@ -31,6 +31,7 @@ const Popover = (props) => {
             // eslint-disable-next-line react/jsx-props-no-spreading
             {...props}
             fullscreen={props.isSmallScreenWidth ? true : props.fullscreen}
+            hasBackdrop={props.isSmallScreenWidth ? true : props.hasBackdrop}
             animationInTiming={props.disableAnimation && !props.isSmallScreenWidth ? 1 : props.animationInTiming}
             animationOutTiming={props.disableAnimation && !props.isSmallScreenWidth ? 1 : props.animationOutTiming}
         />
diff --git a/src/pages/home/report/ContextMenu/PopoverReportActionContextMenu.js b/src/pages/home/report/ContextMenu/PopoverReportActionContextMenu.js
index d28e7db1e..0843f0918 100644
--- a/src/pages/home/report/ContextMenu/PopoverReportActionContextMenu.js
+++ b/src/pages/home/report/ContextMenu/PopoverReportActionContextMenu.js
@@ -288,7 +288,8 @@ class PopoverReportActionContextMenu extends React.Component {
                     animationOutTiming={1}
                     measureContent={this.measureContent}
                     shouldSetModalVisibility={false}
-                    fullscreen
+                    hasBackdrop={true}
+                    coverScreen={false}
                 >
                     <BaseReportActionContextMenu
                         isVisible
diff --git a/src/styles/getModalStyles/getBaseModalStyles.js b/src/styles/getModalStyles/getBaseModalStyles.js
index 7346a4951..0466a2c33 100644
--- a/src/styles/getModalStyles/getBaseModalStyles.js
+++ b/src/styles/getModalStyles/getBaseModalStyles.js
@@ -151,7 +151,7 @@ export default (type, windowDimensions, popoverAnchorPosition = {}, containerSty
                 ...modalStyle,
                 ...popoverAnchorPosition,
                 ...{
-                    position: 'absolute',
+                    position: 'fixed',
                     alignItems: 'center',
                     justifyContent: 'flex-end',
                 },

diff --git a/node_modules/react-native-modal/dist/modal.style.d.ts b/node_modules/react-native-modal/dist/modal.style.d.ts
index 1734c97..b9d09ad 100644
--- a/node_modules/react-native-modal/dist/modal.style.d.ts
+++ b/node_modules/react-native-modal/dist/modal.style.d.ts
@@ -1,6 +1,6 @@
 declare const _default: {
     backdrop: {
-        position: "absolute";
+        position: "fixed";
         top: number;
         bottom: number;
         left: number;
diff --git a/node_modules/react-native-modal/dist/modal.style.js b/node_modules/react-native-modal/dist/modal.style.js
index 191ea2f..b6aa1ca 100644
--- a/node_modules/react-native-modal/dist/modal.style.js
+++ b/node_modules/react-native-modal/dist/modal.style.js
@@ -1,7 +1,7 @@
 import { StyleSheet } from 'react-native';
 export default StyleSheet.create({
     backdrop: {
-        position: 'absolute',
+        position: 'fixed',
         top: 0,
         bottom: 0,
         left: 0,

Details

I have implemented a simple trap focus for accessibility. I have also enabled shouldCloseOnOutsideClick to be true per default (btw, this was implemented but never used, weird). I have also applied a small patch to react-native-modal on the backdrop style to cover this issue without having to set the coverScreen property to true.

Pros:

1. Fix this issue on all browsers
2. TAB Accessibility
3. Fix a lot of bugs (setting coverScreen to false fixes a lot of things)

Cons:

1. You can click outside the container block of the modal (e.g. you can select another chat while the modal is open)
2. The very first TAB press will focus on the modal, you will have to press TAB once again to focus the first menu item. This can be fixed easily but doing so will introduce the issue back (getting back to square 1), I think that's what react native does.
3. Pressing TAB makes the text get deselected on Firefox and Safari <-- I guess that's how those browsers work, focus and text selection is complicated

Thoughts

I think with this proposal we are somewhere in the middle or above, providing the most important features / accessibility with the least drawbacks. Let me know what do you think about this.

PS: If you apply the react-native-modal patch, make sure to clear the cache (rm -rf node_modules/.cache) and restart the app (patches does not support hotreload).

[mollfpr]

Thanks again @s77rt for the update and the details.

Fix a lot of bugs (setting coverScreen to false fixes a lot of things)

Do you mind details about the bugs?

Btw per this thread we freeze reports for accessibility issues, so I don't think we want to log another tab accessibility from the CONS either.

I'm bringing this issue to the team for opinions. Sorry for the back and forth.

cc @srikarparsi

[s77rt]

@mollfpr
About the bugs, for starters:

1. Open any chat
2. Resize the window so only the chat is shown (as seen in mobile)
3. Open the actions menu (click the + button)
4. Use the browser back button (or device backpress)
5. You are now stuck; modal is still open, can't be closed and you can't navigate away

I have stated this behaviour somewhere else in the GH issues (where I proposed same modal modification)

[srikarparsi]

The back and forth is actually good, thank you @mollfpr and @s77rt for the quick suggestions and fixes.

[melvin-bot]

@johncschuster, @mollfpr, @srikarparsi Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

[mollfpr]

I feel this proposal is over-engineered and too risky causing a regression. I'll deep dive into the root cause while waiting for others' proposals. Thanks!

cc @srikarparsi

[s77rt]

The issue is related to the modals.
You will experience the same thing with any other modal e.g. bootstrap modal

Currently I only see that this can be fixed by not using modals in the first place as we can't force browsers behaviours.

The change may seem too much but that's only due to the existing code that does the coupling of two separated prop into one (hasBackdrop and coverScreen into fullscreen) I had to make changes without breaking other modals.

I would say to close this as not-to-be-fixed or at least hold, but the more I investigate accessibility issues it's always about modals. Not to mention this bug

[mollfpr]

Thanks, @s77rt for the investigation.

Yes, currently we know that the problem is from the RNW modal. But the fact that it only works in Chrome is still not clear to me.

I'll wait a few more days for a new proposal.

[iwiznia]

AFAIK we said we would not be focusing on supporting firefox, so I think we should close this issue.

[mollfpr]

@srikarparsi @johncschuster Do we want to close this issue?

[s77rt]

@srikarparsi @johncschuster @mollfpr
Before closing, please consider this bug as well.
The mentioned bug is not browser/platform specific but the proposed solution shall fix it too.

[mollfpr]

@s77rt Is this the bug that you mention?

[s77rt]

@mollfpr No, the bug you linked is about the keyboard staying open.
The bug i'm talking about is more annoying and make the app basically unusable and broken. I didn't report the bug yet.
Open a modal, then click the back button in browser or the backpress on mobile and you are now stuck.

Kooha-2022-11-27-17-26-53.mp4

[mollfpr]

No, the bug you linked is about the keyboard staying open.

Whops my bad.

The bug i'm talking about is more annoying and make the app basically unusable and broken. I didn't report the bug yet.
Open a modal, then click the back button in browser or the backpress on mobile and you are now stuck.

I've seen that bug log before, but I couldn't find it now.

[melvin-bot]

@johncschuster, @mollfpr, @srikarparsi Huh... This is 4 days overdue. Who can take care of this?

[mollfpr]

@srikarparsi @johncschuster Should we close this because we not focusing on Firefox support?

[johncschuster]

I'm in favor of closing the issue since it's Firefox-specific. @srikarparsi, what are your thoughts?

[srikarparsi]

I agree, let's close out this issue since it's Firefox-specific. @s77rt did you say you already reported this issue or did someone else report it?

[s77rt]

@srikarparsi No it has not been reported anywhere. I was hoping to get it silently fixed here or here Should I report this to be handled separately?

[srikarparsi]

Yes, can you please do that @s77rt and I'm good to close out this issue as well @johncschuster. Most of the changes you proposed in this issue would probably still apply but it would be better to put it under the new issue since that's the problem we're solving.

[johncschuster]

Thanks, @srikarparsi! I'll go ahead and close this issue.