T/515 Prevent of throwing an error on mouse move #527
Conversation
beatadelura
added
browser:chrome
| @@ -0,0 +1,20 @@ | |||
| @bender-ui: collapsed | |||
There was a problem hiding this comment.
File name rather than mouseover.md should also mention scroll, so scrollmouseover.md?
|
|
||
| 1. Open developer console. | ||
| 2. Focus on editor. | ||
| 3. Scroll down/up. |
There was a problem hiding this comment.
Can 3 and 4 be combined? Write that the editor should be scrolled using a mouse, so this will automatically force mousing over.
plugins/tableselection/plugin.js
Outdated
| var cell = target && target.getAscendant( { td: 1, th: 1 }, true ), | ||
| table = target && target.getAscendant( 'table', true ); | ||
| } | ||
|
|
There was a problem hiding this comment.
You can check if object is wrapped on HTMLElement either by checking if getName method exists or by checking type property.
Here it would be simpler to leave the original logic untouched, but just to do an "early return", by adding
if ( !evt.data.getTarget().getName ) {
return;
}
At the beginning of the function.
That will be safer way of fixing it. So please revert the changes in this file, and just do an early return.
There was a problem hiding this comment.
Right, it's a better solution :) Thanks!
| } ) ); | ||
| } ); | ||
|
|
||
| assert.pass(); |
There was a problem hiding this comment.
In that construction the test would always pass despite having exception thrown.
You should put assert.pass(); after the editor.document.fire call.
| var tests = { | ||
| // #515 | ||
| 'test mouseover on scrollbar': function( editor ) { | ||
| var bot = bender.editors.scrollable, |
There was a problem hiding this comment.
Bot is a second argument passed to test case, so you should just use argument.
|
|
||
| var tests = { | ||
| // #515 | ||
| 'test mouseover on scrollbar': function( editor ) { |
There was a problem hiding this comment.
Bot is passed as an argument here, see comment 2 lines below.
| 'use strict'; | ||
|
|
||
| bender.editors = { | ||
| scrollable: { |
There was a problem hiding this comment.
Please rename this property to classic and add a divarea editor too, see https://github.com/ckeditor/ckeditor-dev/blob/b8d9a84/tests/core/editable/readonly.js#L36-L41
| @@ -51,5 +51,4 @@ | |||
| </table> | |||
| </div> | |||
|
|
|||
|
|
|||
There was a problem hiding this comment.
This change is not needed and should be reverted.
…nstead of checking existing method.
|
@mlewand PTAL |
mlewand
removed
browser:chrome
| var bot = bender.editors.scrollable, | ||
| editable = bot.editable(); | ||
| 'test mouseover on scrollbar': function( editor, bot ) { | ||
| // var bot = bender.editors.classic, |
There was a problem hiding this comment.
Don't comment the code, just remove it 😉
| @@ -6,20 +6,21 @@ | |||
| 'use strict'; | |||
|
|
|||
| bender.editors = { | |||
| scrollable: { | |||
| classic: { | |||
There was a problem hiding this comment.
What i meant here is to define two separate editors. One classic (basically same definition as it was before, but different name). And the other one, called divarea with divarea added to extraPlugins.
| @@ -6,8 +6,7 @@ | |||
|
|
|||
| 1. Open developer console. | |||
| 2. Focus on editor. | |||
| 3. Scroll down/up. | |||
| 4. Mouse over scrollbar. | |||
| 3. Use mouse to scroll editor and point the cursor over the scrollbar. | |||
There was a problem hiding this comment.
This sentence is not clear enough. Enough would be some thing "Use mouse to scroll down the editor." - and simple as that. It implies that there will be cursor over the scroll bar.
|
LGTM |
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?
Prevent of throwing an error in the console if target is undefined.
Closes #515
Closes #528