Marks and Selection no longer works correctly in some cases

[mdenuit]

I didn't include a JSFiddle neither a GIF because you can reproduce the bug on slatejs.org. I'm on Debian Stretch using Chrome Version 59.0.3071.115 (Official Build) (64-bit).

I'd like to know if you have the same issue as me: I took the rich-text example to work on it, but there seems to be a bug in Slate since I have the same problems on the examples page (http://slatejs.org/#/rich-text and http://slatejs.org/#/iframes). If you click at the end of the text or at the beginning, not selecting any text, and enable bold then disable it without typing any text, Selection becomes buggy (however it's not the case if I type some text before disabling bold). Then whenever I move the cursor to a portion of text that was bold, the button does not become active anymore and if I select this portion of text and try to remove the bold mark, it does not work anymore. After reproducing this bug, other mark buttons such as italic do not become active when they should. Doing my own toggleMark() function with some() to do some debugging, there seems to be no more bold mark on the selected portion of text (which is bold of course), so it tries to add it, indefinitely.

Thank you

[danburzo]

@matmat94 Hmm, sounds like it may have something to do with the changes recently merged, https://github.com/ianstormtaylor/slate/pull/990

Unfortunately I cannot reproduce (on Chrome 61/MacOS) the issue by following (to the best of my knowledge) the steps you provided. Can you please create a short video that demonstrates the steps?

Later edit: Got it, managed to repro on my machine as well. Thanks for reporting! Seems like something has regressed...

[mdenuit]

No problem, very nice plugin btw. Here's a gif as asked.

[TimYi]

@matmat94 select a collapsed range, and then make a toggleMark change, selection will be wrong, marks of the selection is incorrect. By the way, the selection mechanism is the combination of onSelect in content model and Changes.select in on-selection model, the code is not very clear for me to understand.

[TimYi]

These codes may have something wrong, in at-current-range, line 229. When the selection is a collapsed range, it will be called.

const marks = document.getActiveMarksAtRange(selection).remove(mark)
const sel = selection.set('marks', marks)
change.select(sel)

I don't know why select the other part will still have a wrong selection after change.select(sel). in my opinion, reselect at the same point, it should show the same mark as toggleMark have not done, since change.select(sel) does not change the mark of any content.

[bengotow]

Hey folks—I've stumbled across this one as well. It looks like marks are attached to the selection (via the code above) for collapsed selections, but I think there's somewhere in the codebase where selection is being re-created and those marks are being reset (to the wrong / old ones), preventing things like toggleMark and removeMark from having any effect on collapsed ranges, but I haven't found it yet.

[bengotow]

Ok—I'm pretty confident this is still an issue and have narrowed it down.

When you're using the Slate composer within React, this code is attached to the window's selectionchange event, triggering onSelect 100ms after any selection change in the window:

    _this.onNativeSelectionChange = (0, _lodash2.default)(function (event) {
      if (_this.props.readOnly) return;

      var window = (0, _getWindow2.default)(event.target);
      var activeElement = window.document.activeElement;

      if (activeElement !== _this.element) return;

      _this.props.onSelect(event);
    }, 100);

Seems pretty reasonable to me. That runs slate-react/lib/plugins/after.js: onSelect which uses findRange to convert the browser's native selection into a Slate range, and then does this:

    range = range.normalize(document);
    change.select(range);

Changes.select doesn't check whether the current selection and new selection are the same (in my case they are, since the user is typing in the focused editor), so calling select() with a range and without marks effectively clears the marks that are being stored on the selection.

This is consistent with my experimentation - if I type /fast enough/ I can sometimes keep the mark I want by typing at least one character before the 100ms timer fires.

Based on this, I think this is specific to the React wrapper. There's logic in slate-react/components/content.js to ignore onSelect events caused by the user typing or modifying the Slate state (_this.tmp.isUpdatingSelection), but it is not catching in this case. It may be that isUpdatingSelection isn't designed for the selectionchange event to be debounced 100ms.

Update: Yup. Looks like the selectionchange event handler in question was only added a month ago: https://github.com/ianstormtaylor/slate/commit/4e021f58b5186e61d7ace2a89a3052412d8ff5e7. 💥

[bengotow]

Hey @ianstormtaylor — I'd like to fix this as far down into the core as possible to avoid similar sorts of problems (I'm actually not sure the scenario above is the only such case).

I think that in addition to bailing with a no-op if isEmpty(props), Changes.select (https://github.com/ianstormtaylor/slate/blob/master/packages/slate/src/changes/on-selection.js#L55) should probably bail if the only change is going from marks: Set to marks: null. It seems like operations that remove / toggle marks remove them from the set, so going from a selection with marks to the same selection without marks defined shouldn't ever be meaningful?

Let me know what you think and I'll submit a PR!