Skip to content
This repository has been archived by the owner on Feb 6, 2023. It is now read-only.

Android support #175

Closed
wants to merge 2 commits into from
Closed

Conversation

cgestes
Copy link
Contributor

@cgestes cgestes commented Mar 8, 2016

This fixes some issues for chrome support on Android. (including crosswalk)

  • use the provided selection in editOnInput
  • remove former text provided in compositionstart

remaining issues:

  • moving the selection while composition is active discard the composition
  • calling restoreEditorDOM() close the keyboard

I believe this can be merged after review without waiting for the other fixes. The experience is already much improved.

@@ -40,7 +40,7 @@ function editOnInput(): void {
var domSelection = global.getSelection();

var {anchorNode, isCollapsed} = domSelection;
if (anchorNode.nodeType !== Node.TEXT_NODE) {
if (anchorNode.nodeType !== Node.TEXT_NODE && anchorNode.nodeType !== Node.ELEMENT_NODE) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

is this ok?

Choose a reason for hiding this comment

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

What's the scenario here? For Android text replacement, the span can be the anchor node?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No idea, it just happens. Will check more carefully next time I debug draft-js

Choose a reason for hiding this comment

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

Were you able to get more details on this case?

@facebook-github-bot
Copy link

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla - and if you have received this in error or have any questions, please drop us a line at cla@fb.com. Thanks!

@@ -88,7 +88,7 @@ function editOnInput(): void {

var anchorOffset, focusOffset, startOffset, endOffset;

if (isGecko) {
if (true || isGecko) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I need to find how to check for chrome under Android

@facebook-github-bot
Copy link

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

@cgestes cgestes changed the title WIP: Android support Android support Mar 8, 2016
let selection = editorState.getSelection();
//let selecti
if (formerComposedChars) {
if (selection.isCollapsed()) {

Choose a reason for hiding this comment

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

Nit: Combine conditionals.

cgestes added 2 commits March 15, 2016 13:21
This fixes an issue on Android. It looks ok for others browser also
@hellendag
Copy link

Ok, I'm going to import this and do some testing.

@hellendag
Copy link

@facebook-github-bot import

@facebook-github-bot
Copy link

Thanks for importing. If you are an FB employee go to Phabricator to review.

@sugarshin
Copy link

sugarshin commented May 24, 2016

👍

@jaaberg
Copy link
Contributor

jaaberg commented Jun 7, 2016

What's the status on this one, @hellendag? Would be nice to improve the Android support.

@hellendag
Copy link

Patching and testing.

@@ -21,7 +21,7 @@ var findAncestorOffsetKey = require('findAncestorOffsetKey');
var nullthrows = require('nullthrows');

var isGecko = UserAgent.isEngine('Gecko');

var isAndroid = UserAgent.isPlatform('Android');

Choose a reason for hiding this comment

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

Can we be more specific about targeting browser engines? Or does this need to apply across Android?

@ghost ghost added the CLA Signed label Jul 12, 2016
@butchmarshall
Copy link

butchmarshall commented Jul 28, 2016

  • calling restoreEditorDOM() close the keyboard

It looks like calling restoreEditorDOM setState changes the active element from the div -> body -> div rapidly. This causes the keyboard to close.

Saw this by monitoring the documents focusin/focusout events.

It looks like it closes in react.js right after .$editor0 node is removed.

Open at this point
image

next step it closes
image

React removes the entire <div class="DraftEditor-editorContainer"> element when updating via setState. This obviously removes the child contenteditable="true" which had the focus keeping the keyboard open. This would definitely make it loose focus.

This is caused by... {containerKey: this.state.containerKey + 1}

Which it looks like is used to force a complete re-render.

@hellendag - moving key: 'editor' + this.state.containerKey, from the ref: 'editorContainer' to the data-contents="true" element in DraftEditor.prototype.render fixes this. I don't know enough to know what the side-effects be though?

Why force the re-render of everything, and not just the data-contents?

image

I've created a pull request #572 to illustrate the changes.

@ghost ghost added the CLA Signed label Jul 29, 2016
@dannyshisler
Copy link

Is there any update on this? I could really do with this Android keyboard issue being resolved. Thanks

@andyesp
Copy link

andyesp commented Aug 25, 2016

What is the status on this? Do you need help testing?

@walter0331
Copy link

Hi, I also have some text composition issues on iOS devices, are these issues related?#626

@jan4984
Copy link

jan4984 commented Sep 21, 2016

for adding myself to participants

@StevenLangbroek
Copy link

@hellendag @cgestes is there any word on when this might land? it's blocking us from implementing Draft in our codebase.

oosthoek added a commit to oosthoek/draft-js that referenced this pull request Oct 10, 2016
@pklingem
Copy link

We're also waiting anxiously for this, commenting for notifications, apologies for noise.

@kevinmartin
Copy link

@pklingem If you want notifications without giving everyone "noise", you could just press the Subscribe button on the right sidebar of any issue...

@pklingem
Copy link

thanks @kevinmartin I just clicked subscribe.

@jan4984
Copy link

jan4984 commented Nov 2, 2016

please some one merge this PR?

@kenjiru
Copy link

kenjiru commented Nov 26, 2016

I've ran into this issue when porting my app to Cordova. I was a bit surprised to discover that draft doesn't support Android. Now this issue is the only thing blocking us from releasing our app.

robbertbrak added a commit to robbertbrak/draft-js that referenced this pull request Nov 28, 2016
@mitermayer
Copy link
Contributor

Hi @cgestes , it looks like this pull request has been abandoned, so I am going to close it. If someone is still interested in working on this please feel free to reopen it!

@mitermayer mitermayer closed this Dec 15, 2016
AndrewSouthpaw added a commit to AndrewSouthpaw/draft-js that referenced this pull request Feb 16, 2017
Based off facebookarchive#175, this commit
fixes some issues with autocomplete behavior on Android Chrome.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.