Skip to content
This repository has been archived by the owner on Jun 26, 2020. It is now read-only.

Arrow keys handling #17

Merged
merged 9 commits into from
Dec 14, 2016
Merged

Arrow keys handling #17

merged 9 commits into from
Dec 14, 2016

Conversation

szymonkups
Copy link
Contributor

@szymonkups szymonkups commented Dec 12, 2016

Fixes ckeditor/ckeditor5#5038. Please review after closing: ckeditor/ckeditor5-engine#717.

//
// @param {module:engine/model/selection~Selection} modelSelection
// @returns {module:engine/model/element~Element|null}
function getSelectedElement( modelSelection ) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was wondering whether this should stay here or be moved as model's selection method. We have similar one in view's selection.

Copy link
Member

Choose a reason for hiding this comment

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

Makes sense moving it there. But it needs to have the same semantics like the method in the view. Are they identical?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, they have same semantics. I will create an issue in ckeditor5-engine.

@Reinmar
Copy link
Member

Reinmar commented Dec 13, 2016

All works except this case:
dec-13-2016 17-33-06

@szymonkups
Copy link
Contributor Author

szymonkups commented Dec 14, 2016

I've fixed issue from previous comment and added test checking it. But I noticed one thing - when data is loaded and image is the first element in the document filler is added before it. This is probably an engine issue, could you confirm?

@Reinmar
Copy link
Member

Reinmar commented Dec 14, 2016

I've fixed issue from previous comment and added test checking it. But I noticed one thing - when data is loaded and image is the first element in the document filler is added before it. This is probably an engine issue, could you confirm?

Hm... Hard to tell. The _getDefaltRange() method is called couple of times when content is being loaded. It's last calls seem to return a range containing the image:

image

So, I think that this is a bug in the engine. I'll report a ticket for it, but please change this sample so image isn't the first element in the root.

@Reinmar
Copy link
Member

Reinmar commented Dec 14, 2016

const modelDocument = this.editor.document;
const schema = modelDocument.schema;
const modelSelection = modelDocument.selection;
const objectElement = getSelectedElement( modelSelection );
Copy link
Member

Choose a reason for hiding this comment

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

Should use the new engine function

const objectElement = getSelectedElement( modelSelection );

// if object element is selected.
if ( objectElement && schema.objects.has( objectElement.name ) ) {
Copy link
Member

Choose a reason for hiding this comment

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

This code has nothing to do with widgets. This is interesting, cause it means that we can move all that to the engine easily. That can be a good option. Perhaps it will be easily doable to have basic widget constructs in the engine. However, as a what? Addition to the editing controller? Maaaybe. But it's so much unnecessary code that I don't know ;|.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's a valid point. We can make a discussion about it when we will extracting widgets functionalities from image plugin.

@Reinmar Reinmar merged commit 006a433 into master Dec 14, 2016
@Reinmar Reinmar deleted the t/15 branch December 14, 2016 15:50
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
2 participants