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

BubblingEmitterMixin #9023

Merged
merged 52 commits into from
Mar 8, 2021
Merged

BubblingEmitterMixin #9023

merged 52 commits into from
Mar 8, 2021

Conversation

niegowski
Copy link
Contributor

@niegowski niegowski commented Feb 12, 2021

Suggested merge commit message (convention)

Feature (engine): Introducing BubblingEmitterMixin and ArrowKeysObserver. Closes #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 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].

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.


Additional information

# Conflicts:
#	packages/ckeditor5-list/src/todolistediting.js
@niegowski
Copy link
Contributor Author

Notes from F2F talk with @Reinmar:

  • We don't want to generalize the solution because currently we don't see another usage for it (we could change it in the future)
  • The name is ok because it's inside the view part of the engine code (we already have document, node, element in both view and model parts of the engine)
  • We could use dedicated BubblingEventInfo while triggering the event so we could provide a custom "start position" for the bubbling so it could start bubbling from the click position instead of the current selection
  • We will add eventPhase and currentContext to the BubblingEventInfo (as mentioned in the TODOs in code)

import { isArrowKeyCode } from '@ckeditor/ckeditor5-utils';

/**
* Arrow keys observer introduces the {@link module:engine/view/document~Document#event:arrowKey} event.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please align it a bit to match other observers' docs.


import EventInfo from '@ckeditor/ckeditor5-utils/src/eventinfo';

describe( 'EventInfo', () => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Update the name :d

view.scrollToTheSelection();
}, { priority: 'low' } );
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd keep this. We listened on low to make sure that if someone didn't pass any priority (and want to override this), that they don't have to do anything else.

Same in: shift enter, delete and perhaps somewhere else.

@@ -68,6 +68,6 @@ export default class ShiftEnter extends Plugin {

editor.execute( 'shiftEnter' );
view.scrollToTheSelection();
}, { priority: 'low' } );
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here.

if ( emitter._addEventListener ) {
emitter._addEventListener( event, callback, options );
} else {
// Allow listening on objects that do not implement Emitter interface.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Clarify that the use cases is in the tests.

if ( emitter._removeEventListener ) {
emitter._removeEventListener( event, callback );
} else {
// Allow listening on objects that do not implement Emitter interface.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Clarify that the use cases is in the tests.

@Reinmar
Copy link
Member

Reinmar commented Feb 22, 2021

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].

@niegowski
Copy link
Contributor Author

@ckeditor/qa-team could you please test it? This is a massive change in event handling. Please focus on the custom enter/delete/delete forward/arrow keys handling in the context of blockquotes, lists, to-do lists, block widgets (widget type around), tables, links (2 step caret movement)

@Reinmar
Copy link
Member

Reinmar commented Feb 23, 2021

@ckeditor/qa-team could you please test it? This is a massive change in event handling. Please focus on the custom enter/delete/delete forward/arrow keys handling in the context of blockquotes, lists, to-do lists, block widgets (widget type around), tables, links (2 step caret movement)

I'd add that it's a lot about nesting. Nesting things inside other things. Like blockquotes in tables, images in block quotes, etc. Those are the most tricky cases. But the PR could've affected also more typical ones with flat structures.

# Conflicts:
#	packages/ckeditor5-list/tests/todolistediting.js
@Mgsy
Copy link
Member

Mgsy commented Feb 23, 2021

I've tested it and haven't noticed any weird behaviours. It seems to be fine 👍

@Reinmar Reinmar merged commit 5527283 into master Mar 8, 2021
@Reinmar Reinmar deleted the i/8640 branch March 8, 2021 14:22
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 this pull request may close these issues.

Research bubbling model events 2SCM fails when a link is next to an image (widget)
4 participants