Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

web.whatsapp.com - see bug description #13503

Closed
jayne-mast opened this issue Nov 15, 2017 · 13 comments
Closed

web.whatsapp.com - see bug description #13503

jayne-mast opened this issue Nov 15, 2017 · 13 comments
Labels
browser-firefox depends-gecko issue caused by Gecko priority-critical Re-triaged Items re-triaged by Oana&Sergiu, to see which ones are still valid severity-important A non-core broken piece of functionality, not behaving the way you would expect. type-event-textInput Issues related to the non-standard textInput event.
Milestone

Comments

@jayne-mast
Copy link

URL: https://web.whatsapp.com

Browser / Version: Firefox 57.0
Operating System: Mac OS X 10.13
Tested Another Browser: Yes

Problem type: Something else
Description: selecting emoji from the Mac emoji picker adds 3 emoji instead of 1
Steps to Reproduce:
When writing a message to a contact in WhatsApp, and you select an emoji from the Mac emoji picker, it adds 3 of the same emoji in the text field. And the first of the 3 is also smaller before you post it.

I've tested it in Chrome as well, and it worked fine.
Screenshot Description

From webcompat.com with ❤️

@adamopenweb
Copy link
Collaborator

Thanks for the report @Pverbeek. I can confirm in Firefox 59 for OSX, that 3 emoji's are added from the native MacOS emoji picker. Testing in Chrome, only 1 emoji appears as expected.

Setting as needs diagnosis.

@wisniewskit
Copy link
Member

wisniewskit commented Jan 20, 2018

Something about this handler in this script is interacting strangely with the OSX emoji panel, causing the issue:

onBeforeInput: function(e, t) {
  var i = e.data,
      a = j.exec(i);
  return a ? (t.replaceSelection(a[0]), !0) : !1
},

In my tests, the emoji is already added to the comment-box before this handler is even called (resulting in the first, "smaller" copy of the emoji). Then this handler is called twice with the same emoji character (say, "X") as the value of e.data. This results in two calls to replaceSelection("X"), each of which use document.execCommand to insert the character with document.execCommand("insertHTML"):

key: "replaceSelection",
value: function(e) {
    if (!this.props.readOnly) {
        var t = this.refInput;
        this._hasFocus(t) || (G.focus(t), this.selection && this.restoreSelection(this.selection));
        var i = this.trimToFit(e);
        if (i.length) {
            this.props.multiline || (i = i.replace(/\r?\n/g, ""));
            var a = window.getSelection().getRangeAt(0);
            s(a, this.refInput) && (i = "\n" + i), a.deleteContents();
            var n = t || {},
                r = n.scrollTop,
                o = n.scrollHeight;
            if (document.execCommand("insertHTML", !1, K(i, this.plugins)), this.onSelect(), t) {
                var c = t.scrollHeight,
                    d = t.clientHeight;
                if (d >= c) return;
                t.scrollTop = (r || 0) + (c - o)
            }
        }
    }
}

And so, we end up with three copies of the emoji, one being the already-inserted "smaller" one, followed by two more <img> tags (which are the resulting HTML strings that K(i, this.plugins) call returns to document.execCommand):

X
<img class="copyable-text selectable-text b61 emoji wa" data-plain-text="X" alt="X" draggable="false" style="background-position:0px -80px;" src="data:image/gif;base64,(omitted)" data-is-emoji=true">
<img class="copyable-text selectable-text b61 emoji wa" data-plain-text="X" alt="X" draggable="false" style="background-position:0px -80px;" src="data:image/gif;base64,(omitted)" data-is-emoji=true">

This would imply two issues; one being why the smaller version appears, and the other being why the handler is called twice.

I began investigating the second issue first. It turns out that the actual event that is fired to trigger their onBeforeInput handler is compositionend. My logging indicates that only one actual call to addEventListener("compositionend") is ever made by their app, however two native compositionend events are being fired. Both events are identical, aside from having different timeStamp values (that is, they each have the same emoji character in event.data). That's why two more emoji are appearing.

This would seem to imply that Firefox is erroneously sending two compositionend events, but unfortunately a reduced test-case does not exhibit this problem:

<!doctype html><html><body>
<div contenteditable></div>
<script>
document.querySelector("div").addEventListener("compositionend", e => {
  document.body.appendChild(document.createTextNode(e.timeStamp);
});
</script>
</body></html>

This only triggers one compositionend event when using the OSX emoji-picker, and even if I alter that test-case to have it add two event similar event listeners, the timeStamp values on the compositionend events are identical (implying only one event is actually sent).

In addition the WhatsApp code does not ever seem to call EventTarget.prototype.dispatchEvent, so I'm not yet sure where that second event is coming from.

However, this still leaves the first issue, where the first, "smaller" version of the emoji is being inserted. I have not investigated this case yet, but they seem to be listening for beforeinput events as well. Firefox doesn't support beforeinput yet, so perhaps they are simply canceling that event to prevent the "smaller" version from ever appearing (though I vaguely recall that these these events cannot actually be canceled). Alternatively, perhaps Chrome simply does not add the emoji if a handler is present, or perhaps another code-path is being taken by Chrome for some reason.

I will take another stab at this after I've had some rest.

@softvision-sergiulogigan softvision-sergiulogigan added the Re-triaged Items re-triaged by Oana&Sergiu, to see which ones are still valid label Apr 26, 2018
@softvision-sergiulogigan

This issue is still reproducible on Mac OS 10.13 and Nightly 61.

@miketaylr miketaylr added the severity-important A non-core broken piece of functionality, not behaving the way you would expect. label Aug 29, 2018
@karlcow karlcow added the status-needsinfo-wisniewskit ping @wisniewskit label Oct 15, 2018
@wisniewskit
Copy link
Member

@softvision-sergiulogigan, this is no longer reproducing for me on today's nightly build on OSX 10.14.2, even on Firefox 57, so it seems that WhatsApp has fixed it on their end (or perhaps it still reproduces on older OSX 10.13 versions, but I have none with which to test).

@wisniewskit wisniewskit removed the status-needsinfo-wisniewskit ping @wisniewskit label Jan 7, 2019
@adamopenweb
Copy link
Collaborator

Still reproduces in MacOS 10.13 using Firefox Nightly for me.

@wisniewskit wisniewskit added the status-needsinfo-denschub ping @denschub label Jan 7, 2019
@wisniewskit
Copy link
Member

@denschub, would you happen to have a machine with 10.13 on it to see if this still reproduces there for you? If so, then we at least have a lead that it might be a Firefox bug.

@denschub
Copy link
Member

@wisniewskit I have upgraded to 10.13, but unfortunately, I do not have a whatsapp account, so I'm not much of a use here, sry.

@denschub denschub added status-needsinfo-wisniewskit ping @wisniewskit and removed status-needsinfo-denschub ping @denschub labels Jan 17, 2019
@wisniewskit
Copy link
Member

Funny enough, I just tried again today and the problem is still happening for me too on 10.14 (I guess I just didn't repeat the STR properly last time).

@wisniewskit
Copy link
Member

wisniewskit commented Jan 18, 2019

It turns out that the replaceSelection method from my previous comment only adds the second of the three emoji, when document.execCommand is called in that function. This is the same code that's called when WhatApp Web's own emoji picker is used, so it stands to reason it's the only one they intend to appear.

However, after the replaceSelection call is made, a second copy of the emoji is added as well, but only if the document.execCommand happens (if I comment it out, only the first small emoji appears). And since the markup is the same for both of those emoji, they must be doing the same thing twice for some unknown reason. Here is the final markup:

X
<img class="copyable-text selectable-text b61 emoji wa" data-plain-text="X" alt="X" draggable="false" style="background-position:0px -80px;" src="data:image/gif;base64,(omitted)" data-is-emoji=true">
<img class="copyable-text selectable-text b61 emoji wa" data-plain-text="X" alt="X" draggable="false" style="background-position:0px -80px;" src="data:image/gif;base64,(omitted)" data-is-emoji=true">

These results imply to me that a default action is taking place to add the small emoji, which it isn't being stopped. And indeed, it turns out that the events they're listening for in their custom onBeforeInput handler are not ones that Firefox will fire, or that can be default-prevented:

beforeInput: {
    phasedRegistrationNames: {
        bubbled: "onBeforeInput",
        captured: "onBeforeInputCapture"
    },
    dependencies: ["topCompositionEnd", "topKeyPress", "topTextInput", "topPaste"]
},

Firefox only fires composition and input events for the emoji symbols added via the OSX emoji picker, none of which can be cancelled in any effective way. On the other hand, Chrome fires no composition events at all when the OSX emoji picker is used, but instead fires beforeinput, textInput, and then input (Safari does the same, but reverses the order of the first two events). Since the first two of those are cancellable, no small emoji is added.

Unfortunately there is no trivial fix here for WhatsApp.

First off, they should be listening for the standard beforeinput event, even if they also want to handle textInput for legacy support (it was removed from the spec, and Firefox is unlikely to support it anytime soon). But even if they do listen for the standard beforeinput event, Firefox still has to implement support for it, so they're stuck.

That means WhatsApp can only really remove the extra emoji after-the-fact. This should do doable, because they won't see any compositionend events except in Firefox, and those events have a data property equal to the emoji character. So they could reasonably filter out the browser-added emoji, then replace it with their own.

...Of course, there is still that third mystery copy of the emoji. It turns out that one is caused because the events aren't prevented, and so when they try to document.execCommand("insertHTML") to insert their own, it makes Firefox send a second chain of input events, including another compositionend. This of course ends up calling the same code, which embeds the last extra emoji.

Since they can't prevent default on those events, they would have to work around it by listening for the second compositionend and ignoring it, but that might be tricky or racy, given the amount of code they have here, and I haven't been able to find a simple work-around that doesn't seem to cause some inputs to potentially be lost.

So to summarize, the interop issues here are:

  • Chrome and Safari don't fire composition events on IME-added text, just beforeinput and input.
  • Firefox doesn't support the non-standard textInput event, or the standard beforeinput variant (which WhatsApp should really be using these days).

And I'm not sure there's much WhatsApp Web can do to fix it reliably, even though it might be possible (as there are no other events fired when using the emoji picker which could be prevented, from what I'm seeing).

I've filed bz1520983 about the first, and set bz903746 up for discussion on whether Firefox needs to try again to standardize the textInput event again for webcompat.

@miketaylr, what do you think is the best course of action here? Firefox at least needs to get beforeinput implemented before this kind of "emoji replacment" approach work for WhatsApp. We might have to also look into usage counters/stats for textInput, if those are available anywhere.

@wisniewskit wisniewskit added status-needsinfo-miketaylr ping @miketaylr and removed status-needsinfo-wisniewskit ping @wisniewskit labels Jan 18, 2019
@miketaylr
Copy link
Member

It looks like 0.69% https://www.chromestatus.com/metrics/feature/timeline/popularity/830... which is pretty high.

@miketaylr
Copy link
Member

@miketaylr, what do you think is the best course of action here?

I think the bugs you filed have this covered. It would be interesting to keep an eye on textInput...

@miketaylr miketaylr added type-event-textInput Issues related to the non-standard textInput event. and removed status-needsinfo-miketaylr ping @miketaylr labels Jan 31, 2019
@miketaylr miketaylr modified the milestones: needsdiagnosis, duplicate Jan 31, 2019
@miketaylr
Copy link
Member

In the meantime... probably not much we can do here until we implement either beforeinput or textInput, so I'm going to mark this as a duplicate.

@miketaylr miketaylr added the depends-gecko issue caused by Gecko label Jan 31, 2019
@wisniewskit wisniewskit self-assigned this Feb 6, 2019
@wisniewskit
Copy link
Member

Adam and I cannot reproduce this anymore (I tested in OSX 15.5.0 on the latest nightly Firefox build; emoji only appear once for me, regardless of whether I use the command-control-space OSX emoji picker, or the WhatsApp built-in picker).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
browser-firefox depends-gecko issue caused by Gecko priority-critical Re-triaged Items re-triaged by Oana&Sergiu, to see which ones are still valid severity-important A non-core broken piece of functionality, not behaving the way you would expect. type-event-textInput Issues related to the non-standard textInput event.
Projects
None yet
Development

No branches or pull requests

8 participants