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

There should be some way exclude DOM events fired by certain UIElements from default handlers #4600

Closed
oleq opened this issue Apr 4, 2019 · 14 comments · Fixed by #8243
Closed
Assignees
Labels
domain:dx This issue reports a developer experience problem or possible improvement. intro Good first ticket. package:widget support:2 An issue reported by a commercially licensed client. type:feature This issue reports a feature request (an idea for a new functionality or a missing option).

Comments

@oleq
Copy link
Member

oleq commented Apr 4, 2019

Problem

ATM the widget takes over the DOM events like mousedown (also keydown, delete) and processes them as it thinks fit.

I realized when implementing #1509 that this behaviour causes some issues. For instance, because mousedown is handled by the widget, I wasn't able to create any dynamic form elements (<select>) using React inside a UIElement in the widget. They render, that's alright, but you can't click them, focus etc. because widget wants to handle everything.

What I thought was a solution

Unfortunately the problem is not as simple as "remove custom [event] handling when even target is UIElement or a descendant" because by doing this, we'd prevent widget selection when an UIElement is clicked, for instance a media widget uses UIElement to display the YouTube preview, and we definitely want clicking it to focus the widget.

I also tried to attach a native DOM mousedown listener on the <select> I rendered, and stop propagation of the event before it reaches the editable, before CKEditor's widget logick kicks in. I would make sense except that... React uses a single global listener for each event and it heavily depends on the fact that the event does bubble towards the root of the application. Stopping it like that simply breaks React so it's a dead–end.

An actual solution

What I guess we need is some API to tell the widget that a particual HTMLElement should be excluded from custom event handling.

How do we do that?

cc @jodator @Reinmar @mlewand

@jodator
Copy link
Contributor

jodator commented Apr 4, 2019

Things to remember that not all blame is on Widget side AFAIR there were other features that made <select> inside UIElement not usable.

The solution is to make some events transparent to CKEditor since we cannot call evt.stopPropagation().

First thought (looks kinda bad but kinda OK at the same moment): decorate event with some uniqally enough property (Symbol are still a thing?):

import {
    makeEventTransparentForCKEditor
} from '@ckeditor/ckeditor5-utils/dom/transparentevents';

domElement.addEventListener( 'focus', ( ev t) => {
    makeEventTransparentForCKEditor( evt );
}, { capture: true } );

// let's say something like this;
makeEventTransparentForCKEditor( evt ) {
    evt.__ckEventPreventDefault = true; // or evt[ eventSymbol ] = true;
};

or even:

import { hideEventsFromTheEditor } from '@ckeditor/ckeditor5-utils/dom/transparentevents';

hideEventsFromTheEditor( domElement );

The above will add previous listener for all events that are handled by CKEditor ('click', 'mousedown', etc).

The editor features will skip such events or maybe event better our DomEmitterMixin could skip events from firing if this symbol is set.

@mlewand mlewand transferred this issue from ckeditor/ckeditor5-widget Oct 9, 2019
@mlewand mlewand added this to the nice-to-have milestone Oct 9, 2019
@mlewand mlewand added status:confirmed type:feature This issue reports a feature request (an idea for a new functionality or a missing option). package:widget labels Oct 9, 2019
@lslowikowska lslowikowska added the support:1 An issue reported by a commercially licensed client. label Dec 17, 2019
@Reinmar Reinmar added squad:red domain:dx This issue reports a developer experience problem or possible improvement. labels Jun 19, 2020
@Reinmar
Copy link
Member

Reinmar commented Jun 19, 2020

Related issue: #4465. The workaround posted there may actually be resolving the issues mentioned in this ticket.

@Reinmar
Copy link
Member

Reinmar commented Jun 23, 2020

Idea: Use data-ck-ignore-events on such an element.

@Reinmar Reinmar self-assigned this Jun 23, 2020
@Reinmar Reinmar modified the milestones: nice-to-have, iteration 34 Jun 23, 2020
@Reinmar
Copy link
Member

Reinmar commented Jun 24, 2020

