Expensify
[$250] Firefox - Chat - Highlighted text disappears when selecting a part of the text and opening context menu reported by @daraksha-dk
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:
- Go to any chat
- Select a part of any message
- Open the context menu for any message
- 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:
https://user-images.githubusercontent.com/43996225/200359601-bea73485-ac8c-47be-b1e9-e066f8ed472a.mp4
https://user-images.githubusercontent.com/43996225/200359623-ca3aa506-ed77-482f-9a91-45afdb7c8ac3.mp4
Expensify/Expensify Issue URL:
Issue reported by: @daraksha-dk
Slack conversation: https://expensify.slack.com/archives/C01GTK53T8Q/p1667833686198119
Triggered auto assignment to @johncschuster (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.
![melvin-bot[bot] avatar](https://avatars.githubusercontent.com/in/179547?v=4 "Published by a pub.dev verified publisher"){kind=small}
This issue looks unique and I can reproduce this by doing the following:
- Go to any chat
- Highlight any text already in the chat window
- right click the text to bring up the context menu (highlighting remains on the text)
- 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?
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.
Current assignee @johncschuster is eligible for the External assigner, not assigning anyone new.
Triggered auto assignment to Contributor-plus team member for initial proposal review - @thesahindia (External)
Triggered auto assignment to @stitesExpensify (External), see https://stackoverflow.com/c/expensify/questions/7972 for more details.
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
https://user-images.githubusercontent.com/16493223/200602547-5dbe1f9b-3cf5-4126-801e-f788821c885d.mp4
Current assignee @johncschuster is eligible for the External assigner, not assigning anyone new.
Triggered auto assignment to Contributor-plus team member for initial proposal review - @Santhosh-Sellavel (External)
Current assignee @stitesExpensify is eligible for the External assigner, not assigning anyone new.
@stitesExpensify This missed my list somehow, I'm not sure I can get to this sooner. Maybe we could delegate this to new C+s whoever wants to pickup!
cc @johncschuster
Proposal (Updated)
diff --git a/src/components/Modal/BaseModal.js b/src/components/Modal/BaseModal.js
index 91b1867a6..7c7c546e3 100644
--- a/src/components/Modal/BaseModal.js
+++ b/src/components/Modal/BaseModal.js
@@ -26,7 +26,13 @@ class BaseModal extends PureComponent {
constructor(props) {
super(props);
+ this.saveSelection = this.saveSelection.bind(this);
+ this.restoreSelection = this.restoreSelection.bind(this);
this.hideModal = this.hideModal.bind(this);
+
+ this.state = {
+ savedSelection: null
+ };
}
componentDidUpdate(prevProps) {
@@ -42,6 +48,35 @@ class BaseModal extends PureComponent {
this.hideModal(this.props.isVisible);
}
+
+ saveSelection() {
+ const selection = window.getSelection();
+ if (selection.rangeCount > 0) {
+ this.setState({
+ savedSelection: {
+ focusNode: selection.focusNode,
+ range: selection.getRangeAt(0)
+ }
+ });
+ }
+ }
+
+ restoreSelection() {
+ const savedSelection = this.state.savedSelection;
+ const currentSelection = window.getSelection();
+
+ if (savedSelection === null) {
+ return;
+ }
+
+ if (savedSelection.focusNode.isEqualNode(currentSelection.focusNode)) {
+ return;
+ }
+
+ currentSelection.removeAllRanges();
+ currentSelection.addRange(savedSelection.range);
+ }
+
/**
* Hides modal
* @param {Boolean} [callHideCallback=true] Should we call the onModalHide callback
@@ -89,11 +124,20 @@ class BaseModal extends PureComponent {
// Note: Escape key on web/desktop will trigger onBackButtonPress callback
// eslint-disable-next-line react/jsx-props-no-multi-spaces
onBackButtonPress={this.props.onClose}
+ onModalWillShow={() => {
+ this.saveSelection();
+ // We don't want to wait till the modal show up to restore the selection
+ // Because the selection will be lost once the modal start appearing which may take time due to animation
+ // Thus we set a 10ms timeout to restore selection
+ // TODO: Use an event listener instead of the timeout
+ setTimeout(() => this.restoreSelection(), 10);
+ }}
onModalShow={() => {
if (this.props.shouldSetModalVisibility) {
Modal.setModalVisibility(true);
}
this.props.onModalShow();
+ this.restoreSelection();
}}
propagateSwipe={this.props.propagateSwipe}
onModalHide={this.hideModal}
Details
Basically same approach as the original proposal but cleaner, does not have the side effect as the original proposal, and moved the logic to the modal (the root of the issue)
The flicker effect is still there, I don't think it can be completely prevented using this approach but we can reduce the flickering chances, setting animationInTiming and animationInTiming to 1 reduced the flickering probability greatly, So maybe for the context menu we set it to 1 for Firefox, or just keep it if that's acceptable
📣 @mollfpr You have been assigned to this job by @stitesExpensify! Please apply to this job in Upwork and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑💻 Keep in mind: Code of Conduct | Contributing 📖
Current assignee @johncschuster is eligible for the External assigner, not assigning anyone new.
Current assignee @mollfpr is eligible for the External assigner, not assigning anyone new.
Triggered auto assignment to @srikarparsi (External), see https://stackoverflow.com/c/expensify/questions/7972 for more details.
@s77rt we avoid using setTimeout for a solution. Please update your proposal, thank you!
Also, I reproduce the issue in Safari too.
https://user-images.githubusercontent.com/25520267/201828492-0a78ea79-9dda-412f-b54c-5e7f320b87c6.mov
Proposal (Updated 2)
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/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',
},
Details
In this proposal I fixed the root issue which is the modal itself. i had to set coverScreen to false and keep hasBackdrop set to true. Those two props were coupled by fullscreen so I had to make some changes to only fallback to fullscreen if those props are not explicitly set. (btw, setting coverScreen to false seems to fix a lot of accessibility issues; plus there is no reason to be set true). Also I had to fix a hidden style issue; the modal content position style prop should be set to fixed not absolute. Because all the modal positioning calculations are based on the viewport never on the parent node.
This proposal does not introduce the flicking problem because we are not resetting the selection, it's just never lost. This fix the issue on all browsers. Opening the context menu will keep the selected text selected.
https://user-images.githubusercontent.com/16493223/201887358-30e2846a-49ad-4594-8c14-47dc77064dbe.mp4
@mollfpr
Thanks for the updated proposal, the result looks promising!
Can you do the RCA? Why the hasBackdrop/coverScreen can cause this problem?
Also, your proposal seems to produce another regression. On mWeb when opening the context menu modal, the report header is not covered by the modal overlay.
NOT A FULL RCA:
The cause is coverScreen setting it to true uses the react-native Modal, and this is basically the issue. Not sure what part exactly on the Modal component is the issue, I guess it have to deal with the fact that the modal is appended to the document. The selected text node container is not a sibling to the modal, thus the selection is lost.
Setting coverScreen just creates another View which get added as a sibling to the current selected element's container. In this case the selected text is preserved.
The use of hasBackdrop is required to have the backdrop obviously, nothing more than that.
The issue you described is because hasBackdrop will only get applied to the modal (View in this case) and it's only covering the part where the modal was instanced. Fixing the issue will require the report header to be a sibling of the report content
or i think this can be fixed easily be setting coverScreen back to true on small screens
Proposal (Updated 3)
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/Popover/index.js b/src/components/Popover/index.js
index 9a2e20ed3..00c55ef84 100644
--- a/src/components/Popover/index.js
+++ b/src/components/Popover/index.js
@@ -31,6 +31,8 @@ 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}
+ coverScreen={props.isSmallScreenWidth ? true : props.coverScreen}
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',
},
Details
same as https://github.com/Expensify/App/issues/12521#issuecomment-1315059153 and fixes the report heading not being covered by the backdrop in mobile (small screens)
Proposal
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/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..e41b2af41 100644
--- a/src/pages/home/report/ContextMenu/PopoverReportActionContextMenu.js
+++ b/src/pages/home/report/ContextMenu/PopoverReportActionContextMenu.js
@@ -4,12 +4,15 @@ import {
} from 'react-native';
import _ from 'underscore';
import * as Report from '../../../../libs/actions/Report';
+import compose from '../../../../libs/compose';
+import withWindowDimensions, {windowDimensionsPropTypes} from '../../../../components/withWindowDimensions';
import withLocalize, {withLocalizePropTypes} from '../../../../components/withLocalize';
import PopoverWithMeasuredContent from '../../../../components/PopoverWithMeasuredContent';
import BaseReportActionContextMenu from './BaseReportActionContextMenu';
import ConfirmModal from '../../../../components/ConfirmModal';
const propTypes = {
+ ...windowDimensionsPropTypes,
...withLocalizePropTypes,
};
@@ -288,7 +291,8 @@ class PopoverReportActionContextMenu extends React.Component {
animationOutTiming={1}
measureContent={this.measureContent}
shouldSetModalVisibility={false}
- fullscreen
+ hasBackdrop={true}
+ coverScreen={this.props.isSmallScreenWidth}
>
<BaseReportActionContextMenu
isVisible
@@ -319,4 +323,7 @@ class PopoverReportActionContextMenu extends React.Component {
PopoverReportActionContextMenu.propTypes = propTypes;
-export default withLocalize(PopoverReportActionContextMenu);
+export default compose(
+ withWindowDimensions,
+ withLocalize,
+)(PopoverReportActionContextMenu);
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',
},
on second thoughts, I think we should not force coverScreen to be true on small screens (on the Popover component level) it's likely to cause more issues on the future, so I think we can just move the logic to explicitly cover this specific case.
@mollfpr what do you think?