Fix for: Element/Image drag to move within table is no longer available #3244
Conversation
plugins/clipboard/plugin.js
Outdated
| this.setData( 'text/html', element.$.outerHTML ); | ||
| } else { | ||
| this.setData( 'text/html', editor.getSelectedHtml( 1 ) ); | ||
| } |
There was a problem hiding this comment.
This is different issue, it also happens on copy/paste. Please see if it is already reported - if not you can report an issue (and maybe link this piece of code as a possible solution). Then we can proceed separately with it.
There was a problem hiding this comment.
PR for the fix - #3290, should be merged before this one. I will remove redundant code during rebase onto latest major.
There was a problem hiding this comment.
After some digging I think #3278 was just some particular case of #972, so I closed it as duplicate. Still, #3290 should be merged before this as it fixes mentioned clipboard issues (also the one mentioned in another comment).
plugins/tableselection/plugin.js
Outdated
| if ( table && table.hasAttribute( ignoredTableAttribute ) ) { | ||
| // (#547) | ||
| if ( table && evt.name == 'dragstart' ) { | ||
| table.data( ignoredTableAttribute, '' ); |
There was a problem hiding this comment.
The element.data() method adds data- prefix so you will end up with data-data-cke-tableselection-ignored. This means this code doesn't add proper ignore attribute and still works which means the attribute may be not needed 🤔
Also the condition for #2945 was removed so you need special attention here to se if nothing gets broken. Btw. I think it will still work with this condition present.
There was a problem hiding this comment.
I would consider the other way around approach too (not saying it will be better but worth checking) - instead of just always allowing dragging (this is what happens after this change basically), maybe don't prevent dragStart event in more case. The same it is done for table && table.hasAttribute( ignoredTableAttribute ) or !cell || cell.hasClass( fakeSelectedClass ) - it should be also possible to be done for image or text elements.
There was a problem hiding this comment.
Firstly, you're obviously right - the attribute was misused and useless 👎 But later I found out that trying to reuse it in this case isn't trival, as after drop we want to remove the temporal attribute, and I didn't see a way to distinguish whether it was added by user with #2945 in mind, or just added for dragging, so I decided to restore removed conditions and mimic that solution but with a new, dedicated attribute (I came up with cke-table-dragging-active), except for the part where I control turning it on and off.
As for the second comment, during tests I didn't encounter and can't really think of any edge-case scenario I would like to cut out, so if you have any of them on mind or find it during review please let me know. For now I would like to give a go to this solution.
Dumluregn
added
the
pr:frozen ❄
What is the purpose of this pull request?
Bug fix
Does your PR contain necessary tests?
All patches which change the editor code must include tests. You can always read more
on PR testing,
how to set the testing environment and
how to create tests
in the official CKEditor documentation.
This PR contains
What is the proposed changelog entry for this pull request?
[#547](https://github.com/ckeditor/ckeditor-dev/issues/547): Fixed: Element/Image drag to move within table is no longer available when using [Table Selection plugin](https://ckeditor.com/cke4/addon/tableselection).What changes did you make?
Added drag&drop handling so that table selection no longer needs to cancel those events.
Closes #547.