Bug 7579 – TinyMCE: Implement execCommand(insertImage, ...)

• Status: RESOLVED
• Resolution: FIXED
• Component: HTML Editing

[Justin Garcia]

Implement execCommand insertImage

[Justin Garcia]

Created attachment 6911 [details]
patch

Implemented InsertHTML and added a layout test.
ReplaceSelectionCommand wasn't pruning empty non-blockflow containers (like <span></span>), or empty blockflow containers with non-zero height (like <div style="border: 1px solid black;"></div>).

-            QPtrList<NodeImpl> blocks;
-            NodeImpl *node = beyondEndNode->enclosingInlineElement();
-            NodeImpl *refNode = m_lastNodeInserted.get();
+            NodeImpl* node = beyondEndNode->enclosingInlineElement();
+            NodeImpl* refNode = m_lastNodeInserted.get();
+            NodeImpl* nodeToPrune = node->parentNode();
             while (node) {
                 if (node->isBlockFlowOrBlockTable())
                     break;
                     
                 NodeImpl *next = node->nextSibling();
-                blocks.append(node->enclosingBlockFlowElement());
                 computeAndStoreNodeDesiredStyle(node, styles);
                 removeNode(node);

Using a QPtrList wasn't unnecessary.  We're removing siblings, no more than one enclosingBlockFlowElement would have survived this removal.
Use parentNode() instead of enclosingBlockFlowElement(), if we only want to prune blockFlows, that should be part of the pruning rule inside shouldPruneNode.

[Darin Adler]

Comment on attachment 6911 [details]
patch

I don't think enabledPaste is a good name for the function any more if it's being used for other insertion. Are you sure that's the right function to use?

I think ReplaceSelectionCommand::prune would read better as a for loop.

[Justin Garcia]

(In reply to comment #2)
> (From update of attachment 6911 [details] [edit])
> I don't think enabledPaste is a good name for the function any more if it's
> being used for other insertion. Are you sure that's the right function to use?

Inadvertently yes, but that's because enabledPaste is wrong and doesn't check that there is something on the pasteboard.  I also found that most of these enabledXXX methods don't check that the selection for editability, which seems wrong to me.  I should fix this.

[David Harrison]

shouldPruneNode() will return true for a rendered leaf node (because it has a no rendered children).  r-

Also, "prune" is not an appropriate name because a) it does not necessarily remove any nodes, and b) it may remove more than just the passed node.  Maybe "removeEnclosingUnrenderedSubtree"?

[Justin Garcia]

Created attachment 6931 [details]
patch

> I don't think enabledPaste is a good name for the function any more if it's
> being used for other insertion. Are you sure that's the right function to use?

I changed this and now use enabledAnySelection.  The enabledXXX functions need work, I filed 7650.

> I think ReplaceSelectionCommand::prune would read better as a for loop.

I disagree, I left the while loop in.

> shouldPruneNode() will return true for a rendered leaf node (because it has a
> no rendered children).  r-

True, but it will never be passed a leaf because 1) prune is only passed containers and 2) prune only ascends straight upward.

> Also, "prune" is not an appropriate name because a) it does not necessarily remove any nodes

I don't think prune implies that nodes will necessarily be removed, only that they will be removed if they are superfluous.

> b) it may remove more than just the passed node.

To make it clear what may be pruned I made the function into removeNodeAndPruneAncestors.  This also frees us from worrying that we'll prune leaves.

[Darin Adler]

Comment on attachment 6931 [details]
patch

Is it correct to just append the value between two single quote characters? Don't we have to do some escaping to handle special characters?

Do we need a local variable for baseURL? If it's just "", why not just pass "" to the function?

Is "" the right thing to use for the base URL? How did you test that in other browsers?

I guess the local variable for selectReplacement is a way to document what the false is, but I'd probably just use "false" for brevity.

A note about the "user interface" variant: To implement that properly we'll have to involve the WebView's delegate as we do for window.alert -- we never want WebKit to put up a sheet or a dialog box without giving the application control.

Overall looks great to me. I'll set review+ despite the special character issue, but that's worth considering.

[Justin Garcia]

Removed the selectReplacement bool and insert the image by creating an image, setting its src, then putting it into a fragment.  Using createFragmentFromMarkup seemed too heavyweight to me.  This avoids the escaping issue.