Bug 7579 - TinyMCE: Implement execCommand(insertImage, ...)
Summary: TinyMCE: Implement execCommand(insertImage, ...)
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: HTML Editing (show other bugs)
Version: 420+
Hardware: Macintosh OS X 10.4
: P2 Normal
Assignee: Justin Garcia
URL:
Keywords:
Depends on:
Blocks: 6627
  Show dependency treegraph
 
Reported: 2006-03-03 14:56 PST by Justin Garcia
Modified: 2006-03-08 14:59 PST (History)
0 users

See Also:


Attachments
patch (11.89 KB, patch)
2006-03-07 01:09 PST, Justin Garcia
harrison: review-
Details | Formatted Diff | Diff
patch (12.10 KB, patch)
2006-03-07 23:00 PST, Justin Garcia
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Justin Garcia 2006-03-03 14:56:10 PST
Implement execCommand insertImage
Comment 1 Justin Garcia 2006-03-07 01:09:12 PST
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.
Comment 2 Darin Adler 2006-03-07 08:58:20 PST
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.
Comment 3 Justin Garcia 2006-03-07 14:33:51 PST
(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.
Comment 4 David Harrison 2006-03-07 18:35:23 PST
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"?
Comment 5 Justin Garcia 2006-03-07 23:00:52 PST
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.
Comment 6 Darin Adler 2006-03-08 00:19:51 PST
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.
Comment 7 Justin Garcia 2006-03-08 14:59:59 PST
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.