Opened 7 years ago

Closed 6 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:


User needs to be able to cancel widget embeding, currently it's impossible:

  1. Open CKE with Embed plugin.
  2. Open dialog for widget inserting.
  3. Paste an URL of resource that has not yet been cached.
  4. Move keyboard focus to OK button with tab key.
  5. Do a following sequence:
    1. Press space (start a request).
    2. Press tab (move to cancel button).
    3. Press space (cancel fetching).

Change History (9)

comment:1 Changed 7 years ago by Piotrek Koszuliński

Status: newconfirmed

comment:2 Changed 7 years ago by Artur Delura

Owner: set to Artur Delura
Status: confirmedassigned

comment:3 Changed 7 years ago by Artur Delura

Current changes in branch:t/13215.

comment:4 Changed 6 years ago by Artur Delura

Status: assignedreview

comment:5 Changed 6 years ago by Piotrek Koszuliński

Status: reviewreview_failed

I rebased branch:t/13215 and pushed few commits. Everything seems to work, however, I do not understand some changes in the tests:

  1. What was the reason for the change in _helpers/tools.js?
  2. What is 'test some'? ;) Besides its name it seems to be ok.
  3. Why the new file jsonp1337.js?

comment:6 in reply to:  5 Changed 6 years ago by Artur Delura

  1. 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.

  1. 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 6 years ago by Artur Delura

Status: review_failedreview

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 = '';

document.body.appendChild( s );
s.removeEventListener( 'error', onError );

Anyway, changes in the same branch:t/13215.

comment:8 Changed 6 years ago by Piotrek Koszuliński

Status: reviewreview_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 6 years ago by Piotrek Koszuliński

Resolution: fixed
Status: review_passedclosed

Merged to major with git:d70da21.

Note: See TracTickets for help on using tickets.
© 2003 – 2021 CKSource – Frederico Knabben. All rights reserved. | Terms of use | Privacy policy