Opened 7 years ago

Closed 7 years ago

Last modified 7 years ago

#1553 closed defect (fixed)

editor._createLink is not a function

Reported by: guest Owned by: gogo
Priority: normal Milestone: 0.97
Component: Xinha Core Version: trunk
Severity: normal Keywords:
Cc: ethan.jucovy@…

Description

There's some confusing code in XinhaCore.js related to execCommand("_createLink"):

	    case "createlink":
	      this._createLink();

The _createLink function is never defined on the Xinha editor object.

From code comments elsewhere, it looks like the intention is to define the function in a plugin, which will be modules/CreateLink if no other plugin (like Linker) overrides it. ("This is the standard implementation of the Xinha.prototype._createLink method")

But, Xinha.prototype._createLink is NOT defined by Linker, nor by CreateLink. Instead, both plugins work by overriding the default behavior of the createlink button on the toolbar.

Default behavior:

    createlink: [ "Insert Web Link", ["ed_buttons_main.png",6,1], false, function(e) { e._createLink(); } ],

CreateLink override:

   editor.config.btnList.createlink[3] = function() { self.show(self._getSelectedAnchor()); }

Linker override:

    editor.config.btnList.createlink[3]
      =  function(e, objname, obj) { linker._createLink(linker._getSelectedAnchor()); };

(Here linker refers to the Linker plugin object, not the xinha editor object. The Linker object defines a _createLink method on itself, but not on the xinha editor object.)

CreateLink is a module, so it will always be loaded and override the default behavior. But there are three problems with this:

  1. It's confusing to somebody reading the code.
  2. It means there is no standard way to trigger a "createlink" command or to listen for a "createlink" event (which would be useful; see #1552)
  3. XinhaCore.js itself assumes the editor object has a _createLink method

XinhaCore.js tries to call editor._createLink() if you double-click on an anchor element:

  this.dblclickList =
  {
    "a": [function(e, target) {e._createLink(target);}],
    "img": [function(e, target) {e._insertImage(target);}]
  };

This causes a Javascript error in browsers, because e._createLink is not defined.

In fact the DoubleClick plugin has to override this definition:

  this.editor.dblClickList = {
    a: [ function(e) {e.config.btnList['createlink'][3](e); } ],

The same approach is used with InsertImage and editor._insertImage, but there it seems to be done (more) correctly: InsertImage defines editor.prototype._insertImage, and execCommand("insertimage") calls that function.

Proposed solutions:

  1. (Better solution) The code should define editor.prototype._createLink, and execCommand("createlink") should be the standard way to trigger a link-creation action.
  2. (Worse solution) References to editor._createLink should be removed.

Attachments (1)

link.diff (3.4 KB) - added by guest 7 years ago.
fixed case issues in original patch (createlink vs createLink)

Download all attachments as: .zip

Change History (5)

comment:1 follow-up: Changed 7 years ago by guest

I've attached a patch which addresses these problems.

(It's from a git diff, so patch -p1 < link.diff - not -p0.)

The patch does the following:

  • Linker and CreateLink now define an editor._createLink function, when they are first loaded. In both cases, the function gets the selected anchor, and then calls the plugin's own "interesting" method (which is historically called .show CreateLink and ._createLink in Linker). The function can also receive an explicit anchor element to be used instead of the selected anchor.
  • editor.execCommand("createlink") is now the standard way to invoke link-creation. To pass an explicit anchor element, use editor.execCommand("createlink", false, anchorEl) instead.
  • The default double-click handler for anchor elements now calls `editor.execCommand("createlink", false, anchorEl). This is defined in XinhaCore.js and also in the DoubleClick plugin.

Further cleanup would be nice - the two linky plugins both define some functions with identical logic that could be moved to core, and I think the same could be said of the default logic for the anchor-double-clicker. But those can be addressed separately, and are not essential.

Changed 7 years ago by guest

fixed case issues in original patch (createlink vs createLink)

comment:2 Changed 7 years ago by ejucovy

  • Resolution set to fixed
  • Status changed from new to closed

I've now committed the patch in r1271

For posterity: I tried to track down when and why the editor._createLink confusion began. I traced it back to http://trac.xinha.org/browser/branches/ray/modules/CreateLink/link.js?rev=892 but I wasn't able to dig any farther, and that changeset doesn't seem to explain why.

comment:3 in reply to: ↑ 1 Changed 7 years ago by ejucovy

Replying to guest:

Further cleanup would be nice - the two linky plugins both define some functions with identical logic that could be moved to core, and I think the same could be said of the default logic for the anchor-double-clicker. But those can be addressed separately, and are not essential.

I filed a ticket to record this "nicetohave", #1554

comment:4 Changed 7 years ago by ejucovy

  • Milestone set to 0.97
Note: See TracTickets for help on using tickets.