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

Adapt iOS16+ dictation #53

Conversation

hellohublot
Copy link

@hellohublot hellohublot commented May 9, 2023

Upstream PR Link

facebook#37188

Summary:

facebook#19687
https://developer.apple.com/forums/thread/711413

When system version is lower than iOS 16, it does not support dictation and keyboard working at the same time, so if we modify the text, the system will immediately interrupt the dictation, so we need to prohibit modification of the text during recording and recognition

When system version is higher than iOS 16, dictation and keyboard can work at the same time, so textInputMode.primaryLanguage is no longer changed to dictation, so we can modify the text during recording, because the system will not interrupt, but we cannot modify the text during recognition, Because the system will temporarily add a _UITextPlaceholderAttachment to display the recognition UIActivityIndicator

Changelog:

[IOS] [FIXED] - Adapt iOS16+ dictation judge condition

Test Plan:

Test Code

constructor(props) {
  super(props)
  this.state = {
    value: '',
    logList: [],
  }
}

render() {
  return (
    <View style={{ flex: 1, padding: 50, backgroundColor: 'white'}}>
      <TextInput 
        style={{ marginTop: 20, height: 50, borderWidth: 1, borderColor: 'black' }} 
        placeholder={'please input'}
        placeholderTextColor={'gray'}
        value={this.state.value}
        onChangeText={value => {
          let logList = this.state.logList
          logList.push(value.length <= 0 ? 'null' : value.replace(/\uFFFC/g, '_uFFFC'))
          this.setState({ value, logList })
        }}
        onEndEditing={() => this.setState({ value: '', logList: [] })}
      />
      <FlatList
        style={{ marginTop: 20, maxHeight: 300, borderWidth: 1, borderColor: 'black' }}
        data={this.state.logList}
        renderItem={({item}) => (
            <Text>{item}</Text>
        )}
      />
    </View>
  )
}

Case A: Required < iOS16

  1. ensure that iOS Dictation TextInput ends abruptly between words facebook/react-native#18890 can work well and dictation will not be interrupted immediately
_case_a.mp4

Case B: Required >= iOS16

  1. ensure that iOS Dictation TextInput ends abruptly between words facebook/react-native#18890 can work well and dictation will not be interrupted immediately
_case_b.mp4

Case C: Required >= iOS16

  1. start dictation
  2. then do not speak any words
  3. then end dictation
  4. verify that onChangeText will callback "\uFFFC" once
  5. and then verify onChangeText callback an empty string "" once
_case_c.MP4

Case D: Required >= iOS16

  1. start dictation
  2. input some text while speaking some words
  3. then end dictation
  4. and verify that the onChangeText callback work fine.
_case_d.MP4

Case E: Required >= iOS16

  1. start dictation
  2. say a word
  3. and then switch the keyboard to other language
  4. verify that dictation will not end
  5. continue say some word
  6. verify the onChangeText callback work fine.
_case_e.MP4

@hellohublot hellohublot force-pushed the Fix_iOS_Dictation_Extra_Character_Expensify branch from 4df9132 to dd6f045 Compare May 24, 2023 01:40
@hellohublot
Copy link
Author

@flodnv Thanks, It's all done, I just cherry-pick the (facebook@e8b4bb0)

@hellohublot hellohublot force-pushed the Fix_iOS_Dictation_Extra_Character_Expensify branch from dd6f045 to 1683d1e Compare May 25, 2023 13:06
@hellohublot hellohublot requested a review from flodnv May 25, 2023 14:22
Copy link

@flodnv flodnv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@flodnv
Copy link

flodnv commented May 29, 2023

@hellohublot any idea why the GH action is failing? 😕

@hellohublot
Copy link
Author

Thanks, I didn't modify any android code, but I can give you some clues

According to https://github.com/Expensify/react-native/actions/runs/5080321413/jobs/9137029909, the CI error message is

/app/sdks/hermes/API/hermes/hermes.cpp:639:32: error: non-virtual member function marked 'override' hides virtual member functions
         const jsi::Value &value) override;
                                  ^
/app/ReactCommon/jsi/jsi/jsi.h:340:3: note: hidden overloaded virtual function 'facebook::jsi::Runtime::setPropertyValue' declared here: type mismatch at 1st parameter ('facebook::jsi::Object &' vs 'const jsi::Object &')

The reason for the error here is because facebook::jsi::Object should be const facebook::jsi::Object

So we can integrate this PR facebook@03b17d9#diff-4d691d151cba9dc2f79cb3ae582fc99ab8d639dda355d367aef25fc136ad0f93R340, but I guess that will also take some time, so we can merge our PR first, because It doesn't modify any android code

@flodnv
Copy link

flodnv commented May 30, 2023

I'm a bit confused about your proposal, won't that result in breaking the GH action for everyone?

Additionally you seem to suggest your PR did not break it? However, I see a more recent PR (example) that is working 😕

@hellohublot
Copy link
Author

hellohublot commented May 30, 2023

@flodnv Thanks.
https://github.com/Expensify/react-native/actions/workflows/test-docker-android.yml
From here, we can see that our Test Docker Android Image has not been executed successfully for several months, which coincides with the submission time of the PR I suggested to integrate.

