Bug fix for throwing error in console where style combo was open #659
Conversation
| </textare> | ||
|
|
||
| <script> | ||
| if ( bender.tools.env.mobile ) { |
There was a problem hiding this comment.
Why this test is ignored on mobile devices?
There was a problem hiding this comment.
Error was thrown only in console. Generally during tests it's hard to analyse what happen on mobile console. So even if bug potentially might still exist, it won't be visible in such environment. That's why manual tests are ignored on mobile for this fix.
| 1. Press empty button in toolbar,next to style selection. | ||
| 1. Press style drop down menu to open style list. | ||
|
|
||
| **Expected:** There is nothing happen. |
| var editor = bot.editor; | ||
| var selection = editor.getSelection(); | ||
|
|
||
| var stub = sinon.stub( CKEDITOR.dom.selection.prototype, 'getNative', function() { |
There was a problem hiding this comment.
What is the purpose of this stub?
There was a problem hiding this comment.
I add short comment in code before stub to have this info in case of future cases.
Generally, when combo was opened, there was execute operation which operate on selection. During this process selection was reset. So in my test I wasn't able to simulate None selection and always receive Caret selection. This behaviour affect on test and doesn't allow on proper error reproduction.
To be precise I wasn't able to get elementPath = null from here: https://github.com/ckeditor/ckeditor-dev/blob/master/plugins/stylescombo/plugin.js#L145
because of way how this work in those places:
https://github.com/ckeditor/ckeditor-dev/blob/master/core/editor.js#L860-L870
here is this.getSelection() which generally get selection based on native one here:
https://github.com/ckeditor/ckeditor-dev/blob/master/core/selection.js#L1342
And that's why I stub getNative() to reset native selection and always get None type.
|
I add some comments also under your review comments. Might be helpful ;) |
What is the purpose of this pull request?
bug fix
Does your PR contain necessary tests?
yes
This PR contains
What changes did you make?
close #646