this function is a bit confusing to me, so I think I might be misunderstanding. it looks like it’s recreating the same kind of string-based HTML parsing we had before; with the assumption that < opens a tag and > closes a tag.

it also appears to traverse backwards through the HTML which is even more fraught because some text genuinely represents HTML non-text nodes in some context while not in others, something that cannot be revealed when walking backwards.

Thank you for reviewing, I'd love to improve this. This is intended to correct when the given htmlIndex is within a tag. Given:

some <strong>words</strong> test

If we receive htmlIndex = 9, then our marker would fall within the <strong> tag:

some <str\uFFFDong>words</strong> test

When we parse HTML later with create(), a marker in the HTML content itself could break parsing if it's mid-tag. The goal is to avoid inserting our marker character in a place that would break either the HTML tag or entities e.g. &am\uFFFDp;.

But you're right, this edge-case workaround still bakes in the assumption that we can per-character parse HTML without context. Ideally htmlIndex is never within a tag though. Maybe we can just ignore that case, since we're getting htmlIndex from another user's richTextOffsetToHtmlIndex() that should be correctly anchored in HTML. We also might be able to return some default value if create() returns invalid markup.

Open to any ideas if you have them!

if we’re using the marker-based approach, how are we ending up inside a tag?

the problem with using < and > like this is that is gives hardly any guarantee that it will slide in front of or behind a tag. it’s also incredibly likely that it will move markers that were already in a text node into a tag.

many definitely try to parse HTML this way. we have forty years of prior art…

if we’re using the marker-based approach, how are we ending up inside a tag?

We're receiving these markers from other peers with possibly different CRDTs, so this was an attempt to add some edge-case detection. You're correct though, this doesn't catch "edge cases" as much as it will probably make problems with regular content to a greater degree.

Thank you for the wisdom here, it's really helpful. I'm going to modify htmlIndexToRichTextOffset() not to use this method for tag detection. At very least we can remove manual parsing and just fall back to some default, or maybe we can look at the output of create() to determine if something unexpected happened.

this approach should work, but this is a curious set of characters to use, particularly U+FFFD which serves in so many common roles.

as non-characters, U+FFFE and U+FFFF should work, but also we can try any of the private-use area characters, the easiest being those starting at U+E000

for what reasons did you choose these characters?

\uFFFD is just the classic replacement character, but I wanted to add some backups in the unlikely case real content contained it. Happy to do whatever here though, would you recommend U+E000-U+E002?

the private-use characters are for special purposes like this, contained within a system. they intentionally have no meaning and should never have meaning between systems.

U+FFFD has a very specific universal meaning and a lot of texts will contain it, oftentimes by accident (because, while it should be reserved for display-only, many tools end up replacing invalid byte sequences with it anyway)

we should be careful with interval functions, especially if there’s any potential for long compute time. the problem is that they can stack up in the even queue and if the system experiences high load, it may not be able to run them fast enough to clear it out.

another available approach is to set a mean time between runs instead of schedulings

let timerHandle;
const runner = () => {
	rerenderCursorsAfterDelay();
	timerHandle = setTimeout( runner, CURSOR_REDRAW_INTERVAL_MS );
};
return () => clearTimeout( timerHandle );

the benefit here is natural deferral of these updates when the editor is laggy and decreased pressure on the main thread. the risk is that if there’s an exception in that execution context then the timer won’t be reset. this is easy to fix by creating a light-weight scheduler on setInterval() whose only purpose is to ensure that the timer is set if it hasn’t been reset.

let timerHandle;
setInterval( () => {
	if ( timerHandle ) {
		return;
	}

	timerHandler = setTimeout( () => {
		timerHandle = null;
		runner();
	}, 0 );
}, CURSOR_REDRAW_INTERVAL_MS );

and should anyone find this intermingling ugly, there are ways to separate the recursion.

const setDelayedInterval( runner, CURSOR_REDRAW_INTERVAL_MS );

Cool pattern, and makes sense to me. Implemented in 648b72a.

just noting that this function is already debounced (currently by 500ms)

just a considerably less-significant thought, but at this point is there an incredible need to predefined these? if we pick a candidate from the private-use area, of which we are almost certainly able to find one (typically the first try), we could just spin on the loop. is a count of three important?

for ( let marker = 0xE000; marker <= 0xF8FF; marker++ ) {
	if ( ! text.includes( marker ) ) {
		return marker;
	}
}

return null;

will it matter to us if we have distant markers for the start and end of the selection? (anchor and focus, of which the “start” can be after the “end” in the document for backwards selections).

if so, this candidate selection can test for pairs and there are simple schemes like even/odd to determine anchor and focus/start and end. a RegExp could even be faster, but probably isn’t.

There's no reason that we have exactly 3, used this pattern in 5a6d535 but with a limit of 0x10 guesses.

nice. you noticed and accounted for the defect in my code snippet. 👏

I figured '\uE000' wasn't exactly the same as 0xE000 but I did have to test to find out.

it does not appear to do this

but i understand it as written, maybe just update this docblock for clarity

I can see how "measures the delay" might not convey what I intended. I tried another go in 7ddd2fc!

if we're worried about an exception in the callback, why not try / catch?

This appears to work the same, with a bit less code: 0dd859f. Good idea.

Also removed the console.log in 9fefe8c.

Minor: Worth pulling this outside the component to line 12?

Done in 26631e8!