Bug 21840 – execCommand insertImage inserts image into wrong place

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

[Julie Parent]

Given the following html: <b>inside</b>outside
1. Select "outside"
2. execCommand('insertImage', false, 'myFavoriteImageUrl')

result: <b>inside<img src='myFavoriteImageUrl'></b> (image inside B tag)
expected result: <b>inside</b><img src='myFavoriteImageUrl'> (image outside B tag)

Click "eval once" in the link above to see demo.
Also seen in Chrome.

You can run the same demo in FF 3 and see that the image goes outside the B tag.

[Julie Parent]

Note that the same thing happens if you use execCommand('insertHTML', false, "myHtml")

[Julie Parent]

Both insertImage and insertHtml use executeInsertFragment under the covers, this is probably where the bug is.

[Tony Chang]

Created attachment 47384 [details]
Patch

[Tony Chang]

ReplaceSelection first deletes the selected text, then inserts the new HTML.  Unfortunately, DeleteSelection updates the cursor position after the delete to a visible position.  After the text is deleted, all we have left is <b>inside</b> and the visible position is inside the </b>.

To work around this, I save the insertion position as a dom node + offset (in this case, it would be that parent of the bold node with an offset of 1) before the delete and set the insertion position after the delete has happened rather than just using the cursor position.

[Tony Chang]

Comment on attachment 47384 [details]
Patch

I have a better fix for this that also fixes bug 23993.

[Tony Chang]

Created attachment 47595 [details]
Patch

[Tony Chang]

(In reply to comment #6)
> Created an attachment (id=47595) [details]
> Patch

This fixes the bug by changing the destination position in ReplaceSelection to be outside the end of preceding nodes as long as the visual position doesn't change.  This had the nice side effect of removing some extraneous spans from some other tests.

This also fixes bug 23993.

[Eric Seidel (no email)]

Comment on attachment 47595 [details]
Patch

Bah. Chrome/WebKit lost all my work I had typed into this field. :(

My comments again, in brief:
- ChangeLog needs information as to what your rebaseline changes are?  I don't understand why we're adding SPANs in some tests, etc.
- Tests are clearer when they include the expected results in them, and print PASS/FAIL.  backspace-avoid-preceding-style.html does not include its own expected results (it's OK as is, but could be better).
- I think I would have split this while() out into early returns:
 110     while (!pos.node()->hasTagName(brTag) && enclosing != pos.node() && VisiblePosition(pos) == VisiblePosition(pos.next()))
- What happens in the "return pos" case?  Should we ASSERT then?  Isn't that someone calling this function with an invalid position?  Or certainly there is a case where we try to walk out of the current position, and we should ASSERT then, no?
- It's not clear to me why enclosingBlockFlowElement is the right way to avoid the preceding nodes.

Why isn't this just the downstream position?  Or maybe I'm mixing my terms.  I think downstream positions are same equivalent visible position but the "last" position inside that equivilancy set.

[Tony Chang]

Created attachment 47670 [details]
Patch

[Tony Chang]

(In reply to comment #8)
> (From update of attachment 47595 [details])
> Bah. Chrome/WebKit lost all my work I had typed into this field. :(
> 
> My comments again, in brief:
> - ChangeLog needs information as to what your rebaseline changes are?  I don't
> understand why we're adding SPANs in some tests, etc.

These spans were previously necessary to undo styles added by preceding
nodes.  For example, <b>foo<span style="text-weight:normal">bar</span></b>
is now just <b>foo</b>bar.  The pixel tests all pass, so this is a less HTML
alternative.

Updated the ChangeLog to describe this.

> - Tests are clearer when they include the expected results in them, and print
> PASS/FAIL.  backspace-avoid-preceding-style.html does not include its own
> expected results (it's OK as is, but could be better).

I've updated backspace-avoid-preceding-style to now say "PASS".

> - I think I would have split this while() out into early returns:
>  110     while (!pos.node()->hasTagName(brTag) && enclosing != pos.node() &&
> VisiblePosition(pos) == VisiblePosition(pos.next()))

Sure, done.

> - What happens in the "return pos" case?  Should we ASSERT then?  Isn't that
> someone calling this function with an invalid position?  Or certainly there is
> a case where we try to walk out of the current position, and we should ASSERT
> then, no?

I don't understand the question.  We return the updated position.  It's ok if we
don't need to update the position, but they're all valid.

> - It's not clear to me why enclosingBlockFlowElement is the right way to avoid
> the preceding nodes.

This is kind of tricky.  It's because the visual position is the same, even
though we're in a different block.  For example, the two positions below are
the same visually:
<div>foo^</div>^

but if we insert in the latter, we end up in a new paragraph.
I added a comment trying to explain this.

> Why isn't this just the downstream position?  Or maybe I'm mixing my terms.  I
> think downstream positions are same equivalent visible position but the "last"
> position inside that equivilancy set.

I tried using downstream originally, but it doesn't seem to work.  I think it's
because downstream wants to return positions that are either in a text node or
just before a non-text node (according to a comment in Position.cpp).

[Ojan Vafai]

This patch does the wrong thing with the following html:
<b>foo|bar|</b>baz

Where "|" is the start/end of the selection. It inserts the new HTML after the b tag instead of inside of it.

[Tony Chang]

(In reply to comment #11)
> This patch does the wrong thing with the following html:
> <b>foo|bar|</b>baz
> 
> Where "|" is the start/end of the selection. It inserts the new HTML after the
> b tag instead of inside of it.

You're right, it does the wrong thing, but that's the behavior I'm seeing on ToT w/o the patch.  I don't think this patch regresses that behavior.

[Enrica Casucci]

(In reply to comment #12)
> (In reply to comment #11)
> > This patch does the wrong thing with the following html:
> > <b>foo|bar|</b>baz
> > 
> > Where "|" is the start/end of the selection. It inserts the new HTML after the
> > b tag instead of inside of it.
> 
> You're right, it does the wrong thing, but that's the behavior I'm seeing on
> ToT w/o the patch.  I don't think this patch regresses that behavior.

I'm not 100% convinced that this is the right approach, I'll do some investigation and I'll get back to you.

[Ojan Vafai]

Nevermind, as tony said, the case I'm pointing out currently fails on trunk and is a separate, but related bug. Test page: http://www.plexode.com/cgi-bin/eval3.py#ht=%3Cdiv%20id%3Douter%20contentEditable%3E%3Cb%20id%3Dinner%3EInside%3C%2Fb%3EOutside%3C%2Fdiv%3E&ohh=1&ohj=0&jt=var%20b%20%3D%20inner.firstChild%3B%0Awindow.getSelection().setBaseAndExtent(b%2C%201%2C%20b%2C%206)%3B%0Adocument.execCommand(%22insertImage%22%2C%20false%2C%20%22http%3A%2F%2Fwww.google.com%2Fimages%2Fart.gif%22)%3B%0Aouter.innerHTML&ojh=1&ojj=0&ms=100&oth=0&otj=0&cex=1

[Enrica Casucci]

(In reply to comment #14)
> Nevermind, as tony said, the case I'm pointing out currently fails on trunk and
> is a separate, but related bug. Test page:
> http://www.plexode.com/cgi-bin/eval3.py#ht=%3Cdiv%20id%3Douter%20contentEditable%3E%3Cb%20id%3Dinner%3EInside%3C%2Fb%3EOutside%3C%2Fdiv%3E&ohh=1&ohj=0&jt=var%20b%20%3D%20inner.firstChild%3B%0Awindow.getSelection().setBaseAndExtent(b%2C%201%2C%20b%2C%206)%3B%0Adocument.execCommand(%22insertImage%22%2C%20false%2C%20%22http%3A%2F%2Fwww.google.com%2Fimages%2Fart.gif%22)%3B%0Aouter.innerHTML&ojh=1&ojj=0&ms=100&oth=0&otj=0&cex=1

I think that a simpler and cleaner approach would be to modify DeleteSelectionCommand to add a placeholder in a case like this.
Creating a placeholder when you are deleting is easier than trying to find the correct position after you deleted.
It has the benefit of minimizing the code changes without introducing a new way of finding the position, while still keeping the benefit of eliminating all the spans.

[Tony Chang]

(In reply to comment #15)
> I think that a simpler and cleaner approach would be to modify
> DeleteSelectionCommand to add a placeholder in a case like this.
> Creating a placeholder when you are deleting is easier than trying to find the
> correct position after you deleted.
> It has the benefit of minimizing the code changes without introducing a new way
> of finding the position, while still keeping the benefit of eliminating all the
> spans.

That was my first attempt at fixing the bug :)

The problem was that adding a placeholder changes the behavior of DeleteSelection.  It changes how mergeParagraphs works and it changes the logic about whether to insert a break or not at the end of a document.  This causes lots of other layout tests to fail.

This lead me to the current approaches on this bug: keep track of the position by not modifying the DOM.  The first patch on this bug does that, but doesn't handle the isCaret() case so it doesn't fix bug 23993.  This could be fixed, but it ends up being more complicated because it has to handle cases where the insertion position is deleted by DeleteSelection (because, e.g., dom nodes are copied or paragraphs are merged).  It turns out that it is simpler to just compute the position after the all the DOM modifications have ended.

[Enrica Casucci]

(In reply to comment #16)
> (In reply to comment #15)
> > I think that a simpler and cleaner approach would be to modify
> > DeleteSelectionCommand to add a placeholder in a case like this.
> > Creating a placeholder when you are deleting is easier than trying to find the
> > correct position after you deleted.
> > It has the benefit of minimizing the code changes without introducing a new way
> > of finding the position, while still keeping the benefit of eliminating all the
> > spans.
> 
> That was my first attempt at fixing the bug :)
> 
> The problem was that adding a placeholder changes the behavior of
> DeleteSelection.  It changes how mergeParagraphs works and it changes the logic
> about whether to insert a break or not at the end of a document.  This causes
> lots of other layout tests to fail.
> 
> This lead me to the current approaches on this bug: keep track of the position
> by not modifying the DOM.  The first patch on this bug does that, but doesn't
> handle the isCaret() case so it doesn't fix bug 23993.  This could be fixed,
> but it ends up being more complicated because it has to handle cases where the
> insertion position is deleted by DeleteSelection (because, e.g., dom nodes are
> copied or paragraphs are merged).  It turns out that it is simpler to just
> compute the position after the all the DOM modifications have ended.

I'm not suggesting that you leave a placeholder all the time. 
If you limit the scope of the change to DeleteSelection to only cases where the lack of a placeholder leads to ambiguity about where to insert the new content, you should be fine.

[Tony Chang]

(In reply to comment #17)
> I'm not suggesting that you leave a placeholder all the time. 
> If you limit the scope of the change to DeleteSelection to only cases where the
> lack of a placeholder leads to ambiguity about where to insert the new content,
> you should be fine.

Right, I tried that.  I added a flag to DeleteSelection and only set the flag in ReplaceSelectionCommand.cpp when needed.  One example problem is that when you press the delete key, this calls DeleteSelectionCommand::doApply, which calls DeleteSelectionCommand::mergeParagraphs, which calls CompositeEditCommand::moveParagraphs which calls ReplaceSelectionCommand which causes the extra BR to be inserted.  In this case, we don't want a BR since we're just deleting.

If you think it's worthwhile, I will resurrect the patch and upload it for comparison.

[Adam Barth]

Comment on attachment 47670 [details]
Patch

This discussion seems to have stalled a month ago.  I'm not an editing expert, but this patch seems reasonable to me.  In the interest of moving forward, I'm marking this r+.

Tony: Please give Enrica some time to respond before landing this patch.

Enrica: If you still have concerns, please feel free to convert my r+ to an r-.

[Enrica Casucci]

(In reply to comment #19)
> (From update of attachment 47670 [details])
> This discussion seems to have stalled a month ago.  I'm not an editing expert,
> but this patch seems reasonable to me.  In the interest of moving forward, I'm
> marking this r+.
> 
> Tony: Please give Enrica some time to respond before landing this patch.
> 
> Enrica: If you still have concerns, please feel free to convert my r+ to an r-.

I think the approach is reasonable. I don't have any objection.

[Tony Chang]

Committed r55762: <http://trac.webkit.org/changeset/55762>

[Tony Chang]

*** Bug 23993 has been marked as a duplicate of this bug. ***