Backspace and content editing broken on Pale Moon about draft-js

[sophiebits]

Unfortunately those errors/warnings you list are actually expected for the moment and don't affect the behavior of the demo; you see the same warnings in any other browser. So that is a red herring. But hopefully the source is clean enough that it is possible to debug for someone who is motivated.

[wolfbeast]

That's a real pity; I was hoping this would actually point at something specific that would allow for rapid pinpointing of the culprit. Is there a way to get more debugging info out of the demo page that could help?

[sophiebits]

No, nothing built-in.

[hellendag]

Thanks for the report!

Is this function executing properly for each keydown? https://github.com/facebook/draft-js/blob/master/src/component/handlers/edit/editOnKeyDown.js#L78

[wolfbeast]

Who are you asking, @hellendag ?
At first glance I don't see anything wrong with the code in that function. Pale Moon supports KeyboardEvent.key (preferred) and KeyboardEvent.which (deprecated) which seems to be used here, and even KeyboardEvent.keyCode (also deprecated).

[wolfbeast]

Question: are you relying on being sent a "keyup" event as well as a "keydown"? Because that may not happen if someone holds down the key (it's OS and browser dependent whether a "keyup" is sent for hold-repeat keys or not).

[sophiebits]

I don't think so, and that would be pretty unlikely anyway given that every other browser also sends many keydown events and only one keyup. Because Draft.js works well on all other browsers and Pale Moon is very rare, it is unlikely that anyone from Facebook will prioritize finding a fix for this (sorry!). But if this is important to you I encourage you to edit the code and figure out why the behavior is different. If there is a simple change we can make to improve compatibility I am sure we are likely to merge it. You might also find that it is an issue with Pale Moon's event handling that can be improved on the browser side.

[wolfbeast]

If I look at event handling, then what I don't understand is why Firefox 26 would work (you say you've tested this), and Pale Moon would not -- it should not have any different behavior in that respect. If it relies on having the .repeat boolean flag (introduced in Firefox 28) then that would make some sense at least, but that wasn't available in v26 either.

What you are asking of me here is that I somehow completely audit this editor to somehow find out in code I didn't write where the code hangs up? Somehow reverse-engineer this and draw up process flow from code instead of the other way around? Aside from developing my browser?

As for the "rarity" of Pale Moon, that's debateable, but Facebook has been very clear about pointedly and expressly ignoring my requests at their address so far (including the form replies without follow-ups, lack of UA detection forcing us to fake Firefox or even Opera with varying success to have a usable site...). Doesn't exactly set a nice precedent for me to do your development work for you, now does it? But hey if you'd rather not have the half a million+ users we have to have a good experience, then please continue, by all means.
Sorry if I sound jaded.

I do need more to go on than "here have a blurb of js with no documentation and figure it out". So if you can at least point me in the right direction as to where to look... that might make a difference. I have a feeling it may actually be something really simple, but knowing where this simple change is needed (either in scripting or in the browser) is the crux.

[hellendag]

Sorry it hasn't been a positive experience working with Facebook so far, it's understandable that that would be frustrating. Our UI/JS infra team is a pretty lean engineering org with a lot of ground to cover, so we do end up focusing on larger browser cohorts. That said, none of us wants to see anyone left behind, so I can try to lend a hand here.

One of my hopes from sharing this project is that we can move toward eliminating issues like these, since anyone can now investigate and play around with the code. I certainly don't mean for you to have to solve all problems for Pale Moon users, but I would hope that it is at least a better situation to have the code open than closed so that we can work together to find problem areas in the code. :)

In this case, given your repro steps, it seems like we can probably focus directly on keydown handling.

Backspace behavior is triggered by keydown. keyup is not currently observed by any Draft handlers. The repeat flag is not used.

Within editOnKeyDown, the key event is mapped to a command using the keyBindingFn prop, which by default is getDefaultKeyBinding. (https://github.com/facebook/draft-js/blob/master/src/component/handlers/edit/editOnKeyDown.js#L79) The which value is used, as defined within React core (https://github.com/facebook/react/blob/dff05beeff9562647950461830ce42d484ca55fa/src/renderers/dom/client/syntheticEvents/SyntheticKeyboardEvent.js#L59). If this value corresponds to the backspace key, we determine the appropriate editor command based on modifier keys.

This command is then used to operate on the current EditorState, i.e. within https://github.com/facebook/draft-js/blob/master/src/component/handlers/edit/commands/keyCommandPlainBackspace.js. The resulting model change is then provided to the top-level component via the onChange prop function.

My hunch here is that something is going wrong in the key handling steps. When running an example, does the which value populate with the expected code while pressing/holding the backspace key?

[wolfbeast]

I think I'm making some progress regardless. Thanks to @hellendag who pointed in the right direction with the keydown function and some trial-and-error on my end monitoring events.

Something that is different in behavior in Pale Moon is that preventDefault() on keydown does not cancel the following keypress event. This is different than what Chrome or current Firefox (and I assume IE) does. I referenced to W3C spec to see what it says in case we are not spec compliant in Pale Moon:

A default action MAY be cancelled through the invocation of the Event.preventDefault() method.

So the problem here is undefined (permitted either way) behavior in the spec that happens to have browser parity with "the big three". I gather this puts the event handler in draft.js in a confused state since the element will immediately be changed and a character will be inserted due to the keypress event which you want to prevent with preventDefault() in the case of command keys.

Unfortunately I wouldn't have a clue how to edit the draft.js code to take this into account, or even how to test this (and don't have the opportunity to familiarize myself with it); it builds on a specific framework I'm not familiar with.