I pushed https://github.com/ckeditor/ckeditor5/compare/i/4600 with a prototype and it works just fine. Better than I thought.

@Reinmar
Copy link
Member

Reinmar commented Jun 24, 2020

And I found a better workaround than what we had in #4465:

[
    'click', 'mousedown', 'mouseup', 'mousemove', 'paste', 'cut', 'copy', 'drop', 'focus', 'blur', 'beforeinput', 'keydown', 'keyup'
].forEach( eventName => {
    editor.editing.view.document.on( eventName, ( evt, data ) => {
        if ( data.domTarget.matches( '[data-cke-ignore-events], [data-cke-ignore-events] *' ) ) {
            evt.stop();
        }
    }, { priority: 999999999 } );
} );

It's still not as reliable as what we can introduce inside observers (e.g. does not cover focus or selection, although, I don't know how important are those), but it seems to work in cases that I tested.

@oleq
Copy link
Member Author

oleq commented Jul 6, 2020

but it seems to work in cases that I tested.

Did you test it within a React app?

@Reinmar
Copy link
Member

Reinmar commented Jul 7, 2020

Yes, I did prototype it with a React app.

@Reinmar Reinmar modified the milestones: iteration 34, iteration 35 Jul 27, 2020
@lslowikowska lslowikowska added support:2 An issue reported by a commercially licensed client. and removed support:1 An issue reported by a commercially licensed client. labels Jul 27, 2020
@Reinmar
Copy link
Member

Reinmar commented Aug 9, 2020

I have a problem with adding support for <select> elements. <input> elements work fine when combined with data-cke-ignore-events. But for some reason, on FF only, <select> does not work – it does not open the options panel on click. No events are caught by DomObserver so the check isn't even executed.

I thought that it may be a problem with native contentEditable=true/false, but it works fine in https://jsfiddle.net/5d761g4j/. So, there's something that we do differently. Perhaps some listener that I'm not aware of or some styling that's applied in the editor. Unfortunately, this will require more thorough investigation.

@Reinmar
Copy link
Member

Reinmar commented Aug 9, 2020

Oh, I think I know the answer. The fix that I did for selectionchange is stupid. Selectionchange's target is always the document. And I've just added code that check whether evt.target is an element and ignore the event only if it is (as I could not use target.matches() when the target is the document). The check for selectionchange must be smarter there – I need to get selection's common ancestor (or focus/anchor) and do the check for that node to ignore selection change events that happen inside [data-cke-ignore-events].

@Reinmar
Copy link
Member

Reinmar commented Aug 10, 2020

Doh, blocking selection change does not have any effect on this.

I also checked that blocking the renderer does not help either. And that we don't style the <select> element in a strange way too.

Finally, I checked that DomEmitterMixin doesn't catch any other event that I might've been unaware of. 

To debug this further, we'd need to start stripping next layers of what CKEditor 5 adds over contentEditable, but this will be extremely time-consuming.

For now, <select> elements will not be therefore handled on Firefox.

@asimkt
Copy link
Contributor

asimkt commented Sep 28, 2020

@Reinmar Did you get a chance to test with <button> element? I tried adding a mouse click handler to a button and a div. Both are not working.

@mlewand
Copy link
Contributor

mlewand commented Oct 12, 2020

Regarding the documentation part:

@Reinmar Reinmar removed their assignment Oct 12, 2020
mlewand added a commit that referenced this issue Oct 20, 2020
…ll not propagate it's events to CKEditor API. Closes #4600.
@asimkt
Copy link
Contributor

asimkt commented Oct 20, 2020

🎉

@Kaviarasi
Copy link

@Reinmar , I have a div say 'xyz' inside UIElement and both has "data-cke-ignore-events" to be true. I made the div 'xyz' to be contenteditable by using native contenteditable attribute.. But it doesn't work. If I click on the div, focus is going back to editor content. Not able to click and have caret inside the div inside the UIElement.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
domain:dx This issue reports a developer experience problem or possible improvement. intro Good first ticket. package:widget support:2 An issue reported by a commercially licensed client. type:feature This issue reports a feature request (an idea for a new functionality or a missing option).
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants