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

[Piotrek Koszuliński]

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

[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

[Piotrek Koszuliński]

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

[Olek Nowodziński]

Changes in ​branch:t/13213.

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

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

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

[Piotrek Koszuliński]

Following Fred's comment - R- :)

Another thing - please add:

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

[Olek Nowodziński]

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

[Piotrek Koszuliński]

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?

[Olek Nowodziński]

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.

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

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

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

[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).

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

[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)?

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

[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().

[Olek Nowodziński]

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.

[Piotrek Koszuliński]

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

[Olek Nowodziński]

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.

[Piotrek Koszuliński]

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

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

[Piotrek Koszuliński]

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.

[Olek Nowodziński]

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 :)

[Olek Nowodziński]

Again, changes in ​branch:t/13213c.

[Piotrek Koszuliński]

Merged to major with ​git:d8ac852.