fix contextMenu on Mobile #3659

Viacheslav80 · 2021-06-17T19:57:48Z

Details

/Users/syva80/Expensify.cash/src/pages/home/report/ReportActionItemFragment.js
I'm removing the prop "selectable"

components/RenderHTML.js. --> components/RenderHTML/index.js and index.native.js (remove prop "textSelectable")

Fixed Issues

Fixes #2279

Tests

Launch the app and login
Select any message from LHN
Long press on any message

QA Steps

Same as above

Tested On

Screenshots

Web

Mobile Web

Desktop

iOS

Android

roryabraham

@Viacheslav80 There is far too much code duplication between the native + web implementation here. It should be very obvious what the difference between these two files are. It depends on circumstances, but a common pattern in this repo is to do something like this:

RenderHTML/
---| BaseRenderHTML // cross-platform implementation
---| index.js // lightweight web implementation wrapping cross-platform base component
---| index.native.js // leightweight native implementation wrapping cross-platform base component

Right now, it's very difficult to review this PR because the difference between the web and native implementation is not obvious.

Viacheslav80 · 2021-06-21T17:12:13Z

@roryabraham thanks. I am fix it

Viacheslav80 · 2021-06-22T20:05:29Z

Updated

src/components/RenderHTML/BaseRenderHTML.js

src/components/RenderHTML/renderHTMLPropTypes.js

src/components/RenderHTML/index.native.js

src/components/RenderHTML/index.js

Viacheslav80 · 2021-06-24T10:57:18Z

Updated

roryabraham

Almost done! Only a couple very minor comments remaining.

src/components/RenderHTML/BaseRenderHTML.js

src/components/RenderHTML/index.js

roryabraham · 2021-06-24T17:53:57Z

Also, there are merge conflicts

Viacheslav80 · 2021-06-25T09:28:23Z

Updated

roryabraham · 2021-06-25T18:56:34Z

This does not seem to work on mobile web:

Viacheslav80 · 2021-06-25T20:10:25Z

