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

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

Merged
merged 1 commit into from
Apr 15, 2019

Conversation

hypest
Copy link
Contributor

@hypest hypest commented Apr 13, 2019

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!

Cleanup needs to continue and remove the rest of the ZWJs if any.
@daniloercoli
Copy link
Contributor

daniloercoli commented Apr 15, 2019

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
Contributor Author

hypest commented Apr 15, 2019

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

// 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
Contributor

daniloercoli commented Apr 15, 2019

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

@hypest
Copy link
Contributor Author

hypest commented Apr 15, 2019

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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants