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

Research bubbling model events #8640

Closed
niegowski opened this issue Dec 10, 2020 · 5 comments · Fixed by #9023
Closed

Research bubbling model events #8640

niegowski opened this issue Dec 10, 2020 · 5 comments · Fixed by #9023
Assignees
Labels
domain:dx This issue reports a developer experience problem or possible improvement. type:task This issue reports a chore (non-production change) and other types of "todos".

Comments

@niegowski
Copy link
Contributor

Provide a description of the task

There are 3 event types that currently use a hacky way of prioritizing handlers: enter, delete, keydown (for arrow keys). Those listeners often use priority.high + [some number] (or similar) to make sure that for example handler for a list item that is inside a blockquote will be triggered before the blockquote handler.

There is an idea to introduce some sort of bubbling events over the model elements. Features could register that their handler is supposed to be executed if a given element name was encountered while bubbling up the model document tree (or custom matcher for widgets etc.)

@niegowski
Copy link
Contributor Author

Some initial research on the current state of things and how could it be replaced by bubbling (context element).

The delete event

Feature Priority Comment Context element
List High + 10 Outdent if the first position in the first list item listItem
BlockQuote High + 5 Break quote if in first empty block inside quote blockQuote
Widget type around High + 1 Fake caret handling before/after/on widget widget
Widget High Deleting before/after widget $text
Delete Normal Execute “delete” or “forwardDelete” command $root

The enter event

Feature Priority Comment Context element
List Normal Outdent if empty list item listItem
Widget type around Normal New paragraph before/after widget widget
CodeBlock Normal New line or break codeBlock
BlockQuote Normal - 10 Break quote if empty block inside quote blockQuote
Enter Low Execute “enter” command $root

Arrow keys event

Feature Priority Comment Context element
Widget type around High + 10 Fake caret widget at high priority
Typing - 2 step caret movement High + 1 (should be above WTA) $text
Widget High Selection on/off widget split to widget and $text
Table High - 10 Navigation between table cells table
Widget - prevent default High - 20 Stop and prevent default if widget is selected widget
Widget vertical navigation Normal Navigation to closest limit element $text
Jump over filler/ui element Normal   $text
Jump over to-do list checkmark Normal On left arrow press listItem
Fake selection container Lowest Prevent escaping $root

Notes:

Bubbling starting from the selected node or the deepest end of the selection and going up the model document tree.

@niegowski
Copy link
Contributor Author

niegowski commented Dec 10, 2020

Also "Jump over filler/ui element" and "Fake selection container" features are strictly related to DOM so maybe they should stay the way they are currently.

And also for "Widget - prevent default" - I'm not sure if this is needed since there is a handler in the fake selection container.

@niegowski
Copy link
Contributor Author

niegowski commented Dec 14, 2020

Changes prepared in branch: i/8137...i/8640

Examples:

// enter.js
const enterObserver = editor.editing.addObserver( EnterModelObserver );

this.listenTo( enterObserver, 'enter', ( evt, data ) => { ... } );

// blockquoteediting.js
const enterObserver = editor.editing.getObserver( EnterModelObserver );

this.listenTo( enterObserver.for( 'blockQuote' ), 'enter', ( evt, data ) => {

The delete event

Feature Priority Comment Context element
List Normal (ModelObserver) Outdent if the first position in the first list item listItem
BlockQuote Normal (ModelObserver) Break quote if in first empty block inside quote blockQuote
Widget type around Normal (ModelObserver) Fake caret handling before/after/on widget $object
Widget Normal (ModelObserver) Deleting before/after widget $text
Delete Normal (ModelObserver) Execute “delete” or “forwardDelete” command generic

The enter event

Feature Priority Comment Context element
List Normal (ModelObserver) Outdent if empty list item listItem
Widget type around Normal (ModelObserver) New paragraph before/after widget $object
CodeBlock Normal (ModelObserver) New line or break codeBlock
BlockQuote Normal (ModelObserver) Break quote if empty block inside quote blockQuote
Enter Normal (ModelObserver) Execute “enter” command generic

Arrow keys event

Feature Priority Comment Context element
Fake selection container Highest Prevent escaping (prevent default)  
Typing - 2 step caret movement Normal (ModelObserver) (should be above WTA) $text @ highest
Widget type around Normal (ModelObserver) Fake caret $object @ high and $text @ high
Widget Normal (ModelObserver) Selection on/off widget $object and $text
Table Normal (ModelObserver) Navigation between table cells table
Widget - prevent default Normal (ModelObserver) Stop and prevent default if a widget is selected $root
Widget vertical navigation Normal (ModelObserver) Navigation to closest limit element $text
Jump over to-do list checkmark Normal (ModelObserver) On left arrow press listItem
Jump over filler/ui element Low    
Fake selection container Lowest Handle selection move  

@Reinmar Reinmar added the domain:dx This issue reports a developer experience problem or possible improvement. label Dec 22, 2020
@Reinmar
Copy link
Member

Reinmar commented Jan 14, 2021

There's ADR in https://github.com/cksource/ckeditor5-internal/pull/542/files, but I'd like to comment here to keep it in one place.

I like the general idea to use some sort of bubbling mechanism to execute listeners. The analysis that you made is super helpful for people to understand the scale of the problem and how a particular solution will solve it.

However, I have strong doubts regarding the API.

We've got view listeners for now and we use them. They are not perfect (as most abstractions), but they do the job (until they are too many of them, naturally). One thing is clear, though, and this is – if I need to listen to some user interactions I should use a view listener. Actually, I'm 95% correct because there's also the KeystrokeHandler. This is an alternative method for listening to certain keyboard interactions. And it's confusing. Should I use it or a view listener when listening for the Enter key? We've got a rule to only use it for feature keystrokes like Ctrl+B, Ctrl+I that map nicely to a command (but we're inconsistent with this anyway).

Adding a new mechanism that's going to work on model elements will have an even bigger impact on confusing people. Especially that we'd arbitrarily set a list of events that bubble.

Therefore, I'd prefer if we didn't introduce something new but rather find the means to implement this mechanism within the current observers. Also, we'd need a deep-dive guide covering "Listening to user interactions" because I feel that we'll be crossing some sort of complexity line here whatever we'd choose to do.

@Reinmar Reinmar modified the milestones: iteration 39, iteration 40, nice-to-have Jan 22, 2021
@niegowski
Copy link
Contributor Author

@Reinmar I updated the ADR and the branch to use the view tree instead of the model for bubbling and it looks really clean and nice. Please have a look.

@niegowski niegowski modified the milestones: nice-to-have, iteration 40 Feb 11, 2021
@Reinmar Reinmar modified the milestones: iteration 40, iteration 41 Feb 28, 2021
Reinmar added a commit that referenced this issue Mar 8, 2021
Feature (engine): Introduced bubbling of `view.Document` events, similar to how bubbling works in the DOM. Bubbling allows listening on a view event on a specific kind of an element, hence simplifying code that needs to handle a specific event for only that element (e.g. `enter` in `blockquote`s only). Read more in the documentation: **\[TODO\]**. Closes #8640.

Feature (engine): Introduced `ArrowKeysObserver`. See #8640.

Fix (utils): The `EmitterMixin#listenTo()` method is split into listener and emitter parts. The `ObservableMixin` decorated methods reverted to the original method while destroying an observable.

Other (typing): The `TwoStepCaretMovement` feature is now using bubbling events. Closes #7437.

BREAKING CHANGE: We introduced bubbling of `view.Document` events, similar to how bubbling works in the DOM. That allowed us to reprioritize many listeners that previously had to rely on `priority`. However, it means that existing listeners that use priorities may now be executed in a wrong moment. The listeners to such events should be reviewed in terms of when they should be executed (in what context/element/phase). You can find more information regarding bubbling in the documentation: **\[TODO\]**. See #8640.

Internal (widget): The enter, delete and arrow key events handling moved to the usage of the bubbling observer.

Internal (block-quote): The enter and delete events handling moved to the usage of the bubbling observer.

Internal (code-block): The enter event handling moved to the usage of the bubbling observer.

Internal (list): The enter and delete events handling moved to the usage of the bubbling observer.

Internal (table): The arrow keys handling moved to the usage of the bubbling observer.
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. type:task This issue reports a chore (non-production change) and other types of "todos".
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants