WIP – Cleanup & coverage #629
Conversation
I think it's better if extension always use the proxy method `execAction` instead of calling directly core option.
|
👌 |
|
I didn't find a way to test these lines: // Selection encompasses a single element
if (this.rangeSelectsSingleNode(range) && range.startContainer.childNodes[range.startOffset].nodeType !== 3) {
return range.startContainer.childNodes[range.startOffset];
}If anyone got an idea. |
From https://technet.microsoft.com/fr-fr/ff974693?f=255&MSPPError=-2147217396 About rangeCount: This property always returns a 1 with a valid selection. [...] Calling isCollapsed is recommended to detect an empty range. If a selection is empty, isCollapsed will return true.
When user hits "ENTER" and on blur
Thanks to Firefox that shuffle arguments and force me to use `toContains` instead
| return selection.getRangeAt(0); | ||
| this.deprecated('Util.getSelectionRange', 'Selection.getSelectionRange', 'v5.0.0'); | ||
|
|
||
| return Selection.getSelectionRange(ownerDocument); |
There was a problem hiding this comment.
This might be a bit dangerous, I believe Util will be defined before Selection...
There was a problem hiding this comment.
Well, are you sure? The demo still works great on my Mac and tests seems to be ok with that. Maybe you can give a try on your own to validate there is no such case.
I really prefer to move these functions in Selection.
There was a problem hiding this comment.
Agreed, they definitely belong in Selection. I don't believe this code will actually fail, since these functions can't be called until all of the js has loaded.
What I was pointing out though, if you look at the Gruntfile, we are including the JS files in this order:
srcFiles = [
'src/js/util.js',
'src/js/defaults/buttons.js',
'src/js/defaults/options.js',
'src/js/extension.js',
...
]So we'll be calling into Selection from inside Util, even though Util is defined before Selection. Just something we'll want to remember.
|
These changes are outstanding, thanks a ton @j0k3r! Regarding the lines you weren't able to cover, I'm not really sure what they are for either, did you ever figure that out? |
|
Just remove and wait for someone to open an issue? 💣 |
|
@daviferreira I still prefer to let the code like that instead of removing it and wait for an issue 😱 @nmielnik this code is related to retrieve the selected element / content. I guess this is a copy/paste from SO. But I didn't find a case (while playing with jsfiddle) to trigger it .. |
WIP – Cleanup & coverage
No description provided.