Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[$250] mWeb - Workspace - Long pressing workspace name, 2 selection handlers are not shown below the text #55953

Open
1 of 8 tasks
vincdargento opened this issue Jan 29, 2025 · 17 comments
Assignees
Labels
Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 External Added to denote the issue can be worked on by a contributor Help Wanted Apply this label when an issue is open to proposals by contributors

Comments

@vincdargento
Copy link

vincdargento commented Jan 29, 2025

If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!


Version Number: V9.0.91-0
Reproducible in staging?: Yes
Reproducible in production?: Yes
If this was caught on HybridApp, is this reproducible on New Expensify Standalone?: N/A
If this was caught during regression testing, add the test name, ID and link from TestRail: N/A
Email or phone of affected tester (no customers): N/A
Issue reported by: Applause Internal Team
Device used: Redmi note 10s android 13
App Component: Workspace Settings

Action Performed:

  1. Go to https://staging.new.expensify.com/home
  2. Go to workspace settings
  3. Tap overview
  4. Tap workspace name and long press the text
  5. Note 2 selection handlers are not shown below the text
  6. Tap description and long press the text
  7. Note 2 selection handlers are shown below the text

Expected Result:

Long pressing workspace name, 2 selection handlers must be shown below the text.

Actual Result:

Long pressing workspace name, 2 selection handlers are not shown below the text.

Workaround:

Unknown

Platforms:

  • Android: Standalone
  • Android: HybridApp
  • Android: mWeb Chrome
  • iOS: Standalone
  • iOS: HybridApp
  • iOS: mWeb Safari
  • MacOS: Chrome / Safari
  • MacOS: Desktop

Screenshots/Videos

bug.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~021886917200214989968
  • Upwork Job ID: 1886917200214989968
  • Last Price Increase: 2025-02-11
Issue OwnerCurrent Issue Owner: @ikevin127
@vincdargento vincdargento added Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 labels Jan 29, 2025
Copy link

melvin-bot bot commented Jan 29, 2025

Triggered auto assignment to @anmurali (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details. Please add this bug to a GH project, as outlined in the SO.

@melvin-bot melvin-bot bot added the Overdue label Feb 3, 2025
Copy link

melvin-bot bot commented Feb 4, 2025

@anmurali Eep! 4 days overdue now. Issues have feelings too...

@anmurali anmurali added the External Added to denote the issue can be worked on by a contributor label Feb 4, 2025
@melvin-bot melvin-bot bot changed the title mWeb - Workspace - Long pressing workspace name, 2 selection handlers are not shown below the text [$250] mWeb - Workspace - Long pressing workspace name, 2 selection handlers are not shown below the text Feb 4, 2025
Copy link

melvin-bot bot commented Feb 4, 2025

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

@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Feb 4, 2025
Copy link

melvin-bot bot commented Feb 4, 2025

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

@ikevin127
Copy link
Contributor

♻ I tested on both staging / dev and can't reproduce. Tested on a Pixel 6 Emulator using Chrome mWeb browser.

Android: mWeb Chrome
android-mweb.webm

This might be device specific / OS related given that it was tested on an older version of Android OS (13, current being 15):

Device used: Redmi note 10s android 13

Given this, I'd ask QA to test on multiple devices with latest available Android OS update installed and report on whether it's consistently reproducible it's consistent on all devices - otherwise I'd close as device specific / OS related and NAB coming from E/App.

cc @anmurali

Copy link

melvin-bot bot commented Feb 10, 2025

@anmurali, @ikevin127 Huh... This is 4 days overdue. Who can take care of this?

@melvin-bot melvin-bot bot added the Overdue label Feb 10, 2025
@ikevin127
Copy link
Contributor

Not overdue, callback to #55953 (comment) for re-testing since I wasn't able to reproduce and I assume that's also why we didn't get any proposal yet.

@melvin-bot melvin-bot bot removed the Overdue label Feb 10, 2025
@QichenZhu
Copy link
Contributor

QichenZhu commented Feb 10, 2025

@ikevin127 This is reproducible on Android 15 and Samsung S25 Ultra in BrowserStack.

I tested on both staging / dev and can't reproduce. Tested on a Pixel 6 Emulator using Chrome mWeb browser.

I think it's because the emulator comes with an old Chrome version. I suspect this is a Chrome bug introduced in recent months.

Screen.Recording.2025-02-11.at.12.39.24.PM.mov

@ikevin127
Copy link
Contributor

ikevin127 commented Feb 11, 2025

I suspect this is a Chrome bug introduced in recent months.

@QichenZhu 💯 I think you're right on the money with this one, on a quick search I just found the same exact issue posted in Chromium:

reproduced with:

Chrome Version: 130.0.6723.73
OS: Android

meaning anyone with a Chrome version equal or newer than 130.0.6723.73 is experiencing this issue, with the possibility for the offending release version to be even older than that.

Note: I just up-voted as Impacted with 8 accounts to get some attention on the issue and hopefully have somebody fix soon.

Edit: I assume that this could be the offending commit, the addition of the Android specific OnUpdateNativeViewTree method in the touch_handle.cc file.

@anmurali ♻ I guess this can be ❌ closed as Chrome browser specific issue, not Expensify related.

@QichenZhu
Copy link
Contributor

@ikevin127 Thank you so much for looking into this. I’ve noticed we usually work around bugs that we can't fix in root, like this Chrome bug and the workaround. If possible, I’d suggest leaving this open for a while if we can find an easy workaround.

cc @anmurali

Copy link

melvin-bot bot commented Feb 11, 2025

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

@ikevin127
Copy link
Contributor

ikevin127 commented Feb 11, 2025

@QichenZhu Sure, no problem! I have nothing against keeping the issue Open 👍

I doubtthat we can find a workaround for this like we did for the other one given that it seems to be coming from the browser engine and it relates to a behaviour which does not seem to offer much leeway in terms of fixing other than straight from the source code and rebuild the Chrome app, in contrast to the example you shared where the inputType was changed to something else in order to change behaviour as a client-side workaround.

I might give a shot to trying to build the Chrome app for Android from source and verifying whether the findings mentioned in #55953 (comment) are indeed what caused the issue at a browser level.

@QichenZhu
Copy link
Contributor

@ikevin127 Yeah, fixing it upstream is challenging, and even if we fix it, we can't guarantee users will update to the latest browser version.

However, the issue isn’t as obvious on other websites as it is in our app. For example, the search bars on Google and GitHub work fine, and this demo does too. So there are probably ways to avoid it or at least make it less of a problem.

@ikevin127

This comment has been minimized.

@ikevin127
Copy link
Contributor

♻ Upon further investigating, I discovered a workaround: our BaseTextInput component accepts type as prop which can have our TextInput component render 3 different types of input.

With default and mask types the issue is is still reproducible, but I tested with type="markdown" and parser={() => []} returning empty array in order to disable markdown styling and the issue will be fixed because react-native-live-markdown is not using input element under the hood, but a customized div.

🗒 The type="markdown" prop can be passed to our WorkspaceNamePage input conditionally using isMobileChrome in order to target this workaround only for Android: mWeb Chrome where the issue is reproducible.

Note: isMobileChrome returns false on iOS: mWeb Chrome because it checks for Android & Chrome.

🔄 Workaround drawbacks

  • issue is also reproducble on the login page (BaseLoginForm) input where we have inputMode set to email in order to have autocomplete functionality, if we were to pass type="markdown" there would remove the autocomplete functionality
  • issue is also reproducible on all TextInput components (entire app) because for most of them we don't pass the type prop meaning their type is default, and also for the ones where we pass type="mask" (just 2 in search filters page)

@QichenZhu Based on this, if you want to write-up a formal proposal to implement this workaround feel free to do so and we'll see what CME has to say about this workaround and its drawbacks once assigned. If you're not interested, let me know and I'll post this on Slack to look for a volunteer.

@QichenZhu
Copy link
Contributor

Proposal

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

Recent versions of Android Chrome don't show selection handles in the workspace name input.

What is the root cause of that problem?

The root cause is in Chromium as mentioned in @ikevin127's comment.

Upstream issue link: Text selection handles not present on text input field.

This affects recent Android Chrome versions but does not affect 126.0.6463.0 or earlier.

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

  1. Help with the upstream issue, like upvoting and commenting. But even if it's fixed there, it will take time to benefit end users since we can't guarantee they'll update to the latest browser version.

  2. In the meantime, work around this in our app by replacing the text input with a markdown input, but disable the parser so it behaves like a normal input. Big thanks to @ikevin127 for the idea!

import {isMobileChrome} from '@libs/Browser';

type={isMobileChrome() ? 'markdown' : 'default'}
parser={() => []}

What specific scenarios should we cover in automated tests to prevent reintroducing this issue in the future?

Not needed, as this issue comes from the browser, not from the logic of our app.

What alternative solutions did you explore? (Optional)

  1. Apply the workaround globally. This will cover almost all single-line inputs in the app, except for email and masked inputs.
import { isMobileChrome } from '@libs/Browser';

function TextInput(props: BaseTextInputProps, ref: ForwardedRef<BaseTextInputRef>) {

const isSingleLineInput = (!props.type || props.type === 'default') && (!props.inputMode || props.inputMode === 'text') && !props.multiline;
const mobileChromeWorkaround = isMobileChrome() && isSingleLineInput ? {
    type: 'markdown',
    parser: () => [],
} as const : {};

textInputContainerStyles={[labelAnimationStyle as StyleProp<ViewStyle>, props.textInputContainerStyles]}

            {...mobileChromeWorkaround}
        />
  1. Another possible workaround is to adjust the styles of text inputs, inspired by the GitHub search bar. It's a bit hacky though.
import {isMobileChrome} from '@libs/Browser';

App/src/styles/index.ts

Lines 1254 to 1262 in 628a915

baseTextInput: {
...FontUtils.fontFamily.platform.EXP_NEUE,
fontSize: variables.fontSizeNormal,
lineHeight: variables.lineHeightXLarge,
color: theme.text,
paddingTop: 23,
paddingBottom: 8,
paddingLeft: 0,
borderWidth: 0,

            ...(isMobileChrome() ? {
                marginTop: 23,
                marginBottom: 8,
                paddingTop: 0,
                paddingBottom: 0,
                paddingRight: 1,
                position: 'sticky',
                left: 0,
            } : {}),
        }

Copy link

melvin-bot bot commented Feb 12, 2025

@anmurali @ikevin127 this issue was created 2 weeks ago. Are we close to approving a proposal? If not, what's blocking us from getting this issue assigned? Don't hesitate to create a thread in #expensify-open-source to align faster in real time. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 External Added to denote the issue can be worked on by a contributor Help Wanted Apply this label when an issue is open to proposals by contributors
Projects
Status: No status
Development

No branches or pull requests

4 participants