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

Fix disappearing undo steps after multiple selection changes #2792

Merged
merged 25 commits into from
Feb 19, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
25 commits
Select commit Hold shift + click to select a range
822686f
Add manual tests for copyformatting undo bug.
Jan 30, 2019
d66b1ec
Add fix to not refresh snapshot when there is no selected format.
Jan 30, 2019
71bf1c6
Correct manual test description, add unit test.
Feb 5, 2019
ede079b
Add changelog entry.
Feb 5, 2019
91c5095
Add issue reference, correct spelling mistake.
Feb 11, 2019
bdccfcf
Remove unreachable code.
Feb 11, 2019
a4ea15a
Extend manual test case with new scenario.
Feb 11, 2019
d20e91d
Addunit test for redo case.
Feb 11, 2019
da96980
Add redo unit tests.
Feb 11, 2019
e808244
Add proper target to event.
Feb 11, 2019
0e03a7b
Update test description, simplify manual test, change name of unit te…
Feb 12, 2019
b96338f
Add unit test.
Comandeer Feb 14, 2019
f5694fb
Add manual test.
Comandeer Feb 14, 2019
1833554
Include #2470 in tests.
Comandeer Feb 14, 2019
dcb4705
Add changelog entries.
Comandeer Feb 14, 2019
6edeceb
Add ticket reference to the code.
Comandeer Feb 14, 2019
f2f0afd
Fix unit test for IE8. Fix typo.
Comandeer Feb 14, 2019
fa90cd1
Fix code style issue.
Comandeer Feb 14, 2019
9d94ac4
Remove snapshot indicator in manual test, which works incorrectly and…
Feb 15, 2019
0d72dea
Correct unit test, to not fail on build version.
Feb 15, 2019
c05f7cd
Fix code style issues with CSS.
engineering-this Feb 16, 2019
050ad15
Remove unnecessary assert, correct helper name.
Feb 19, 2019
6e76d77
Update description of manual test.
Feb 19, 2019
d63537f
Remove referrence to unused tools, remove variables and id attributes…
Feb 19, 2019
a3361ff
Update link in changelog entry.
Comandeer Feb 19, 2019
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
3 changes: 3 additions & 0 deletions CHANGES.md
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,9 @@ Fixed Issues:
* [#2756](https://github.com/ckeditor/ckeditor-dev/issues/2756): Fixed: The [Auto Link](https://ckeditor.com/cke4/addon/autolink) plugin causes an error when typing in [Source Editing Mode](https://ckeditor.com/docs/ckeditor4/latest/guide/dev_sourcearea.html).
* [#1986](https://github.com/ckeditor/ckeditor-dev/issues/1986): Fixed: Cell Properties dialog from [Table Tools](https://ckeditor.com/cke4/addon/tabletools) plugin shows styles that are not allowed through [`config.allowedContent`](https://ckeditor.com/docs/ckeditor4/latest/api/CKEDITOR_config.html#cfg-allowedContent).
* [#2565](https://github.com/ckeditor/ckeditor-dev/issues/2565): [IE, Edge] Fixed: Buttons in the [Editor Toolbar](https://ckeditor.com/cke4/addon/toolbar) are activated by clicking them with right mouse button.
* [#2780](https://github.com/ckeditor/ckeditor-dev/issues/2780): Fixed: Undo steps disappears after multiple change of selection.
* [#2470](https://github.com/ckeditor/ckeditor-dev/issues/2470): [Firefox] Fixed: Widget's nested editable blurred upon focus.
* [#2655](https://github.com/ckeditor/ckeditor-dev/issues/2655): [Chrome, Safari] Fixed: Widget's nested editable can't be focused under certain circumstances.

## CKEditor 4.11.2

Expand Down
13 changes: 5 additions & 8 deletions plugins/copyformatting/plugin.js
Original file line number Diff line number Diff line change
Expand Up @@ -91,7 +91,8 @@
} );

editor.on( 'contentDom', function() {
var editable = editor.editable(),
var cmd = editor.getCommand( 'copyFormatting' ),
editable = editor.editable(),
// Host element for apply formatting click. In case of classic element it needs to be entire
// document, otherwise clicking in body margins would not trigger the event.
// Editors with divarea plugin enabled should be treated like inline one – otherwise
Expand All @@ -101,14 +102,13 @@
copyFormattingButtonEl;

editable.attachListener( mouseupHost, 'mouseup', function( evt ) {
if ( getMouseButton( evt ) === CKEDITOR.MOUSE_BUTTON_LEFT ) {
// Apply formatting only if any styles are copied (#2780, #2655, #2470).
if ( getMouseButton( evt ) === CKEDITOR.MOUSE_BUTTON_LEFT && cmd.state === CKEDITOR.TRISTATE_ON ) {

This comment was marked as resolved.

editor.execCommand( 'applyFormatting' );
}
} );

editable.attachListener( CKEDITOR.document, 'mouseup', function( evt ) {
var cmd = editor.getCommand( 'copyFormatting' );

if ( getMouseButton( evt ) === CKEDITOR.MOUSE_BUTTON_LEFT && cmd.state === CKEDITOR.TRISTATE_ON &&
!editable.contains( evt.data.getTarget() ) ) {
editor.execCommand( 'copyFormatting' );
Expand Down Expand Up @@ -424,10 +424,7 @@
documentElement = CKEDITOR.document.getDocumentElement(),
isApplied;

if ( !isFromKeystroke && cmd.state !== CKEDITOR.TRISTATE_ON ) {
return;

} else if ( isFromKeystroke && !copyFormatting.styles ) {
if ( isFromKeystroke && !copyFormatting.styles ) {
plugin._putScreenReaderMessage( editor, 'failed' );
plugin._detachPasteKeystrokeHandler( editor );
return false;
Expand Down
15 changes: 15 additions & 0 deletions tests/plugins/copyformatting/applyformat.js
Original file line number Diff line number Diff line change
Expand Up @@ -437,6 +437,21 @@

bender.tools.selection.setWithHtml( editor, '<table><tr><td>Ce{ll 1</td></tr><tr><td>Ce}ll 2</td></tr></table>' );
assert.areSame( tableConstant, determineContext(), 'Selection within two rows' );
},

// (#2780, #2655, #2470)
'test applyFormat not fired without copied styles': function() {
var editor = this.editor,
spy = sinon.spy( editor, 'execCommand' ),
isIe8 = CKEDITOR.env.ie && CKEDITOR.env.version < 9;

editor.document.fire( 'mouseup', new CKEDITOR.dom.event( {
button: isIe8 ? 1 : CKEDITOR.MOUSE_BUTTON_LEFT,
target: editor.editable()
} ) );
spy.restore();

assert.areSame( 0, spy.callCount );
}
} );
}() );
64 changes: 64 additions & 0 deletions tests/plugins/copyformatting/manual/nestededitablefocus.html
Original file line number Diff line number Diff line change
@@ -0,0 +1,64 @@
<style>
#square {
margin-top: 20px;
width: 100px;
height: 100px;
border: 1px #000 solid;
background-color: red;
Comandeer marked this conversation as resolved.
Show resolved Hide resolved
}
</style>
<h2>Classic editor</h2>
<div id="classic">
<div class="test1">x<div class="content">Widget1</div>y</div>
Comandeer marked this conversation as resolved.
Show resolved Hide resolved
</div>

<div id="square"></div>

<script>
CKEDITOR.addCss( '.cke_editable { margin: 0; padding: 50px; }' +
'.test1 { border: 1px #000 solid; }' );
CKEDITOR.replace( 'classic', {
extraPlugins: 'test1'
} );

CKEDITOR.plugins.add( 'test1', {
requires: 'widget',
init: function( editor ) {
editor.widgets.add( 'test1', {
button: 'Create autoparagraph test',
pathName: 'test-widget',

template:
'<div class="test1">' +
'<div class="content"></div>' +
'</div>',

editables: {
content: {
selector: '.content',
allowedContent: 'br'
}
},

allowedContent: 'div(test1,content)',
requiredContent: 'div(test1)',

upcast: function( element ) {
return element.name == 'div' && element.hasClass( 'test1' );
},

init: function() {
var square = CKEDITOR.document.getById( 'square' );

this.editables.content.on( 'focus', function() {
square.setStyle( 'background-color', 'green' );
} );
this.editables.content.on( 'blur', function() {
square.setStyle( 'background-color', 'red' );
} );
}

} );
}
} );
</script>
17 changes: 17 additions & 0 deletions tests/plugins/copyformatting/manual/nestededitablefocus.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
@bender-tags: bug, 4.11.3, 2655, 2470
@bender-ui: collapsed
@bender-ckeditor-plugins: widget, wysiwygarea, copyformatting

1. Click inside "Widget 1" text to focus nested editable.
2. Click above the widget (above the black border).
3. Click once more inside "Widget 1" text.

## Expected

* Focus is moved into nested editable and stays there.
* Square under the editor is green.

## Unexpected

* Focus is moved into nested editable for a fraction of second and then disappears.
* Square under the editor is red.
16 changes: 16 additions & 0 deletions tests/plugins/copyformatting/manual/undointegration.html
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
<h2>First editor</h2>
<div id="editor1" contenteditable="true">
<p><strong><u><span style="color: #f00;" class="important">Apollo 11</span></u></strong> <s>was the</s> <span style="color: #f00;" class="important">spaceflight</span> that landed the first humans, Americans <a href="http://en.wikipedia.org/wiki/Neil_Armstrong" title="Neil Armstrong">Neil Armstrong</a> and <a href="http://en.wikipedia.org/wiki/Buzz_Aldrin" title="Buzz Aldrin">Buzz Aldrin</a>, on the Moon on July 20, 1969, at 20:18 UTC. <span style="background-color: #f00;" bogus-attr="2">Armstrong</span> became the first to step onto the lunar surface 6 hours later on July 21 at 02:56 UTC.</p>
</div>

<h2>Second editor</h2>
<div id="editor2" contenteditable="true">
<p><strong><u><span style="color: #f00;" class="important">Apollo 11</span></u></strong> <s>was the</s> <span style="color: #f00;" class="important">spaceflight</span> that landed the first humans, Americans <a href="http://en.wikipedia.org/wiki/Neil_Armstrong" title="Neil Armstrong">Neil Armstrong</a> and <a href="http://en.wikipedia.org/wiki/Buzz_Aldrin" title="Buzz Aldrin">Buzz Aldrin</a>, on the Moon on July 20, 1969, at 20:18 UTC. <span style="background-color: #f00;" bogus-attr="2">Armstrong</span> became the first to step onto the lunar surface 6 hours later on July 21 at 02:56 UTC.</p>
</div>

<script>
CKEDITOR.replace( 'editor1' );
CKEDITOR.replace( 'editor2', {
extraPlugins: 'divarea'
} );
</script>
20 changes: 20 additions & 0 deletions tests/plugins/copyformatting/manual/undointegration.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
@bender-ui: collapsed
@bender-tags: bug, copyformatting, 4.11.3, 2780
@bender-ckeditor-plugins: copyformatting, toolbar, wysiwygarea, undo, basicstyles

## Test both editors
1. Add some content to editor (e.g. new line), to "activate" undo step.
2. Start to click around editor to change selection inside editor (20-25 times). Selection **have to differ** between adjacent steps.

### Expected:
Undo UI button is active.
### Unexpected:
Undo UI became disabled.

Comandeer marked this conversation as resolved.
Show resolved Hide resolved
3. Click undo button to revert change made in point 1. Make sure that Redo button is activated.
4. Make new selection inside editor.

### Expected:
Redo UI button is active.
### Unexpected:
Redo UI button became disabled.
71 changes: 71 additions & 0 deletions tests/plugins/copyformatting/undointegration.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,71 @@
/* bender-tags: copyformatting */
/* bender-ckeditor-plugins: wysiwygarea, toolbar, copyformatting, undo */
/* bender-ui: collapsed */

( function() {
'use strict';

var leftMouseButton = CKEDITOR.env.ie && CKEDITOR.env.version < 9 ? 1 : CKEDITOR.MOUSE_BUTTON_LEFT;

bender.editor = {
config: {
allowedContent: true
}
};


bender.test( {
// (#2780)
'test basic undo integration': function() {
var editor = this.editor,
bot = this.editorBot,
sel = editor.getSelection(),
spans;

resetUndoAndCreateFirstSnapshot( bot );

spans = editor.editable().find( 'span' );
for ( var i = 0; i < editor.undoManager.limit; i++ ) {
sel.selectElement( spans.getItem( i % 2 ) );
editor.document.fire( 'mouseup', new CKEDITOR.dom.event( {
button: leftMouseButton,
target: editor.editable()
} ) );
}

assert.areSame( 1, editor.undoManager.index, 'There shouldn\'t be new undo steps and editor should remain on the 1st step.' );
assert.isTrue( editor.undoManager.undoable(), 'Editor should have a possibility to undo.' );
},
// (#2780)
'test basic redo integration': function() {
var editor = this.editor,
bot = this.editorBot,
sel = editor.getSelection();

resetUndoAndCreateFirstSnapshot( bot );

bot.execCommand( 'undo' );

sel.selectElement( editor.editable().find( 'span' ).getItem( 0 ) );
editor.document.fire( 'mouseup', new CKEDITOR.dom.event( {
button: leftMouseButton,
target: editor.editable()
} ) );

assert.isTrue( editor.undoManager.redoable(), 'Editor should has possibility to redo.' );
}
} );

function resetUndoAndCreateFirstSnapshot( bot ) {
var editor = bot.editor;

bot.setHtmlWithSelection( '<p><span>foo</span> []bar <span>baz</span></p>' );

editor.undoManager.reset();
editor.fire( 'saveSnapshot' );

editor.insertText( '1' );
editor.fire( 'saveSnapshot' );
}

} )();