-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
[RNMobile] Fix dictation regression on iOS #49452
Conversation
Flaky tests detected in ff64c55. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/5132777788
|
This commit also addresses formatting issues in the file caused by previous merge conflicts.
👋 Thanks @SiobhyB for continuing to tackle this! 🙇 The change you've proposed here looks good, but I wondered why we had this dictation override in the first place. From what I can tell, the history is:
😅 This made me wonder if we can frame the problem this way:
From here I created five tests: Test 1
Test 2
Test 3
Test 4
Test 5
Results
|
iOS 15.7.1 | iOS / iPadOS 16.4.1 | |
---|---|---|
Test 1 | ✅ | ❌ |
Test 2 | ✅ | ❌ |
Test 3 | ✅ | ❌ |
Test 4 | ✅ | ❌ |
Test 5 | ❌ |
gutenberg/test-dictation-fix
iOS 15.7.1 | iOS / iPadOS 16.4.1 | |
---|---|---|
Test 1 | ✅ | ✅ |
Test 2 | ✅ | ✅ |
Test 3 | ✅ | ✅ |
Test 4 | ✅ | ✅ |
Test 5 | ❌ | ❌ |
Dictation overrides removed
For this test I removed all workarounds:
- Reverting the code changed here (as well as the subsequent fixes mentioned above)
⚠️ Only tested on iOS 15 and 16⚠️ Behavior change: A space is added when dictation is disabled, but that appears to be normal iOS behavior (confirmed in the Notes app). However, this could lead to extraneous spaces so we'd need to consider adding trimming logic.
iOS 15.7.1 | iOS / iPadOS 16.4.1 | |
---|---|---|
Test 1 | ✅ | ✅ |
Test 2 | ✅ | ✅ |
Test 3 | ✅ | ✅ |
Test 4 | ✅ | ✅ |
Test 5 | ✅ | ✅ |
Bonus, this feature seemed to be enabled:
Thoughts
- I'm fine to proceed with the original change if we can fix the test 5 regression.
- It'd be great if we could remove the workarounds at some point if we determine for sure that they're not needed.
- It's possible I've missed a critical test, so I'd love another set of 👁️s. I didn't test the language pop-up mentioned at the top of this PR.
- We should consider dropping platforms older than iOS 14 (stats 1, stats 2)
I'd love to hear your thoughts on this @SiobhyB. Thanks!
@twstokes, thanks so much for the thorough and thoughtful testing, I truly appreciate it. 🙇♀️ I had overlooked the fifth test case. I followed the same tests that you outlined using an iPhone XR (iOS 16.4.1) and came up with the following results:
With all of the overrides removed, I'm unfortunately able to consistently replicate the issue with an added I then tested removing the
However, that brings us back to the issue with content loss. 🤔 I'll explore possible other ways around the |
Ah! Thanks for this - I didn't know about the post list part. 🙇 Yep, Test 3 now fails for me as well. 😅
Knowing that the OBJ character is causing this I don't think this is "normal" anymore. |
This reverts commit 3a0d386.
@twstokes, I've updated the approach to adding dictating text to the following, which is working for all tests cases in my testing: gutenberg/packages/react-native-aztec/ios/RNTAztecView/RCTAztecView.swift Lines 367 to 369 in 316276d
Looking forward to hearing whether this also works for and whether you have any thoughts! |
I just found an issue with |
👋 Hey @SiobhyB, I wanted to recap some thoughts on my side from our discussion today:
Two potential drawbacks of this approach that I can identify are:
|
This refactor follow the discussion here: #49452 (comment) The hacks are unnecessary for iOS versions 15 and higher. As we haven't been able to pinpoint a fix that caters to the older versions, and considering the lower numbers, we have opted to remove the hacks.
@twstokes, thank you for discovering that RN issue! A recent internal post by @guarani made me aware that the WordPress app dropped support for iOS 13 ~8 months ago. In addition, the post confirmed that iOS 14 app usage was low (~1% of all sessions) and that we would not be locking users out of using the feature by dropping support:
With that in mind, as well as the good efforts we've gone to trying to support the older versions, I agree that we should remove the dictation hacks and replace the |
// Replace occurrences of the obj symbol ("\u{FFFC}") | ||
textView.text = textView.text?.replacingOccurrences(of: "\u{FFFC}", with: "") | ||
|
||
if let newPosition = textView.position(from: textView.beginningOfDocument, offset: originalPosition) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Without this code, the cursor would always be repositioned (along with the dictated text) after dictating in the middle of a paragraph. I experimented with a few different ways to repositioning the cursor, and this is the one that seemed to work the best.
I've been able to reproduce one issue with this approach:
- Begin dictating text into a paragraph block.
- Without ending dictation, move the cursor into the middle of the text and begin dictating.
- Notice that the cursor can appear out of place when the text moves onto a new line.
- The cursor will correct itself as you continue dictating, and the text will remain in the correct position.
Note, this doesn't happen if you end dictation and then re-start it to add text into the middle of the text. I'd consider this somewhat of an edge case, which corrects itself and doesn't impact the actual text entry.
In the interest of shipping a timely fix, and if there aren't any obvious other solutions, I'd like to propose that we go with this one for now and address improving the cursor positioning in a separate PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That sounds reasonable to me @SiobhyB. 👍
@twstokes, this is ready for review again when you have the chance 🙇♀️ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM @SiobhyB - thanks for your work on this issue. I made one note about test 5, but I don't think it should block this PR.
I tested both iOS 15 and 16. 🚀
|
||
func removeUnicodeAndRestoreCursor(from textView: UITextView) { | ||
// Capture current cursor position | ||
let originalPosition = textView.offset(from: textView.beginningOfDocument, to: textView.selectedTextRange?.start ?? textView.beginningOfDocument) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When running test 5 on iOS 16 I seemed to encounter an off-by-one where the cursor advanced its position when I disabled dictation. I'm not super familiar with the textView cursor position logic, but I wonder if when we calculate the offset under the condition that textView.selectedTextRange?.start
isn't nil, and subtracted one from the resulting integer, if that would resolve the problem.
Any thoughts on that @SiobhyB? I'm unsure of the pitfalls of that logic when the integer is 0
. IMO we shouldn't let it hold up this PR, so I'm OK with noting it for a subsequent fix.
Dropping a note about this fix that was merged in the RN codebase: facebook/react-native#37188. |
@twstokes, thank you so much for all of your help on this PR. 🙇♀️ I'll go ahead to merge as-is, so that the main fix is with users for the next 22.6 release, but have created an issue outlining the cursor issues at #51227. I've assigned myself and will work on it during HACK week next week.
Awesome! Looking over the guidance around when fixes get included in RN releases, it seems this might be included in the |
These changes address an issue with dictation not working as expected on devices running iOS 16 or later.
Proposed fix for wordpress-mobile/gutenberg-mobile#5165.
Related PRs:
Gutenberg Mobile
: Fix dictation regression on iOS wordpress-mobile/gutenberg-mobile#5612WordPress iOS
: TESTING ONLY: DO NOT MERGE: Fix dictation issue on iOS wordpress-mobile/WordPress-iOS#20669What?
The changes in this PR address an issue with dictation not working as expected on devices running iOS 16 or later.
Why?
Following improvements made to dictation in iOS 16, a previous hack that avoided an empty string being propagated with dictation, stopped working.
Now, with devices that are running iOS 16 or later, text added with iOS dictation is lost when tapping into the editor or typing while the dictation is still recording. This causes unexpected content loss, and could be viewed as an accessibility issue for those who rely on dictation to use the editor.
How?
By removing the previous dictation-related hacks, we fallback to the default dictation system, which works without causing any content loss for later iOS versions. The reasoning behind following this approach is detailed here.
In addition, it's still necessary to remove the
obj
symbol that's added due to an RN bug. This is done directly within the textViewDidChange() function.Testing Instructions
An installable build is available at wordpress-mobile/WordPress-iOS#20669 for testing on a physical device.
The following steps need to be tested with both iOS 16 or later as well as on an earlier version of iOS:
Test 1: Ensure new blocks don't replace old ones⤵️
Based on testing steps from this PR: #49154
Test 2: Verify there is no content loss while dictating⤵️
Based on bug testing steps from this issue: wordpress-mobile/gutenberg-mobile#5165
Test 3: Verify no obj symbol is added after dictation⤵️
Based on testing steps from this PR: wordpress-mobile/gutenberg-mobile#1969
Test 4: Verify block expands with content⤵️
Only happens with an external keyboard (only applicable to iPad or Simulator with physical keyboard input)
Test 5: Verify text position is retained after dictating in middle of paragraph⤵️
Based on this PR: wordpress-mobile/gutenberg-mobile#1969
In addition, verify that there are no failed tests across the Gutenberg, Gutenberg Mobile, and WordPress iOS PRs signalling unwanted side effects.
Screenshots or screencast
The following screencast shows dictation working, even after tapping the language pop up and resuming dictation (previously this led to content loss):
dictation.mov