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

Fixed Ctrl+A focusing inline editor if it was started and ended with a list #2274

Merged
merged 7 commits into from
Aug 3, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGES.md
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ Fixed Issues:
* [#1084](https://github.com/ckeditor/ckeditor-dev/issues/1084) Fixed: Using the "Automatic" option with [Color Button](https://ckeditor.com/cke4/addon/colorbutton) on a text with color already defined sets an invalid color value.
* [#1348](https://github.com/ckeditor/ckeditor-dev/issues/1348): Fixed: [Enhanced Image](https://ckeditor.com/cke4/addon/image2) plugin aspect ratio locking uses old width and height on image URL change.
* [#966](https://github.com/ckeditor/ckeditor-dev/issues/966): Fixed: Executing [`editor.destroy()`](https://docs.ckeditor.com/ckeditor4/latest/api/CKEDITOR_editor.html#destroy) during [file upload](https://docs.ckeditor.com/ckeditor4/latest/api/CKEDITOR_fileTools_uploadWidgetDefinition.html#onUploading) throws error. Thanks to [Maksim Makarevich](https://github.com/MaksimMakarevich)!
* [#1719](https://github.com/ckeditor/ckeditor-dev/issues/1719): Fixed: <kbd>Ctrl</kbd>/<kbd>Cmd</kbd> + <kbd>A</kbd> focuses inline editor if starting and ending with a list. Thanks to [theNailz](https://github.com/theNailz)!

## CKEditor 4.10

Expand Down
3 changes: 1 addition & 2 deletions plugins/widgetselection/plugin.js
Original file line number Diff line number Diff line change
Expand Up @@ -22,10 +22,9 @@
editor.on( 'contentDom', function( evt ) {

var editor = evt.editor,
doc = editor.document,
editable = editor.editable();

editable.attachListener( doc, 'keydown', function( evt ) {
editable.attachListener( editable, 'keydown', function( evt ) {
Copy link
Member

Choose a reason for hiding this comment

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

I would be nice to provide some simple explanation comment with the issue ticket reference for this change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

TBH I don't think it is needed there, it's fairly logical that ctrl/cmd+a should be catched from within the editable. It's actually shame that we didn't catch this earlier, because in fact it didn't make sense in case of inline editors. 🙂

// Ctrl/Cmd + A
if ( evt.data.getKeystroke() == CKEDITOR.CTRL + 65 ) {
// Defer the call so the selection is already changed by the pressed keys.
Expand Down
2 changes: 1 addition & 1 deletion tests/plugins/widgetselection/integration.js
Original file line number Diff line number Diff line change
Expand Up @@ -181,7 +181,7 @@
},

fireSelectAll: function( editor ) {
editor.document.fire( 'keydown', new CKEDITOR.dom.event( {
editor.editable().fire( 'keydown', new CKEDITOR.dom.event( {
keyCode: 65,
metaKey: true,
ctrlKey: true
Expand Down
1 change: 1 addition & 0 deletions tests/plugins/widgetselection/keys.html
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
<input type="text" id="focus-trap">
57 changes: 48 additions & 9 deletions tests/plugins/widgetselection/keys.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,12 +4,36 @@
( function() {
'use strict';

bender.editor = true;
bender.editors = {
classic: {
creator: 'replace'
},
divarea: {
creator: 'replace',
config: {
extraPlugins: 'divarea'
}
}
};

// This function fires keydown event on a given element and propagates to all parents,
// including the document to mimic event bubbling.
function fireBubblingKeyEvent( element, eventData ) {
var elementsChain = element.getParents();

if ( element instanceof CKEDITOR.dom.element ) {
elementsChain.unshift( element.getDocument() );
}

for ( var i = elementsChain.length - 1; i >= 0; i-- ) {
elementsChain[ i ].fire( 'keydown', new CKEDITOR.dom.event( eventData ) );
}
}

function testKeyCombination( editor, eventData, callCount ) {
function testKeyCombination( eventHost, eventData, callCount ) {
var addFillersStub = sinon.stub( CKEDITOR.plugins.widgetselection, 'addFillers' );

editor.document.fire( 'keydown', new CKEDITOR.dom.event( eventData ) );
fireBubblingKeyEvent( eventHost, eventData );

// Test has to be async because widgetSelection.addFillers is called inside setTimeout.
setTimeout( function() {
Expand All @@ -30,17 +54,32 @@
},

'test `ctrl + a` key combination': function() {
var editor = this.editor;
this.editorBot.setHtmlWithSelection( '<p contenteditable="false">Non-editable</p><p>This ^is text</p>' );
var editor = this.editors.classic;
this.editorBots.classic.setHtmlWithSelection( '<p contenteditable="false">Non-editable</p><p>This ^is text</p>' );

testKeyCombination( editor, { keyCode: 65, ctrlKey: true }, 1 );
testKeyCombination( editor.editable(), { keyCode: 65, ctrlKey: true }, 1 );
},

'test ctrl + alt + a key combination': function() {
var editor = this.editor;
this.editorBot.setHtmlWithSelection( '<p contenteditable="false">Non-editable</p><p>This ^is text</p>' );
var editor = this.editors.classic;
this.editorBots.classic.setHtmlWithSelection( '<p contenteditable="false">Non-editable</p><p>This ^is text</p>' );

testKeyCombination( editor.editable(), { keyCode: 65, ctrlKey: true, altKey: true }, 0 );
},

// (#1719)
'test ctrl + a in the same document but outside of the editable': function() {
var eventData = {
keyCode: 65,
ctrlKey: true
},
focusTrap = CKEDITOR.document.getById( 'focus-trap' );

this.editorBots.divarea.setHtmlWithSelection( '<ul><li>Sample text</li></ul><p>Paragraph</p><ul><li>Sample ^text</li></ul>' );

focusTrap.focus();

testKeyCombination( editor, { keyCode: 65, ctrlKey: true, altKey: true }, 0 );
testKeyCombination( focusTrap, eventData, 0 );
}
} );

Expand Down
21 changes: 21 additions & 0 deletions tests/plugins/widgetselection/manual/inlinefocussteal.html
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
<h2>Divarea</h2>

<div id="editor1">
<ul>
<li>foo</li>
</ul>
<p>bar</p>
<ul>
<li>baz</li>
</ul>
</div>

<input type="text" value="init">

<script>
if ( !CKEDITOR.env.webkit || bender.tools.env.mobile ) {
bender.ignore();
}

CKEDITOR.replace( 'editor1' );
</script>
15 changes: 15 additions & 0 deletions tests/plugins/widgetselection/manual/inlinefocussteal.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
@bender-tags: 4.10.1, bug, 1719
@bender-ui: collapsed
@bender-ckeditor-plugins: wysiwygarea,toolbar,list,widgetselection,elementspath,divarea

1. Focus the text input below the editor.
1. Press `ctrl/cmd + a`.
1. Type "foo".

## Expected

The input value is now "foo".

## Unexpected

Input value remains unchanged ("init") and the focus goes into the editor, where foo gets typed.