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

Wrong cursor when hovering disabled TextInput #9327

Closed
jbcullis opened this issue Jan 3, 2022 · 9 comments · Fixed by #9437
Closed

Wrong cursor when hovering disabled TextInput #9327

jbcullis opened this issue Jan 3, 2022 · 9 comments · Fixed by #9437
Assignees
Milestone

Comments

@jbcullis
Copy link

jbcullis commented Jan 3, 2022

Problem Description

When we have a TextInput set with focusable={false} editable={false} - the cursor will still change on hover.

Steps To Reproduce

Create the following element:

Hover over the element to see the cursor change.

Expected Results

Cursor should ignore disabled elements.

CLI version

npx react-native --version

Environment

System:
    OS: Windows 10 10.0.22000
    CPU: (16) x64 Intel(R) Core(TM) i9-9980HK CPU @ 2.40GHz
    Memory: 21.85 GB / 31.73 GB
  Binaries:
    Node: 16.13.0 - C:\Program Files\nodejs\node.EXE
    Yarn: 1.22.17 - ~\AppData\Roaming\npm\yarn.CMD
    npm: 8.1.3 - C:\Program Files\nodejs\npm.CMD
    Watchman: Not Found
  SDKs:
    Android SDK: Not Found
    Windows SDK:
      AllowDevelopmentWithoutDevLicense: Enabled
      AllowAllTrustedApps: Enabled
      Versions: 10.0.18362.0, 10.0.19041.0, 10.0.22000.0
  IDEs:
    Android Studio: Not Found
    Visual Studio: 17.0.31912.275 (Visual Studio Community 2022)
  Languages:
    Java: Not Found
  npmPackages:
    @react-native-community/cli: Not Found
    react: 17.0.2 => 17.0.2
    react-native: 0.66.0 => 0.66.0
    react-native-windows: 0.66.5 => 0.66.5
  npmGlobalPackages:
    *react-native*: Not Found

Target Platform Version

10.0.19041

Target Device(s)

Desktop

Visual Studio Version

No response

Build Configuration

No response

Snack, code example, screenshot, or link to a repository

No response

@jbcullis jbcullis added the bug label Jan 3, 2022
@ghost ghost added the Needs: Triage 🔍 New issue that needs to be reviewed by the issue management team (label applied by bot) label Jan 3, 2022
@chrisglein
Copy link
Member

This is likely the behavior of the XAML TextBox control and just isn't matching expectation. I've seen that sort of thing before (e.g. microsoft/microsoft-ui-xaml#5487). Certainly an IsReadOnly TextBox still shows the cursor (because you can copy and select out of it). That same policy may have been made for disabled+readonly.

@TatianaKapos Can you check the behavior in a plain UWP app and see if we're changing the text input behavior in RNW or if this is the platform default?

@chrisglein chrisglein added Area: TextInput and removed Needs: Triage 🔍 New issue that needs to be reviewed by the issue management team (label applied by bot) labels Jan 6, 2022
@chrisglein chrisglein added this to the Backlog milestone Jan 6, 2022
@TatianaKapos
Copy link
Contributor

TatianaKapos commented Jan 7, 2022

@chrisglein Looks like UWP does change the cursor for a Textbox that is disabled, however the web version of react does not change the cursor (https://snack.expo.dev/@tatianak/textinput). Editable is linked to isReadOnly, so I'm curious what the expected output for focusable is and what it's linked too?

UWP App Output

ApplicationFrameHost_i2Qiq9P8Lp.mp4

React Native Output

ApplicationFrameHost_JeB6TEgsgk.mp4

@chrisglein
Copy link
Member

@chrisglein Looks like UWP does change the cursor for a Textbox that is disabled, however the web version of react does not change the cursor (https://snack.expo.dev/@tatianak/textinput). Editable is linked to isReadOnly, so I'm curious what the expected output for focusable is and what it's linked too?

I thought focusable was supposed to match to enabled/disabled (like your UWP example shows), but it could be something more granular - you'd have to check.

There's definitely mojo to match XAML's TextBox (which is a very opinionated control) to RN's TextInput (which is supposed to be less opinionated about visual styling). Maybe this is something lost in the shuggle.

Certainly seeing how Web behaves provides context (as in it doesn't match expectations).

@Balkoth
Copy link

Balkoth commented Jan 17, 2022

I don't understand why simplistic web behaviours, dominate the rich experience of client applications.

@jbcullis
Copy link
Author

jbcullis commented Jan 17, 2022

@Balkoth Well, it would make more sense if the text was selectable, but it's not even though the cursor suggests so.
But if you're going that direction, you'd also want to make Text elements selectable for consistency.

@TatianaKapos
Copy link
Contributor

@chrisglein
With playground working, I was able to find out that focusable is mapped to IsTabStop in the ControlViewManager.

Interestingly the Microsoft Docs for that property state, " If the reason you don't want the control to be a tab stop is because it's not interactive in your UI, you might want to set IsEnabled to false to make the lack of interaction more obvious. Many controls have predefined visual states that are invoked for IsEnabled =false, such as "graying out" text in labels." So perhaps we want to set both properties for focusable?

@chrisglein
Copy link
Member

With playground working, I was able to find out that focusable is mapped to IsTabStop in the ControlViewManager.

Ask around and see if you can get the history of this. Marking something as able to receive keyboard with IsTabStop makes sense, if the control wasn't already able to get focus. Just a plain Panel, for example. But for a control that already has that built in (e.g. TextInput), messing with IsTabStop makes less sense then setting IsEnabled or something like that.

@TatianaKapos
Copy link
Contributor

Ask around and see if you can get the history of this

Here's the PR that introduces the property with the corresponding issue. Run down looks like ScrollView needed to access isTabStop to allow for navigation if it had no focusable children. Setting isEnabled for focuasable would make an issue for ScrollView if focusable was false (ie scrolling would not be able to work).

But, adding isEnabled to just TextInput's focasable can be an easy fix.

@NickGerleman
Copy link
Collaborator

With playground working, I was able to find out that focusable is mapped to IsTabStop in the ControlViewManager.

Ask around and see if you can get the history of this. Marking something as able to receive keyboard with IsTabStop makes sense, if the control wasn't already able to get focus. Just a plain Panel, for example. But for a control that already has that built in (e.g. TextInput), messing with IsTabStop makes less sense then setting IsEnabled or something like that.

It's by design that "focusable" maps to only IsTabStop. Individual controls expose separate props that look more like "enabled".

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants