Problem

  • Plugins allowing to insert/edit embedded content are not able to establish the proper selection context for plugin operations.

Details

  • A plugin that inserts complex content into the editor needs to be able to determine whether the cursor or current focus is within the inserted content, and be able to override the editor's selection/context in case it is.
  • For example, when using a plugin to embed code snippets into the content, moving the cursor/focus into that code, and invoking the plugin once again, then you end up with an embedded code snippet within the embedded code snippet. That is, because the plugin is not able to shift the selection/focus/context of the editor to the existing code snippet - in a cross-editor way.
  • Currently, most plugins that need to do this resort to implementing an editor-specific plugin, since Wysiwyg provides no API methods to adjust an editor's selection.
  • The adjusted selection/context obviously pertains to a specific plugin only. In case the current focus is within an content element that belongs to the plugin, then only that plugin needs the adjusted context for itself. In other words: It's not about updating the global selection/context of the editor.
  • To determine whether the current focus is within a plugin's content element, plugins can already use jQuery, which provides fast methods.
  • However, in order to set a custom context in a way the editor understands it (when calling the insert() and other methods later), the plugin needs a cross-editor API method to build a native editor selection context.
  • Plugins need to figure out the context for isNode() already, so that would be the location where a plugin would also want to set a custom context, in case of a match.

Proposed solution

  • Allow plugins to store a custom plugin.context from within plugin.isNode().
  • The required format of plugin.context is not clear yet. A simple DOM element may be sufficient, but we might as well need a string representation (as provided to .isNode() methods). It's also possible that it needs to be a true selection object.
  • Add a editor.instance.setSelection() method that the plugin may call before it calls editor.instance.insert(); i.e., before the actual insert/replace happens.

Related issues

#784434: Image goes in headings, Strip the WYSIWYG set block type from image on insert.
#594322: Add nicEdit insert method.
#693418: WYSIWYG API: data.node not always consistent
#1060552: Fire isNode on mouseUp and keyUp.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

TwoD’s picture

I'd love to be able to use a library like Rangy for much of this since it takes care of the cross-browser issues when dealing with selections. Granted, many of the major editors do this already, but some don't have a selection API at all so there would be nothing for us to build a cross-editor API around, unless we basically reinvent Rangy ourselves.

Not being able to move the selection "up" when they discover an element is "theirs" is indeed a big issue for some plugins. Several image/media handling modules would like to be able to select the topmost element in a small hierarchy when an image placeholder is clicked, so moving it will move captions etc as well. As it is now, plugins are forced to show single element placeholders without captions or other complex representations since the user can easily break them when not treated as a single unit.

I believe a full selection object is the most reliable context we could provide plugins. A string would require us to guess which one they'd like to select if it appears several times in the full contents, and a DOM element might not be granular enough for those wanting to select just a word. There's also the matter of making the distinction between text/source mode and WYSIWYG mode clear enough.

#1060552: Fire isNode on mouseUp and keyUp. is an interesting issue, but I don't think it's directly relevant here, not unless we also intend to change the reason for isNode() getting called (in-editor selection changed vs key event).

sun’s picture

Interesting. Obviously, it would be awesome if we wouldn't have to invent something ourselves, but - I may be mistaken - I think that some editors expect a "native" selection object to work with; which may contain (or is extended from) the browser's native selection object.

Very good point on partial text nodes - so a selection object it's going to be.

EugenMayer’s picture

Sounds pretty good and complete to me, very good writeup Daniel.

Just to be sure its not forgetten, i know that its Drupal default way to "first current stable, then backport" in the issues, but we would explictly require a Drupal 6 version, not a Drupal 7 version (what comes down to the sponsoring).

TwoD’s picture

@EugenMayer, since this would be a pretty new part of Wysiwyg that would mostly deal with the editor and browser APIs, I can with great confidence say that the code used in the D7 version would require minimal porting for use with D6.

@sun, if the editors want a native selection/range object, we'll be able to fetch it from Rangy and pass it on. The idea would be to fetch the current selection - either from the editor's API or directly via Rangy depending on what's practical - verify the range is inside the editor, add any meta-info we need, pass it to the plugin, check if it was modified, and finally notify the editor of any changes by translating the selection to what the editor expects - if needed at all.
We should not have to worry about the user changing the selection while plugins are processed, it's over quickly and I think all such events are even blocked while scripts run. We might on the other hand have to worry about what happens inside the editor if the selection is drastically changed by a plugin. I'm thinking about "dirty" temporary elements placed here and there by the editor. The editor's own selection API would deal with making these elements transparent to native plugins, so we might have to replicate that somehow.

We'll have to provide plugins with enough info to determine if they're operating in a WYSIWYG or Source mode or they'll end up breaking things badly. When in Source mode there will also be different markup types to consider, like XHTML, Markdown, BBCode etc. All plugins should be compatible with the WYSIWYG mode since it's guaranteed to be an [X]HTML DOM, but they should not touch the content unless they're compatible with the markup type used in Source mode. Some of this is meta-info currently in the data object passed to invoke(), but I think we can improve that. To make that work, Wysiwyg will have to know what markup type the editor's going to output... That can get out of hand quickly so let's focus on just XHTML for now. The "channel" for relaying the markup type must however exist from the beginning so plugins can default to doing nothing once support for other markup types is added.

webdeli’s picture

Wysiwyg will have to know what markup type the editor's going to output... That can get out of hand quickly so let's focus on just XHTML for now.

Therefore, how about a option along the lines of:

'#markuptype' => 'xhtml', be introduced?

TwoD’s picture

You mean for the text_format FAPI type?
That would be handy, if we could agree on what to do with '#markuptype' => 'unknown', and if it could be done per text format.

TwoD’s picture

Issue summary: View changes

Updated issue summary.

Yaron Tal’s picture

As far as I could see, this issue only applies to the ckeditor, as other editors either don't implement isNode() and invoke() or already always return a context (instead of sometimes a null object).
As for finding the context the plugin wants to change and keeping it cached so the plugin doesn't have to find it in both isNode() and invoke() I think this is something the plugins themselves should take care of (by means of a property or helper method).
The only thing that WYSIWYG needs to change, is to always return a context for the plugin to use. As I described in #2153751: isNode() has no way of getting a context when no element is selected TinyMCE already returns the context, although it works a bit different from the way ckeditor works.

I think there are two ways to get this to work.
1. Allow the plugin to access the whole selection variable in isNode() and invoke() so they can also use selection.getStartElement() to be able to traverse up the dom. This will be fully backwards compatible, because the current parameters will not change. As noted earlier, the selection object is coming from the editor, so is probably editor specific.
2. Use selection.getStartElement() if selection.getSelectedElement() returns no selection. This will most likely fix more plugins than it will break (but will most likely break something). Also it is (arguably) more expected behaviour.

I created a patch for both solutions, but I think #1 is a better solutions because it gives plugins more control over the context and is 100% backwards compatible. The selector object might be ckeditor-specific, but since there are only 2 editors using it (tinymce and ckeditor) and both editor implementations already work differently on this, I think this is a way to make plugins act the same on both editors.

I understand this issue is bigger than this, but my opinion is that other things need to be handled by the plugins themselves.

gmclelland’s picture

Status: Active » Needs review

Patches attached - I think you probably want this set to needs review

dutchyoda’s picture

I reworked solution #1 to work with the latest version.