Double-clicking linked placeholder opens two dialogs

[Piotrek Koszuliński]

1. Open placeholder sample.
2. Select placeholder.
3. Link it.
4. Doubie-click it.

Actual: Two dialogs are opened.

Expected: Placeholder dialog.

[Jakub Ś]

I was able to reproduce this problem from CKEditor 4.3.4 but this is only because placeholder didn't work in 4.3-4.3.3. If it was working version would most likely be 4.3.

Reproducible in every browser.

[Anna Tomanek]

The same issue appears with the MathJax widget, btw.

[Marek Lewandowski]

As Anna pointed out issue occurs for both inline widgets. This problem was solved by Olek in ​git:c32ad5d7c2d9 - we need to find a good place for that feature, because other plugins might want to reuse it.

I suggest adding config variable into widget definition like:

widgetDef.blockLinkDialog (defaults to false for compatibility reasons) - If true original link dialog will be blocked upon double-click on widget.

The thing to discuss is a good place for the code, whether it should be widget or link plugin.

[Marek Lewandowski]

We have a related issue #11037 (it's about missing context-menu entries). I think we should think about providing easy integration for at least these two features (context menu, disabling link dialog).

[Piotrek Koszuliński]

I don't think that these cases are anyhow related. And I don't understand what do you mean by easy integration - all this can be achieved easily, but the currently implemented logic is incorrect.

[Marek Lewandowski]

Raw code which does the job is in ​t/12140 at dev.

Sample with prop in widget definition pushed to ​t/12140b at dev.

What I wish to emphasis is that with second solution you can fix common dialog issue simple by adding blockLinkDialog: true to mathjax widget definition. I was not sure regarding the place where this feature should land, whether it should be implemented in widget, or link plugin.

[Piotrek Koszuliński]

This issue was caused by ​git:e0e69d094 (#11417). I don't know if this was the direct cause, but that we broke the logic there.

The thing is - if widget.edit() is called and editing is triggered widget#doubleclick event has to be cancelled so editor#doubleclick is cancelled too.

We could revert #11417 completely, but I think that we may do it better. Before #11417 widget#doubleclick was always cancelled, regardless of whether widget has a dialog (widgetDefinition.dialog) or whether widget#edit event was cancelled. Code will make more sense if widget.edit() will return true if default editing action was performed (definition specifies dialog and edit event wasn't cancelled). Based on that we will cancel doubleclick or not. Additionally, we need an information in widget#doubleclick docs explaining this: ​http://dev.ckeditor.com/ticket/11417#comment:1.

[Marek Lewandowski]

Force pushed (overwrited) branch ​t/12140.

[Piotrek Koszuliński]

1. Does not make sense, because this is inside callback: ​https://github.com/cksource/ckeditor-dev/commit/acf01edd2a50c833b4e067ea9652b7c0768249d1#diff-d932030cd4e22ec8b0e30a362269b7e8R1054
2. ​https://github.com/cksource/ckeditor-dev/commit/acf01edd2a50c833b4e067ea9652b7c0768249d1#diff-c981abda5f48940a4ddfc07a3caddc8fR84 - assert msgs should explain expected result (all messages are incorrect)
3. ​https://github.com/cksource/ckeditor-dev/commit/acf01edd2a50c833b4e067ea9652b7c0768249d1#diff-c981abda5f48940a4ddfc07a3caddc8fR165 - don't need to use replaceMethod and revert - you work on widget instance which does not leak from the test case.
4. Please add a note explaining how to use widget#doubleclick in its docs.

So just minor issues. Patch is working, tests are passing.

[Marek Lewandowski]

Fixes pushed to ​t/12140, will be squashed before merge.

[Piotrek Koszuliński]

I pushed one additional commit to ​branch:t/12140.

[Marek Lewandowski]

Fixed with ​git:ab1714faa9 (merged to master) at dev.