Opened 7 years ago

Closed 7 years ago

Last modified 5 years ago

#9087 closed feature (fixed)

Dialog: Allow registering elements outside a dialog for use when a modal dialog is open

Reported by: psyafter Owned by: psyafter
Priority: minor Milestone: 1.10.2
Component: ui.dialog Version: 1.10.0
Keywords: Cc:
Blocked by: Blocking:

Description (last modified by Scott González)

When using CKEditor in UI Dialog there is a problem with editor popups.

jsfiddle: http://jsfiddle.net/psyafter/dUThT/

1) click on button 2) in ckeditor double click on text "click me" 3) in editor dialog there is a problem edit textboxes


Solution

See comment #30 for an extension that can be loaded after jQuery UI to register CKEditor elements as valid targets.

Change History (39)

comment:2 Changed 7 years ago by Scott González

Owner: set to psyafter
Status: newpending

To be honest, this seems like a strange behavior to implement. Why are you putting a full editor inside a modal dialog? Do you have suggestions for how to sanely handle this?

comment:3 in reply to:  2 Changed 7 years ago by psyafter

Status: pendingnew

Replying to scott.gonzalez:

To be honest, this seems like a strange behavior to implement. Why are you putting a full editor inside a modal dialog? Do you have suggestions for how to sanely handle this?

CKEditor in ui dialog is because of requirements. In past versions of jquery-ui there was patch that worked about 2 years. link to patch: http://forum.jquery.com/topic/can-t-edit-fields-of-ckeditor-in-jquery-ui-modal-dialog#14737000001648387

But today, after jquery-ui-dialog (and overlay) has been redesigned - patch not work because of missing $.ui.dialog.overlay. I believe jquery-ui team can rewrite a patch (with existing ui core conventions)

comment:4 Changed 7 years ago by Scott González

Status: newpending

I believe jquery-ui team can rewrite a patch (with existing ui core conventions)

If I knew a solution, I wouldn't be asking you for one. We also wouldn't still be fighting z-index and stacking issues after several years. We have what we believe to be the best solution we've come up with so far, which is to append popups to the element they're coming from. Popping up any dialog from a modal dialog is bad UX, but anticipating the various ways that this is done from all other JS libraries isn't possible.

What if we allowed you to register a selector that would define elements which should override the modal behavior? Then you could manually register ".cke_dialog". It's hacky and I don't like it, but I think it would work. What do you think?

comment:5 in reply to:  4 Changed 7 years ago by psyafter

Status: pendingnew

Replying to scott.gonzalez:

What if we allowed you to register a selector that would define elements which should override the modal behavior? Then you could manually register ".cke_dialog". It's hacky and I don't like it, but I think it would work. What do you think?

1) for a start - which lib is responsiblee for preventing editing? jquery-ui or ckeditor? 2) your solution is something like "white list" where each registered element will not be parsed by jquery-ui-dialog functionality? (it will be nice solution)

comment:6 Changed 7 years ago by Scott González

which lib is responsiblee for preventing editing?

Clearly jQuery UI. You've asked a jQuery UI dialog to prevent editing of everything that is not inside the dialog. It's doing what you've asked.

your solution is something like "white list" where each registered element will not be parsed by jquery-ui-dialog functionality?

Correct.

comment:7 in reply to:  6 Changed 7 years ago by psyafter

Replying to scott.gonzalez: fine, thanks for responses. solution that you propose will be really useful.

comment:8 Changed 7 years ago by djQuery

In Google Chrome Version 24.0.1312.70 I was unable to reproduce your issue. I was able to edit the inputs in the link properties pop up just fine.

Were you experiencing this issue in a particular browser?

Ok just tried it in Firefox v 18.0.2 and I am able to reproduce the issue.

Last edited 7 years ago by djQuery (previous) (diff)

comment:9 in reply to:  8 Changed 7 years ago by psyafter

Replying to djQuery:

In Google Chrome Version 24.0.1312.70 I was unable to reproduce your issue. I was able to edit the inputs in the link properties pop up just fine.

In Google Chrome 24.0.1312.57m is reproduced too

comment:10 Changed 7 years ago by psyafter

test case updated to last jquery-ui-1.10.1

comment:11 Changed 7 years ago by Oskar

I have the same bug. In which version will it be fixed? Updated to jquery ui 1.10.1, but the bug is still there.

comment:12 Changed 7 years ago by Scott González

Milestone: none1.10.2
Status: newopen
Summary: ui dialog prevent editing inputs in CKEditor popupsDialog: Allow registering elements outside a dialog for use when a modal dialog is open
Type: bugfeature

I'm marking as valid, but we're not going to fix this directly in jQuery UI because we feel that this is bad UX. What we're willing to do is expose a method that can be overridden to allow an API like the one I suggested.

comment:13 Changed 7 years ago by Scott González

Resolution: fixed
Status: openclosed

Dialog: Extract check for which elements can gain focus into its own method for overriding. Fixes #9087 - Dialog: Allow registering elements outside a dialog for use when a modal dialog is open.

Changeset: 51eb28e76e372fe0af12724edff0b5780b5e5ed0

comment:14 Changed 7 years ago by Scott González

This change allows you to write an extension like so:

$.widget( "ui.dialog", $.ui.dialog, {
	_allowInteraction: function( event ) {
		return !!$( event.target ).closest( ".other-popups" ).length || this._super( event );
	}
});
Last edited 7 years ago by Scott González (previous) (diff)

comment:15 in reply to:  14 Changed 7 years ago by psyafter

Replying to scott.gonzalez:

This change allows you to write an extension like so:

very nice! when 1.10.2 will be released?

comment:16 Changed 7 years ago by Scott González

We don't set specific dates for releases, but we generally release about once per month.

comment:17 Changed 7 years ago by pmckensey

I am still seeing the same bug with CKEditor and jqueryui version 1.10.2

comment:18 Changed 7 years ago by Scott González

@pmckensey The "bug" will continue to exist for all versions, unless you explicitly write an exception as mentioned above. See http://bugs.jqueryui.com/ticket/9087#comment:14

comment:19 Changed 7 years ago by altieres

@scott.gonzalez, I haven't understood the 14th comment, need I do something? or the ckeditor team need to implement that inside ckeditor? please explain yourself a little more...

comment:20 Changed 7 years ago by Scott González

@altieres It's up to the CKEditor team whether they add code for this. The bottom line is that if you want to have something that isn't part of the DOM inside the dialog, you'll need to overwrite _allowInteraction() to support whatever other elements you want. Take a look at the current implementation which hacks in support for datepicker to get an idea of how this works.

comment:21 Changed 6 years ago by tj.vantoll

#9378 is a duplicate of this ticket.

comment:22 Changed 6 years ago by Scott González

#9173 is a duplicate of this ticket.

comment:23 Changed 6 years ago by faren451

