Editor Drivers Abstraction

[askvortsov1]

This will allow drop-in replacements of the editor with a more advanced WYSIWYG solution such as ProseMirror

See #2566

[askvortsov1]

Should we provide BasicEditorDriver as a (deprecated) alias for this? All the logic from there is still there, but we'd need to include something from forum in common. Although in this case I don't think it's too bad since BasicEditorDriver doesn't import anything from the rest of forum.

[SychO9]

Since the logic is the same, and there's always a chance an extension could've used it, I don't see why not.

Interesting that it was in common though since it was only used in forum.

[askvortsov1]

This actually brings up an interesting point: it was in common, not forum, because it didn't depend on any stuff in forum. And that might be something we want to keep: TextEditor might be eventually useful for editing stuff on the admin side that uses formatting (e.g. if tags descriptions adopt formatting).

For now why don't we just add it to forum, but we can keep an eye out for this in the future? Or should we have it (and TextEditor) in common now?

[SychO9]

Agreed.

[dsevillamartin]

Not looked very deep into this, just some stuff that stood out initially.

[SychO9]

Code looks good. Tested locally.

There's one reference to SuperTextArea left in ComposerState though.

[SychO9]

Since the logic is the same, and there's always a chance an extension could've used it, I don't see why not.

Interesting that it was in common though since it was only used in forum.

[SychO9]

emoji and mentions aren't working properly, the dropdown isn't showing at the proper position we're typing in, and it errors out here with, I haven't investigated the issue

addComposerAutocomplete.js:148 Uncaught TypeError: Cannot read property 'top' of undefined
    at addComposerAutocomplete.js:148
    at addComposerAutocomplete.js:154
    at BasicEditorDriver.ts:22
    at Array.forEach (<anonymous>)

and

Uncaught TypeError: Cannot read property 'scrollTop' of undefined
    at g (addComposerAutocomplete.js:163)
    at addComposerAutocomplete.js:179
    at BasicEditorDriver.ts:22
    at Array.forEach (<anonymous>)
    at HTMLTextAreaElement.i (BasicEditorDriver.ts:21)

[askvortsov1]

You're on the editor abstraction branches of both those extensions, right? Also, could you share the test post that caused it to break?

[SychO9]

You're on the editor abstraction branches of both those extensions, right? Also, could you share the test post that caused it to break?

Yea, I'm on all the relevant dev branches, as for the test post, just triggering the dropdowns with either @ or : or directly clicking the buttons triggers the errors.

[askvortsov1]

You're on the editor abstraction branches of both those extensions, right? Also, could you share the test post that caused it to break?

Yea, I'm on all the relevant dev branches, as for the test post, just triggering the dropdowns with either @ or : or directly clicking the buttons triggers the errors.

Ah.... When I converted it to use input listeners, we lost the ability to get the event target value, which broke this. I'll change the caret position method to return position relative to viewport, since the editor driver has access to the dom/underlying mechanism (and so can figure this out), whereas the input listeners really have no clue, and so shouldn't be responsible for locating it. This will DRY the dropdown positioning code between emoji and mentions.

On that topic, the input listeners part of this is cool, but it's probably the weakest part of the new system in terms of design. Ideally it'd be nice to pass e, but that's not really possible because alternative implementations might not have an e (although currently my wysiwyg approach implements input listeners by just listening to the vanilla js/dom events. Constructing a new abstraction for an "input event" is challenging and needs a fair bit more research. The one thing I think we could do so far is provide the type of event (click, input, keypress), but even that I'm not 100% sure about. I left this out of the initial implementation because we could always add arguments later, but if there's strong sentiment towards having them now, we can try and do so.

[askvortsov1]

Actually never mind, I am no longer convinced fully DRYing away the mentions/emoji dropdown positioning code is a good idea, as it depends on the dropdown, and I don't think that should be an argument.

Fixed, you'll also need to pull in the latest version of the other packages to test.

[KyrneDev]

Played around with this for a little while from the perspective of an extension dev and I really like it. It's super easy to work with now!