Opened 9 years ago

Last modified 4 years ago

#5291 assigned New Feature

Replace alert messages with user-defined UI

Reported by: Sa'ar Zac Elias Owned by: Sa'ar Zac Elias
Priority: Normal Milestone:
Component: General Version: 3.0
Keywords: IBM Cc: monahant@…, edithk@…

Description

In some cases, CKEditor uses an alert to display messages, i.e. invalid . but, in some cases, the developer would like to use his own UI to display these messages. so, i think it can be replaced by an internal function that the developer can overwrite, or make an optional config entry with a callback that will overwrite the original alert.

Attachments (4)

5291.patch (3.9 KB) - added by Sa'ar Zac Elias 9 years ago.
5291_2.patch (6.3 KB) - added by Sa'ar Zac Elias 9 years ago.
5291_3.patch (11.5 KB) - added by Sa'ar Zac Elias 9 years ago.
5291_4.patch (11.4 KB) - added by Sa'ar Zac Elias 9 years ago.

Download all attachments as: .zip

Change History (19)

Changed 9 years ago by Sa'ar Zac Elias

Attachment: 5291.patch added

comment:1 Changed 9 years ago by Sa'ar Zac Elias

Keywords: Review? added
Milestone: CKEditor 3.3
Owner: set to Sa'ar Zac Elias
Status: newassigned
Version: SVN (CKEditor)

Introducing a new event named 'userError' which alerts user about errors as default or uses a custom implementation pointing to a global named CKEDITOR_USERERROR if defined.
This is only the basic idea for review, so only one alert in search and replace dialog is using this method.
Added a sample page using this feature.

comment:2 Changed 9 years ago by Alfonso Martínez de Lizarrondo

Keywords: Review- added; Review? removed
Milestone: CKEditor 3.3

Naming an event "userError" when it's trying to show an alert is highly misleading. The name should be just "alert"

Any usage of global variables like this "CKEDITOR_USERERROR" should be avoided as the code can just use the event fired and follow the same coding style than the rest of customizations.

A sample page should work better, now the message is behind the dialog and people will say that this is useless.

And an important part of window.alert is that it behaves in a modal way, the code is stopped until the user clicks OK, so if this is gonna be implemented it must provide a callback function and adjust the calling code to continue the execution with that callback (although the callback can be omitted in some places).

Also, this same set of changes will be needed for window.confirm (window.prompt isn't used currently).

comment:3 Changed 9 years ago by Alfonso Martínez de Lizarrondo

Last comment :-) I think that instead of ckeditor.js, the default handler should be in the theme.js file

Changed 9 years ago by Sa'ar Zac Elias

Attachment: 5291_2.patch added

comment:4 in reply to:  2 Changed 9 years ago by Sa'ar Zac Elias

Keywords: Review? added; Review- removed

Replying to alfonsoml:

And an important part of window.alert is that it behaves in a modal way, the code is stopped until the user clicks OK, so if this is gonna be implemented it must provide a callback function and adjust the calling code to continue the execution with that callback (although the callback can be omitted in some places).

I didn't quite understand the idea behind it. the alerts are in use only when there is an error message, so no code is executed.

Last comment :-) I think that instead of ckeditor.js, the default handler should be in the theme.js file

I think this is a bit problematic idea, because if the events will be defined in the theme.js file, there will be no way to remove the default handlers.

I added a function named 'removeListeners', so the user will be able to remove all listeners of an event (for example, this is useful if there is an internal function attached to an event so you can't access the function to remove it).

comment:5 Changed 9 years ago by Alfonso Martínez de Lizarrondo

Keywords: Review- added; Review? removed

let's take this sample pseudo-code

alert( message );
closeDialog()

The current behavior is: a modal alert window is shown and when the user clicks it then the closeDialog() is executed.

If we replace the alert() with a DHTML message made of divs, then it isn't modal, so the message is shown and immediately the dialog is closed. Whatever action follows the alert is executed without waiting for it to return.

That's a bigger problem if the method is prompt, in that case we need to know what's the option clicked by the user before completing the execution.

There's no need for the 'removeListeners' method, each listener can be defined with a priority so we just need to set a low (eg: 20) priority on the default listener and then the custom one can call event.stop() if it has handled the event.

Changed 9 years ago by Sa'ar Zac Elias

Attachment: 5291_3.patch added

comment:6 Changed 9 years ago by Sa'ar Zac Elias

Keywords: Review? added; Review- removed

Added an optional callback, removed the 'removeListeners' method and enhanced the sample page.

comment:7 Changed 9 years ago by Alfonso Martínez de Lizarrondo

Keywords: Review- added; Review? removed

I think that it's getting closer to be a usable replacement, although I would also like to hear other voices about this.

Anyway, the name of the parameters is still slightly misleading, it should try to use more generic terms like they are used in https://developer.mozilla.org/en/DOM/window.alert and https://developer.mozilla.org/en/DOM/window.confirm : "message" and "result".

And do you think that the errorType or confirmAt parameters will be useful? If we don't have a clear reason to add them I would avoid that because it means more time thinking about putting the description correct in all places and some extra bytes that might not ever be used at all.

Changed 9 years ago by Sa'ar Zac Elias

Attachment: 5291_4.patch added

comment:8 Changed 9 years ago by Sa'ar Zac Elias

Keywords: Review? added; Review- removed

Cahnged the parameters' names to 'message', 'type', 'result' and 'where'.
I think these parameters can be very useful, for example when the developer would like to individualize a specific error from the others or make custom description to each error.

comment:9 Changed 9 years ago by Wiktor Walc

Version: SVN (CKEditor) - OLD3.0

How about splitting this patch into two parts:

  • changes required in core in order to make such a plugin that would be reviewed here and committed
  • a pure CKEditor plugin that user could simply load with extraPlugins to be distributed later in a separate package

comment:10 Changed 8 years ago by Frederico Caldeira Knabben

Status: reviewassigned

The idea is not bad. We need to make it current now, updating the samples and the code.

In any case, I would not make any move here until we'll not clearly identify user requests for this.

comment:11 Changed 7 years ago by Teresa Monahan

Cc: monahant@… added
Keywords: IBM added

We would like this functionality in the editor. Some of our products have requested the ability to be able to style the alert and confirm dialogs in keeping with the rest of their UI. Please consider including this is an upcoming CKEditor release.

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

Related ticket #13380.

comment:13 Changed 4 years ago by edithk

Hello,

I have submitted a pull request for your review: https://github.com/ckeditor/ckeditor-dev/pull/223

Following Reinmar's approach (described in ticket 13380) I did the following changes:

  1. Added editor.showAlert() and editor.showConfirm() methods that by default call the native alert() and confirm() functions.
  2. Replaced native alert() and confirm() calls with editor.showAlert() and editor.showConfirm() respectively.
  • This replacement required some additional code changes in order to properly handle callbacks (especially when replacing confirm methods).
  • I replaced all occurrences but the ones in filebrowser plugin and embedbase files which I wasn’t able to activate and test. I will pursue this further should you approve this solution.
  1. Added a plugin (which I called – customMsg) similar to Notification plugin.
  • This plugin overrides editor.showAlert() and editor.showConfirm() methods.
  • The custom dialog itself is css based as was proposed and accepted earlier in this ticket.
  • The dialog display is basic. If this solution is approved, I will invest in styling to give it a ckEditor UI look.

To activate the customMsg plugin, load it in plugins/dialog/plugins.js file:

CKEDITOR.plugins.add( 'dialog', {
requires: 'dialogui,customMsg',
…

Thanks

comment:14 Changed 4 years ago by edithk

Cc: edithk@… added

comment:15 Changed 4 years ago by edithk

Hello,

Do you have any updates ?

Thank you

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