Check if document.createTextRange exists for IE in 'select all' plugin. #562
Conversation
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
mlewand
suggested changes
Jun 26, 2017
f1ames
suggested changes
Jul 4, 2017
| </div> | ||
|
|
||
| <script> | ||
| if ( ( CKEDITOR.env.ie && CKEDITOR.env.version < 11 ) || bender.tools.env.mobile ) { |
There was a problem hiding this comment.
There is CKEDITOR.env.edge flag could be used here.
There was a problem hiding this comment.
As this is only Edge issue we decided that test should be ignored for other browsers.
|
|
||
| **Procedure:** | ||
|
|
||
| 1. Change view to source code |
| **Procedure:** | ||
|
|
||
| 1. Change view to source code | ||
| 2. click `select all` button |
There was a problem hiding this comment.
Keep the list items consistent. It should start with uppercase and end with a dot.
|
|
||
| **Expected result:** | ||
|
|
||
| * The whole code should be selected |
There was a problem hiding this comment.
You can use content instead of code. Also dot on the end of the sentence missing.
| @@ -0,0 +1,12 @@ | |||
| @bender-ui: collapsed | |||
| @bender-tags: tc, 545, 2564, 4.7.1, bug | |||
There was a problem hiding this comment.
No need to put TP issue number (2564).
| @@ -45,6 +45,34 @@ | |||
| acceptableResults, | |||
| bender.tools.html.prepareInnerHtmlForComparison( bender.tools.selection.getWithHtml( editor ), htmlMatchingOpts ) | |||
| ); | |||
| }, | |||
|
|
|||
| 'test selectall in source view': function() { | |||
There was a problem hiding this comment.
The test look s quite complex, I would simplify it:
- The eventsRecorder is not needed, this test should check (according to its description) if
selectAllworks in source mode, no need to check for additional events. - Editor creation can be moved to
bender.editors = { ... }, like:
editorSource: {
name: 'test_source_mode',
startupData: '<p>foo</p><p>bar</p>',
config: {
startupMode: 'source'
}
}
- And then test itself could be simplified to something like:
var editor = this.editors.editorSource;
editor.execCommand( 'selectAll' );
assert.areSame( 'source', editor.mode, 'editor.mode' );
if ( CKEDITOR.env.ie && CKEDITOR.env.version <= 8 ) {
assert.areSame( document.selection.createRange().text.length, 20 );
} else {
assert.areSame( CKEDITOR.document.getActive().$.selectionStart, 0 );
assert.areSame( CKEDITOR.document.getActive().$.selectionEnd, 20 );
}
|
@mlewand Please check if all your review requests got solved. |
mlewand
approved these changes
Jul 6, 2017
|
@f1ames I'm good with it. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
What is the purpose of this pull request?
Bug fix
Does your PR contain necessary tests?
selectallplugin test already exists.This PR contains
What changes did you make?
selectall plugin is now checking if
createTextRangemethod exists in IE.Closes #545