Skip to content

Don't bail when ZWJ cleanup found end-of-text marker#807

Merged
hypest merged 1 commit intodevelopfrom
fix/cleanup-zwj-continue
Apr 15, 2019
Merged

Don't bail when ZWJ cleanup found end-of-text marker#807
hypest merged 1 commit intodevelopfrom
fix/cleanup-zwj-continue

Conversation

@hypest
Copy link
Copy Markdown
Contributor

@hypest hypest commented Apr 13, 2019 •
edited
Loading

Addresses wordpress-mobile/gutenberg-mobile#845

This PR fixes an Aztec issue where the ZWJ cleanup function was bailing too early if the end-of-text marker was found. Instead, cleanup needs to continue and remove the rest of the ZWJs if any.

Gutenberg-mobile PR: wordpress-mobile/gutenberg-mobile#868

To test

Use the gutenberg-mobile PR and follow the steps there, thanks!

@hypest
Don't bail when ZWJ cleanup found end-of-text marker
3a6ab4f 
Cleanup needs to continue and remove the rest of the ZWJs if any.
@hypest hypest added the bug label Apr 13, 2019
@hypest hypest requested a review from daniloercoli April 13, 2019 01:23
@hypest hypest mentioned this pull request Apr 13, 2019
Merged
@daniloercoli
Copy link
Copy Markdown
Contributor

daniloercoli commented Apr 15, 2019 •
edited
Loading

Are there any details about the reason to remove the ZWJ character within the text?
(I mean, not your PR, but why the original code was there).

ZWJ is used to "combine" other characters and display those as single char. If we remove it we're messing up the original text, no?

@hypest
Copy link
Copy Markdown
Contributor Author

hypest commented Apr 15, 2019 •
edited
Loading

Are there any details about the reason to remove the ZWJ character within the text?
(I mean, not your PR, but why the original code was there).

Yeah, so, the Constants.ZWJ is used as a "temporary" character during the parsing process to make empty block elements have some content. See

AztecEditor-Android/aztec/src/main/kotlin/org/wordpress/aztec/AztecTagHandler.kt

Lines 211 to 217 in 7e968c3

// if block element is empty add a ZWJ to make it non empty and extend span
if (HiddenHtmlSpan::class.java.isAssignableFrom(kind)) {
output.append(Constants.MAGIC_CHAR)
} else {
output.append(Constants.ZWJ_CHAR)
}
output.setSpan(last, start, output.length, Spanned.SPAN_EXCLUSIVE_EXCLUSIVE)
We set the span with SPAN_EXCLUSIVE_EXCLUSIVE at that point and that span flag doesn't allow the span to be 0-length.

Regarding disrupting multi-sequence emojis, I guess we got lucky here. In fact, we got ourselves a wrong naming of the constant... we have it as ZWJ (Zero Width Joiner), but the accurate name is ZWS (Zero Width Space) and it's a different code point. The typical ZWJ is 0x200D but the ZWS is 0x200B. So, our cleanupZWJ() doesn't actually affect the emojis. 😌

@daniloercoli
Copy link
Copy Markdown
Contributor

daniloercoli commented Apr 15, 2019 •
edited
Loading

What do you think about changing the name of the ZWJ constant to ZWS?

@hypest
Copy link
Copy Markdown
Contributor Author

hypest commented Apr 15, 2019 •
edited
Loading

What do you think about changing the name of the ZWJ constant to ZWS?

We should do it :nod: (in a separate PR so this one gets merged faster)

@hypest hypest merged commit 1400adb into develop Apr 15, 2019
@hypest hypest deleted the fix/cleanup-zwj-continue branch April 15, 2019 10:04
@jtreanor jtreanor mentioned this pull request Apr 16, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@daniloercoli daniloercoli daniloercoli approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

bug

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@hypest @daniloercoli