Would this be something you can work with?

[hellendag]

Nice, that sounds like our culprit.

Can you log to see if there is any output for the data value in https://github.com/facebook/draft-js/blob/master/src/component/handlers/edit/editOnBeforeInput.js#L76? Since the event is falling through and Firefox fires keypress events even for control keys, we're probably making it to this point with some arbitrary character.

Within the beforeInput polyfill in the React core, we try to handle these Firefox control key keypresses, but we may be missing something here. https://github.com/facebook/react/blob/3b96650e39ddda5ba49245713ef16dbc52d25e9e/src/renderers/dom/client/eventPlugins/BeforeInputEventPlugin.js#L369

[wolfbeast]

Can you log to see if there is any output for the data value in https://github.com/facebook/draft-js/blob/master/src/component/handlers/edit/editOnBeforeInput.js#L76?

I'm sorry but I have no clue how to set up an environment where I can check this. I'm not a web developer.

The polyfill line pointed at seems to be a different scenario:

• As of v27, Firefox may fire keypress events even when no character
• will be inserted.

I don't think we currently do this...

[wolfbeast]

I've looked at the code in the browser and it should be feasible (but by no means trivial) to change this behavior in Pale Moon for browser parity (and to satisfy our Facebook users) if it's a much more complex problem in the editor, but I'd prefer to keep that as a last option since it will impact web compatibility for other sites as well as a number of our extensions and internal input handling in the browser UI and input elements. I'm assuming we're not the only browser out there that takes this path for DOM key events, so having the js here handle it properly would be desirable, regardless.

[wolfbeast]

Maybe I assumed wrong in general browser behavior. 😄

I'll see if I can change our behavior, since reading more into it and having been able to pinpoint the cause here, it seems the more logical behavior to do what your editor expects.

Just as a critical side note: what's been a dragging issue for close to 2 years has been pinpointed with a possible solution in 2 days now. I really wish you'd have opened up the source sooner.

[hellendag]

@wolfbeast Haha, wish I could have. :) Thanks for your patience.

[wolfbeast]

Just for the record, I can confirm that changing this behavior browser-side (with a few hacks to test it) indeed solves this long-standing issue in draft-js when checking the demo page (and I assume on Facebook as well, haven't tested there yet). It does break part of our internal keystroke handling so this is going to take some time to address. If there is a solution or workaround in the meantime for people using Facebook that are currently having this issue, that would be greatly appreciated.

[hellendag]

I can see if there's a good way to discard these keypresses within BeforeInputEventPlugin in the React core (https://github.com/facebook/react/blob/3b96650e39ddda5ba49245713ef16dbc52d25e9e/src/renderers/dom/client/eventPlugins/BeforeInputEventPlugin.js#L369).

What is the which value for the backspace keypress in Pale Moon?

[wolfbeast]

The which value is (predictably) 8 for Backspace.

Edit: Maybe it would be a good idea to discard all control keys. Most of them would have a value of 0 for which. making an early exit or discard for 0 in addition to 8 (and potentially other specially handled keys) would probably be a good idea?

[hellendag]

Maybe it would be a good idea to discard all control keys. Most of them would have a value of 0 for which

Yeah, that was why I asked about backspace. Do you have a list of the control keys that will have non-0 values on keypress? I'm happy to add them here.

[wolfbeast]

Sorry for the delay here. I checked all most common control keys and combinations and it seems only backspace sends a non-0 keypress event; the rest sends 0

[hellendag]

@wolfbeast What do you think is the right approach here, in that case? Is there a reason that backspace should have a non-zero keypress event, or would it be okay to update it to match the other control keys?

[wolfbeast]

"keypress" events have never been clearly defined, as far as I understood. Keypress events traditionally should fire on all actual key presses, including other control keys like Esc, but they don't. So there isn't exactly much rhyme or reason to it across browsers.
I haven't had time to look into internal key handling in our browser (Backspace is used for navigation, among other things, when not focusing input) and simply changing that to 0 may actually break things in the browser unless being very careful - it'll need time to research at the very least. There's probably a good reason why backspace has a non-0 event while other keys decide to give back 0. Reasoning may be browser parity, internal handling, or web compatibility compromises in the past -- I don't know the exact history of this code.

I think the right approach here would be for your script to treat keycode 8 as a control key (and probably a good idea to make this blanketing for all control/non-character keys) and not a character, which would improve compatibility with not just us, but likely other browsers as well that may send non-0 keypress codes for control characters.

[hellendag]

and probably a good idea to make this blanketing for all control/non-character keys

I'm hesitant to enumerate all possible non-character keycode values within the React core -- it's a pretty long list.

I think that if backspace is the problem right now, then let's go ahead and blacklist it. If we run across more keys that are causing this problem, we can come back to it. :)

[hellendag]

I'm putting together a PR for React. hellendag/react@ea6bc72

[wolfbeast]

keyboardevent.key is actually not the problem here unless you want to switch over from char codes to named keys.
What I was meaning to say is ignoring obvious non-character codes (< 0x20) in keypress events as control keys.

Either way, blacklisting backspace would certainly solve the current problem.

[tylercraft]

Closing as the referenced pull request comments that the problem is solved.