1739489 - Emoji input via macOS IME crashes Draft.js editors - impacts facebook.com

Status: RESOLVED FIXED

(Web Compatibility :: Interventions, defect, P1)

Description

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

Steps to repro:

1. visit https://draftjs.org/
2. Focus in demo input
3. trigger macOS emoji IME (ctrl+cmd+space)
4. select any emoji

Error:

DOMException: Node.removeChild: The node to be removed is not a child of this node

This works fine in Firefox 92 but is broken forward (93 and 94 both break).

Note: this does not reproduce if pasting an emoji in, so I'm inclined to believe this is related to the actual input method handling, which may have broader implication for international users.

I know there are several layers here to work through Draft is built on React and there's a lot of DOM manipulation going on. The reference site uses minified code. I haven't had the time to try to reduce nor at least build an example with unminified code (or at least include sourcemaps).

In terms of web impact. Draft is used in the composer on facebook.com - the "what's on your mind" box. It's unclear what actual impact on users may be (lower marketshare, emoji keyboard may not be widely used).

Comment 1

[Karl Dubost💡 :karlcow]

Paul, Thanks for the bug and the steps to reproduce.

That's bad indeed. But it fails also in 92, 91, 90…
I'm extending the range to try to find the regression.

ok found the regression
19:47.85 INFO: Last good revision: 2f1ce60bd920913dad5dea4b4c3dabc51025c7d7
19:47.85 INFO: First bad revision: c09060ee835814453fa5475bec3e0f580fcba510
19:47.85 INFO: Pushlog:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=2f1ce60bd920913dad5dea4b4c3dabc51025c7d7&tochange=c09060ee835814453fa5475bec3e0f580fcba510

This is Bug 1520983

Comment 2

[Karl Dubost💡 :karlcow]

It may be that the code of draftjs is faulty or was handling a case which was similar to Bug 1520983 and now it breaks it.

Thomas,
What do you think?

Comment 3

[BugBot [:suhaib / :marco/ :calixte]]

Set release status flags based on info from the regressing bug 1520983

Comment 4

[Thomas Wisniewski [:twisniewski]]

This appears to be the same issue with DraftJS that causing similar "crashes" on Reddit's rich text ('fancy pants') editor in https://github.com/webcompat/web-bugs/issues/91682#issuecomment-956462866 so i don't think it's related to IME itself, but more likely the precise markup that the regression bug is generating for a contenteditable with eContentCommandInsertText. The error and results are similar, and so is the code:

          o ? (u = r, s = i.stateNode, 8 === u.nodeType ? u.parentNode.removeChild(s) : u.removeChild(s)) : r.removeChild(i.stateNode)

The precise STR are also similar, as I can insert an emoji in some cases, but not others (with the most frustrating case being that inserting an emoji on an otherwise-empty line causes this error and the page to become blank).

It seems that when their editor does not see the same markup being generated for contenteditable as Chrome uses (especially when <br> tags are involved), then somehow it confuses their code and they end up trying to remove a state-maintaining node that is already removed (or something along those lines).

In Reddit's case, that causes the page to reload due to what appears to be their error handler catching the error, reporting it, then giving up and reloading the page. In this case, the page simply goes blank.

