Bug 63100 - Add BeforeChildren and AfterChildren to the Position's anchor types
Summary: Add BeforeChildren and AfterChildren to the Position's anchor types
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: HTML Editing (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Ryosuke Niwa
URL:
Keywords:
Depends on:
Blocks: 63040
  Show dependency treegraph
 
Reported: 2011-06-21 15:40 PDT by Ryosuke Niwa
Modified: 2011-06-24 10:45 PDT (History)
6 users (show)

See Also:


Attachments
work in progress (11.65 KB, patch)
2011-06-21 15:42 PDT, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
Adds BeforeChildren/AfterChildren (17.10 KB, patch)
2011-06-21 18:48 PDT, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
updated after 89440 (17.13 KB, patch)
2011-06-22 10:48 PDT, Ryosuke Niwa
enrica: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ryosuke Niwa 2011-06-21 15:40:47 PDT
In order to resolve the bug 63040, we need to add BeforeChildren and AfterChildren as new anchor types.
Comment 1 Ryosuke Niwa 2011-06-21 15:42:10 PDT
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.
Comment 2 Ryosuke Niwa 2011-06-21 18:48:21 PDT
Created attachment 98096 [details]
Adds BeforeChildren/AfterChildren
Comment 3 Ryosuke Niwa 2011-06-22 10:48:57 PDT
Created attachment 98199 [details]
updated after 89440
Comment 4 Ryosuke Niwa 2011-06-22 10:56:08 PDT
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.
Comment 5 Ryosuke Niwa 2011-06-22 12:00:10 PDT
Can someone review this patch?  It's really important to get these anchor types be added to proceed our refactoring process.
Comment 6 Eric Seidel (no email) 2011-06-22 13:47:52 PDT
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 7 Ryosuke Niwa 2011-06-22 13:49:28 PDT
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.
Comment 8 Ryosuke Niwa 2011-06-22 21:39:54 PDT
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 9 Enrica Casucci 2011-06-23 16:12:36 PDT
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 10 Ryosuke Niwa 2011-06-23 16:17:57 PDT
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.
Comment 11 Ryosuke Niwa 2011-06-23 16:18:22 PDT
(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.
Comment 12 Ryosuke Niwa 2011-06-24 10:45:05 PDT
Committed r89683: <http://trac.webkit.org/changeset/89683>
Comment 13 Ryosuke Niwa 2011-06-24 10:45:21 PDT
Thanks for the review!  I'm so excited to land this patch.