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

Fixing major Android editing issues #2035

Conversation

fabiomcosta
Copy link
Contributor

@fabiomcosta fabiomcosta commented Mar 14, 2019

This PR is a new attempt to address #1895

On #2031 I was trying to make compositions work using the data provided by that event, and even though that works well for most operational system, that doesn't work well for Android, where you can make composition updates in multiple places on the same composition transaction.

This PR is an improvement over that PR, in that it uses a DOM mutation observer during the compositionStart -> compositionEnd interval to detect the changes the user made, and then re-apply those to the ContentState on compositionEnd.

This approach is the one used by Prosemirror (see https://github.com/ProseMirror/prosemirror-view/blob/master/src/domobserver.js), which is the only Rich Text Editor I've tried that works well on Android.
Like previously mentioned, it allows multiple mutations on multiple places in the same composition transaction, which was impossible with the previous approach, and would cause DOM<->state inconsistencies in multiple use cases.

The intent of this PR is not to fix every single Android bug, but to have a consistent editing experience on Android without introducing bugs (ideally).

TODO, known issues:

  • Removing empty line breaks with doesn’t remove blocks.
  • Mutations on the same block (different leaf nodes) are not being properly applied.
  • Selection is not properly updated during composition events
  • Keep inlineStyleOverride working with a consistent behavior

TODO, others:

  • Test on Android Pie v9 API 28
  • Test on Android Oreo v8.1 API 27
  • Test on Android Oreo v8.0 API 26
  • Test on iPhone 12.1 (with Japanese Kana keyboard)
  • Test composition events on Chrome Desktop v73
  • Test composition on IE11 (I didn't know how to test composition events though)
  • Unit tests

Help test this PR

There are 3 ways to try out this PR.

Codesandbox: https://3ymzzlj9n5.codesandbox.io/ (uses draft-js-android-fix-beta.3)

Use the published draft-js-android-fix package: yarn install draft-js-android-fix

Note that this package might not be up-to-date, it's hard for me to guarantee I'll always remember to update the package, but I'll do my best.

The other way is guaranteed to be up-to-date, but has a longer setup:

  • run git clone https://github.com/facebook/draft-js.git
  • run git remote add fabiomcosta https://github.com/fabiomcosta/draft-js.git
  • run git fetch fabiomcosta
  • run git checkout -b attempt_android_fix_with_dom_diff fabiomcosta/attempt_android_fix_with_dom_diff
  • run yarn install (or use npm)
  • run open examples/draft-0-10-0/rich/rich.html, or any other example you'd like to test

Test Plan

On Android (Pie v9 API 28, Oreo v8.1 API 27, Oreo v8.0 API 26)

Type "d[space]"
Renders the letter "d" followed by a space.

Type "da[space]"
Renders the expected highlighted autocomplete option, "day" in this case.

Type "day[space][backspace][backspace][backspace][backspace]"
Properly removes the text without re-adding the previous text.

Type "d[space][enter][backspace][enter]"
Should render "d[space][enter]".

Known issues, that might not be fixed on this PR

  • Pressing ENTER during a composition doesn’t commit it. Ex: type "hel[enter]", and notice that nothing happens when you click enter, but the expected behavior is to commit that composition and add a line break.
  • Removing the last work from a block that has been auto-completed, will incorrectly remove it from the content state. Ex: type "aa" it should render something like "and and ". Now press backspace. At this point the content state is incorrectly set as "and ", but it should be "and and".
  • [minor] Arrow keys won't work during composition

Acknowledgments

Based on ideas from:
#1672
#1774
ianstormtaylor/slate#2565
http://prosemirror.net/

Useful links

Latest draft-js 0.10.5 sandbox (useful to compare with previous behavior): https://ko4zmx633v.codesandbox.io/
Plain contenteditable sandbox: https://q77yqk1nww.codesandbox.io/

@fabiomcosta
Copy link
Contributor Author

There is still some cleanup to be done on this PR before anything.

const documentSelection = getDraftEditorSelection(
editorState,
editorNode.firstChild,
getContentEditableContainer(editor),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just a refactor to use the newly created getContentEditableContainer

package.json Outdated
"description": "A React framework for building text editors.",
"version": "0.10.5",
"version": "0.10.6-beta.0",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This will be reverted before merging


// TODO, check if Facebook still needs this flag or if it could be removed.
// Since there can be multiple mutations providing a `composedChars` doesn't
// apply well on this new model.
Copy link
Contributor Author

@fabiomcosta fabiomcosta Mar 16, 2019

Choose a reason for hiding this comment

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

@claudiopro would you know if we can remove this code or do we need to keep it somehow working?
If we keep it working it will be buggy, because we are now applying multiple mutations (which leads to multiple composedChars) on compositionEnd, which is not compatible with the arguments that we provide to handleBeforeInput here.

If draft_handlebeforeinput_composed_text is still being used at FB, I'd suggest to provide the composedChars from the first mutation, completely ignoring the other mutations, which should be fine for the common case. My main suggestion is still to stop depending on this code/gkflag.

editor.update(newEditorState);
return;
}
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This change is necessary because keyDown on draft-js edit mode on Android doesn't come with the correct keyCode, so, for example, when users press backspace we are not actually executing keyCommandPlainBackspace, which leads to an incorrect content state.

The input event comes with this inputType which lets us execute the correct action.
I still need to make sure older versions of Android also support this property (will do soon).

Copy link
Contributor

@claudiopro claudiopro left a comment

Choose a reason for hiding this comment

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

Please address these linter issues, otherwise looking good 👍

test('Can handle a single mutation', () => {
withGlobalGetSelectionAs({}, () => {
editor._latestEditorState = getEditorState({blockkey0: ''});
const mutations = Map({'blockkey0-0-0': '私'});
Copy link
Contributor

Choose a reason for hiding this comment

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

Please replace with the unicode literal escape sequence '\u79c1'

compositionHandler.onCompositionEnd(editor);
jest.runAllTimers();

expect(editorTextContent()).toBe('私');
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here

const USE_CHAR_DATA = UserAgent.isBrowser('IE <= 11');

class DOMObserver {
observer: MutationObserver;
Copy link
Contributor

Choose a reason for hiding this comment

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

Please initialize this property or make it nullable changing the type to ?MutationObserver.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh, good call.

observer: MutationObserver;
container: HTMLElement;
mutations: Map<string, string>;
onCharData: ({target: EventTarget, type: string}) => void;
Copy link
Contributor

Choose a reason for hiding this comment

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

This should also be initialized, or make it nullable.

* https://w3c.github.io/input-events/#dom-inputevent-inputtype */
const {inputType} = e.nativeEvent;
if (inputType) {
let newEditorState = onInputType(inputType, editorState);
Copy link
Contributor

Choose a reason for hiding this comment

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

This is never reassigned, please declare it as const.

Copy link

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

@claudiopro has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@claudiopro
Copy link
Contributor

Thank you so much for contributing this improvement @fabiomcosta! This is now merged in master 🎉

@facebook-github-bot
Copy link

@claudiopro merged this pull request in 634bd29.

@martin-badin
Copy link

Hello, please create a new release with the tag #next for testing.

@sunild
Copy link

sunild commented Jun 25, 2019

I'd love to see a new release w/this fix too! We're using some draft-js plugins and I haven't figured out how to make them work when installing from git (they can't find draft-js).

fraziermork added a commit to textioHQ/draft-js that referenced this pull request Jul 16, 2019
jdecked pushed a commit to twitter-forks/draft-js that referenced this pull request Oct 9, 2019
Summary:
This PR is a new attempt to address facebookarchive#1895

On facebookarchive#2031 I was trying to make compositions work using the data provided by that event, and even though that works well for most operational system, that doesn't work well for Android, where you can make composition updates in multiple places on the same composition transaction.

This PR is an improvement over that PR, in that it uses a DOM mutation observer during the `compositionStart` -> `compositionEnd` interval to detect the changes the user made, and then re-apply those to the ContentState on `compositionEnd`.

This approach is the one used by [Prosemirror](http://prosemirror.net/) (see https://github.com/ProseMirror/prosemirror-view/blob/master/src/domobserver.js), which is the only Rich Text Editor I've tried that works well on Android.
Like previously mentioned, it allows multiple mutations on multiple places in the same composition transaction, which was impossible with the previous approach, and would cause DOM<->state inconsistencies in multiple use cases.

The intent of this PR is not to fix every single Android bug, but to have a consistent editing experience on Android without introducing bugs (ideally).

**TODO, known issues:**
- [x] Removing empty line breaks with <backspace> doesn’t remove blocks.
- [x] Mutations on the same block (different leaf nodes) are not being properly applied.
- [x] Selection is not properly updated during composition events
- [ ] Keep `inlineStyleOverride` working with a consistent behavior

**TODO, others:**
- [x] Test on Android Pie v9 API 28
- [x] Test on Android Oreo v8.1 API 27
- [x] Test on Android Oreo v8.0 API 26
- [x] Test on iPhone 12.1 (with Japanese Kana keyboard)
- [x] Test composition events on Chrome Desktop v73
- [x] Test composition on IE11 (I didn't know how to test composition events though)
- [x] Unit tests

There are 3 ways to try out this PR.

Codesandbox: https://3ymzzlj9n5.codesandbox.io/ (uses `draft-js-android-fix-beta.3`)

Use the published [draft-js-android-fix](https://www.npmjs.com/package/draft-js-android-fix) package: `yarn install draft-js-android-fix`

Note that this package might not be up-to-date, it's hard for me to guarantee I'll always remember to update the package, but I'll do my best.

The other way is guaranteed to be up-to-date, but has a longer setup:
* run `git clone https://github.com/facebook/draft-js.git`
* run `git remote add fabiomcosta https://github.com/fabiomcosta/draft-js.git`
* run `git fetch fabiomcosta`
* run `git checkout -b attempt_android_fix_with_dom_diff fabiomcosta/attempt_android_fix_with_dom_diff`
* run `yarn install` (or use `npm`)
* run `open examples/draft-0-10-0/rich/rich.html`, or any other example you'd like to test
Pull Request resolved: facebookarchive#2035

Reviewed By: kedromelon

Differential Revision: D14568528

Pulled By: claudiopro

fbshipit-source-id: 16861de52eca41cd98f884b0aecf034213fc1bd0
@michelson
Copy link

Hi @fabiomcosta , this was released as a npm version? I've upgraded to 0.11.4 but the space error on android is still present.

mmissey pushed a commit to mmissey/draft-js that referenced this pull request Mar 24, 2020
Summary:
This PR is a new attempt to address facebookarchive#1895

On facebookarchive#2031 I was trying to make compositions work using the data provided by that event, and even though that works well for most operational system, that doesn't work well for Android, where you can make composition updates in multiple places on the same composition transaction.

This PR is an improvement over that PR, in that it uses a DOM mutation observer during the `compositionStart` -> `compositionEnd` interval to detect the changes the user made, and then re-apply those to the ContentState on `compositionEnd`.

This approach is the one used by [Prosemirror](http://prosemirror.net/) (see https://github.com/ProseMirror/prosemirror-view/blob/master/src/domobserver.js), which is the only Rich Text Editor I've tried that works well on Android.
Like previously mentioned, it allows multiple mutations on multiple places in the same composition transaction, which was impossible with the previous approach, and would cause DOM<->state inconsistencies in multiple use cases.

The intent of this PR is not to fix every single Android bug, but to have a consistent editing experience on Android without introducing bugs (ideally).

**TODO, known issues:**
- [x] Removing empty line breaks with <backspace> doesn’t remove blocks.
- [x] Mutations on the same block (different leaf nodes) are not being properly applied.
- [x] Selection is not properly updated during composition events
- [ ] Keep `inlineStyleOverride` working with a consistent behavior

**TODO, others:**
- [x] Test on Android Pie v9 API 28
- [x] Test on Android Oreo v8.1 API 27
- [x] Test on Android Oreo v8.0 API 26
- [x] Test on iPhone 12.1 (with Japanese Kana keyboard)
- [x] Test composition events on Chrome Desktop v73
- [x] Test composition on IE11 (I didn't know how to test composition events though)
- [x] Unit tests

There are 3 ways to try out this PR.

Codesandbox: https://3ymzzlj9n5.codesandbox.io/ (uses `draft-js-android-fix-beta.3`)

Use the published [draft-js-android-fix](https://www.npmjs.com/package/draft-js-android-fix) package: `yarn install draft-js-android-fix`

Note that this package might not be up-to-date, it's hard for me to guarantee I'll always remember to update the package, but I'll do my best.

The other way is guaranteed to be up-to-date, but has a longer setup:
* run `git clone https://github.com/facebook/draft-js.git`
* run `git remote add fabiomcosta https://github.com/fabiomcosta/draft-js.git`
* run `git fetch fabiomcosta`
* run `git checkout -b attempt_android_fix_with_dom_diff fabiomcosta/attempt_android_fix_with_dom_diff`
* run `yarn install` (or use `npm`)
* run `open examples/draft-0-10-0/rich/rich.html`, or any other example you'd like to test
Pull Request resolved: facebookarchive#2035

Reviewed By: kedromelon

Differential Revision: D14568528

Pulled By: claudiopro

fbshipit-source-id: 16861de52eca41cd98f884b0aecf034213fc1bd0
@NNSTH
Copy link

NNSTH commented Aug 27, 2020

I'm using draft-js version: 0.11.6 and still having lot of Android issues, what am I missing?

@lousander
Copy link

Hey! I've been experiencing some weirds issues with my custom components. It started on version v11.0.0 and seems to be related to this PR. The issue happens when typing brazilian portuguese, we have some letters that use accents e.g., é, ê, ã, etc, so in draft-js they are compositions. Typing any of these triggers an unmount on all our custom components. It can be really troublesome if we have a custom component that does a request or some async code, as it keeps redoing it every time we type any of those letters.

From what I can tell this issue seems to be directly tied to the alterations made here to the DraftEditorCompositionHandler.js file. Apparently after any kind of composition, it calls the method restoreEditorDOM, which is described as "a complete re-render of the DraftEditorContents based on the current EditorState". Before this PR, this method was only called when certain conditions were met (by using the mustReset flag), now it seems to happen everytime a composition is completed. I'm trying to wok out some way to correct this behavior but I'm having some difficulties. @fabiomcosta Do you think you could shed some light on a way to tackle this?

@yongningfu
Copy link

Hey! I've been experiencing some weirds issues with my custom components. It started on version v11.0.0 and seems to be related to this PR. The issue happens when typing brazilian portuguese, we have some letters that use accents e.g., é, ê, ã, etc, so in draft-js they are compositions. Typing any of these triggers an unmount on all our custom components. It can be really troublesome if we have a custom component that does a request or some async code, as it keeps redoing it every time we type any of those letters.

From what I can tell this issue seems to be directly tied to the alterations made here to the DraftEditorCompositionHandler.js file. Apparently after any kind of composition, it calls the method restoreEditorDOM, which is described as "a complete re-render of the DraftEditorContents based on the current EditorState". Before this PR, this method was only called when certain conditions were met (by using the mustReset flag), now it seems to happen everytime a composition is completed. I'm trying to wok out some way to correct this behavior but I'm having some difficulties. @fabiomcosta Do you think you could shed some light on a way to tackle this?

@claudiopro @fabiomcosta When typing Chinese, the problem reappears

@offcall
Copy link

offcall commented Oct 8, 2021

I am experiencing the same problem, if composition is entered next to entity, then mutation contains text from entity and breaks it
image
image
image

@offcall offcall mentioned this pull request Oct 8, 2021
alicayan008 pushed a commit to alicayan008/draft-js that referenced this pull request Jul 4, 2023
Summary:
This PR is a new attempt to address facebookarchive/draft-js#1895

On facebookarchive/draft-js#2031 I was trying to make compositions work using the data provided by that event, and even though that works well for most operational system, that doesn't work well for Android, where you can make composition updates in multiple places on the same composition transaction.

This PR is an improvement over that PR, in that it uses a DOM mutation observer during the `compositionStart` -> `compositionEnd` interval to detect the changes the user made, and then re-apply those to the ContentState on `compositionEnd`.

This approach is the one used by [Prosemirror](http://prosemirror.net/) (see https://github.com/ProseMirror/prosemirror-view/blob/master/src/domobserver.js), which is the only Rich Text Editor I've tried that works well on Android.
Like previously mentioned, it allows multiple mutations on multiple places in the same composition transaction, which was impossible with the previous approach, and would cause DOM<->state inconsistencies in multiple use cases.

The intent of this PR is not to fix every single Android bug, but to have a consistent editing experience on Android without introducing bugs (ideally).

**TODO, known issues:**
- [x] Removing empty line breaks with <backspace> doesn’t remove blocks.
- [x] Mutations on the same block (different leaf nodes) are not being properly applied.
- [x] Selection is not properly updated during composition events
- [ ] Keep `inlineStyleOverride` working with a consistent behavior

**TODO, others:**
- [x] Test on Android Pie v9 API 28
- [x] Test on Android Oreo v8.1 API 27
- [x] Test on Android Oreo v8.0 API 26
- [x] Test on iPhone 12.1 (with Japanese Kana keyboard)
- [x] Test composition events on Chrome Desktop v73
- [x] Test composition on IE11 (I didn't know how to test composition events though)
- [x] Unit tests

There are 3 ways to try out this PR.

Codesandbox: https://3ymzzlj9n5.codesandbox.io/ (uses `draft-js-android-fix-beta.3`)

Use the published [draft-js-android-fix](https://www.npmjs.com/package/draft-js-android-fix) package: `yarn install draft-js-android-fix`

Note that this package might not be up-to-date, it's hard for me to guarantee I'll always remember to update the package, but I'll do my best.

The other way is guaranteed to be up-to-date, but has a longer setup:
* run `git clone https://github.com/facebook/draft-js.git`
* run `git remote add fabiomcosta https://github.com/fabiomcosta/draft-js.git`
* run `git fetch fabiomcosta`
* run `git checkout -b attempt_android_fix_with_dom_diff fabiomcosta/attempt_android_fix_with_dom_diff`
* run `yarn install` (or use `npm`)
* run `open examples/draft-0-10-0/rich/rich.html`, or any other example you'd like to test
Pull Request resolved: facebookarchive/draft-js#2035

Reviewed By: kedromelon

Differential Revision: D14568528

Pulled By: claudiopro

fbshipit-source-id: 16861de52eca41cd98f884b0aecf034213fc1bd0
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.