Opened 3 years ago

Closed 3 years ago

#13213 closed New Feature (fixed)

[Embed] Indicate in the dialog that a resource is being loaded

Reported by: Piotrek Koszuliński Owned by: Olek Nowodziński
Priority: Must have (possibly next milestone) Milestone: CKEditor 4.5.0
Component: General Version: 4.5.0 Beta
Keywords: Cc:

Description

It's not clear for user what happens after pressing the OK button in a dialog.

Attachments (2)

graceful_chrome.png (17.6 KB) - added by Olek Nowodziński 3 years ago.
graceful_firefox.png (23.2 KB) - added by Olek Nowodziński 3 years ago.

Download all attachments as: .zip

Change History (33)

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

Status: newconfirmed

comment:2 Changed 3 years ago by Piotrek Koszuliński

Priority: NormalHigh

comment:3 Changed 3 years ago by Olek Nowodziński

Owner: set to Olek Nowodziński
Status: confirmedassigned

comment:4 Changed 3 years ago by Olek Nowodziński

There's a bug: If the user clicks "Cancel" in the dialog while the resource is loaded (then "OK" in the prompt), an error is thrown

Uncaught TypeError: Cannot read property 'getParent' of null
onLoad.on.that.widget.loadContent.callback @ embedbase.js:39
finishLoading @ plugin.js:220
(anonymous function) @ plugin.js:549

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

#13158 is closed already, so it should be fine on major now. Did you check it on latest major?

comment:6 Changed 3 years ago by Olek Nowodziński

Status: assignedreview

Changes in branch:t/13213.

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

I've got doubts whether introducing the spinner feature for the dialogs system as a CSS solution is the right way. It will work only with skins that support it (meaning - for a long time only with ours). Perhaps it's not that big deal, because it's not a crucial element of the embed dialog, but I'm RFCing.

This also made me thing about notifications (the same problem). Perhaps it would be worth to notify skin authors that these features were added.

comment:8 Changed 3 years ago by Frederico Caldeira Knabben

@Reinmar, a CSS solution is wonderful for this... but you're totally right to bring this point to light.

We'll be always facing the problem of continuous enhancement of the editor UI. This includes the introduction of new UI elements that should be skinned. Therefore a "gracefully degraded" skin-less solution may be taken in consideration whenever possible.

For this specific case, why not showing a unicode character by default and then let the skin override it. For example: http://www.fileformat.info/info/unicode/char/231b/index.htm

Ofc, in v4 this may also include some hardcoded inline CSS styles, which in v5 we may consider having a "base" skin which will be part of core. Let's keep this in mind.

comment:9 Changed 3 years ago by Piotrek Koszuliński

Ofc, in v4 this may also include some hardcoded inline CSS styles, which in v5 we may consider having a "base" skin which will be part of core. Let's keep this in mind.

Yeah, having other skins extending the base one would simplify many things.

comment:10 Changed 3 years ago by Piotrek Koszuliński

Status: reviewreview_failed

Following Fred's comment - R- :)

Another thing - please add:

  • the @since tags,
  • a manual test (for the dialog system).

Changed 3 years ago by Olek Nowodziński

Attachment: graceful_chrome.png added

Changed 3 years ago by Olek Nowodziński

Attachment: graceful_firefox.png added

comment:11 Changed 3 years ago by Olek Nowodziński

Status: review_failedreview
  • Added @since tag and manual tests.
  • Used mentioned hour glass symbol and a minimal set of inline styles for graceful degradation.
  • Simplified styles.

At the moment this is what happens when no .cke_dialog_spinner styles (Chrome, Firefox):

It depends on OS and browser but this way or another, the feature is preserved regardless of the skin.

Last edited 3 years ago by Olek Nowodziński (previous) (diff)

comment:12 Changed 3 years ago by Piotrek Koszuliński

Status: reviewreview_failed

It depends on OS and browser but this way or another, the feature is preserved regardless of the skin.

It's enough. Thanks.


  1. According to http://caniuse.com/#feat=border-radius and http://caniuse.com/#feat=css-boxshadow box-radius and box-shadow do not have to be prefixed for few years already, so I removed prefixes from kama/dialog.css and moono/dialog.css. I think that we should review our skins' CSS files and clean them up. Could you handle this in a new ticket?
  2. I haven't noticed before, but there's no single automated test for the new methods. Please add at least one.
  3. Are we sure that displaying spinner in the title is ok? It makes the title move. Not a big deal, but I was thinking that we have also the space in the bottom bar. WDYT?

comment:13 in reply to:  12 Changed 3 years ago by Olek Nowodziński

Status: review_failedassigned

Replying to Reinmar:

It depends on OS and browser but this way or another, the feature is preserved regardless of the skin.

It's enough. Thanks.

You're welcome :)


  1. According to http://caniuse.com/#feat=border-radius and http://caniuse.com/#feat=css-boxshadow box-radius and box-shadow do not have to be prefixed for few years already, so I removed prefixes from kama/dialog.css and moono/dialog.css. I think that we should review our skins' CSS files and clean them up. Could you handle this in a new ticket?

Sure: #13279.

  1. I haven't noticed before, but there's no single automated test for the new methods. Please add at least one.

OK.

  1. Are we sure that displaying spinner in the title is ok? It makes the title move. Not a big deal, but I was thinking that we have also the space in the bottom bar. WDYT?

This was the first place I decided to place it. But it just felt wrong. Perhaps because dialog buttons are aligned to right – a place where I would see the spinner instead, like is some apps with active status bar. Also imagine a static icon (fallback icon) in that place. Isn't that strange?

The only alternative to bottom bar was the title bar, so there it finally landed. I see nothing wrong with moving the title – it makes it even more visible. But yes, if you find this UX confusing, let's wait for more feedback before we close it.

comment:14 Changed 3 years ago by Artur Delura

I would suggest to change the text "OK" in button to the sandglass icon. I think it makes totally sense, beucase user interact with that button but not with the title. Also this button is disabled in ticket:13215 when resource is being loaded.

comment:15 Changed 3 years ago by Wiktor Walc

If the spinner also means that the user should not be allowed to work with the dialog (or that it makes very little sense), then I'd rather display an overlay and the icon in the middle. Or even an empty dialog window with icon/loader placed inside, if the situation happens when the dialog is started.

Last edited 3 years ago by Wiktor Walc (previous) (diff)

comment:16 in reply to:  15 Changed 3 years ago by Artur Delura

Replying to wwalc:

If the spinner also means that the user should not be allowed to work with the dialog

User can click "Cancel" button, close. And it also might depend on context and for what purpose dialog is used (everyone can create their own one), it's a generic component.

comment:17 Changed 3 years ago by Frederico Caldeira Knabben

(1) The currently proposed position looks ok. It would be irritating only if the spinner is used intensively by the dialog, but I don't think that's the case.

(2) The only other possibility for me would be the bottom bar, left-aligned (mirroring the closing "X").

Both (1) and (2) are ok for me, at the current stage where I understand we don't want to bring more drastic changes (like the one proposed by @wwalc).

comment:18 Changed 3 years ago by Piotrek Koszuliński

I like the solution proposed by @wwalc - it sounds like the best UX, because the spinner is then perfectly visible and it clearly indicates that the content of the dialog is locked. However, this would indeed require more work now and would be less useful in other cases when the dialog isn't fully blocked.

So, as no one said that displaying spinner in the toolbar is evil I also think we can go this way.

comment:19 Changed 3 years ago by Frederico Caldeira Knabben

Btw, in terms of API, aren't we being a bit "poor" by just adding the showSpinner() and hideSpinner() methods?

Wouldn't a dialog.busy() method be more semantic and useful?

It would show the spinner and eventually even do more stuff related to putting the dialog in a "busy" state, like disabling the OK button or even blocking the fields (even if those are features to be added in the future only)?

comment:20 Changed 3 years ago by Piotrek Koszuliński

Wouldn't a dialog.busy() method be more semantic and useful?

I think that we could add dialog.busy() as a an extension, but the method that Olek added should also exist. Rich method should be composed from base methods. So I would see it as a separate ticket.

comment:21 in reply to:  20 Changed 3 years ago by Frederico Caldeira Knabben

Replying to Reinmar:

Wouldn't a dialog.busy() method be more semantic and useful?

I think that we could add dialog.busy() as a an extension, but the method that Olek added should also exist. Rich method should be composed from base methods. So I would see it as a separate ticket.

It's not my intention to force to go that way with this reply... it's just to call attention because your reply is not convincing.

