Opened 9 years ago
Closed 9 years ago
#13215 closed New Feature (fixed)
[Embed] Ability to cancel fetching a resource
Reported by: | Piotrek Koszuliński | Owned by: | Artur Delura |
---|---|---|---|
Priority: | Normal | Milestone: | CKEditor 4.5.0 |
Component: | General | Version: | 4.5.0 Beta |
Keywords: | Cc: |
Description
User needs to be able to cancel widget embeding, currently it's impossible:
- Open CKE with Embed plugin.
- Open dialog for widget inserting.
- Paste an URL of resource that has not yet been cached.
- Move keyboard focus to OK button with tab key.
- Do a following sequence:
- Press space (start a request).
- Press tab (move to cancel button).
- Press space (cancel fetching).
Change History (9)
comment:1 Changed 9 years ago by
Status: | new → confirmed |
---|
comment:2 Changed 9 years ago by
Owner: | set to Artur Delura |
---|---|
Status: | confirmed → assigned |
comment:3 Changed 9 years ago by
comment:4 Changed 9 years ago by
Status: | assigned → review |
---|
comment:5 follow-up: 6 Changed 9 years ago by
Status: | review → review_failed |
---|
I rebased branch:t/13215 and pushed few commits. Everything seems to work, however, I do not understand some changes in the tests:
- What was the reason for the change in
_helpers/tools.js
? - What is
'test some'
? ;) Besides its name it seems to be ok. - Why the new file jsonp1337.js?
comment:6 Changed 9 years ago by
- What was the reason for the change in
_helpers/tools.js
?
To be able to prevent executing callback which is called in setTimeout function. By doing this in CKEDITOR.plugins.embedBase._jsonp.sendRequest
it's impossible to prevent this callback which is the topic of this issue.
- Why the new file jsonp1337.js?
This file is to avoid calling error callback because file was not found. Thanks to that question I found out that error event callback is called even if script has been removed already.
comment:7 Changed 9 years ago by
Status: | review_failed → review |
---|
According to point three I found out that it's a normal browser behaviour. I thought that if we remove script element synchromously then it won't throw an error.
function onError() { console.log( 'error' ); } var s = document.createElement('script'); s.addEventListener( 'error', onError ); s.src = 'http://ckeditor.dev/404.js'; document.body.appendChild( s ); s.remove(); s.removeEventListener( 'error', onError );
Anyway, changes in the same branch:t/13215.
comment:8 Changed 9 years ago by
Status: | review → review_passed |
---|
I think that you overcomplicated the tests. The change in embedbase/_helpers/tools.js
is too insecure. It will neither be safe from changes in the JSONP implementation, because you need to assume that the append() method is used. And it will affect appending other scripts. Actually, I don't understand what it is supposed to do because the previous version was nearly identical.
Anyway, let's KISS, as it was. It will not be realistic anyway.
So I started fixing this small issue and ended up:
- changing jsonp1337.js to void.js - please use meaningful names.
- fixing tests which could not pass on IE8 - please always check tests on this browser.
- simplifying how sendRequest is being overridden in tests.
- and more.
The branch started looking bad because most of the initial code was touched again, so I squashed all commits into one.
comment:9 Changed 9 years ago by
Resolution: | → fixed |
---|---|
Status: | review_passed → closed |
Merged to major with git:d70da21.
Current changes in branch:t/13215.