This PR https://github.com/Expensify/react-native/pull/57/checks didn't run Test Docker Android Image, Because the target branch that these PRs want to merge is not Expensify-react-native:main, pull_request.branches.main in test-docker-android.yml cannot be triggered, so they will not execute Test Docker Android Image

@flodnv
Copy link

flodnv commented May 31, 2023

Hmmm thanks for the details; this is confusing, I've posted about it here: https://expensify.slack.com/archives/C01GTK53T8Q/p1685557998465289

@hellohublot hellohublot changed the base branch from main to ExpensifyRC1-0.72.0-alpha.0 June 1, 2023 02:00
Summary:
facebook#19687
https://developer.apple.com/forums/thread/711413

When system version is lower than `iOS 16`, it does not support `dictation` and `keyboard` working at the same time, so if we modify the text, the system will immediately interrupt the `dictation`, so we need to prohibit modification of the text during `recording` and `recognition`

When system version is higher than `iOS 16`, `dictation` and `keyboard` can work at the same time, so `textInputMode.primaryLanguage` is no longer changed to `dictation`, so we can modify the text during `recording`, because the system will not interrupt, but we cannot modify the text during `recognition`, Because the system will temporarily add a `_UITextPlaceholderAttachment` to display the recognition `UIActivityIndicator`

## Changelog:

<!-- Help reviewers and the release process by writing your own changelog entry.

Pick one each for the category and type tags:

[ANDROID|GENERAL|IOS|INTERNAL] [BREAKING|ADDED|CHANGED|DEPRECATED|REMOVED|FIXED|SECURITY] - Message

For more details, see:
https://reactnative.dev/contributing/changelogs-in-pull-requests
-->

[IOS][FIXED] - Adapt iOS16+ dictation judge condition

Pull Request resolved: facebook#37188

Test Plan:
Test Code
```javascript
constructor(props) {
  super(props)
  this.state = {
    value: '',
    logList: [],
  }
}

render() {
  return (
    <View style={{ flex: 1, padding: 50, backgroundColor: 'white'}}>
      <TextInput
        style={{ marginTop: 20, height: 50, borderWidth: 1, borderColor: 'black' }}
        placeholder={'please input'}
        placeholderTextColor={'gray'}
        value={this.state.value}
        onChangeText={value => {
          let logList = this.state.logList
          logList.push(value.length <= 0 ? 'null' : value.replace(/\uFFFC/g, '_uFFFC'))
          this.setState({ value, logList })
        }}
        onEndEditing={() => this.setState({ value: '', logList: [] })}
      />
      <FlatList
        style={{ marginTop: 20, maxHeight: 300, borderWidth: 1, borderColor: 'black' }}
        data={this.state.logList}
        renderItem={({item}) => (
            <Text>{item}</Text>
        )}
      />
    </View>
  )
}
```

Case A:  Required < iOS16
1. ensure that facebook#18890 can work well and dictation will not be interrupted immediately

https://github.com/facebook/react-native/assets/20135674/e69a609c-2dc4-48fc-8186-f9e5af3ac879

Case B: Required >= iOS16
1. ensure that facebook#18890 can work well and dictation will not be interrupted immediately

https://github.com/facebook/react-native/assets/20135674/caa97e18-c7c4-4a08-9872-b50130f73bf4

Case C: Required >= iOS16
1. start dictation
3. then do not speak any words
4. then end dictation
5. verify that `onChangeText` will callback "\uFFFC" once
6. and then verify `onChangeText` callback an empty string "" once

https://user-images.githubusercontent.com/20135674/235960378-90155ec5-a129-47bc-825b-ee6cb03e7286.MP4

Case D: Required >= iOS16
1. start dictation
3. input some text while speaking some words
4. then end dictation
5. and verify that the `onChangeText` callback work fine.

https://user-images.githubusercontent.com/20135674/235960411-e479d9ab-856a-4407-a644-986426825133.MP4

Case E: Required >= iOS16
1. start dictation
2. say a word
3. and then switch the keyboard to other language
4. verify that dictation will not end
6. continue say some word
8. verify the `onChangeText` callback work fine.

https://user-images.githubusercontent.com/20135674/235960450-351f1aaf-80c0-4d1c-b5c9-3e2cd7225875.MP4

Reviewed By: sammy-SC

Differential Revision: D45563187

Pulled By: dmytrorykun

fbshipit-source-id: 7467b313769896140434f60dcb3590d0b3c1aa15
@hellohublot hellohublot force-pushed the Fix_iOS_Dictation_Extra_Character_Expensify branch from 1683d1e to dbcfdf8 Compare June 1, 2023 02:08
@hellohublot
Copy link
Author

@flodnv Thanks, I have switched branch to ExpensifyRC1-0.72.0-alpha.0, #49 (review) because it seems like only this branch will be released.

Copy link

@flodnv flodnv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!

@flodnv flodnv merged commit f05684c into Expensify:ExpensifyRC1-0.72.0-alpha.0 Jun 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants