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

WidgetBuffer creates wrap opportunity in chrome #989

Closed
mrdrogdrog opened this issue Nov 6, 2022 · 19 comments · May be fixed by codemirror/view#60
Closed

WidgetBuffer creates wrap opportunity in chrome #989

mrdrogdrog opened this issue Nov 6, 2022 · 19 comments · May be fixed by codemirror/view#60

Comments

@mrdrogdrog
Copy link

mrdrogdrog commented Nov 6, 2022

Describe the issue

Hello,

We're using CodeMirror 6 in the development of HedgeDoc 2. So far I'm very impressed by what you've done so far. Great piece of work ❤️

Anyway. There is one problem. As a collaborative editor we want to show the remote cursors. For this task we use a widget (hedgedoc/hedgedoc#2957 to be precise), which causes trouble in combination with EditorView.lineWrapping. To be precise: Chrome takes the widgetBuffer as wrap opportunity because it is an img tag.

hedgedoc-2022-11-04_22.03.14.mp4

It seems like this commit is causing the issue. While I understand that this solution is maybe solving some other problems, it's causing this one which makes it unusable for us. ☹️

Quick solution idea

What about letting the widget itself choose if the buffer should be an img or a span?

Additional Context

hedgedoc/hedgedoc#2957
hedgedoc/react-client#2451

Browser and platform

Chrome

Reproduction link

No response

@tamo
Copy link

tamo commented Nov 12, 2022

I have a reproduction link without hedgedoc or remote cursors.
image
As you can see, the line is split after the <img class=cm-widgetBuffer> even though the line content is only a single long word.

@marijnh
Copy link
Member

marijnh commented Nov 12, 2022

I am aware of this, but haven't been able to find a good workaround. The zero-width space thing we had before caused a lot of trouble because, after the browser does its thing on some native edit action, it was had to distinguish the spaces that were inserted for this hack from spaces that should actually be in the document. Also I'm pretty sure (and a rough test confirms) that zero-width spaces are also used as wrap opportunities by browsers—are you sure the patch you linked made a difference here?

What we need, basically, is a bit of DOM that is A) treated as a regular editable inline element by the browser so that it avoids the slew of browser bugs around having a cursor next to an uneditable element, B) is invisible, C) is easy to distinguish from content text, and D) does not create a wrap point. If you find some obscure element or style that lets us do this, do let me know!

@marijnh
Copy link
Member

marijnh commented Nov 12, 2022

(Possibly unicode character 0x2060, "word joiner" can be useful here. But again, we will need to support such characters appearing in the document as well, and somehow tell leftovers from our widget buffers apart from inserted content.)

@mrdrogdrog
Copy link
Author

mrdrogdrog commented Nov 12, 2022

As far as you've described you just need a dummy element whose only purpose is to exist and don't influence anything, right?
Have you tried custom tags ?
You don't need any logic or lifecycle behind them for this purpose. Just place an element mit a custom name in the dom like... <codemirror-widgetbuffer/>. It's valid HTML5 because of the dash inside and if you don't define any logic then the browser should ignore it in the rendering/layouting step and it shouldn't create a wrap opportunity 🤔
It's just a quick idea. I couldn't test it myself so far.

@marijnh
Copy link
Member

marijnh commented Nov 12, 2022

Have you tried custom tags ?

Those won't be treated as significant inline text editing elements by browsers (or a <span> would also work). So the cursor will still be regarded as directly next to the uneditable element even though such an element is between them.

@mrdrogdrog
Copy link
Author

mrdrogdrog commented Nov 12, 2022

then what about an empty span?

edit: hmpf.. this is also likely to be ignored because you need actual content, right?

@mrdrogdrog
Copy link
Author

yes.. that's really tricky o.ö

@tamo
Copy link

tamo commented Nov 13, 2022

Oh...

FYI, I have just found that chrome devs also think that elements with "position: absolute" style should not introduce soft wrap opportunities (though they have left the crbug open for years). So I think this could be, theoretically, solved by "position: absolute", if only the crbug were fixed... 😢

@hubgit
Copy link
Contributor

hubgit commented Nov 15, 2022

Unfortunately even without the WidgetBuffer I think you'll probably find that any decoration which splits a word will cause a wrap opportunity.

The only way I know of to resolve this is to draw the collaborator cursors in a separate layer, as CodeMirror does for the main cursor.

@mrdrogdrog
Copy link
Author

Actually I had this problem while working on remote cursors and solved that using nbsp inside of the widget.

@mrdrogdrog
Copy link
Author

mrdrogdrog commented Nov 20, 2022

I just had another idea (thanks bathtub 🛀). How about wrapping the whole text before and after the widget buffers into editable spans and let only them do the text wrapping?

In the following example you would only allow wrapping on elements with class text.

<span class="not-wrappable">
<span class="wrappable">i'm a text</span><img>[...]<img><span class="wrappable"> after the widget</span>
</span>

@marijnh
Copy link
Member

marijnh commented Nov 22, 2022

Unfortunately even without the WidgetBuffer I think you'll probably find that any decoration which splits a word will cause a wrap opportunity.

This is a good point. This patch exports the logic used by drawSelection for external code, and may be a better fit for drawing additional cursors than widget decorations.

Could you take a look and let me know if this feature would work for you?

@mrdrogdrog
Copy link
Author

This looks interesting. I'll take a look later.

@tamo
Copy link

tamo commented Nov 26, 2022

This is a good point. This patch exports the logic used by drawSelection for external code, and may be a better fit for drawing additional cursors than widget decorations.

Could you take a look and let me know if this feature would work for you?

Looks like it is working! Thank you!
https://github.com/tamo/y-codemirror.next/blob/ce2bfbd8447a1c41727a09350d141733f9ca9714/src/y-remote-selections.js

Here are a couple of ideas I got while writing it:

  1. Maybe a hook in LayerView.destroy would be useful.
    I set a listener in mount() but the listener will never be off.

  2. measureRange may be useful if exported.
    I would have used measureCursor and measureRange if they had been exported.
    I just copied measureCursor and commented out selection markers.

marijnh added a commit to codemirror/view that referenced this issue Nov 30, 2022
FEATURE: The static `RectangleMarker.forRange` method exposes the logic used
by the editor to draw rectangles covering a selection range.

Issue codemirror/dev#989
marijnh added a commit to codemirror/view that referenced this issue Nov 30, 2022
FEATURE: Layers can now provide a `destroy` function to be called when
the layer is removed.

Issue codemirror/dev#989
@marijnh
Copy link
Member

marijnh commented Nov 30, 2022

@tamo Could you take a look and see if attached patches would cover your requirements?

@marijnh
Copy link
Member

marijnh commented Nov 30, 2022

I'm going to close this issue as wontfix, since adding content inline necessarily influences break points, and it seems really hard to control that in a way that doesn't cause more problems than it solves.

@marijnh marijnh closed this as completed Nov 30, 2022
@mrdrogdrog
Copy link
Author

mrdrogdrog commented Dec 6, 2022

Just to let you know: The layer method works fine so far. We use it right now to create a new cursor plugin that fits our use case. Thank you for your help ❤️
But it would be nice if you would create a release of codemirror/view with the exported measurement functions. :)

@marijnh
Copy link
Member

marijnh commented Dec 7, 2022

I've tagged @codemirror/view 6.7.0

@tamo
Copy link

tamo commented Dec 11, 2022

Thank you all, I'm happy to know that hedgedoc works well now.

@tamo Could you take a look and see if attached patches would cover your requirements?

Thanks for the patch. Probably it's good enough.
I tried to use them in @yjs /y-codemirror.next but I failed because I was not skilled enough.
Maybe the yjs dev will adopt the layers functionality.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants