Ckeditor5: Link creation window moves to the top-left side of a browser while nesting list item

[Mgsy]

Steps to reproduce

1. Create an ordered list with two items.
2. Select some text in second item and transform it into a link.
3. Unselect.
4. Click on this link to open link creation window.
5. Press "TAB".

Current result

Link creation window moves to the top-left side of a browser.

Other informations

OS: Windows 10, Ubuntu 16.04
Release: 0.10.0

Screenshot

[Reinmar]

I can't reproduce it any more. I think it was fixed in the current milestone. But I can't reproduce it on https://ckeditor5.github.io either, so please check it on the latest master version, @Mgsy.

[Mgsy]

@Reinmar, I checked this one again and it's still reproducable. I was able to reproduce this issue on https://ckeditor5.github.io, local build and article preset /ckeditor5-presets/tests/manual/article.html.

I recorded a screencast, I hope it would be helpful.

[Reinmar]

Confirmed.

[oskarwrobel]

I was analyzing this some time ago. My idea was to stop keystroke propagation after focusing link balloon and prevent List feature from making an indent, but this is rather not possible because both are listening on the same element and stopPropagation() can't help.

The simplest working fix I found (requires https://github.com/ckeditor/ckeditor5-ui/pull/286):

diff --git a/src/link.js b/src/link.js
index 1e08493..1a15be4 100644
--- a/src/link.js
+++ b/src/link.js
@@ -275,15 +275,7 @@ export default class Link extends Plugin {
                        //  * the selection remains in the original link element,
                        //  * there was no link element in the first place, i.e. creating a new link
                        else {
-                               // If still in a link element, simply update the position of the balloon.
-                               if ( renderSelectedLink ) {
-                                       this._balloon.updatePosition();
-                               }
-                               // If there was no link, upon #render, the balloon must be moved
-                               // to the new position in the editing view (a new native DOM range).
-                               else {
-                                       this._balloon.updatePosition( this._getBalloonPositionData() );
-                               }
+                               this._balloon.updatePosition( this._getBalloonPositionData() );
                        }
                } );

[oleq]

The story is quite complex. There are multiple issues involved and each one must be addressed separately.

The actual positioning issue

To start off, there's a bug in the Link feature that causes the link balloon to float off screen under certain conditions.

And the particular condition we're talking about is that as the List feature indents/outdents the list item, it re-renders the editing view (document), and by doing that it obsoletes the references in the ContextualBalloon#_stack. The remaining targets in the stack point to the old DOM elements, which have been replaced by the new ones upon render.

As @oskarwrobel correctly noticed, if ContextualBalloon#updatePosition is called without a specific new position after the indentation, the balloon uses the last position from the _stack and thus attaches the BalloonPanelView to an element which is no longer in DOM. That's why the balloon lands in the (0,0) of the screen.

Keystroke collision

But that's not the end of the story.

Link uses Tab to focus the form. List uses Tab to indent the lists. They can't do that at the same time because it would be super-weird.

I decided that focusing the UI has greater priority because it's a key accessibility feature. Without it, it's impossible to move the focus to the link form using just the keyboard. And besides, the user can hit Esc first to hide the link form and then hit Tab to indent the list item with a link (BTW, we must figure out some way to announce to the screen readers tag e.g. a panel is open but not focused).

Both features will remain accessible if the Link is more important. The other way around, we're losing an important piece of the Link feature. We can revisit the priorities when (if?) we implement an editor-wide keystroke to focus the floating panels (other than Tab).

And to make sure just one feature handles the keystroke some changes in the code are necessary:

1. Both features should use the same API. ATM Link uses the (Editing)KeystrokeHandler but List uses direct keydown listener to handle keystrokes. It shouldn't – it's a job for the (E)KH.
2. ATM, if a listener in the (E)KH cancels like
   js hk.set( 'Tab', ( data, cancel ) => { cancel(); } );
   it only causes preventDefault and stopPropagation in DOM. It does not prevent other Tab callbacks from executing and that needs to be fixed if the Link callback is to be executed but the List one ignored.
3. (E)KH callbacks must have priorities just like Emitter events to make sure that no matter the order plugins are loaded, Link callback will always prevail over the List.

Changes

• The https://github.com/ckeditor/ckeditor5-utils/tree/t/ckeditor5/447 branch in ckeditor5-utils introduces priorities to the KeystrokeHandler and proper keystroke cancellation (solves 2. and 3.).
• The https://github.com/ckeditor/ckeditor5-core/tree/t/ckeditor5/447 branch in ckeditor5-core aligns EditingKeystrokeHandler to the above.
• The https://github.com/ckeditor/ckeditor5-list/tree/t/ckeditor5/447 branch in ckeditor5-list introduces EditingKeystrokeHandler integration in the List feature (solves 1.).
• The https://github.com/ckeditor/ckeditor5-link/tree/t/ckeditor5/447 branch in ckeditor5-link solves:
  
  
  
  • The weird positioning of the balloon because of outdated references. I know that the fix is not super–important since the List no longer engages on Tab but we're gonna git this issue again when developing other features that use Tab key anyway.
  
  
  • The Link keyboard integration should always have greater priority than the one of the List (solves 3.).
  
  

The branch constellation in https://github.com/ckeditor/ckeditor5/tree/t/447. Have a nice review!

[oskarwrobel]

Fix in Link feature (except setting priority) is not necessary in this case because list item won't be transformed into a nested list item on Tab press while link balloon is displayed. However, it's safer to pass new position data to each updatePosition() call because this probably will save us from the future issues related to collaboration or undo.

Big ❤️ for priorities in keystroke handler, this is something that is really necessary for us.

[Reinmar]

I saw most of the changes. They look good. I hoped that the keydown:<keystroke> event will be public (and this will definitely become a followup for 1.0.0) but there are some issues with publishing it on the KH now, so we can skip it.

[Reinmar]

@oleq, could you make proper tickets and PRs for changes described in https://github.com/ckeditor/ckeditor5/issues/447#issuecomment-320698055?

Those are not that straightforward changes so it won't be good to close them like that. I don't even know now which changes were merged already.

[oleq]

@Reinmar I just split the entire issue into sub-issues. Have fun reviewing it ;-)

[Reinmar]

🎉 , all done!