Bug 63100

Created attachment 98069 [details] work in progress Here's my first attempt. format-block-contenteditable-false.html and move-left-right.html mysteriously fail for now.

Created attachment 98096 [details] Adds BeforeChildren/AfterChildren

Created attachment 98199 [details] updated after 89440

The reason I'd like to introduce BeforeChildren and AfterChildren anchor types now is to ease the pain to transition from legacy editing positions to new (non-legacy) editing positions. Because old legacy positions were almost always anchored before/after nodes, the editing code in general isn't good at supporting a position which hangs off of a non-text node with an offset. Adding BeforeChildren and AfterChildren, and restricting nodes that can be used for OffsetInAnchor to be text nodes will make it easier for us to mitigate this issue. (I'm also going to add a new constructor that takes Text* and offset to Position). Once Text*, offset constructor and the patch for this bug is landed, then I can start removing places where we instantiate positions that are offsets in non-text node.

Can someone review this patch? It's really important to get these anchor types be added to proceed our refactoring process.

Comment on attachment 98199 [details] updated after 89440 View in context: https://bugs.webkit.org/attachment.cgi?id=98199&action=review > Source/WebCore/dom/Position.cpp:371 > + // FIXME: Position after anchor shouldn't be considered as at the first editing position for node > + // since that position resides outside of the node. Perhaps. This was done to match old behavior of Position(img, 0), etc.

Comment on attachment 98199 [details] updated after 89440 View in context: https://bugs.webkit.org/attachment.cgi?id=98199&action=review >> Source/WebCore/dom/Position.cpp:371 >> + // since that position resides outside of the node. > > Perhaps. This was done to match old behavior of Position(img, 0), etc. Right. We'd have to fix it at some point.

Now that the bug 63181 has been fixed, this is the only blocker for the bug 63040 - "positionIsOffsetInAnchor should only accept text nodes". Once this patch is landed, we can start removing positions that are offsets in the middle of a non-text nodes.

Comment on attachment 98199 [details] updated after 89440 View in context: https://bugs.webkit.org/attachment.cgi?id=98199&action=review Overall it looks good. Are all the editing tests passing? > Source/WebCore/ChangeLog:19 > + the offset in the anchor node for AfterChildren. I believe here it should be "the last offset" > Source/WebCore/ChangeLog:25 > + (WebCore::Position::computeNodeAfterPosition): Returns the first child of the anchor node for BeforeChildren And for AfterChildren ? > Source/WebCore/dom/Position.cpp:351 > + // since that position resides outside of the node. What would be the fix here? Why do you return true then?

Comment on attachment 98199 [details] updated after 89440 View in context: https://bugs.webkit.org/attachment.cgi?id=98199&action=review >> Source/WebCore/ChangeLog:19 >> + the offset in the anchor node for AfterChildren. > > I believe here it should be "the last offset" Oops, right. Will fix. >> Source/WebCore/ChangeLog:25 >> + (WebCore::Position::computeNodeAfterPosition): Returns the first child of the anchor node for BeforeChildren > > And for AfterChildren ? and null for AfterChildren.

(In reply to comment #9) > (From update of attachment 98199 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=98199&action=review > > Overall it looks good. Are all the editing tests passing? Yes, all editing tests passed at least when I ran them last night.

Committed r89683: <http://trac.webkit.org/changeset/89683>

Thanks for the review! I'm so excited to land this patch.