So my best guess is that this is more generally related to interop issues with contenteditable; bug 1341152. Especially around the markup being generated (br tags when Chrome doesn't use them, etc). But finding out which precise combination of those bugs is the culprit seems tricky.

What do you think, Masayuki-san?

Comment 5

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

That <br> bit makes sense. I was able to refine the repro slightly. If the emoji inserted is not at the beginning of a line, then it works fine.

Comment 6

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Although I've not investigated what's going on actually, but according to the comments in this bug, sounds like that this is related to bug 503838. Our editor put <br> element if the line becomes empty or created at end of a block or text ends with collapsible white-space. The last case makes fixing bug 503838 much difficult. Once we stop appending <br> element for the last case, we also need to change normalizing white-spaces too. That's currently not managed in a place, trying to "fix" the situation during modifying the DOM tree in a couple of times per handling an edit operation. Therefore, we need to fix bug 1658699, but it will take long time of our resource...

Comment 7

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

If we'd back out bug 1520983, some web apps may become impossible to cancel user input with beforeinput event for macOS's accented character input and Emoji input. So backing it out is also risky.

Bug 1520983 was originally reported for WhatsApp, but it was fixed by the web app side before bug 1520983 gets fixed. If WhatsApp also uses Draft.js, the fix is not in the upstream.

Comment 8

[Hsin-Yi Tsai (she/her) [:hsinyi]]

S2 as it impacts a popular website.

Comment 9

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Tom, do you think if it is possible to create a site intervention to fix and/or fixing directly the draftjs?

Comment 10

[Thomas Wisniewski [:twisniewski]]

It seems plausible at first glance that we can at least improve the situation, but I'll have to really dive in and experiment before I can assess things properly.

For instance, if DraftJS is relying on easy-to-intercept events like input events, then we could potentially intercept them to rewrite the markup Firefox writes into the contenteditable editor widget, to better match the format of markup they're expecting (for instance, changing edits which add a <br> tag into ones where <div> is used instead).

The problem is that there is a LOT of potential ground to cover, so unless we're lucky there might be too many corner cases or other listeners on the page to reasonably do something like that.

Comment 11

[Thomas Wisniewski [:twisniewski]]

By the way I just stumbled on bug 1633399, wherein it seems like this has been an open issue with DraftJS which has caused similar crashes two years ago on Twitter, which was reported to them in https://github.com/facebook/draft-js/issues/2412

Comment 12

[Thomas Wisniewski [:twisniewski]]

So I took a look at this crash again out of morbid curiosity. This is the code which fails (in https://draftjs.org/main.429b13eb.js):

o ? (u = r, s = i.stateNode, 8 === u.nodeType ? u.parentNode.removeChild(s) : u.removeChild(s)) : r.removeChild(i.stateNode)

That line is called twice upon an emoji being inserted with the STR (on an empty line), in both Chrome and Firefox:

1: <div class="DraftEditor-root">
      .parentNode.removeChild(
         <div class="class="public-DraftEditorPlaceholder-root public-DraftEditorPlaceholder-hasFocus">)
2: <span data-offset-key="#####-0-0">
      .parentNode.removeChild(<br data-text="true">)

So Chrome is also adding a <br> node which they are removing... good to know.
However, in Firefox the second line fails because that <br> node.. has a null parentNode?

That's very strange. I'll continue investigating ASAP.

Comment 13

[Karl Dubost💡 :karlcow]

Hmm I wonder what the DOM looks like for each browser just before calling a second time .parentNode.removeChild
because on WebKit, Blink, and Firefox, all of them returns a TypeError.

with

data:text/html,<!doctype><html><meta%20charset='utf-8'></html>

var test = document.body;
console.log(test);
var br = document.createElement('br');
test.appendChild(br);
var br_to_remove = document.getElementsByTagName('br')[0];
console.log(br_to_remove);
br_to_remove.parentNode.removeChild(br_to_remove);
br_to_remove.parentNode.removeChild(br_to_remove);

Comment 14

[Ryan VanderMeulen [:RyanVM]]

Broken Facebook and Reddit seems like an issue we should be bumping in priority. What can we do to get this moving forward hopefully in time for 97 now that 96 is about to ship?

Comment 15

[Hsin-Yi Tsai (she/her) [:hsinyi]]

(In reply to Thomas Wisniewski [:twisniewski] from comment #12)

So I took a look at this crash again out of morbid curiosity. This is the code which fails (in https://draftjs.org/main.429b13eb.js):

o ? (u = r, s = i.stateNode, 8 === u.nodeType ? u.parentNode.removeChild(s) : u.removeChild(s)) : r.removeChild(i.stateNode)

That line is called twice upon an emoji being inserted with the STR (on an empty line), in both Chrome and Firefox:

1: <div class="DraftEditor-root">
      .parentNode.removeChild(
         <div class="class="public-DraftEditorPlaceholder-root public-DraftEditorPlaceholder-hasFocus">)
2: <span data-offset-key="#####-0-0">
      .parentNode.removeChild(<br data-text="true">)

So Chrome is also adding a <br> node which they are removing... good to know.
However, in Firefox the second line fails because that <br> node.. has a null parentNode?

That's very strange. I'll continue investigating ASAP.

Tom, any further updates? Thank you.

Comment 16

[Thomas Wisniewski [:twisniewski]]

Yes, I just dug into this again.

It turns out that in Firefox, the event being fired during the emoji insertion is an input event (with type=insertText), while in Chrome it's a textInput event.

And in fact if I remove all references to textInput from the code using a local file override for that script, then Chrome acts as Firefox does and "crashes".

I can't quite figure out why this ultimately happens due to the minified code being super subtle, but it does seem to rely on textInput, so we could resolve this issue by adding support for textInput events in bug 903746.

Comment 17

[Thomas Wisniewski [:twisniewski]]

Actually, with Emilio's help I realized that we can also change all occurrences of textInput to beforeinput in that script, and have this line return true, and things also seem to work on Firefox:

    var Jn = C && 'TextEvent' in window && !Zn,

That line maps to this line in the BeforeInputEventPlugin React-DOM plugin: https://github.com/facebook/react/blob/cae635054e17a6f107a39d328649137b83f25972/packages/react-dom/src/events/plugins/BeforeInputEventPlugin.js#L44

// Webkit offers a very useful `textInput` event that can be used to
// directly represent `beforeInput`. The IE `textinput` event is not as
// useful, so we don't use it.
const canUseTextInputEvent =
  canUseDOM && 'TextEvent' in window && !documentMode;

So we could also reach out to React DOM and known affected sites like Reddit, in the hopes of fixing this. But my gut tells me that as long Chrome and WebKit support textInput, we'll keep running into this sort of thing.

@masayuki-san, do you have any thoughts on this?

Comment 18

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Yeah, of course, it's the best approach that React DOM fixes the bug.

It seems that we could implement the non-standard textinput event because the event interface is really simple:
https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/events/text_event.idl

However, dispatching additional event may cause:

• some damage to the performance
• incompatible data value if given data is rich text
• another incompatible issue due to running different path in other apps in the wild

So I think that unless a lot of web apps are broken, we should not implement the new non-standard event.

Could you contact React DOM people?

Comment 19

[Thomas Wisniewski [:twisniewski]]

Thanks! I will contact them, but out of curiosity, would it not be feasible to only fire the extra event if there is a listener added for those events to mitigate the first issue (and could you help explain why the other issues aren't already interop issues we ought to fix)?

I think if we're going to push to not support this event, we ought to push the other vendors to likewise remove it. Otherwise we'll just have more issues down the line until eventually we have to adopt it anyway.

Comment 20

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to Thomas Wisniewski [:twisniewski] from comment #19)

Thanks! I will contact them, but out of curiosity, would it not be feasible to only fire the extra event if there is a listener added for those events to mitigate the first issue (and could you help explain why the other issues aren't already interop issues we ought to fix)?

It's possible, but we don't count listeners strictly and the storing space is a bit field due to required a lot of instances. Therefore, the approach is used only for really expensive events such as mouseenter and mouseleave.
https://searchfox.org/mozilla-central/rev/6eab79ab46c362b0191c4dad919f4cea2d6efb40/dom/events/EventListenerManager.h#149-164

I think if we're going to push to not support this event, we ought to push the other vendors to likewise remove it. Otherwise we'll just have more issues down the line until eventually we have to adopt it anyway.

I'd be happy if we could detect whether we're running Draft.js or not. Then, we could fire textinput event only for Draft.js...

Comment 21

[Thomas Wisniewski [:twisniewski]]

I just spoke with masayuki-san to hasten making a safe call here. Here are our decisions:

1. We would like to avoid implementing textInput for now, as it's unclear if there are a lot of sites breaking like this due to it not being supported, and because beforeinput is essentially the standard version of the event anyhow.

2. I will investigate using our webcompat system addon to (effectively) do what I did in comment 17, so that the DraftJS and Reddit page both use beforeinput instead of textInput, to work around the breakage as quickly as possible.

3. I will also contact React-DOM to consider changing their code to use beforeinput instead of textInput, where it's available, to fix this. I will also check if Reddit is willing to likewise update/alter their React-DOM code.

4. If at some point we determine that textInput is simply necessary for broader interop, we should investigate if simply aliasing textinput to beforeinput is a better path forward than implementing it as its own event (maybe, maybe not).

Comment 22

[Karl Dubost💡 :karlcow]

• Issue 891444: Deprecate textInput from Chrome
• Status of textinput event? ~July 2015
• No specific bug for removing textInput on the WebKit project

Comment 23

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Thank you for the great summary and decision, Thomas(In reply to Thomas Wisniewski [:twisniewski] from comment #21)

I just spoke with masayuki-san to hasten making a safe call here. Here are our decisions:

1. We would like to avoid implementing textInput for now, as it's unclear if there are a lot of sites breaking like this due to it not being supported, and because beforeinput is essentially the standard version of the event anyhow.

2. I will investigate using our webcompat system addon to (effectively) do what I did in comment 17, so that the DraftJS and Reddit page both use beforeinput instead of textInput, to work around the breakage as quickly as possible.

3. I will also contact React-DOM to consider changing their code to use beforeinput instead of textInput, where it's available, to fix this. I will also check if Reddit is willing to likewise update/alter their React-DOM code.

4. If at some point we determine that textInput is simply necessary for broader interop, we should investigate if simply aliasing textinput to beforeinput is a better path forward than implementing it as its own event (maybe, maybe not).

It sounds good. Thank you both for the great summary and the agreed steps moving forward.

Comment 24

[Karl Dubost💡 :karlcow]

For the memory lane:

from https://github.com/facebook/react/blob/cae635054e17a6f107a39d328649137b83f25972/packages/react-dom/src/events/plugins/BeforeInputEventPlugin.js#L40-L44

// Webkit offers a very useful `textInput` event that can be used to
// directly represent `beforeInput`. The IE `textinput` event is not as
// useful, so we don't use it.
const canUseTextInputEvent =
  canUseDOM && 'TextEvent' in window && !documentMode;

The code is full of comments related to the use of textInput

this one is interesting.
https://github.com/facebook/react/blob/cae635054e17a6f107a39d328649137b83f25972/packages/react-dom/src/events/plugins/BeforeInputEventPlugin.js#L421-L425

* `beforeInput` is spec'd but not implemented in any browsers, and
 * the `input` event does not provide any useful information about what has
 * actually been added, contrary to the spec. Thus, `textInput` is the best
 * available event to identify the characters that have actually been inserted
 * into the target node.

This is not true anymore
https://caniuse.com/mdn-api_htmlelement_beforeinput_event

And there is https://github.com/facebook/react/issues/11211

Probably fixing react will go a long way to help browser vendors deprecate textInput in the long term

Comment 25

[Hsin-Yi Tsai (she/her) [:hsinyi]]

According to the decision and steps in comment 12, Web Compatibility:Desktop looks a better module to serve this. Feel free to move back if I misunderstood something.

Comment 26

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Bug 1735227 is another story of this bug. It occurs with Chinese IME on Linux whose composition string is immediately committed without composing state.

Comment 27

[Bob Hood [:bhood]]

This is a Webcompat P1. Can we get somebody assigned, please?

Comment 28

[Hsin-Yi Tsai (she/her) [:hsinyi]]

FWIW, I talked with :twisniewski two weeks ago. He was aiming for working on this in a week or two. Maybe he has updates?

Comment 29

[Thomas Wisniewski [:twisniewski]]

Yes, indeed I've just confirmed that these "crashes" on facebook.com and draftjs.org are fixed with an webcompat site intervention which maps beforeinput to textInput (and also window.TextEvent to window.InputEvent).

It may also be triggering Facebook to provide a different user-experience for the editor, though I don't use Facebook myself so I cannot be 100% sure. I'm also not sure if this will fix anything else, but it doesn't seem to be breaking anything obvious in my initial tests, so I will propose a patch, so we can dogfood it a little and determine if it's the way we want to go (and whether we want to limit it to nightly only or OSX only for the foreseeable future).

Comment 30

[Thomas Wisniewski [:twisniewski]]

Attachment: Bug 1739489 - Add a site intervention for draftjs.org and facebook.com which remaps textinput events to beforeinput, to fix 'crashes' with Mac OSX emoji picker; r?denschub

Comment 31

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

FYI: Facebook started to rollout their new editor which will replace DraftJS. So be careful when you test your addon.
https://twitter.com/trueadm/status/1472377356044099587

Comment 32

[Thomas Wisniewski [:twisniewski]]

Then I wonder if it's even worth deploying this patch, if a new editor is imminent anyway?

Comment 33

[Brian Birtles (:birtles)]

(In reply to Thomas Wisniewski [:twisniewski] from comment #32)

Then I wonder if it's even worth deploying this patch, if a new editor is imminent anyway?

It's been imminent for quite a while so I suspect it might still be useful for a few months?

(I'm more concerned about the long tail of draft.js sites that will never update to lexical since it sounds like it's an entirely different API/approach.)

Comment 34

[Thomas Wisniewski [:twisniewski]]

@Brian, I'm just not sure how defensively we can code this intervention for the future editor, given that we have no info/demos to base decisions on, and this intervention effectively "adds support" for textInput events (by essentially just aliasing them to beforeinput events).

Given that, it could end up being safe to just continue having the intervention active even when the new editor arrives, but I worry that might inspire Facebook to rely more on textInput events in the meantime (though given history here, they may just use them regardless of anything we do).

Moreover, the intervention is stuck applying to only specific sites, not the hidden long tail, as I don't know of a way to reliably detect draftJS early enough to re-route its event handlers this way. It's easy enough to add sites to the list, at least. (Or to disable it, even remotely, if it happens to break something down the line).

Those concerns don't seem like a deal-breaker to me, but before shipping this I would at least like to confirm whether we only want it to apply on MacOS, and if we want to give it some time to bake on pre-release channels (to avoid having to do any chemspill releases because I missed some non-obvious breakage on Facebook). If we want either of those things, the patch will need revision.

Comment 35

[Brian Birtles (:birtles)]

Hi Thomas,

I'm afraid that's not my call to make. I think that's up to Hsin-Yi.

I don't use a MacOS so I've never experienced the symptoms in this bug. My comments are just based on the fact that draft.js is quite a popular library on a lot of sites now (I use it in one of mine) and based on what we know about Lexical, it seems like a lot of those sites might never upgrade.

I can see that shipping the intervention might shift incentives or make testing harder. That said, I think we have an open communication channel with lexical developers and they are more able to incorporate feedback than was the case for draft.js which appears to be mostly unmaintained.

Comment 36

[garywill]

Similar bug on Linux fcitx IME: https://bugzilla.mozilla.org/show_bug.cgi?id=1756738
I tested just now: On Draft.js, fcitx first-inputting CJK character crashes web.

BTW (I'm not sure if related):

• 2017 issue https://github.com/facebook/draft-js/issues/1320
• recently(ff 96) fixed similar IME bug(fcitx) : https://bugzilla.mozilla.org/show_bug.cgi?id=1742039

Comment 37

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Yeah, it's not only for macOS. Same fix is enabled in Linux too (bug 1712269). So at least, it should be enabled on macOS and Linux.

(Technically, it's broken on all platforms using the new eContentCommandInsertText internal event, currently, it's used only on macOS and Linux.)

Comment 38

[Hsin-Yi Tsai (she/her) [:hsinyi]]

I think having this intervention approach stuck to specific websites makes sense that we should let it impact as minimum sites as possible. Note that we have other variants of this issue on twitter (see see-also) that we should cover, too. Alao, I would vote for baking this a bit on pre-releas.

Comment 39

[Hsin-Yi Tsai (she/her) [:hsinyi]]

(In reply to Karl Dubost💡 :karlcow from comment #22)

• Issue 891444: Deprecate textInput from Chrome
• Status of textinput event? ~July 2015

Usage on Chrome is still high

textInputEventOnContentEditable: 0.4% for Jan. 2022
textInputEventOnInput: > 0.5% for Jan. 2022, still growing
textInputEventOnTextArea: 0.15% for Jan. 2022

• No specific bug for removing textInput on the WebKit project

Comment 40

[Karl Dubost💡 :karlcow]

Hsin-Yi,
yes there are risks.

We have to be careful with stats on Chrome. It's a good indicator but not the only one. Sometimes the high usage can be related to code handling multiple browsers. Sometimes not. such as this code
https://github.com/feigebaobei/react-read/blob/26fc12e0fd8aec77ed82a11fa93500ae0997d1f9/packages/react-dom/src/events/plugins/BeforeInputEventPlugin.js

And yes I agree targeted interventions would probably be the best option for now.

Comment 41

[Thomas Wisniewski [:twisniewski]]

Alright, then since this seems to not be OS-specific, I will land the intervention after the code freeze/split, so we have more time to let it bake. Does that sound acceptable, Hsin-Yi?

Comment 42

[Hsin-Yi Tsai (she/her) [:hsinyi]]

(In reply to Thomas Wisniewski [:twisniewski] from comment #41)

Alright, then since this seems to not be OS-specific, I will land the intervention after the code freeze/split, so we have more time to let it bake. Does that sound acceptable, Hsin-Yi?

Yes, sounds good to me! Thanks!

Comment 43

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/05ff0c5e4132
Add a site intervention for draftjs.org and facebook.com which remaps textinput events to beforeinput, to fix 'crashes' with Mac OSX emoji picker; r=denschub,webcompat-reviewers

Comment 44

[Cristina Cozmuta (:CrissCozmuta)]

https://hg.mozilla.org/mozilla-central/rev/05ff0c5e4132

Comment 45

[Donal Meehan [:dmeehan]]

:twisniewski, I'm wondering if it is worth uplifting this to beta to get it out earlier than 100
Please consider and uplift request, and I could take it before RC build or consider it as a ridealong if it's not too risky

Comment 46

[Thomas Wisniewski [:twisniewski]]

Donal, I'm not against it, but this work-around hasn't been heavily tested yet by Facebook users, so there is no telling if it causes other, worse problems that it resolves.

If we're willing to take that risk on beta, then uplifting the patch should be relatively easy. Should we later find we need to back-out due to unforeseen issues, and we don't have another beta release to ship, we would need to send out an update to the webcompat system addon to all affected branches.

Comment 47

[Donal Meehan [:dmeehan]]

Thanks, Thomas for this information.
Considering this week is RC week and the information you provided, we should let this ride the train.

Comment 48

[Weng Xuetian]

Hi, I just wonder what about twitter.com? Current workarounds doesn’t seem to be enabled for twitter (I came from other bug duplicated this one).

Comment 50

[Thomas Wisniewski [:twisniewski]]

@Weng, pardon the delay, I somehow missed your comment. Could you provide me with some precise steps to reproduce this problem on Twitter? I can't seem to make it crash (maybe I am just not trying the same things).

Comment 51

[Hsin-Yi Tsai (she/her) [:hsinyi]]

(In reply to Thomas Wisniewski [:twisniewski] from comment #50)

@Weng, pardon the delay, I somehow missed your comment. Could you provide me with some precise steps to reproduce this problem on Twitter? I can't seem to make it crash (maybe I am just not trying the same things).

I am guessing this is the bug with the STRs that :wengxt was thinking about.
https://bugzilla.mozilla.org/show_bug.cgi?id=1735227

Comment 52

[Ryan VanderMeulen [:RyanVM]]

Is this something we should consider taking on ESR91 also or wait for the next release (ESR102) for that channel to get the intervention?

Comment 53

[Thomas Wisniewski [:twisniewski]]

RyanVM, the concern with doing so is that Facebook might update their website, and this patch could start causing more issues than it resolves. I don't suspect that the risk of that happening is too high, but Facebook is working on a site update to make this fix obsolete, so it's worth considering before we do any such uplift (as we would need to do one more update to roll it back if it happens early enough to want to fix ESR91).

Comment 54

[Ryan VanderMeulen [:RyanVM]]

Let's just leave 91 as-is and let this ride the newer train.