fix: Allow space in html with contenteditable field

[rommni]

Add a check before intercept the space event to handle case of contenteditable inside qx.ui.embed.Html()

fix #10130

[rommni]

This is a little bit scary since it's in an abstract class and could have far-reaching implications, but this looks safe AFAICT...

To be honnest i was a little scary to do changes on this file. However it seems that the edit is quite small and shouldn't have unexpected effect. If someone see a possible problem please don't mind to yell it ;)

[oetiker]

how about adding a "Backport" label to things that should seem worthy of adding to 6 as well

[oetiker]

I think we do need the patch, but it is probably a bit too early in the file ... look here https://github.com/qooxdoo/qooxdoo/blob/3f74b97fab98b9e483b21c811f4b48692c79f8c1/source/class/qx/ui/root/Abstract.js#L262 that is the place where the target is handled. So the editContent case should be handled here

[johnspackman]

I'm curious to know why other editors don't have this problem, but OTOH the example that @rommni has given is pretty distilled down to showing that a contentEditable div is not behaving as you'd expect; in the code, el is the DOM for the qx.ui.embed.Html and presumably originalTarget is the actual embedded contentEditable <div>.

However there is an issue in that originalTarget is not always the contentEditable div, for example if the cursor is in a nested child element; I changed the code to this:

var button1 = new qx.ui.embed.Html('<div contenteditable=true><p><b>Nospace!</b></p></div>');

and the originalTarget is the <b> and it has contentEditable set to inherit

It seems that the original implementation only works when the embedded element is contentEditable, because the DOM event will bubble up to the Qooxdoo widget.

Also, given that the nature of the test whether this widget is editable, it seems slightly wrong that a global handler like this is working that out by sniffing the encapsulated DOM - after all, in theory at least AIUI an editor could be implemented with a canvas tag, ie the implementation of the widget is private.

So, how about this: qx.ui.core.Widget should expose a new API method called something like isAcceptsKeyboardInput (which returns false by default) and that is what this global handler checks; that method would be refined, so qx.ui.embed.Html knows that it has to check every DOM element from originalTarget up to getContentElement looking for contentEditable==="true", whereas qx.ui.form.AbstractField sniffs depending on DOM name

[level420]

@johnspackman quilljs does not have the problem "unable to enter space" either. See https://tinyurl.com/yyda8ptu

[cboulanger]

It seems we have the following choices:

1. We leave everything as it is, but add a page in the documentation that instructs how to correctly embed a HTML editor so that the space-eating behavior does not occur.
2. We include the fix after making sure that this doesn't break anything
3. We do some more research and create a solution that fixes the root of the problem.

Plus, a qooxdoo package that exposes the editor as a qooxdoo widget (and works around the problem) is always welcome!

[oetiker]

It seems we have the following choices:

1. We leave everything as it is, but add a page in the documentation that instructs how to correctly embed a HTML editor so that the space-eating behavior does not occur.

2. We include the fix after making sure that this doesn't break anything

3. We do some more research and create a solution that fixes the root of the problem.

Plus, a qooxdoo package that exposes the editor as a qooxdoo widget (and works around the problem) is always welcome!

as pointed out in the example, there definitely IS a problem ... and even the original code is trying to handle it:

https://github.com/qooxdoo/qooxdoo/blob/3f74b97fab98b9e483b21c811f4b48692c79f8c1/source/class/qx/ui/root/Abstract.js#L264

but it leaves out the contentEditable check ... this fix at least would be appropriate in any case ... even without going all the way (like @johnspackman suggests).

[rommni]

Sorry guys not much time these days. Your comments seems interesting we will need to investigate more. I will try to do that as soon as I have some time.

[cboulanger]

@rommni Any progress on the issue? We do want to release a first beta version of v7.0.0 quite soon and it would be great to have the fix included (maybe also in a v6.0.1 maintenance release)

[hkollmann]

Should we close this?

[johnspackman]

no, leave it open for now - @rommni if it's OK with you I'd like to take over this PR? Im doing some work in that area so it might be convenient for both of us?