Closed Bug 1625475 Opened 5 years ago Closed 5 years ago

Text cursor moving in text box on Twitter

Categories

(Web Compatibility :: Site Reports, defect, P2)

Product:
Web Compatibility
Component:
Site Reports
Type:
defect

Tracking

(firefox74 affected, firefox75 affected, firefox76 affected, firefox77 affected)

Status:
RESOLVED WORKSFORME
Tracking Flags:
Tracking Status
firefox74 --- affected
firefox75 --- affected
firefox76 --- affected
firefox77 --- affected

People

(Reporter: sir.romston, Unassigned, NeedInfo)

References

Details

(Keywords: enterprise)

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:74.0) Gecko/20100101 Firefox/74.0

Steps to reproduce:

On Twitter, when I start writing in a text box and I leave the focus of the text box for copying other text or add emoji, the cursor stay at the same place.

Actual results:

But as soon as I write or paste or insert emoji, the text is inserted at the end or the start of the text, even if the cursor is still in the middle of the text. Also, it is impossible to copy the text, or to delete the text with the Delete key or the backspace key.

Expected results:

The only workaround I have found is I have to put the focus on the URL bar of Firefox and then give the focus back on the text box. But the problem is recurring very often. It could happen 3-4 times or the same text I write.

The bug doesn't occur on Chrome or IE.

I've tried disabling all plugins and reinstalling a fresh installation of Firefox and the bug is still occurring.

I can't reproduce. Could you please attach a screenshot or screencast?

Flags: needinfo?(sir.romston)

With pleasure, here it is: https://screencast-o-matic.com/watch/cYetoVy8r9

Flags: needinfo?(sir.romston)

I can reproduce it, but only when I add en emoji.

uncaught exception: Object is logged in the console. Debugging this will be a nightmare because their JS in unreadable.

Hi,

Thanks for the details. I was able to reproduce on windows 10 pro, on the following versions

Firefox Nightly version74.0a1 (2020-01-07) (64-bit),
Beta 73.015 (64-bit)
Release 72.0 (64-bit)

I will move this over to a component so developers can take a look over it. If is not the correct component please feel free to change it to an appropriate one.

Thanks for the report.

Best regards, Clara.

Component: Untriaged → Keyboard Navigation
Status: UNCONFIRMED → NEW
Ever confirmed: true
Component: Keyboard Navigation → DOM: UI Events & Focus Handling
Product: Firefox → Core

Hey Mike, would your team be able to help initial investigation according to comment 4? Thanks!

Flags: needinfo?(miket)
Whiteboard: [wfh]

Tom, can you take a look at this one?

Flags: needinfo?(miket) → needinfo?(twisniewski)

This is bizarre. I was able to reproduce this once by simply adding an emoji in their UI, and then it wouldn't let me click prior to the emoji until it was removed. Then I opened Chrome to compare behavior, and those steps did not reproduce anymore. Instead, I had to delete the emoji, and then their editor lost track of where my cursor ought to be. And when I deleted all of the text, suddenly the page went blank and showed me a "something went wrong, but don't fret - it's not your fault" message. Then I tried those steps again, and this time I couldn't even delete the text I had highlighted spanning the emoji, or delete text by pressing the delete/backspace keys, just by replacing it with other letters/text. And when I did that, I finally saw a console error:

Error: Got unexpected null or undefined 4 bundle.RichTextCompose.d800a4e4.js:formatted:14143
    exports bundle.RichTextCompose.d800a4e4.js:14143
    exports bundle.RichTextCompose.d800a4e4.js:5285
     ...
    <anonymous> main.317027f4.js:1

But their code is quite complex, and I'll have to take another look with a clearer mind.

It seems that it has something to do with inserting an emoji, which causes the markup in the editor to stop being a single <span> with the text, to three <spans>, with one all for the emoji (containing an SVG background-image), and leading/trailing text-spans. It does not seem to be able to cope with that situation properly in Firefox for some reason. Sometimes I can't even delete all of the text with delete/backspace if it has an emoji.

Flags: needinfo?(twisniewski)
Priority: -- → P3
Priority: P3 → P2

Oof. I'm not even seeing the "undefined exceptions" any more today, and enabling pause-on-all-exceptions in the debugger shows nothing as I reproduce the issues I mentioned before, so debugging this became even more challenging.

But I was at least reliably able to use these STR today:

  1. Begin a tweet, and enter a few characters of text ("asdf" or something).
  2. Leaving the caret where it is, use the Twitter emoji picker to insert any emoji.
  3. Click something to hide the emoji picker.
  4. Click in the middle of the text in step 1 to move the caret.
  5. Type text, and notice the editor actually inserts it after the emoji at the end.

First off, it's worth noting that they're using DraftJS and its Emoji Plugin (or perhaps customized versions).

I also noticed that Twitter was changing the scripts as I was testing, so they are likely working on this UI right now (hopefully good timing?).

After staring at the code and events for a while, I noticed that when I click around normally, the internal DraftJS method _getCaret is normally called, but it is not called just after I have added an emoji and dismissed the emoji-picker. That explains why it would not see the caret where it actually is, and place text in the wrong place later. And it also explains why de-focusing the editor and re-focusing it would fix things, since it checks the caret's position when that happens.

This is the stack trace when I can successfully click on the text to move the caret properly:

    bundle.RichTextCompose.c0f21054.js:5702:9 (the line `return e.getSelection().getStartOffset()` in `_getCaret`, fwiw)
    t bundle.RichTextCompose.c0f21054.js:5702
    t bundle.RichTextCompose.c0f21054.js:5724
    t bundle.RichTextCompose.c0f21054.js:10396
    t shared~bundle.DirectMessages.78b4f784.js:1
    t bundle.RichTextCompose.c0f21054.js:330
    exports bundle.RichTextCompose.c0f21054.js:3777
    _buildHandler bundle.RichTextCompose.c0f21054.js:384
    ta vendors~main.e0482f54.js:20234
    unstable_runWithPriority main.4b48df44.js:10
    ta vendors~main.e0482f54.js:20233
    unstable_flushControlled vendors~main.e0482f54.js:20752
    _buildHandler bundle.RichTextCompose.c0f21054.js:383
    a vendors~main.e0482f54.js:15330
    p vendors~main.e0482f54.js:15349
    O vendors~main.e0482f54.js:15394
    O vendors~main.e0482f54.js:15403
    C vendors~main.e0482f54.js:15424
    P vendors~main.e0482f54.js:15416
    R vendors~main.e0482f54.js:15472
    kn vendors~main.e0482f54.js:17019
    Ia vendors~main.e0482f54.js:20396
    ze vendors~main.e0482f54.js:15818
    Cn vendors~main.e0482f54.js:17053
    Fa vendors~main.e0482f54.js:20418
    unstable_runWithPriority main.4b48df44.js:10
    Fa vendors~main.e0482f54.js:20417
    Tn vendors~main.e0482f54.js:17034
    fBound eval:1543
    onEvent eval:760
    proxy eval:728
    (Async: EventListener.handleEvent)

It does not get past the _buildHandler line if I have just finished adding an emoji, clicked away the picker, and clicked to change the caret's location. Again, Firefox still actually ends up putting the caret in the "right" place, but same code above the trace is never called.

That's because when it's working, it ends up ultimately calling the editor's onMouseUp handler, followed by its onSelect handler. However, after using the emoji picker, only the onMouseUp handler is called, not the onSelect one, so it never realizes the caret is moving. Funny enough, onMouseUp itself is defined as a no-op:

    m = r.isBrowser('Chrome') ? g : function (e) {},

So it's the code wrapping around it in _buildHandler which must be keeping it from getting further.

However, upon noticing that Chrome-sniff, I immediately tried modifying that line so that Firefox receives the same value for m/onMouseUp as Chrome, and I'm not able to reproduce using the STR above anymore. And looking at the mouse-up handler that Chrome gets, I think that makes sense:

function (e) {
      if (e._blockSelectEvents || e._latestEditorState !== e.props.editorState) {
        if (e._blockSelectEvents) {
          var t = e.props.editorState.getSelection();
          r.logBlockedSelectionEvent({
            anonymizedDom: 'N/A',
            extraParams: JSON.stringify({
              stacktrace: (new Error).stack
            }),
            selectionState: JSON.stringify(t.toJS())
          })
        }
      } else {
        var n = e.props.editorState,
        s = a(n, o(e)),
        c = s.selectionState;
        c !== n.getSelection() && (n = s.needsRecovery ? i.forceSelection(n, c)  : i.acceptSelection(n, c), e.update(n))
      }
    }

It also looks like the main DraftJS repository is still doing the same thing:

const selectionHandler: (e: DraftEditor) => void = isChrome
  ? onSelect
  : e => {};

  // and later,
  onMouseUp: selectionHandler,

So I fear this is likely affecting other sites using DraftJS with their emoji picker. And it doesn't look like it's an interop issue, but perhaps a case where they were working around an interop issue, and now that work-around is no longer needed because Firefox now matches Chrome's behavior?

It's hard for me to tell, but at least the fix itself is easy enough, if we can reach out to DraftJS and any site known to be using it.

(The code in question was in this script on Twitter's site while I was testing, in case that helps pin-point it more quickly).

https://github.com/facebook/draft-js/commit/aed35d2269f1fda412befb3b9d7489f529641eb7

Masayuki have we made changes to when the select element is fired recently?

Flags: needinfo?(masayuki)

I don't know. But this sounds like that it's similar to bug 1623499, but it's reproducible even with older Firefox. If they were caused by same thing, some common framework might have been regressed.

Smaug, did you reviewed something which may changed selectionchange event timing? (Mirko is in PTO, unfortunately...)

Flags: needinfo?(masayuki) → needinfo?(bugs)

I don't recall any changes to selectionchange timing recently.

Does this reproduce on 68?

Flags: needinfo?(bugs)

(In reply to Olli Pettay [:smaug] from comment #13)

I don't recall any changes to selectionchange timing recently.

Does this reproduce on 68?

It's reproducible for me on 68 and 67 on Win10, following Thomas' STR in comment 10.

Hello! I help maintain Draft.js. Hsin-Yi reached out and brought this to my attention. For context, this is the code pointed out above. I'm linking to the source for easier reference:

DraftEditorEditHandler.js:

// ... imports
const onSelect = require('editOnSelect');

const isChrome = UserAgent.isBrowser('Chrome');

const selectionHandler: (e: DraftEditor) => void = isChrome
  ? onSelect
  : e => {};

const DraftEditorEditHandler = {
  // ... other handlers.
  onSelect,
  // In certain cases, contenteditable on chrome does not fire the onSelect
  // event, causing problems with cursor positioning. Therefore, the selection
  // state update handler is added to more events to ensure that the selection
  // state is always synced with the actual cursor positions.
  onMouseUp: selectionHandler,
  onKeyUp: selectionHandler,
};

(Source for editOnSelect.js).

As noted by the comment and the relevant commit, adding this selection handler to the mouseup and keyup events triggers redundant state updates, although it doesn't cause any bugs. I can easily make Firefox run the same behaviour as Chrome.

I am curious though— is this behaviour that Chrome exhibited (an now Firefox does too) in which onSelect isn't firing as much as it did, the correct behaviour?

In my understanding, the onSelect listens to selectionchange event rather than select event because select event is fired only when the focused element is a form control (meaning won't be fired on contenteditable editor).

I added a selectionchange listener to the document of Twitter, and I tried reproducing this with the STR in comment 10. Then, I confirmed that selectionchange event is fired as expected both on Firefox (Nightly) and Chrome (Release) when I click middle of the text node. So, it might be a bug of React if the onSelect is not called as expected...

According to the investigation, this is the problem on the website. Moving the component.
Though we can have an easy tweak on DraftJs, Masayuki suggested we talk with React first as that seems where the undesired behavior starts (see comment 16).

NI mr.kev so that he gets the notification about comment 16.

Component: DOM: UI Events & Focus Handling → Desktop
Product: Core → Web Compatibility
Version: 74 Branch → unspecified
Flags: needinfo?(mr.krzych00)

NI mr.kev so that he gets the notification about comment 16.

(just fixing the minor typo in the needinfo)

Flags: needinfo?(mr.krzych00) → needinfo?(mr.kev)

(In reply to Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 5/3 ~ 5/6) from comment #16)

In my understanding, the onSelect listens to selectionchange event rather than select event because select event is fired only when the focused element is a form control (meaning won't be fired on contenteditable editor).

I added a selectionchange listener to the document of Twitter, and I tried reproducing this with the STR in comment 10. Then, I confirmed that selectionchange event is fired as expected both on Firefox (Nightly) and Chrome (Release) when I click middle of the text node. So, it might be a bug of React if the onSelect is not called as expected...

I made a small demo React demo here: https://miketaylr.com/bzla/onselect/

It seems like React's onSelect is getting called for the contenteditable element in Firefox and in Chrome. That makes me think the bug is in fact in DraftJS. I'll open a bug there then we can give Twitter a heads up.

(I would be interested in knowing what circumstances Chrome doesn't fire onselect for contenteditable... perhaps my TC is too simple).

miketaylr: Thank you for your investigation and filing the issue!

Hello, got busy with other work, but having a look at this again. I'll post a PR to make Firefox follow the same behaviour as Chrome 👍

Flags: needinfo?(mr.kev)

Submitted https://github.com/facebook/draft-js/pull/2414

I pinged some people so it gets reviewed soon— it's not too complicated so hopefully by tomorrow. Still unsure why our onSelect isn't firing as often as it should, it'll be worth having a look in the future but for now just focusing on fixing the bug. miketaylr: thanks for confirming this is not a bug in React, and thanks to everyone for pitching in to this!

I'm not able to reproduce the issue on my side. I was /edit/move cursor/copy/delete text before emoji.

Tested with:
Browser / Version: Firefox Nightly 86.0a1 (2021-01-20)
Operating System: Windows 10 Pro

Romston are you still able to reproduce it?

Status: NEW → RESOLVED
Closed: 5 years ago
Flags: needinfo?(sir.romston)
Resolution: --- → WORKSFORME
Keywords: enterprise
Whiteboard: [wfh]
You need to log in before you can comment on or make changes to this bug.