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

Android support #175

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
5991cec
editOnInput: Android support
cgestes Mar 15, 2016
d6bb473
Composition: remove former text when provided in compositionstart
cgestes Mar 15, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 25 additions & 4 deletions src/component/handlers/composition/DraftEditorCompositionHandler.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@ const RESOLVE_DELAY = 20;
let resolved = false;
let stillComposing = false;
let textInputData = '';
let formerTextInputData = '';

var DraftEditorCompositionHandler = {
onBeforeInput: function(e: SyntheticInputEvent): void {
Expand All @@ -50,7 +51,8 @@ var DraftEditorCompositionHandler = {
* A `compositionstart` event has fired while we're still in composition
* mode. Continue the current composition session to prevent a re-render.
*/
onCompositionStart: function(): void {
onCompositionStart: function(e): void {
formerTextInputData = e.data;
stillComposing = true;
},

Expand Down Expand Up @@ -125,6 +127,9 @@ var DraftEditorCompositionHandler = {
const composedChars = textInputData;
textInputData = '';

const formerComposedChars = formerTextInputData;
formerTextInputData = '';

const editorState = EditorState.set(this.props.editorState, {
inCompositionMode: false,
});
Expand All @@ -149,12 +154,28 @@ var DraftEditorCompositionHandler = {
this.exitCurrentMode();
this.removeRenderGuard();

let contentState = editorState.getCurrentContent();
let selection = editorState.getSelection();
if (formerComposedChars && selection.isCollapsed()) {
var anchorOffset = selection.getAnchorOffset() - formerComposedChars.length;
if (anchorOffset < 0) {
Copy link

Choose a reason for hiding this comment

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

When can this happen? Can this conditional be removed? It feels sketchy.

anchorOffset = 0;
}
const toRemoveSel = selection.merge({anchorOffset});
contentState = DraftModifier.removeRange(
editorState.getCurrentContent(),
toRemoveSel,
'backward',
);
selection = contentState.getSelectionAfter();
}

if (composedChars) {
// If characters have been composed, re-rendering with the update
// is sufficient to reset the editor.
const contentState = DraftModifier.replaceText(
editorState.getCurrentContent(),
editorState.getSelection(),
contentState = DraftModifier.replaceText(
contentState,
selection,
composedChars,
currentStyle,
entityKey
Expand Down
3 changes: 2 additions & 1 deletion src/component/handlers/edit/editOnCompositionStart.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,9 +18,10 @@ var EditorState = require('EditorState');
* The user has begun using an IME input system. Switching to `composite` mode
* allows handling composition input and disables other edit behavior.
*/
function editOnCompositionStart(): void {
function editOnCompositionStart(e): void {
this.setRenderGuard();
this.setMode('composite');
this._onCompositionStart(e);
Copy link

Choose a reason for hiding this comment

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

I think this private method might have problems if the class is munged. Also, if compositionstart is firing while we're in EDIT mode, then I don't think this function call has meaning.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

compositionstart is only fired while we are in editmode from what I saw. the composition one has never been called while I tested.
How is it supposed to be called?

Copy link

Choose a reason for hiding this comment

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

Right. compositionstart is used as a flag to indicate that the editor should switch to composite mode. This swaps in DraftEditorCompositionHandler as the editor's handler bundle, and all subsequent events are handled there until the editor switches back to edit mode. So you shouldn't need to add any code here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Compositionstart can have a non-empty data event which indicate the part of text that is going to be replaced by composition. (on most major web browser).

One alternative solution would be to stop caring about those events and diff the DOM at strategical points, updating the model based on content and not on half broken events. Not sure if it's easily doable.

Copy link

Choose a reason for hiding this comment

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

Compositionstart can have a non-empty data event which indicate the part of text that is going to be replaced by composition. (on most major web browser).

We could check for e.data here, and if a value is present, use Modifier.removeRange to track the modified state and remove the range from the model. It would probably have to be flagged as natively rendered content to avoid re-rendering the editor during the composition. Would that help solve the issue here?

One alternative solution would be to stop caring about those events and diff the DOM at strategical points, updating the model based on content and not on half broken events.

I'd prefer not to do DOM diffing.

Perhaps we could do something similar to the spellcheck handling in editOnInput -- grab the text content from the DOM and replace the equivalent range in the model. No diffing needed, just accommodate the DOM change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Modifier.removeRange is called with the e.data value of compositionstart later in the current patch. I wonder what is best, the text is not removed while in composition, it just get potentially updated at the end.

Replace DOM content within the model was what I had in mind. Will check editOnInput why I have time.

I'll be off next week.

this.update(
EditorState.set(this.props.editorState, {inCompositionMode: true})
);
Expand Down
6 changes: 3 additions & 3 deletions src/component/handlers/edit/editOnInput.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ var findAncestorOffsetKey = require('findAncestorOffsetKey');
var nullthrows = require('nullthrows');

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

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

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?

var DOUBLE_NEWLINE = '\n\n';

/**
Expand All @@ -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?

Copy link

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

Copy link

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?

return;
}

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

var anchorOffset, focusOffset, startOffset, endOffset;

if (isGecko) {
if (isAndroid || isGecko) {
// Firefox selection does not change while the context menu is open, so
// we preserve the anchor and focus values of the DOM selection.
anchorOffset = domSelection.anchorOffset;
Expand Down