Android 8.0, 8.1 and 9.0 Support by thesunny · Pull Request #2553 · ianstormtaylor/slate

thesunny · 2019-01-21T22:59:24Z

Is this adding or improving a feature or fixing a bug?

Adding a feature. Support for Android devices.

What's the new behavior?

The new behavior is that Android will work on versions 8.0, 8.1 and 9.0.

Live example here: https://thesunny.github.io/slate/#/rich-text

How does this change work?

This is a complicated PR and encompasses more than a month of full-time work and has been in progress for over a year. Some of the details can be found in this issue:

fix editing with "soft keyboards" (eg. Android, IMEs) #2062

Many issues and how they were overcome are described in the PR itself in a markdown file as well as comments in the plugin itself:

packages/slate-react/src/plugins/ANDROID.md
packages/slate-react/src/plugins/android.js

This PR avoids impacting any other browsers by existing in an android plugin that only gets added to Android browsers. The reason is that the logic in Android is so different than the other browsers that code reuse didn't make sense.

Note: There are some known bugs but I don't think they crash the editor. They do, however, sometimes end up with the wrong text which you can fix and these are edge cases.

I'm PR'ing this because this is a significant improvement over the current state of the editor on Android. It is also before I attempt to create a Combo object that will try to fix the issues in a different way and which may introduce bugs of its own. The Combo object will simplify most of the complex objects and will also be required to fix Android API 24 and 25 (Android 7.00, 7.1.1) and probably needed for Android 6.1 if we want to attempt that in the future.

Have you checked that...?

The new code matches the existing patterns and styles.
The tests pass with yarn test.
The linter passes with yarn lint. (Fix errors with yarn prettier.)
The relevant examples still work. (Run examples with yarn watch.)

Does this fix any issues or need any specific reviewers?

Fixes:
#2062
#2368

Reviewers:
@ianstormtaylor

…selection after input

…e compositionStart does not fire

…tion

jtadmor · 2019-01-22T17:21:04Z

Is the main issue just that this android plugin needs to come before the existing slate-react BeforePlugin()?

Would a mechanism that allows users to import BeforePlugin and manually provide a truly custom plugin list be sufficient?

thesunny · 2019-01-22T19:00:10Z

@jtadmor Not entirely sure what you are asking but this plugin will just work. It adds the correct behavior through a plugin to Android browsers only.

I'm just asking for review in case I missed something.

thesunny · 2019-01-22T19:19:50Z

So, my apologies, it feels like my PR wording is confusing.

I have pointed out that there are known bugs in this PR but my plan is to merge even with the known bugs. This is because even with the bugs, this is a marked improvement over the existing version which is not usable on Android.

I'm asking for review in case there are other issues which should prevent this merge from happening like, for example, breaking behavior on other browsers.

My plan is to fix the known issues using a Combo library but felt like this PR makes Android usable and hence we should merge at this point.

jtadmor · 2019-01-23T17:41:17Z

@thesunny

I guess what I'm asking is how much of this should be put into slate itself vs. just maintained as a separate plugin. If slate / slate-react allowed you to put this plugin before the BeforePlugin, would that make less code be required to go into the core lib?

thesunny · 2019-01-25T03:19:50Z

@jtadmor

In my opinion this goes into Slate proper for these reasons:

All other browsers are supported in Slate proper and not as plugins including iOS support which has lower market share than Android.
It's a reliable way to keep Android support up-to-date with current Slate releases including when Slate has breaking changes.
We modify the content module in Slate which can't be done in a plugin
We add the android plugin before Slate's before plugin. This can't be done by a regular plugin that you add to the editor.
Android uses modules inside of Slate that aren't exposed as an export
There are modules created for Android support which would benefit other parts of Slate

Some of the technical limitations could be overcome; however, as a whole, there is more benefit to keeping it in Slate proper. The primary benefit to separating it is a smaller deliverable if you don't need Android support; however, the additional size is small compared to the benefits IMO.

