[HOLD for payment 2024-03-13] [$500] [Simplified Collect] [Workflows] Workflow submenu is selected after page reload

[luacmartins]

Coming from here, toggling on a option seems to select submenu item content see video below:

Screen.Recording.2024-02-23.at.11.40.22.PM.mov

Steps to reproduce:

1. In the Workspace Workflows page, toggle a switch and reload the page
2. Toggle off the switch and toggle it back on, the sub menu item will be selected

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~01f0c4fa5757dac171
• Upwork Job ID: 1761102687235653632
• Last Price Increase: 2024-02-23
• Automatic offers:
• ishpaul777 | Reviewer | 0
• dukenv0307 | Contributor | 0

[melvin-bot]

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

[melvin-bot]

Job added to Upwork: https://www.upwork.com/jobs/~01f0c4fa5757dac171

[melvin-bot]

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

[luacmartins]

Assigning @ishpaul777 since you reviewed the original PR

[ZhenjaHorbach]

Proposal

Please re-state the problem that we are trying to solve in this issue.

Workflow submenu is selected after page reload

What is the root cause of that problem?

Additional Information: To reproduce this issue, just quickly press Switch

The main problem with the issue is that when we press Switch
We are trying to select text automatically
But since the switch has no text
The browser selects the next available text
That is, subtitle

We can reproduce a similar situation when we press on the space near text on any website

Screen.Recording.2024-02-23.at.22.28.16.mov

What changes do you think we should make in order to solve the problem?

To fix this issue we can just add styles.userSelectNone for Switch to prevent any similar issues

App/src/components/Switch.tsx

Line 38 in fae8db3

style={[styles.switchTrack, !isOn && styles.switchInactive]}

And now when we press the switch
It won't try to select the closest text

What alternative solutions did you explore? (Optional)

As alternative we can add this style only for ToggleSettingOptionRow

App/src/pages/workspace/workflows/ToggleSettingsOptionRow.tsx

Lines 72 to 76 in b4dab19

	<Switch
	accessibilityLabel={subtitle}
	onToggle={toggleSwitch}
	isOn={isEnabled}
	/>

But it doesn't make sense for me

Before

Screen.Recording.2024-02-23.at.20.32.49.mov

After

Screen.Recording.2024-02-23.at.20.41.22.mov

[rayane-djouah]

@ishpaul777 do we need to enable any betas besides isPolicyExpenseChatEnabled to test this? I'm not getting workflows settings on my collect workspace

[ishpaul777]

@rayane-djouah you'll need to follow these steps

1 Create a collect policy in OldDot
2 Execute the following JS snippet in console the OldDot policy to enable workspace chats creation:
3 if (Url.getCurrentPageName()!="policy" ){ alert('Only run this snippet from a policy editor!'); } else { var p = Policy.getCurrent(); p.policy.isPolicyExpenseChatEnabled = "true"; p.save().done(function(){$.jGrowl('Workspace chat creation enabled!')}); }

@ZhenjaHorbach I'll test your proposal in my morning, but i remember seeing a similar issue resolve with similar approach so your solution looks promising but your Root cause lacks proper explaination can you please update for better explaination?

[ZhenjaHorbach]

@rayane-djouah you'll need to follow these steps

1 Create a collect policy in OldDot
2 Execute the following JS snippet in console the OldDot policy to enable workspace chats creation:
3 if (Url.getCurrentPageName()!="policy" ){ alert('Only run this snippet from a policy editor!'); } else { var p = Policy.getCurrent(); p.policy.isPolicyExpenseChatEnabled = "true"; p.save().done(function(){$.jGrowl('Workspace chat creation enabled!')}); }

@ZhenjaHorbach I'll test your proposal in my morning, but i remember seeing a similar issue resolve with similar approach so your solution looks promising but your Root cause lacks proper explaination can you please update for better explaination?

I update my proposition

[dukenv0307]

Proposal

Please re-state the problem that we are trying to solve in this issue.

In the Workspace Workflows page, toggle a switch and reload the page. Toggle off the switch and toggle it back on, the sub menu item will be selected.

What is the root cause of that problem?

Whenever we use PressableWithFeedback in our App, in this case for the Switch, it will implicitly convert the role that we set to a corresponding HTML element.

In case of BUTTON role, it will convert to a button html element, and it will have no such issue of clicking on button leading to selected text, because the native button in HTML will not allow that.

The problem here is that the switch role is not being mapped to button html element in react-native-web, although it's a button. Instead it's being rendered as a div, which obviously allows double click to select text.

Here's official web standard doc on the switch role: https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/Roles/switch_role, it uses the button HTML element (also here). Another doc of button html element, with switch in Permitted ARIA roles.

What changes do you think we should make in order to solve the problem?

We should map the switch role to button html element in react-native-web here by this one line change:

switch: 'button'

We can initially create a patch in the App to fix this first, then can create a upstream PR to fix in in react-native-web, this will benefit all users of react-native-web too.

To quickly test the fix locally, please go here in your machine node_modules/react-native-web/dist/modules/AccessibilityUtil/propsToAccessibilityComponent.js, and add the change.

What alternative solutions did you explore? (Optional)

There're other roles that should be mapped to button, but are not, the fix for them should be the same. I think those are out of scope of the issue, we're only dealing with switch here.

[ishpaul777]

Thanks for updating proposal @ZhenjaHorbach, i tested your solution and it works but I am more inclined to fix this upstream so we'll not face this issue in future.

The problem here is that the switch role is not being mapped to button html element in react-native-web, although it's a button. Instead it's being rendered as a div, which obviously allows double click to select text.

I found @dukenv0307 RCA correct and solution working as expected, lets fix this upstream 🚀

🎀 👀 🎀 C+ reviewed

[melvin-bot]

Current assignee @luacmartins is eligible for the choreEngineerContributorManagement assigner, not assigning anyone new.

[melvin-bot]

📣 @ishpaul777 🎉 An offer has been automatically sent to your Upwork account for the Reviewer role 🎉 Thanks for contributing to the Expensify app!

Offer link
Upwork job

[melvin-bot]

📣 @dukenv0307 🎉 An offer has been automatically sent to your Upwork account for the Contributor role 🎉 Thanks for contributing to the Expensify app!

Offer link
Upwork job
Please accept the offer 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 📖

[ishpaul777]

not overdue, we are still waiting for a PR from @dukenv0307

[dukenv0307]

I'm working on this issue

[dukenv0307]

@ishpaul777 this PR is ready for review.

[melvin-bot]

Reviewing label has been removed, please complete the "BugZero Checklist".

[melvin-bot]

The solution for this issue has been 🚀 deployed to production 🚀 in version 1.4.47-10 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

• Map role 'switch' to button element #37663

If no regressions arise, payment will be issued on 2024-03-13. 🎊

For reference, here are some details about the assignees on this issue:

• @ishpaul777 requires payment automatic offer (Reviewer)
• @dukenv0307 requires payment automatic offer (Contributor)

[melvin-bot]

BugZero Checklist: The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:

• [@ishpaul777] The PR that introduced the bug has been identified. Link to the PR:
• [@ishpaul777] The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment:
• [@ishpaul777] A discussion in #expensify-bugs has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner. Link to discussion:
• [@ishpaul777] Determine if we should create a regression test for this bug.
• [@ishpaul777] If we decide to create a regression test for the bug, please propose the regression test steps to ensure the same bug will not reach production again.
• [@adelekennedy] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

[ishpaul777]

[@ishpaul777] The PR that introduced the bug has been identified. Link to the PR: #36345 we intentionally merged this one with the bug as it was not a minor issue and blocking the merge

[@ishpaul777] The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment: #36345 (review)

[@ishpaul777] A discussion in #expensify-bugs has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner. Link to discussion: NOT REQUIRED

[@ishpaul777] Determine if we should create a regression test for this bug. - yes

[@ishpaul777] If we decide to create a regression test for the bug, please propose the regression test steps to ensure the same bug will not reach production again. 👇

[ishpaul777]

Regression test proposal:

1. Go to any of the page with a toggle switches, for example: Workflows page.
2. Quickly toggle on/off the switch
3. Verify no text content is selected while toggling the switch.

Do we agree 👍 or 👎?

[adelekennedy]

payments made!

[ishpaul777]

we are all set to close this one!