Opened 4 years ago

Closed 4 years ago

#12140 closed Bug (fixed)

Double-clicking linked placeholder opens two dialogs

Reported by: Piotrek Koszuliński Owned by: Marek Lewandowski
Priority: Normal Milestone: CKEditor 4.4.3
Component: General Version: 4.3.4
Keywords: Cc:

Description

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

Actual: Two dialogs are opened.

Expected: Placeholder dialog.

Change History (13)

comment:1 Changed 4 years ago by Jakub Ś

Status: newconfirmed
Version: 4.3.4

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.

comment:2 Changed 4 years ago by Anna Tomanek

The same issue appears with the MathJax widget, btw.

comment:3 Changed 4 years ago by Marek Lewandowski

Owner: set to Marek Lewandowski
Status: confirmedassigned

comment:4 Changed 4 years ago by 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.

comment:5 Changed 4 years ago by 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).

comment:6 Changed 4 years ago by 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.

comment:7 Changed 4 years ago by 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.

comment:8 Changed 4 years ago by 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.

comment:9 Changed 4 years ago by Marek Lewandowski

Status: assignedreview

Force pushed (overwrited) branch t/12140.

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

Status: reviewreview_failed
  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.

comment:11 Changed 4 years ago by Marek Lewandowski

Status: review_failedreview

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

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

Status: reviewreview_passed

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

comment:13 Changed 4 years ago by Marek Lewandowski

Resolution: fixed
Status: review_passedclosed

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

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