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

Add partial backspace support to editOnInput for Android devices #1067

Closed
wants to merge 1 commit into from

Conversation

mmmoussa
Copy link
Contributor

Summary

This pr addresses two parts of the problem raised in #1066.

The first part is that it adds support for entity deletion within editOnInput. It does this by detecting whether the start of the selection is a mutable entity, and if it's not, falling back to the backspace method normally used as part of the keyDown event handler.

The second part is that it identifies cases where the selection is incorrect and then corrects it before falling back on the backspace method. The correction is performed by using the end of the DOM's selection as the start of the selection to evaluate, and the end of the last editor state's selection as the end of the selection to evaluate.

Known issues

The backspace selection range cannot be correctly determined when the starting selection edge is in the middle of a span. In such cases, the span containing the starting selection edge does not have any of its contents deleted, and temporarily the DOM selection understanding is incorrect until the cursor is moved.

@flarnie
Copy link
Contributor

flarnie commented Mar 17, 2017

"It does this by detecting whether the start of the selection is a mutable entity, and if it's not, falling back to the backspace method normally used as part of the keyDown event handler."
I'd like to understand that part of the change more.
It's true that in android, when the user hits 'delete', that triggers the 'editOnInput' callback, and in that case we could just call the 'keyCommandPlainBackspace' method.

  • What about the other cases where 'editOnInput' gets called? It gets used for a bunch of things. Maybe we should split it into some separate handlers - it's getting convoluted I think.
  • Why would we not call 'keyCommandPlainBackspace' when there is a mutable entity present?

@flarnie
Copy link
Contributor

flarnie commented Mar 17, 2017

So some ideas from our meeting:

  • It seems like the core issue is that Draft tries to listen to keyDown and it's not there on delete in Android with that keyboard. We could file a bug and/or file a bug and push to have it fixed.
  • we could try to write unit tests around this; that might make the issues more clear and help us find other changes that will fix the problem
    • for inspiration look at the DOM selection unit tests
  • let's be careful and understand this behavior before merging this or any similar change
  • let's be intentional about which conditional blocks we gate to Android; that might warrant more discussion
    • we might look at the way we handle 'cut' where we let the DOM get mutated and then force a re-render to get it back into a good state

Hope that is helpful, and sorry that we're not prepared to merge this change.

Copy link
Contributor

@sophiebits sophiebits left a comment

Choose a reason for hiding this comment

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

(switching away from github review labels; requesting changes to match earlier review above)

@flarnie
Copy link
Contributor

flarnie commented Oct 14, 2018

Since we never resolved the questions and problems with this PR, and nobody is currently working on fixing the known issues with Android web, I'm closing this.

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.

4 participants