exactly, something broke((

parasharrajat · 2021-06-27T14:45:13Z

src/components/RenderHTML/index.native.js

+    defaultProps,
+} from './renderHTMLPropTypes';
+
+const RenderHTML = ({html, debag}) => (


Suggested change

const RenderHTML = ({html, debag}) => (

const RenderHTML = ({html, debug}) => (

Viacheslav80 · 2021-06-28T10:31:24Z

Updated

roryabraham · 2021-06-28T19:37:06Z

I'm observing some undesirable behavior on mobile web where the whole page is highlighted:

Viacheslav80 · 2021-06-29T14:45:40Z

Updated

roryabraham

Nice, this seems to be working well now!

OSBotify · 2021-06-29T16:21:26Z

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

parasharrajat · 2021-06-29T16:45:57Z

src/components/PressableWithSecondaryInteraction/index.js

+     * @param {touchstart} e - TouchEvent.
+     * https://developer.mozilla.org/en-US/docs/Web/API/TouchEvent
+     */
+    preventDefault = (e) => {


I don't think we support this pattern in Class component yet.

Good catch! I will create a follow-up to clean this up!

parasharrajat · 2021-06-29T16:46:08Z

Just a heads up, I don't think we should have blocked the touch on the Web devices that support the Touch interface. So a check for small screens while preventing touch would have been great.

roryabraham · 2021-06-29T17:45:01Z

I don't think we should have blocked the touch on the Web devices that support the Touch interface. So a check for small screens while preventing touch would have been great.

Well, this is only for PressableWithSecondaryInteraction. I guess you're saying that it won't work with a regular touch interaction?

parasharrajat · 2021-06-29T17:47:17Z

Oh yeah. But May it will block the context menu on WEb-touch devices while using touch. Anyways we can keep it as it is for now.

roryabraham · 2021-06-29T17:50:24Z

Follow-up here

OSBotify · 2021-06-29T19:26:11Z

🚀 Deployed to staging in version: 1.0.74-1🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅

parasharrajat · 2021-06-29T21:00:40Z

@roryabraham Serious regression from blocking the touch event.

Can't able to close the Edit message box. Click on cancel or save changes that are not working on Mweb.
Another, mWeb iOS - Can't scroll messages #3806

rdjuric · 2021-06-29T22:07:19Z

Yea, I was trying to track when #3806 started happening and it's on this commit a1f2c94.

roryabraham · 2021-06-29T23:53:14Z

Revert PR here. So @Viacheslav80 this means that you'll need to create a follow-up PR to fix the context menu on mobile web without causing other regressions.

Viacheslav80 · 2021-06-30T05:29:00Z

@roryabraham ok. I am do it today

parasharrajat · 2021-06-30T09:08:10Z

@Viacheslav80 I think it's fine if you don't include the fix for blue screen/ full-page highlight. I will tackle that in #2705. But you can share your findings here about that if you want. It would be a great help, Thanks.

kavimuru · 2021-06-30T16:59:18Z

iOS - Chat - Context menu flickers continously

Expected Result:

No visual issues and context menu opens without any issue

Actual Result:

Context menu flickers continuously

Actions Performed:

Launch the app and login
Select any message from LHN
Long press on any message

Platform:

iOS ✔️
mWeb
Android

Build:

1.0.75-0

Notes/Images/Video:

Bug5134006_RPReplay_Final1625058835.mp4

isagoico · 2021-06-30T23:17:39Z

Confirmed this is fixed #3659 (comment) 🎉

Viacheslav80 added 2 commits June 17, 2021 16:41

fix contextMenu on Mobile

a679909

fix eslint

eb94af7

Viacheslav80 requested a review from a team as a code owner June 17, 2021 19:57

MelvinBot requested review from sketchydroide and removed request for a team June 17, 2021 19:58

roryabraham requested changes Jun 21, 2021

View reviewed changes

add BaseRenderHTML

e429d0b

roryabraham requested changes Jun 23, 2021

View reviewed changes

Viacheslav80 added 2 commits June 24, 2021 11:46

defaultProp textSelectable

f0cca6c

fix errors

e2fc7ec

roryabraham requested changes Jun 24, 2021

View reviewed changes

src/components/RenderHTML/BaseRenderHTML.js Outdated Show resolved Hide resolved

src/components/RenderHTML/index.js Outdated Show resolved Hide resolved

Viacheslav80 added 2 commits June 25, 2021 08:40

fix comments

69722fe

merge with main

ca56242

add isSmallScreenWidth in RenderHTML + ReportActionItemFragment

1111fa3

parasharrajat suggested changes Jun 27, 2021

View reviewed changes

fix debug prop

44cf52a

fix selection on longPress

a1f2c94

roryabraham approved these changes Jun 29, 2021

View reviewed changes

roryabraham merged commit b5a3bab into Expensify:main Jun 29, 2021

parasharrajat reviewed Jun 29, 2021

View reviewed changes

roryabraham mentioned this pull request Jun 29, 2021

[No QA] Use regular function instead of arrow function #3795

Merged

OSBotify mentioned this pull request Jun 29, 2021

Deploy Checklist: Expensify.cash 2021-06-29 #3802

Closed

22 tasks

rdjuric mentioned this pull request Jun 29, 2021

mWeb iOS - Can't scroll messages #3806

Closed

Viacheslav80 mentioned this pull request Jun 30, 2021

fix_press_edit_buttons + delayLongPress -> 200 ms #3820

Closed

5 tasks

parasharrajat mentioned this pull request Jun 30, 2021

mWeb - Chat - "View details" in chat is not responding #3826

Closed

parasharrajat mentioned this pull request Jul 9, 2021

iOS/Android - Message - Copy message menu is not displayed when pressing on the message #2279

Closed

Viacheslav80 mentioned this pull request Jul 13, 2021

Fix copy message menu #3999

Closed

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix contextMenu on Mobile #3659

fix contextMenu on Mobile #3659

Viacheslav80 commented Jun 17, 2021 •

edited

Loading

roryabraham left a comment

Viacheslav80 commented Jun 21, 2021

Viacheslav80 commented Jun 22, 2021 •

edited

Loading

Viacheslav80 commented Jun 24, 2021

roryabraham left a comment

roryabraham commented Jun 24, 2021

Viacheslav80 commented Jun 25, 2021

roryabraham commented Jun 25, 2021

Viacheslav80 commented Jun 25, 2021

parasharrajat Jun 27, 2021

Viacheslav80 commented Jun 28, 2021

roryabraham commented Jun 28, 2021

Viacheslav80 commented Jun 29, 2021

roryabraham left a comment

OSBotify commented Jun 29, 2021

parasharrajat Jun 29, 2021

roryabraham Jun 29, 2021

parasharrajat commented Jun 29, 2021

roryabraham commented Jun 29, 2021

parasharrajat commented Jun 29, 2021

roryabraham commented Jun 29, 2021

OSBotify commented Jun 29, 2021

parasharrajat commented Jun 29, 2021

rdjuric commented Jun 29, 2021

roryabraham commented Jun 29, 2021

Viacheslav80 commented Jun 30, 2021

parasharrajat commented Jun 30, 2021 •

edited

Loading

kavimuru commented Jun 30, 2021

isagoico commented Jun 30, 2021

	const RenderHTML = ({html, debag}) => (
	const RenderHTML = ({html, debug}) => (

fix contextMenu on Mobile #3659

fix contextMenu on Mobile #3659

Conversation

Viacheslav80 commented Jun 17, 2021 • edited Loading

Details

Fixed Issues

Tests

QA Steps

Tested On

Screenshots

Web

Mobile Web

Desktop

iOS

Android

roryabraham left a comment

Choose a reason for hiding this comment

Viacheslav80 commented Jun 21, 2021

Viacheslav80 commented Jun 22, 2021 • edited Loading

Viacheslav80 commented Jun 24, 2021

roryabraham left a comment

Choose a reason for hiding this comment

roryabraham commented Jun 24, 2021

Viacheslav80 commented Jun 25, 2021

roryabraham commented Jun 25, 2021

Viacheslav80 commented Jun 25, 2021

parasharrajat Jun 27, 2021

Choose a reason for hiding this comment

Viacheslav80 commented Jun 28, 2021

roryabraham commented Jun 28, 2021

Viacheslav80 commented Jun 29, 2021

roryabraham left a comment

Choose a reason for hiding this comment

OSBotify commented Jun 29, 2021

parasharrajat Jun 29, 2021

Choose a reason for hiding this comment

roryabraham Jun 29, 2021

Choose a reason for hiding this comment

parasharrajat commented Jun 29, 2021

roryabraham commented Jun 29, 2021

parasharrajat commented Jun 29, 2021

roryabraham commented Jun 29, 2021

OSBotify commented Jun 29, 2021

parasharrajat commented Jun 29, 2021

rdjuric commented Jun 29, 2021

roryabraham commented Jun 29, 2021

Viacheslav80 commented Jun 30, 2021

parasharrajat commented Jun 30, 2021 • edited Loading

kavimuru commented Jun 30, 2021

iOS - Chat - Context menu flickers continously

Expected Result:

Actual Result:

Actions Performed:

Platform:

Build:

Notes/Images/Video:

isagoico commented Jun 30, 2021

Viacheslav80 commented Jun 17, 2021 •

edited

Loading

Viacheslav80 commented Jun 22, 2021 •

edited

Loading

parasharrajat commented Jun 30, 2021 •

edited

Loading