While it was a hard problem to solve, the amount of code isn't that big and implementing the Combo module later will make it smaller still. Just did a quick check and minified, I estimated it is about 10k (Slate is 175k) and using Slate's average gzip compression ratio, it should be 2k of deliverable (Slate is 40k).

edit: You also need slate-react which adds another 64k/20k (minified/gzipped).

ianstormtaylor

Hey @thesunny, thank you so much for this pull request! And for all of research and work you've put into Android support. I really appreciate it.

I've added a bunch of questions. Although it's probably only the start, since it's a really big pull request so it's hard to review quickly.

But my biggest concern is that the plugin is separate from the existing DOM-based event logic plugin. This seems like it's going to make Android very hard to maintain over time? Since it won't get any of the changes as new logic is added. And it also seems like it means that we don't get the benefit of improved IME logic in other platforms?

I'm going to revert the merging of this pull request, because I'd like to have the chance to review it thoroughly before it gets merged, because it's very large and complex. There will likely need to be changes made to make it easier to maintain going forward. Otherwise I fear it will quickly become outdated. I hope you understand.

ianstormtaylor · 2019-01-29T02:49:31Z

+  IS_FIREFOX,
+  HAS_INPUT_EVENTS_LEVEL_2,
+} from 'slate-dev-environment'
+import ANDROID_API_VERSION from '../utils/android-api-version'


Why isn't this part of slate-dev-environment?

I was a little worried about adding all the android-api-version code into slate-dev-environment.

I can move this all over into slate-dev-environment but to clarify I'd have to add the Android API version code in here as well as that's what I need to determine the input level support.

I can export it as ANDROID_API_VERSION.

If that's fine I can move it all over. Please let me know if that's fine.

@thesunny that sounds fine to me. We can always split it apart later if it needs to be, but keeping all of the environment logic in one place is a good way to start.

Added ANDROID_API_VERSION and updated HAS_INPUT_EVENTS_LEVEL_2. Done.

ianstormtaylor · 2019-01-29T02:50:20Z

+    // when doing an auto-suggest on a fully enclosed text with bold though.
+    // Most likely it still needs other fix issues though.
+    //
+    // if (IS_ANDROID) return


If this is commented out now, why keep it?

Code cleanup that I missed. Sorry about that.

Removal. Done.

ianstormtaylor · 2019-01-29T02:58:04Z

+
+  for (const eventName of events) {
+    plugin[eventName] = function(event, editor, next) {
+      debug(eventName, { event })


Why do we need this plugin in addition to the debug() statements in the other plugins?

This plugin should be removed from the default plugins in plugins/dom.js.

One question I have is whether we might want to leave the library in the code as it is valuable during debugging. I relied on this when developing the Android plugin. The reason is it allowed me to see when an event passed through a plugin. For example, I can see if the android plugin allowed or disallowed an event from passing through to the before plugin. The debug statements in the before plugin don't tell us this definitively as sometimes they return before the debug message gets called.

Also, sometimes I would place this debug plugin after the before plugin to see what made it through there as well.

If I'm working on Android again, I'd probably add it back in for the duration of my work and then remove it when I'm done. It would be simpler if it wasn't in a separate NPM package which I have to add to package.json and remove.

I've removed it from plugins/dom just now (Done). Please let me know what you'd prefer about keeping or removing the library as a whole. Thanks.

@thesunny that makes sense, I'm okay with keeping it in the source here. And potentially we can improve it over time to make it more useful UX wise.

Acknowledged. Left it in.

ianstormtaylor · 2019-01-29T02:59:11Z

@@ -0,0 +1,16 @@
+/**


Why do we need this? This is going to be confusing to people. Can you inline this logic instead? or if it's really used in 10+ places, rename it so it's not confusing.

Yep, had this logic used in multiple places but ended up in one place after refactoring.

Removed and function put directly into dom-snapshot. Done.

ianstormtaylor · 2019-01-29T03:01:20Z

@@ -0,0 +1,77 @@
+import findRange from './find-range'
+
+export default function getSelectionFromDOM(window, editor, domSelection) {


Can you rename this to getDomSelection?

Wouldn't the rename be confusing? It seems to imply that the return value would be a DOM selection. I named it this way because most methods in Slate appear written from the point of view of Slate. So in this instance, we are getting the Slate (implied) selection from the DOM selection. I can see my name was not clear (since it confused you) but perhaps something like getSlateSelectionFromDomSelection? I'm not passionate about it though so if you are still happy with getDomSelection I will change.

Very fair. Okay let's keep the current naming and I can figure out a rename in the future after it's in master.

ianstormtaylor · 2019-01-29T03:02:16Z

@@ -1,77 +1,14 @@
-import findRange from './find-range'
+import getSelectionFromDOM from './get-selection-from-dom'


Can you rename this to setDomSelection?

Please see comment on getSelectionFromDOM and let me know if you'd still like to rename it.

Going to leave it as per your other comment. Thanks.

ianstormtaylor · 2019-01-29T03:02:31Z

@@ -1,5 +1,19 @@
 import findPoint from './find-point'


Can you rename this to setDomText?

Same as the other few comments. Feel like the name might still confusing but let me know and I'll rename it to whatever is preferred.

Going to leave it as per your other comment. Thanks.

ianstormtaylor · 2019-01-29T03:03:19Z

+   */
+
+  apply(editor) {
+    if (editor == null) throw new Error('editor is required')


Why would editor ever be null?

TypeScript knows. 😇 (with --strict ofc)

Nope, it shouldn't be. It was a guard during development because the method didn't originally take an editor object and I saw a few bugs when I forgot to add the editor. I can remove it.

ianstormtaylor · 2019-01-29T03:03:57Z

@@ -0,0 +1,52 @@
+import closest from './closest'


Could you rename this to DomSnapshot instead? as that seems more clear.

ianstormtaylor · 2019-01-29T03:06:21Z

@@ -0,0 +1,624 @@
+import Debug from 'debug'


Why is this implemented as a separate plugin? It seems like it's going to make it really hard to maintain, since most fixes/changes won't be kept in sync? And it also means that we don't get any of the IME fixes that Android made in other platforms?

So last year when I took a stab at this, I tried implementing Android as part of the regular before and after plugins; however, I found it difficult to implement because the changes were almost always to fix Android specific issues and they almost always had side effects that affected other browsers. I ended up having very low confidence that I didn't break things in other editors, and a very high rate of Android specific code mixed in and making it difficult to reason about the code for other browsers (lots of "if Android do this else do that statements").

Moving Android into a separate plugin is one reason development went so much faster this time. I didn't have to worry about regressions in other browsers.

However, that said, my hope is that we will see what's common between the different editors and the way they handle compositions and we could perhaps merge them together... possibly. But I can't say that with high confidence because most of the problems seem specific to Android. I feel a higher confidence level in abstracting compositions into a pluggable composition library that we could customize for each browser.

An example of Android having specific changes that don't affect other browsers is that in the normal Slate before plugin, there is some code in there that says something like if a composition ends but a new composition starts again then we should treat it as if the composition hasn't ended yet. That logic doesn't work in Android so I created an isStrictComposing in addition to the isComposing flag. So now we had two separate but similar concepts of isComposing. But later, we have to split code on whether to use isComposing or isStrictComposing depending on the browsers. Also as I dove deeper this time around, I realized that Android will actually do a compositionEnd without having a corresponding compositionStart which was a kind of implied composition. There is no corresponding logic in other browsers that I'm aware of. These situations had to be handled differently. Furthermore, there is just a lot of really convoluted logic that figures out intent through the combination of events and not a single event. This is in my code which I was planning to abstract into a Combo object.

I can see how looking at my code one might think that there must be an easier way. There is no such complexity in any of the other browsers. That was my feeling at first as well. I will continue to simplify and abstract the code to make it more understandable. I'm fairly confident though that at this point in time, a separate Android plugin makes the most sense and is, in fact, easier rather than harder to keep in sync by virtue of the fact that its easier to understand the code by (IMO) an order of magnitude.

You asked about IME fixes in other platforms as well. I do feel like the IME fixes for Android can be implemented into the rest of Slate but perhaps in another step; for example, there are already a lot of nice abstracted pieces that we can move into the regular handling that wasn't abstracted before. My first step was to get something working for Android though.

This reverts commit 17cdeae.

thesunny · 2019-01-29T18:39:16Z

Sure @ianstormtaylor, I'll do my best to answer your questions.

thesunny · 2019-01-29T21:08:53Z

@ianstormtaylor One question, should I resubmit the PR request with this branch? Not sure of the workflow here.

ianstormtaylor · 2019-01-29T23:58:50Z

@thesunny thanks! Yup, if you can resubmit the PR since I needed to revert it, that would be perfect. And we can discuss the plugin vs. core in that new PR I think.

thesunny · 2019-01-30T01:31:50Z

@ianstormtaylor I think I've addressed all the addressable issues except the plugin issue now. Let me know if there's anything else.

* Allow the dev server to work for non localhost host * Refactored set-selection-from-dom into utils as prep for Android support * Show debug onInput at start if triggered * Added and refactored to use set-text-from-dom-node with improved set selection after input * Remove unnecessary console.log in set-text-from-dom-node * Fixes to pass linter * Adds basic composition to Android API27 including fixing one bug where compositionStart does not fire * Fix some of the enter handling in API 27 and 28 * Add fixes for API 25 * Add debug for slate:update instead of separate render and updateSelection * Add API 26 fix for ignoring all but Enter in onKeyDown * Fix enter on Android 26 and 27 * Revert onSelect bug. Editor API 26 and 27 stable-ish * Fix enter at beginning and end of word in API 26 and 27 * Fix enter handling at end of line API 26 and 27 * Fix reversion of enter bug when not at end of line * Rename enter to linefeed which is more accurate * Fix backspace on Android 27 and 28 * Fix enter at end of line then backspace then enter bug in API 26 and 27 * Refactor to simplify reading code * Refactor to use executor and fix the suggestion problem * Fix multi point edit in API 27/28 * Update Android documentation on enter handling * Fix enter in API 26/27 and document 4 different enter cases * Refactor partial into SlateSnapshot * Complete SlateSnapshot refactor * Remove unnecessary plugin comments * Add smoke tests * Rename smoke tests to composition in exmaples * Fix API28 split join and insertion * Fix space then backspace in middle of word bug in API 28 * Add text for middle word space and backspace bug * Add note that the space backspace bug does not exist on API 27 * Fix 'It me. No.' bug in API 26/27 * Fix comments * Update comments to fit Slate style guide * Move a debug statement * Fix zero-width selection placement bug. * Fix 'it is' then enter in middle of 'it' bug * Partial fix of enter, backspace, enter in word * Add and fix comments. Fix selection in zero-width for API26-27 * Fix linting * Fix documentation * Remove snapback from packages * Remove snapback from yarn.lock

…tormtaylor#2562) This reverts commit 17cdeae.

thesunny added 30 commits December 28, 2018 16:30

Allow the dev server to work for non localhost host

fc24ee6

Refactored set-selection-from-dom into utils as prep for Android support

3f24d36

Show debug onInput at start if triggered

cb1496c

Added and refactored to use set-text-from-dom-node with improved set …

441ff83

…selection after input

Merge branch 'master' into refactor-to-utils

5afdf83

Remove unnecessary console.log in set-text-from-dom-node

8bc42d6

Fixes to pass linter

c0fceff

Adds basic composition to Android API27 including fixing one bug wher…

fed4b7f

…e compositionStart does not fire

Fix some of the enter handling in API 27 and 28

9a90a4e

Add fixes for API 25

d6aec3e

Add debug for slate:update instead of separate render and updateSelec…

89963b4

…tion

Add API 26 fix for ignoring all but Enter in onKeyDown

f8391d6

Fix enter on Android 26 and 27

fe1607b

Revert onSelect bug. Editor API 26 and 27 stable-ish

d850cbf

Fix enter at beginning and end of word in API 26 and 27

7aa1592

Fix enter handling at end of line API 26 and 27

29231dc

Fix reversion of enter bug when not at end of line

b79e274

Rename enter to linefeed which is more accurate

14ebbba

Fix backspace on Android 27 and 28

64eb53c

Fix enter at end of line then backspace then enter bug in API 26 and 27

8cb8c6a

Refactor to simplify reading code

c40c10a

Refactor to use executor and fix the suggestion problem

bcecb06

Fix multi point edit in API 27/28

cabfe73

Update Android documentation on enter handling

f6f38cb

Fix enter in API 26/27 and document 4 different enter cases

2bd3e0f

Merge examples navigation improvement

187f343

Refactor partial into SlateSnapshot

ea90fa1

Complete SlateSnapshot refactor

29a7061

Remove unnecessary plugin comments

cc1ad75

Add smoke tests

7f6b7cf

thesunny added improvement feature examples ⚑ ime ⚑ mobile labels Jan 21, 2019

thesunny changed the title ~~Android 8.0, 8.1 and 9.0 Support for Slate~~ Android 8.0, 8.1 and 9.0 Support Jan 21, 2019

ghost reviewed Jan 22, 2019

View reviewed changes

Comment thread packages/slate-react/src/plugins/ANDROID.md

ghost reviewed Jan 22, 2019

View reviewed changes

Comment thread packages/slate-react/package.json Outdated

thesunny added 3 commits January 22, 2019 11:03

Fix documentation

5453773

Remove snapback from packages

7748676

Remove snapback from yarn.lock

822989f

jordoh mentioned this pull request Jan 24, 2019

Consider whether to generate state updates for each composition update ProseMirror/prosemirror#543

Closed

thesunny merged commit 17cdeae into ianstormtaylor:master Jan 28, 2019

ianstormtaylor reviewed Jan 29, 2019

View reviewed changes

ianstormtaylor added a commit that referenced this pull request Jan 29, 2019

Revert "Android 8.0, 8.1 and 9.0 Support (#2553)"

031190a

This reverts commit 17cdeae.

ianstormtaylor mentioned this pull request Jan 29, 2019

Revert "Android 8.0, 8.1 and 9.0 Support" #2562

Merged

ianstormtaylor added a commit that referenced this pull request Jan 29, 2019

Revert "Android 8.0, 8.1 and 9.0 Support (#2553)" (#2562)

391e2cb

This reverts commit 17cdeae.

thesunny mentioned this pull request Jan 30, 2019

Android 8 and 9 Support #2565

Merged

4 tasks

thesunny mentioned this pull request Feb 4, 2019

Facebook#1077 - Fix for input bug on Android - space or a special char clears the input facebookarchive/draft-js#1500

Closed

davidnus pushed a commit to davidnus/slate that referenced this pull request Feb 11, 2019

Revert "Android 8.0, 8.1 and 9.0 Support (ianstormtaylor#2553)" (ians…

a17284f

…tormtaylor#2562) This reverts commit 17cdeae.

		@@ -0,0 +1,77 @@
		import findRange from './find-range'

		export default function getSelectionFromDOM(window, editor, domSelection) {

		@@ -1,77 +1,14 @@
		import findRange from './find-range'
		import getSelectionFromDOM from './get-selection-from-dom'

Conversation

thesunny commented Jan 21, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Is this adding or improving a feature or fixing a bug?

What's the new behavior?

How does this change work?

Have you checked that...?

Does this fix any issues or need any specific reviewers?

Uh oh!

Uh oh!

Uh oh!

jtadmor commented Jan 22, 2019

Uh oh!

thesunny commented Jan 22, 2019

Uh oh!

thesunny commented Jan 22, 2019

Uh oh!

jtadmor commented Jan 23, 2019

Uh oh!

thesunny commented Jan 25, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ianstormtaylor left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

steida Jan 29, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

thesunny commented Jan 21, 2019 •

edited

Loading

thesunny commented Jan 25, 2019 •

edited

Loading

steida Jan 29, 2019 •

edited

Loading

thesunny commented Jan 30, 2019 •

edited

Loading