[P-179] Can't toggle marks

[reinvanimschoot]

Description

When you select text and add a mark (such as bold), deselect and then select the same text again and try to toggle the mark off, it doesn't allow you to do so.

Steps

This can be reproduced on the official sandbox:

https://platejs.org

• Selecting a word and toggle it as bold.
• Then deselect the word (put the cursor somewhere else).
• Select the bold word again.
• You will notice the toolbar button does not light up blue (i.e. it does not recognize the selected word as being bold).
• When you press the bold button again (in an attempt to remove the bold mark) it does not do anything so you are stuck with bold text.

Expectation

Toggling marks works as expected.

Environment

• plate: 18.1.0
• slate: 0.82.1
• slate-react: 0.83.0
• browser: firefox

_P-179

Funding

• You can sponsor this specific effort via a Polar.sh pledge below
• We receive the pledge once the issue is completed & verified

[zbeyens]

Thanks for the issue, can confirm it doesn't happen on Chrome

https://user-images.githubusercontent.com/19695832/192834677-fcac6b13-f014-479d-86f4-ad6d08f4c07b.mp4

[mmourafiq]

I think OP is using double-click instead of just moving the caret over the bold text. The issue happens because the double click / selection also includes spaces before and / or after the text with the mark.

[caifara]

Just confirming the issue is with Firefox. Both Chrome and Safari handle this as expected.

[12joan]

Here's a visualisation of what's going on anchor and focus-wise.

Chrome:

one <b>|two|</b> three four
       ^   ^
       |   Focus
       Anchor

Firefox:

one |<b>two|</b> three four
    ^      ^
    |      Focus
    Anchor

In the latter case, toggleMark incorrectly determines that the mark is inactive, so it tries to add it rather than remove it. This is ultimately because Editor.marks returns {} rather than { bold: true } when the anchor is in an adjacent text node.

The chain of function calls in Plate is toggleMark > isMarkActive > getMark > getMarks > Editor.marks.

We can attempt to fix this either by fixing Editor.marks in Slate, or by writing some custom logic to determine whether a given mark is present in the selected range, bypassing Editor.marks entirely. Does anyone have a preference?

Related Slate bug

Edit: Looks like this PR on Slate might provide the fix we need. It's been in "Changes requested" since May, though. What's the etiquette on adopting someone else's PR? 🙂

[zbeyens]

The idea is to toggle on the mark if there is at least one character without that mark, which is not the case here. So getMarks could check that condition on top of Editor.marks. But it would be better to first fix the double click bug and come back on this one if we find another case triggering this bug.

As the PR's author is inactive, you can fork his branch and @dylans will be happy to release it asap.

[12joan]

I've created a PR in Slate

The idea is to toggle on the mark if there is at least one character without that mark, which is not the case here. So getMarks could check that condition on top of Editor.marks. But it would be better to first fix the double click bug and come back on this one if we find another case triggering this bug.

In its current form, I believe Editor.marks only returns the set of marks for the first text node of the selection, behaviour that is documented in the tests. That could lead to some false negatives, as in the following block:

<text bold italic>|one </text><text italic>two|</text>

Here, Editor.marks would return { bold: true, italic: true }. If we want to determine whether any selected text doesn't have a given mark, we would want a function that returns { italic: true } as the union of the marks for all selected text nodes.

The logic for that function might look something like this: (edited)

```ts const [firstMatch, ...restMatches] = Array.from(Editor.nodes(editor, { match: Text.isText })) if (!firstMatch) return {} const marksForNode = ({ text, ...rest }) => rest const union = { ...marksForNode(firstMatch) } restMatches.forEach(match => { const matchMarks = marksForNode(match) for (const [k, v] in union) { if (!matchMarks[k]) { union[k] = false } } }) return union ```

We can discuss this more in a dedicate issue, perhaps.