Remove progress bar during snapshot creation #1602
Conversation
|
As Easy Image was merged to |
|
This PR does not solve all udno integration problems, but only one in which progress bar HTML (which changes due to tracking upload progress), causes undoManager to save new snapshots, because of changes in HTML (caused by progress bar progress). See more in #1532. |
|
|
||
| **Saved undo steps** counter should not increment on selection manipulation. | ||
|
|
||
| **Important**: Selecting or deselecting widget will create an undo step. |
There was a problem hiding this comment.
Honestly I don't understand these instructions.
First of all, upload is too quick to allow sensible "selection manipulation". Maybe mocking it with slowed progress would be a good idea?
What's more, how to check if counter does not change, when every selection change will change it?
There was a problem hiding this comment.
Slowing down an upload makes sense, as network speed throttling is only available on Chrome (and probably you will need 10mb image to test it properly without it).
As for selection change adding snapshot, I'm not sure, tested it on Chrome, FF, Safari, IE11 and it seems pure selection changes does not cause new undo step:
Maybe there are some particular cases in which id does and some in which it doesn't?
There was a problem hiding this comment.
There is a case with selecting/deselecting widget which triggers undo snapshot, but it is already covered in tests description.
There was a problem hiding this comment.
There are also other cases, like select all. Relaying on undo snapshots count may be misleading. I redesigned the test to check if latest created snapshot contains progressbar.
f1ames
force-pushed
the
t/932-2961-3445
branch
from
c7e065d to
b557626
Compare
|
We won't be able to contain this in the upcomin release. Let's move it to 4.9.1 instead. |
plugins/imagebase/plugin.js
Outdated
| var remove = this.remove; | ||
| this.remove = function() { | ||
| // Remove undo listeners when progress bar is removed. | ||
| CKEDITOR.tools.array.filter( undoListeners, function( listener ) { |
There was a problem hiding this comment.
Why filter is used here? It does not modify original array, so it's left untouched, with all listeners inside. CKEDITOR.tools.array.forEach + undoListener.length = 0 seems like a better alternative.
There was a problem hiding this comment.
Looks like some leftover after poor copy/paste 😭
plugins/imagebase/plugin.js
Outdated
| // (as content will be changed due to progressbar progress). | ||
| undoListeners.push( | ||
| editor.on( 'beforeUndoImage', function() { | ||
| if ( this.wrapper.getParent() ) { |
There was a problem hiding this comment.
This function could be simplified by early return if this.wrapper does not have a parent.
|
|
||
| loader.uploadTotal = 10; | ||
|
|
||
| updateLoader( loader, 2, function() { |
There was a problem hiding this comment.
This whole part of tests is very repetitve. I'd extract it to dedicated helper function.
f1ames
force-pushed
the
t/932-2961-3445
branch
from
b557626 to
3fb8311
Compare
Comandeer
force-pushed
the
t/932-2961-3445
branch
from
986d9cd to
63aecaf
Compare
|
I've rebased to latest major and update tests. |
|
Taking over the review. |
mlewand
added
the
review:easy
f1ames
added
pr:frozen ❄
|
I have revisited this PR and it looks like some minor thing. Since Easy Image plugin is publicly available for some significant amount of time already and we didn't have any similar issue reports it is a low priority. As for the solution itself we can continue with the above (it's 1,5 year old so may need some updates) or try to address this with |
What is the purpose of this pull request?
Task
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 changes did you make?
Progress bar is now removed from HTML during snapshot creation so it does not pollute the images causing undo manager to think something changes.
Partially addresses #1532.