Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

Fix an issue that deleting an emoji may crash the app #34508

Merged
auto-submit merged 6 commits into from
Jul 25, 2022
Merged

Fix an issue that deleting an emoji may crash the app #34508

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
6c1d6cc
fix a problem that deleting an emoji crashes the app
yeatse Jul 8, 2022
6f604cb
Add tests
yeatse Jul 8, 2022
e8c4a37
Update shell/platform/darwin/ios/framework/Source/FlutterTextInputPlu…
yeatse Jul 9, 2022
a28afd0
remove composed characters only if they are emojis
yeatse Jul 9, 2022
8e59927
update comments
yeatse Jul 9, 2022
1382921
update test case to cover the leading thai vowel
yeatse Jul 10, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
remove composed characters only if they are emojis
  • Loading branch information
@yeatse
yeatse committed Jul 23, 2022
commit a28afd0d344bb2fe24c23ffb7ae86cf6dbd5c14e
17 changes: 16 additions & 1 deletion shell/platform/darwin/ios/framework/Source/FlutterTextInputPlugin.mm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,8 @@
#import <Foundation/Foundation.h>
#import <UIKit/UIKit.h>

#include "unicode/uchar.h"
Copy link
Member

@jmagman jmagman Jul 23, 2022

Choose a reason for hiding this comment

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

@dnfield since you jumped in, how do you feel about including this library? I've been trying to think of another way to check if it's an emoji without importing this but also reinventing the wheel. I guess it's okay?

Copy link
Contributor

@dnfield dnfield Jul 23, 2022

Choose a reason for hiding this comment

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

This seems fine to me. This is what ICU is for and there's no getting around having ICU in a flutter app


#include "flutter/fml/logging.h"
#include "flutter/fml/platform/darwin/string_range_sanitization.h"

Expand Down Expand Up @@ -1895,8 +1897,21 @@ - (void)deleteBackward {
UITextRange* oldSelectedRange = _selectedTextRange;
NSRange oldRange = ((FlutterTextRange*)oldSelectedRange).range;
if (oldRange.location > 0) {
NSRange newRange = NSMakeRange(oldRange.location - 1, 1);

NSRange charRange = fml::RangeForCharacterAtIndex(self.text, oldRange.location - 1);
NSRange newRange = NSMakeRange(charRange.location, oldRange.location - charRange.location);
UChar32 codePoint;
BOOL gotCodePoint = [self.text getBytes:&codePoint
maxLength:sizeof(codePoint)
usedLength:NULL
encoding:NSUTF32StringEncoding
options:kNilOptions
range:charRange
remainingRange:NULL];
if (gotCodePoint && u_hasBinaryProperty(codePoint, UCHAR_EMOJI)) {
Copy link
Contributor

@dnfield dnfield Jul 21, 2022

Choose a reason for hiding this comment

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

Why is this getting applied only to emoji?

Aren't there other multi-byte characters that are not emoji that we'd want to apply this to?

Copy link
Contributor Author

@yeatse yeatse Jul 22, 2022

Choose a reason for hiding this comment

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

From #34508 (comment), languages with diacritical marks will expect a backward deletion should only delete one part of the composed character, such as Thai and Hebrew.

We have two ways to distinguish this behavior from emoji's:

  1. check if the byte is a diacritical mark (and other properties I don't know yet)
  2. check if the byte is not in an emoji

I chose the second one to keep consistency with android (see FlutterTextUtils.getOffsetBefore)

Copy link
Contributor

@dnfield dnfield Jul 22, 2022

Choose a reason for hiding this comment

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

Ahhh ok this makes sense.

// We should make sure the entire emoji is deleted.
newRange = NSMakeRange(charRange.location, oldRange.location - charRange.location);
}
_selectedTextRange = [[FlutterTextRange rangeWithNSRange:newRange] copy];
[oldSelectedRange release];
}
Expand Down
6 changes: 6 additions & 0 deletions testing/ios/IosUnitTests/IosUnitTests.xcodeproj/xcshareddata/xcschemes/IosUnitTests.xcscheme
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,12 @@
ReferencedContainer = "container:IosUnitTests.xcodeproj">
</BuildableReference>
</BuildableProductRunnable>
<CommandLineArguments>
<CommandLineArgument
argument = "--icu-data-file-path=Frameworks/Flutter.framework/icudtl.dat"
isEnabled = "YES">
Comment on lines +83 to +84
Copy link
Member

@jmagman jmagman Jul 23, 2022

Choose a reason for hiding this comment

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

Is this needed?

Copy link
Contributor Author

@yeatse yeatse Jul 23, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Yes. Without this the test will fail due to failing to load the ICU data file. Maybe we should file another issue to get rid of this later.

Copy link
Contributor Author

@yeatse yeatse Jul 23, 2022

Choose a reason for hiding this comment

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

Filed flutter/flutter#108221

Copy link
Contributor

@dnfield dnfield Jul 25, 2022

Choose a reason for hiding this comment

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

I think this is fine - in other tests we use the command line argument to load the ICU data file (see run_tests.py).

</CommandLineArgument>
</CommandLineArguments>
</LaunchAction>
<ProfileAction
buildConfiguration = "Release"
Expand Down