[Webkit-unassigned] [Bug 21840] execCommand insertImage inserts image into wrong place

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=21840





--- Comment #10 from Tony Chang (Google) <tony at chromium.org>  2010-01-28 20:41:31 PST ---
(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).

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.