The public API must be as minimal as possible. The more methods we provide, the worst, because people will start using them and you'll be a slave of them forever.

For example, who says that we'll have a spinner in the future? What if we decide to represent "busy" in a different way (progress-bar, pain message, lego-building, etc)? showSpinner() will be useless then and if we'll want to keep compatibility, we'll have to keep it there... and maintain it forever. In the other hand, a generic busy() method is more likely to be valid on the long run, even if its behavior may drastically change with time.

If you want to break features on parts, better then to use privates instead, like _showSpinner().

comment:22 Changed 3 years ago by Olek Nowodziński

Status: assignedreview

I decided to make things even more generic. As we don't know what states dialogs may get in the future (idle, busy, inactive?, locked?) and since each of those could get an unique UX expression, I decided to implement something flexible. It turned out to be fast and easy:

  • CKEDITOR.dialog has now a state property.
    • state is either CKEDITOR.DIALOG_STATE_IDLE or CKEDITOR.DIALOG_STATE_BUSY
    • states may be extended in the future
  • CKEDITOR.dialog has now a setState method.
  • CKEDITOR.dialog has now a state event.
    • Changes in UI happen on state event. It may be a spinner, cover, whatever.

I also implemented the busy state in image2 dialog in the very last commit (play with large images). If the API is fine, we can move it to a separate ticket, anyway.

Changes in branch:t/13213b.

comment:23 Changed 3 years ago by Piotrek Koszuliński

Status: reviewreview_failed

The public API must be as minimal as possible. The more methods we provide, the worst, because people will start using them and you'll be a slave of them forever.

(Leaving aside this particular case) We are slaves anyway, but as soon as we'll expose too small subset of methods we will also have a poor API. In my opinion it is better to expose few more useful methods and at the same time ignore those developers who do not understand that progress require changes. Moreover, if we do not expose enough API developers start using private methods or find some weird ways to achieve their goals – not cool as well.

For example, who says that we'll have a spinner in the future?

I agree. I realised this later. More generic method name would need to be found like indicateBusy().

Leaving all the theory aside, if we're going to have the "busy" state we should also disable the "ok" button and handle closing dialog. Either we have minimum (like a tool to show that we're busy), or we have a complete solution.

PS. Please also fix the API docs (default – 1 vs 2).

comment:24 Changed 3 years ago by Olek Nowodziński

Status: review_failedassigned

comment:25 Changed 3 years ago by Olek Nowodziński

Status: assignedreview

Changes in branch:t/13213c

  • It's now the dialog which state controls the OK button.
  • Visually improved the disabled state of the OK button (Moono). I didn't touch Kama because it depends on sprites.
  • Removed Image2 integration. Since the OK button depends on dialog's state, image2 tests needs some improvements, i.e. it's not possible to "click" OK from code as long as the image is loaded (IDLE state). We could extract it to a separate ticket.

comment:26 Changed 3 years ago by Piotrek Koszuliński

One thing I'm not sure - shouldn't the state be automatically reset once dialog is hidden?

comment:27 in reply to:  26 Changed 3 years ago by Olek Nowodziński

Replying to Reinmar:

One thing I'm not sure - shouldn't the state be automatically reset once dialog is hidden?

I thought about it but and I decided to leave it up to developers because I couldn't anticipate all the possible consequences. If you feel it's gonna be safe, I'm cool with it.

comment:28 Changed 3 years ago by Piotrek Koszuliński

Status: reviewreview_failed

I think that we can safely assume that if dialog is closed and opened again it's state should be back to normal. Right now it's up to us to decide and this sounds like a natural behaviour.

All the rest is fine, so I think it's the last R- :D.

comment:29 in reply to:  28 Changed 3 years ago by Olek Nowodziński

Status: review_failedassigned

Replying to Reinmar:

I think that we can safely assume that if dialog is closed and opened again it's state should be back to normal. Right now it's up to us to decide and this sounds like a natural behaviour.

All the rest is fine, so I think it's the last R- :D.

Truly heart–warming :)

comment:30 Changed 3 years ago by Olek Nowodziński

Status: assignedreview

Again, changes in branch:t/13213c.

comment:31 Changed 3 years ago by Piotrek Koszuliński

Resolution: fixed
Status: reviewclosed
Type: BugNew Feature

Merged to major with git:d8ac852.

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