Bug 63100 – Add BeforeChildren and AfterChildren to the Position's anchor types

• Component: HTML Editing
• Status: RESOLVED FIXED

[Ryosuke Niwa]

In order to resolve the bug 63040, we need to add BeforeChildren and AfterChildren as new anchor types.

[Ryosuke Niwa]

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.

[Ryosuke Niwa]

Created attachment 98096 [details]
Adds BeforeChildren/AfterChildren

[Ryosuke Niwa]

Created attachment 98199 [details]
updated after 89440

[Ryosuke Niwa]

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.

[Ryosuke Niwa]

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

[Eric Seidel (no email)]

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.

[Ryosuke Niwa]

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.

[Ryosuke Niwa]

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.

[Enrica Casucci]

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?

[Ryosuke Niwa]

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.

[Ryosuke Niwa]

(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.

[Ryosuke Niwa]

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

[Ryosuke Niwa]

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