Whew! Took me forever to find that my problem could be fixed--thanks for posting this fix (in comment #14). But could someone please clarify how to use this with CKEditor or point to an example page? I am using CKEditor 4.2.1 and jQuery UI 1.10.3, and I have included a new js file after including jquery UI:

$.widget( "ui.dialog", $.ui.dialog, {
	_allowInteraction: function( event ) {
		return !!$( event.target ).closest( ".cke_dialog" ).length || this._super( event );
	}
});

This does not fix the issue for me, though. Even before adding this patch / fix, I could edit all non-text / non-textarea elements in my plugin dialog...

With this fix, I have also tried ".cke_dialog" and ".cke_dialog_ui_input_text", but neither works. The text is still non-editable, non-highlightable. Maybe I don't understand how this fix is supposed to work, so any clarification is appreciated. Thanks!

Last edited 6 years ago by faren451 (previous) (diff)

comment:24 Changed 6 years ago by Scott González

@faren451: You'll need to post on the forums or Stack Overflow for help with your specific case.

comment:25 Changed 6 years ago by Kevin H. Kamel

As an FYI this ticket is being referenced as the solution for everyone using CKEditor 4.x nested within a jQuery-UI modal. I literally have read through 50+ posts online that copy/pasted the result above and cite it as the solution to their problems.

Do not even bother copying and pasting the snippet above and expecting this to solve your problems - I tried - it doesn't and can't work. I investigated why, and I don't believe that the poster ever expected it to be cited as the solution to our problem, rather he likely expected the CKEditor people to provide some type of proper solution for the observed problem, which they have failed to provide as of today.

The hook above does provide hope, but it falls short of being a proper solution for a number of reasons:

  • The base method likely provided functionality that you want
  • The author of the proposed solution above is likely not familiar with CKEditor, and did not account for the nested iframe that CKEditor uses.
  • Due to the implementation of CKEditor, clicks on the nested iframe in CKEditor will generate an event.target as a "document", instead of generating an event.target as an "element". A document is not an element, so the .closest() call will return zero elements. This means that all interaction with CKEditor will be explicitly blocked from that point on.

My solution is attached below, but I'm sure there is something wrong with it. If anyone has better ideas I'd love to hear them - I want to preserve as much of the native functionality as possible while allowing the nested CKEditor to work until I change over to the newly supported IPE functionality:

$.widget( "ui.dialog", $.ui.dialog, {
  _allowInteraction: function( event ) {
    if ( $( event.target ).closest(".ui-dialog").length ) {
      return true;
    }

    if (!(event.target instanceof HTMLElement)) {
      return true;
    }

    if ( $( event.target ).closest(".cke_dialog").length ) {
      return true;
    }

    // TODO: Remove hack when datepicker implements
    // the .ui-front logic (#8989)
    return !!$( event.target ).closest(".ui-datepicker").length;
  },
  _moveToTop: function (event, silent) {
    if (!event || !this.options.modal) {
      this._super(event, silent);
    }
  }
});

I realize this probably is not the best place for such a dialog, but the problem is being discussed on many different forums and there is no single place to note that the provided solution will not work out of the box for most implementations.

comment:26 Changed 6 years ago by Scott González

@kamelkev I think it's fine to have this conversation here. I'm even willing to update the body of the ticket to clearly show the best solution, once the community has agreed on a working solution.

A few suggestions:

Do not manually re-implement the built-in logic; you're killing the upgrade path if you do. Use this._super( event ) to determine if the built-in logic allows interaction. This would result in something like:

_allowInteraction: function( event ) {
    if ( this._super( event ) ) {
        return true;
    }

    // custom logic goes here
}

Don't use instanceof. While the reasons for avoiding instanceof are generally tied to supporting cross-window usage, and you're explicitly trying to check for that case, it's not actually checking what you want to check. You could instead just check if event.target.ownerDocument matches this.document[ 0 ] to determine if the event originated in the same document.

Can you explain why you needed to proxy _moveToTop()?

comment:27 Changed 6 years ago by Kevin H. Kamel

Hi Scott, thanks for commenting. I was afraid people had completely lost interest in this. Definitely looking for the "right" way to do it.

It was an accident to paste moveToTop here, but it is relevant to the conversation. Basically the moveToTop adjustment handles a previous issue the CKEditor/jQueryUI community had whereby some iframe populated drop downs would lose their contents the second time they were clicked. There are a bunch of solutions out there for handling the problem, I found this one to be the simplest. I have tested it on a bunch of browsers, but I do not have it in production yet.

Great suggestions, I reimplemented and started our team testing it on all common browsers. So far the following appears to meet all requirements:

$.widget( "ui.dialog", $.ui.dialog, {
 /*! jQuery UI - v1.10.2 - 2013-12-12
  *  http://bugs.jqueryui.com/ticket/9087#comment:27 - bugfix
  *  allowInteraction fix to accommodate windowed editors
  */
  _allowInteraction: function( event ) {
    if ( this._super( event ) ) {
      return true;
    }

    if ( event.target.ownerDocument != this.document[ 0 ] ) {
      return true;
    }

    if ( $( event.target ).closest( ".cke_dialog" ).length ) {
      return true;
    }
  },

 /*! jQuery UI - v1.10.2 - 2013-10-28
  *  http://dev.ckeditor.com/ticket/10269 - bugfix
  *  moveToTop fix to accommodate windowed editors
  */
  _moveToTop: function ( event, silent ) {
    if ( !event || !this.options.modal ) {
      this._super( event, silent );
    }
  }
});

comment:28 Changed 6 years ago by Scott González

Great. Can you report back once you've had this in production for a while, or done rigorous in-house testing? Once you've confirmed this, I'll update the ticket description. Hopefully other users will test out your fix as well so they can report success/failure in their apps.

If you want to put together a reduced test case showing the iframe issue using just jQuery UI, we can get that portion of the fix into dialog as well. I just want to keep anything CKEditor-specific out of jQuery UI.

comment:29 Changed 6 years ago by Kevin H. Kamel

QA came back and found a problem with the functionality of CKEditor under IE8/9/10/11. Basically there are some drop downs within the primary toolbar that are misbehaving. Under Firefox the drop downs (which produce an iframe displaying the drop down content) were not displaying the second time, which is why I was applying the _moveToTop fix above, however there is apparently a similar problem under IE where the drop downs display properly, but immediately disappear after displaying.

I rolled back to 1.9.x and sure enough the problem doesn't exist there - so it's likely related to the same issues that were affecting other parts of the tool.

The good news is that this appears to be the last issue that needs resolution for me to attempt a production release. The bad news is that I have literally no idea how this display/hide event is occurring.

I'm a little surprised to be hearing about this now, as I can find no-one else complaining about this specific problem. Maybe nobody else got this far with getting CKEditor 4.x running within a 1.10.x jQuery UI modal, but I find that a little hard to believe.

From what I can tell the following series of events occurs:

  1. Click link activating the jQueryUI dialog
  2. Click link for drop down within jQueryUI dialog
  3. Upon clicking observe that the drop down did not appear, however appears the textarea is now selected
  4. Click link for drop down within jQueryUI dialog
  5. Upon clicking observe that the drop down now has appeared, however almost immediately it will disappear.

You can see from the above sequence that no event actually occurs which would cause a change in focus. It seems the drop down is disappearing after a timer goes off or something. I have no such timers in my code.

From here my next plan is to write up an isolated test on jsfiddle to attempt to get help finding a solution.

If anyone has ideas on what is causing this, would love to hear them. We are pretty close at having something viable.

comment:30 Changed 6 years ago by Kevin H. Kamel

Found a final workaround for the aforementioned issue. Anyone interested may visit the following fiddle with working CKEditor 4.3.1 within a jQueryUI 1.10.2 dialog. I had problems with the page scrolling all the way to the top with jQuery 1.10.3, issues that I do not plan to dig into right now.

Fiddle: http://jsfiddle.net/kamelkev/HU8Qt/3/

Code that we have tested in IE 8/9/10, and latest versions of autoupdated browsers (Firefox,Chrome,Safari):

$.widget( "ui.dialog", $.ui.dialog, {
 /*! jQuery UI - v1.10.2 - 2013-12-12
  *  http://bugs.jqueryui.com/ticket/9087#comment:27 - bugfix
  *  http://bugs.jqueryui.com/ticket/4727#comment:23 - bugfix
  *  allowInteraction fix to accommodate windowed editors
  */
  _allowInteraction: function( event ) {
    if ( this._super( event ) ) {
      return true;
    }

    // address interaction issues with general iframes with the dialog
    if ( event.target.ownerDocument != this.document[ 0 ] ) {
      return true;
    }

    // address interaction issues with dialog window
    if ( $( event.target ).closest( ".cke_dialog" ).length ) {
      return true;
    }

    // address interaction issues with iframe based drop downs in IE
    if ( $( event.target ).closest( ".cke" ).length ) {
      return true;
    }
  },
 /*! jQuery UI - v1.10.2 - 2013-10-28
  *  http://dev.ckeditor.com/ticket/10269 - bugfix
  *  moveToTop fix to accommodate windowed editors
  */
  _moveToTop: function ( event, silent ) {
    if ( !event || !this.options.modal ) {
      this._super( event, silent );
    }
  }
});

I am deploying this during the first week of January at which point I will report back again.

comment:31 Changed 6 years ago by Kevin H. Kamel

Ok I have had this in production for a few weeks now. We have fairly high volume where the above fix is deployed - everything seems working as expected.

comment:32 Changed 6 years ago by Scott González

Description: modified (diff)

comment:33 Changed 6 years ago by Scott González

Thanks, I've updated the ticket body to guide any users who find this ticket directly to your patch.

comment:34 Changed 6 years ago by tj.vantoll

#9767 is a duplicate of this ticket.

comment:35 Changed 6 years ago by ThiefMaster

It would be nice to have this fix in 1.11!

comment:36 in reply to:  30 Changed 6 years ago by marineam

Thanks a lot kamelkev for your input, this code snippet works like a charm. Hope this issue will get fixed in next jQueryUI release.

Do you have any idea on how to fix the page scrolling issue? When I open the editor in a dialog, it scrolls all the way up to the top of the page (jQuery 1.11.0, jQueryUI 1.10.4 and CKEditor 4.4.0).

(sorry to post here, couldn't find any open ticket related to this).

Thanks, M

Replying to kamelkev:

Found a final workaround for the aforementioned issue. Anyone interested may visit the following fiddle with working CKEditor 4.3.1 within a jQueryUI 1.10.2 dialog. I had problems with the page scrolling all the way to the top with jQuery 1.10.3, issues that I do not plan to dig into right now.

Fiddle: http://jsfiddle.net/kamelkev/HU8Qt/3/

Code that we have tested in IE 8/9/10, and latest versions of autoupdated browsers (Firefox,Chrome,Safari):

$.widget( "ui.dialog", $.ui.dialog, {
 /*! jQuery UI - v1.10.2 - 2013-12-12
  *  http://bugs.jqueryui.com/ticket/9087#comment:27 - bugfix
  *  http://bugs.jqueryui.com/ticket/4727#comment:23 - bugfix
  *  allowInteraction fix to accommodate windowed editors
  */
  _allowInteraction: function( event ) {
    if ( this._super( event ) ) {
      return true;
    }

    // address interaction issues with general iframes with the dialog
    if ( event.target.ownerDocument != this.document[ 0 ] ) {
      return true;
    }

    // address interaction issues with dialog window
    if ( $( event.target ).closest( ".cke_dialog" ).length ) {
      return true;
    }

    // address interaction issues with iframe based drop downs in IE
    if ( $( event.target ).closest( ".cke" ).length ) {
      return true;
    }
  },
 /*! jQuery UI - v1.10.2 - 2013-10-28
  *  http://dev.ckeditor.com/ticket/10269 - bugfix
  *  moveToTop fix to accommodate windowed editors
  */
  _moveToTop: function ( event, silent ) {
    if ( !event || !this.options.modal ) {
      this._super( event, silent );
    }
  }
});

I am deploying this during the first week of January at which point I will report back again.

comment:37 Changed 5 years ago by usmonster

It seems that, since jQuery UI 1.11, the _moveToTop override is no longer necessary (see #9166). Can anyone else confirm? If this is true, then the CKEditor workaround could be reduced to something like this:

$.widget( "ui.dialog", $.ui.dialog, {
 /**
  * jQuery UI v1.11+ fix to accommodate CKEditor (and other iframed content) inside a dialog
  * @see http://bugs.jqueryui.com/ticket/9087
  * @see http://dev.ckeditor.com/ticket/10269
  */
  _allowInteraction: function( event ) {
    return this._super( event ) ||

      // addresses general interaction issues with iframes inside a dialog
      event.target.ownerDocument !== this.document[ 0 ] ||

      // addresses interaction issues with CKEditor's dialog windows and iframe-based dropdowns in IE
      !!$( event.target ).closest( ".cke_dialog, .cke_dialog_background_cover, .cke" ).length;
  }
});

...I also wonder if the event.target.ownerDocument check is necessary or redundant. Thoughts?

Tested with CKEditor 4.4.6 / jQuery UI 1.11.2 on IE9, Firefox 35, Chrome 40.

Last edited 5 years ago by usmonster (previous) (diff)

comment:38 Changed 5 years ago by Kevin H. Kamel

Hi,

I'm going to experiment and see if we can get 1.11.4 working. The last time I tried this I found that my fix was fundamentally flawed, but that it didn't work at all with 1.10.3 - hence why we only posted for 1.10.2

I'm really hoping the above comment is true, and I can just simplify the fix and move on.

Last edited 5 years ago by Kevin H. Kamel (previous) (diff)

comment:39 Changed 5 years ago by Kevin H. Kamel

Hi usmonster,

I agree with your assessment. The override for _moveToTop is no longer necessary. The initial fix from ticket http://bugs.jqueryui.com/ticket/10269 appears to be the only thing we need for this to work in jQuery UI 1.11.4. I noticed your _allowInteraction code is different, I cannot confirm if that works or not. The variant included below has been in production for me for 18 months, and I know for sure that works.

Fiddle: http://jsfiddle.net/kamelkev/80qex6v1/6/

Code that we have tested in IE 9/10/11, and latest versions of autoupdated browsers (Firefox,Chrome,Safari):

$.widget( "ui.dialog", $.ui.dialog, {
 /*! jQuery UI - v1.11.4 - 2015-06-05
  *  http://bugs.jqueryui.com/ticket/9087#comment:27 - bugfix
  *  http://bugs.jqueryui.com/ticket/4727#comment:23 - bugfix
  *  allowInteraction fix to accommodate windowed editors
  */
  _allowInteraction: function( event ) {
    if ( this._super( event ) ) {
      return true;
    }

    // address interaction issues with general iframes with the dialog
    if ( event.target.ownerDocument != this.document[ 0 ] ) {
      return true;
    }

    // address interaction issues with dialog window
    if ( $( event.target ).closest( ".cke_dialog" ).length ) {
      return true;
    }

    // address interaction issues with iframe based drop downs in IE
    if ( $( event.target ).closest( ".cke" ).length ) {
      return true;
    }
  }
});

Last edited 5 years ago by Kevin H. Kamel (previous) (diff)
Note: See TracTickets for help on using tickets.