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

Android 8 and 9 Support #2565

Merged
merged 61 commits into from
Mar 8, 2019
Merged

Conversation

thesunny
Copy link
Collaborator

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

Adding a feature

What's the new behavior?

Add Android 8 and 9 support for Slate.

How does this change work?

Please see #2553

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?

Reviewers: @ianstormtaylor

@Nantris
Copy link
Contributor

Nantris commented Feb 13, 2019

Anything a lowly contributor such as myself can do to help this PR along? I know it's a substantial PR and needs proper review, but mobile support is so damn exciting.

@thesunny - would you be open to merging your PR into the master branch of your Slate repo? Yarn can't handle references to branches on Github when the package is a monorepo, but I'd love to give this some testing in my app.

@thesunny
Copy link
Collaborator Author

@slapbox

I was considering releasing a version on NPM. Maybe under slate-for-android until it gets merged.

@Nantris
Copy link
Contributor

Nantris commented Feb 14, 2019

image

@thesunny
Copy link
Collaborator Author

@slapbox @steida @pradel @gracicot

Just a heads up that I published under these two NPM packages:

  • slate-react-android
  • slate-dev-environment-android

Unfortunately, because of slate-dev-environment-android, you'll have to alias the package in webpack, babel or whatever you are using.

Let me know how it goes!

@Nantris
Copy link
Contributor

Nantris commented Feb 19, 2019

Remaining issues I've found so far:

  1. Backspace timing issue - If you don't wait until the keyboard notices it's at a word boundary and loads its suggestions before continuing to Backspace into a new word it will insert a fragment of a word at the cursor's location
  2. Selecting a word and using Backspace only removes the last character
  3. Copying a series of words only copies the first word
  4. Cutting a word or series of words does neither copies the words into clipboard nor removes the words from the editor
  5. Tapping a spot in the editor sets the focus correctly and opens the keyboard, but onChange events are not fired for that interaction.

All the above results from Pixel 2 running Android 9 | Chrome 72 | GBoard

The weird thing is that I don't recall these being issues in January (At the very least I don't recalls Points 1 and 4 functioning that way.)

So the question is, is it changes to Slate, changes to @thesunny's method of supporting Android, changes to Chrome, or an oversight by us when testing previously? I'm leaning heavily against this being an oversight since I tried hard to take a wrecking ball to @thesunny's work.


Important note for anyone testing this:

If you set up slate-react to resolve to slate-react-android but you do not set slate-dev-environment to resolve to slate-dev-environment-android it will, at first, appear to work, but there are a variety of additional issues you will encounter, especially related to using the Enter key. Be sure to set this up correctly.

@gracicot
Copy link
Contributor

I don't know if this is my fault or the patch, but I have an error when using it on a desktop browser:

error screenshot

The error appears each time I type a character.

@Nantris
Copy link
Contributor

Nantris commented Feb 20, 2019

@gracicot you definitely have slate-dev-environment resolving to slate-dev-environment-android? When I didn't have this I was seeing null for focus.path and anchor.path

@gracicot
Copy link
Contributor

@slapbox I did some research and it still happen with the master branch. I'll try to find out why. Thanks.

@ianstormtaylor
Copy link
Owner

Hey @thesunny, sorry for the delay on this. And thank you again for putting in the work to make this happen, this is really awesome.

Summary: It looks good to me, feel free to merge!


Longer version: it's hard for me—with no Android experience—to fully "approve" this pull request. But I don't think that's necessary for now, and am happy to have it merged in to live alongside the codebase. The only caveat is that I can't always guarantee that changes I make while Slate is in beta will happen to be fully working across Android. And because of that I'll need you (and others's) help to keep it up to date. I hope that makes sense.

But I'm happy to have this merged in. And over time, I think we can work to make it more integrated with the composition handling across other environments, so that we move away from having a separate code path just for Android. Which will hopefully make it easier to maintain over time for everyone. (Although I know this won't be easy, since we'll have to come up with new abstractions to stay sane while working on it 😄)

Anyways, feel free to merge! And thank you very much.

@thesunny
Copy link
Collaborator Author

thesunny commented Mar 8, 2019

Thanks Ian. I've merged master into the PR and there were some unexpected conflicts. I don't think that I missed any weird issues but could a few people please do a quick check to make sure things are working as expected:

https://thesunny.github.io/slate/#/composition/split-join

Note that the "Special" sub-example at this URL may fail but the others should work. I've included them so that I won't forget them the next time I revisit Android which will be some time after I come back from vacation (leaving on March 13 and back on March 20).

@AndyBitz
Copy link

AndyBitz commented Mar 8, 2019

@thesunny I've tested the examples on my Pixel 3, "Split/Join" works, as does "Insertition". First example under "Special" with "it is" works too, the others do not.

Also, selecting multiple words and then hitting the "bold" or "italic" button will apply the changes only to one word. I guess this is related to @slapbox findings.

On a side note, I've been playing around with contenteditable as well and I came up with this pattern which uses MutationObserver, I don't know if this can be applied here or if it would be useful in this case, but feel free to take a look: https://gist.github.com/AndyBitz/aac539f17f81dfe976904fce060d6de7.

And thank you for all your work, this is pretty amazing 🙏

@thesunny
Copy link
Collaborator Author

thesunny commented Mar 8, 2019

@AndyBitz Thank so much for checking the PR.

I took a look at the gist you linked to but I have to admit I'm not sure what you're trying to show me.

I did try to use Mutation observer to revert the DOM to a known safe state but unfortunately it didn't work. At least not all the time. The solution that worked was to save references to the original DOM nodes (before the changes) and re-arrange and re-populate them to their original positions and values.

@thesunny thesunny merged commit 89adf63 into ianstormtaylor:master Mar 8, 2019
@thesunny
Copy link
Collaborator Author

thesunny commented Mar 8, 2019

Yay! Android support is merged!

@AndyBitz
Copy link

AndyBitz commented Mar 8, 2019

@thesunny No worries, I thought the composition event was the main reason why it didn't work on Android and tried to make it work this way, but I guess I oversimplified the solution.

Anyway, thanks again for making this possible!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants