[#704] Fix broken widget after using Ctrl+Z in Edge #734
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,26 @@ | ||
| <textarea id="editor1" name="editor1"> | ||
|
There was a problem hiding this comment. This test fails on IE8 due to There was a problem hiding this comment. I added both things. I know it wasn't necessary, but this not harm anything. ;) |
||
| <figure class="image illustration" style="float:left"> | ||
| <img alt="" height="266" src="http://c.cksource.com/a/1/img/demo/brownie.jpg" width="400" /> | ||
| <figcaption>Bon Appétit!</figcaption> | ||
| </figure> | ||
| <p>Preheat the oven to <strong>350°F</strong> and grease the baking pan. | ||
| Combine the flour, sugar and cocoa powder in a medium bowl. In another small bowl, whisk together the butter and eggs. Stir the two mixtures until just combined. | ||
| Bake the brownies for 25 to 35 minutes. Remove from the oven and let it cool for 5 minutes. | ||
| </p> | ||
| </textarea> | ||
| <pre id="output" style="background-color: lightgreen;"></pre> | ||
| <script> | ||
| if ( !CKEDITOR.env.edge ) { | ||
| bender.ignore(); | ||
| } | ||
|
|
||
| var editor = CKEDITOR.replace( 'editor1', { | ||
| height: '600px' | ||
| } ); | ||
| var output = document.getElementById( 'output' ); | ||
| editor.on( 'instanceReady', function() { | ||
| editor.editable().$.onkeyup = function() { | ||
| output.innerText = editor.editable().getFirst().getName(); | ||
| }; | ||
| } ); | ||
| </script> | ||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,11 @@ | ||
| @bender-ui: collapsed | ||
| @bender-tags: bug, 4.8.0, 704 | ||
| @bender-ckeditor-plugins: image2, wysiwygarea, undo, sourcearea | ||
|
|
||
| ---- | ||
| 1. Put caret in the text on the right and type some text. | ||
| 1. Use `Ctrl + Z` or `Cmd + Z` shortcut to make undo few times. | ||
|
|
||
| **Expected:** Text returns to previous form. Below the editor, you should see `div` on green background. | ||
|
|
||
| **Unexpected:** Text moves below the image. Below editor, it will be written `figure` instead of `div` on the green background. |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,58 @@ | ||
| /* bender-ckeditor-plugins: widget,wysiwygarea,undo,toolbar */ | ||
|
|
||
|
|
||
| ( function() { | ||
| 'use strict'; | ||
|
|
||
| bender.editor = { | ||
| config: { | ||
| allowedContent: true | ||
| } | ||
| }; | ||
|
|
||
| bender.test( { | ||
| // #704 | ||
| 'test keeping widget wrapper in editor when superfluous elements are checked': function() { | ||
| CKEDITOR.plugins.add( 'test', { | ||
| requires: 'widget', | ||
| init: function( editor ) { | ||
| editor.widgets.add( 'test', { | ||
| upcast: function( element ) { | ||
| if ( element.hasClass( 'test' ) ) { | ||
| return true; | ||
| } | ||
| }, | ||
|
|
||
| init: function() { | ||
| this.element.setStyle( 'background-color', '#FFBB00' ); | ||
| this.element.setStyle( 'padding', '30px' ); | ||
| } | ||
| } ); | ||
| } | ||
| } ); | ||
| bender.editorBot.create( { | ||
| name: 'test_widget', | ||
| startupData: '<h2 class="test">Test</h2><p>Preheat the oven to</p>', | ||
| config: { | ||
| extraPlugins: 'test', | ||
| allowedContent: true | ||
| } | ||
| }, function( bot ) { | ||
| var editor = bot.editor; | ||
|
|
||
| editor.editable().fire( 'keydown', new CKEDITOR.dom.event( { | ||
| keyCode: 90, | ||
| ctrlKey: true, | ||
| shiftKey: false | ||
| } ) ); | ||
|
|
||
| var snap = editor.getSnapshot(); | ||
| editor.loadSnapshot( snap ); | ||
|
There was a problem hiding this comment. This two lines just reload the snapshot, it it needed here? There was a problem hiding this comment. Yes.
Re-loading snapshot might be replaced with some kind of triggering undo event, but it would require much more work. First to make some history, and then revert it. So I extract only important part from entire undo process, which is reloading snapshot, what cause the issue in case of using shortcut. This allows on proper simulation behaviour on Edge and proper fail of test in case of Edge environment without bug fix. |
||
|
|
||
| editor.editable().fire( 'keyup', new CKEDITOR.dom.event( {} ) ); | ||
|
|
||
| assert.areSame( 'div', editor.editable().getFirst().getName().toLowerCase() ); | ||
| } ); | ||
| } | ||
| } ); | ||
| } )(); | ||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the reason of this change? Previously elements without
data-cke-tempattribute (so not temporary elements) were removed inside thisif, now it removes only attributeless elements. Is this change necessary?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Entire function
removeSuperfluousElementwas design to remove additionaldivwhich appear in Edge, when all text is removed from editor. This div never had any attribute. Unfortunately author hadn't predict that not only those extradivsmight appear, but also widgets might be construct based ondiv. And in case ofundocommand entire widget might be redraw so we lostretaincustomData attached todiv.This change detects more carefully only those extra divs from Edge, so it becomes more specific. Now divs connected to widgets won't be removed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense 👍