Position should support neighbor-anchored positions

• Component: HTML Editing
• Status: RESOLVED FIXED

[Eric Seidel (no email)]

Position should support neighbor-anchored positions

I'm using this as a tracking bug, for the introduction of neighbor anchored positions.  I have some patches in my tree I could upload here for examples as to what I mean.

Right now we have Position which can hold [node, offset], but sometimes [node, 0] means "first offset in node" and sometimes it means "last offset before node", based on whether or not editingIgnoresContent(node) returns true or not.

I plan to change this so that neighbor-anchoring or container-anchoring is explicit in the Position object.  Then we can have a position [table, 0] which means "first offset in the table" (which we can't currently express using a Position object since tables are ignored content for editing).

[Eric Seidel (no email)]

This will take a few patches to make this work.  Two are attached to bug 24966 and bug 25056 (the big one).  I'll attach the last 3 (smaller) pieces here.

[Eric Seidel (no email)]

Created attachment 29274 [details]
Add containerNode(), computeOffsetInContainerNode(), nodeBeforePosition(), nodeAfterPosition()

 WebCore/ChangeLog        |   21 +++++++++++
 WebCore/dom/Position.cpp |   85 ++++++++++++++++++++++++++++++++++++++++++++++
 WebCore/dom/Position.h   |   17 +++++++++
 3 files changed, 123 insertions(+), 0 deletions(-)

[Eric Seidel (no email)]

Created attachment 29275 [details]
Pre-computed and store AnchorType and isLegacyEditingPosition on Position during construction

 WebCore/ChangeLog        |   29 +++++++++++++++++++++
 WebCore/WebCore.base.exp |    1 +
 WebCore/dom/Position.cpp |   63 ++++++++++++++++++++++++++++++++++++---------
 WebCore/dom/Position.h   |   50 ++++++++++++++++++++----------------
 4 files changed, 108 insertions(+), 35 deletions(-)

[Eric Seidel (no email)]

Created attachment 29276 [details]
Fix Position == operator to match our new fancy positions

 LayoutTests/ChangeLog                              |   21 ++++++++++++
 LayoutTests/editing/execCommand/19089-expected.txt |    2 +-
 .../insert-paragraph-04-expected.checksum          |    2 +-
 .../inserting/insert-paragraph-04-expected.png     |  Bin 29882 -> 29883 bytes
 .../inserting/insert-paragraph-04-expected.txt     |    3 +-
 WebCore/ChangeLog                                  |   20 ++++++++++++
 WebCore/dom/Position.h                             |   33 ++++++++++++++++++--
 7 files changed, 74 insertions(+), 7 deletions(-)

[Eric Seidel (no email)]

The next step will be to remove rangeCompliantEquivalent https://bugs.webkit.org/show_bug.cgi?id=25057.

Next step after that will be to make Range methods (including JSRange stuff) create non-legacy Parent-anchored positions.  This means that positions passed into the engine through range methods will support specifying offsets inside ignored editing content.  We'll have to fix the resulting fallout in the editing code as a result. :)

[Darin Adler]

Comment on attachment 29274 [details]
Add containerNode(), computeOffsetInContainerNode(), nodeBeforePosition(), nodeAfterPosition()

> +    switch (anchorType()) {
> +    case PositionIsOffsetInAnchor:
> +        return m_anchorNode.get();

I prefer indenting the cases inside the switch statements.

> +    case PositionIsOffsetInAnchor:
> +    {

Opening brace should be on same line as case.

> +// FIXME: Position should store an AnchorType up-front, instead of lazily
> +// determining it via editingIgnoresContent(anchorNode())
> +// That would require fixing moveToOffset to know how to recompute the AnchorType
> +// for positions which need historical editing-compatible behavior
> +// (and for explicitly anchored positions to ASSERT_NOT_REACHED())

Or maybe you could just compute the anchor type at construction time for now.

> +    // These are always DOM compliant values.  Editing positions like [img, 0] (aka [img, before])
> +    // will return img->parentNode() and img->nodeIndex() from these functions.
> +    Node* containerNode() const;
> +    int computeOffsetInContainerNode() const;
> +
> +    // These are convenience methods which are smart about whether the position is neighbor anchored or parent anchored
> +    Node* nodeBeforePosition() const;
> +    Node* nodeAfterPosition() const;

I'm not sure I understand the rule for when you use "compute" and when you don't. It seems that nodeBeforePosition and nodeAfterPosition can both be expensive operations that involve calling childNode, so it seems they should have compute in their names unless and until you change the class's design.

r=me, but I am concerned about the "compute" rule.

[Darin Adler]

Comment on attachment 29275 [details]
Pre-computed and store AnchorType and isLegacyEditingPosition on Position during construction

> -Element *Position::element() const
> +Element* Position::element() const

It seems to me that both element() and node() need to eventually be private unless it's clearer whether they should be named anchorNode or what.

> +    Position()
> +        : m_offset(0)
> +        , m_anchorType(PositionIsOffsetInAnchor)
> +        , m_isLegacyEditingPosition(true) // FIXME: Switch the null position to not be in legacy mode
>      {}

Braces should be on separate lines.

>      void clear() { m_anchorNode.clear(); m_offset = 0; }

This should set m_anchorType to PositionIsOffsetInAnchor and set m_isLegacyEditingPosition to true.

> +    // These should only be used for PositionIsOffsetInAnchor positions, unless
> +    // the position is a legacy editing position.
> +    void moveToPosition(PassRefPtr<Node> anchorNode, int offset);
> +    void moveToOffset(int offset);

Do the implementations have an assertion to this effect? Also, I think that moveToPosition seems like a full assignment that could entirely wipe out the old value of Position. I don't think a Position should necessarily have a persistent mode that outlasts it value. Obviously, moveToOffset is different in this respect since it's not a full assignment.

r=me

[Eric Seidel (no email)]

(In reply to comment #6)
> (From update of attachment 29274 [details] [review])
> > +    switch (anchorType()) {
> > +    case PositionIsOffsetInAnchor:
> > +        return m_anchorNode.get();
> 
> I prefer indenting the cases inside the switch statements.

Me too. :)  But that's a violation of the style guide:
http://webkit.org/coding/coding-style.html

r=me to change the style guide to require indenting case: statements.

> > +    case PositionIsOffsetInAnchor:
> > +    {
> 
> Opening brace should be on same line as case.

This wasn't covered in the style guide, so I made a guess. :)

> > +// FIXME: Position should store an AnchorType up-front, instead of lazily
> > +// determining it via editingIgnoresContent(anchorNode())
> > +// That would require fixing moveToOffset to know how to recompute the AnchorType
> > +// for positions which need historical editing-compatible behavior
> > +// (and for explicitly anchored positions to ASSERT_NOT_REACHED())
> 
> Or maybe you could just compute the anchor type at construction time for now.

Yeah, done in a separate patch.  I tried to keep these small-as-possible.  This rewrite has been a huge pain, mostly due to my initial attempts trying to change too much at once.

> > +    // These are always DOM compliant values.  Editing positions like [img, 0] (aka [img, before])
> > +    // will return img->parentNode() and img->nodeIndex() from these functions.
> > +    Node* containerNode() const;
> > +    int computeOffsetInContainerNode() const;
> > +
> > +    // These are convenience methods which are smart about whether the position is neighbor anchored or parent anchored
> > +    Node* nodeBeforePosition() const;
> > +    Node* nodeAfterPosition() const;
> 
> I'm not sure I understand the rule for when you use "compute" and when you
> don't. It seems that nodeBeforePosition and nodeAfterPosition can both be
> expensive operations that involve calling childNode, so it seems they should
> have compute in their names unless and until you change the class's design.

Agreed!  Yes, these should definitely be named "compute" since they call childNode() in the parent-anchor case.


I'm not sure I like computeOffsetInContainer() anymore.  I might change callers to use a toParentAnchoredPosition() or makeParentAnchored() call instead.  Not sure yet.

[Eric Seidel (no email)]

Comment on attachment 29275 [details]
Pre-computed and store AnchorType and isLegacyEditingPosition on Position during construction

Waiting to land this one until bug 25056 is resolved.  Will address your comments of course!

[Eric Seidel (no email)]

Comment on attachment 29274 [details]
Add containerNode(), computeOffsetInContainerNode(), nodeBeforePosition(), nodeAfterPosition()

I made Darin's suggested modifications and committed this patch as:
Committing to http://svn.webkit.org/repository/webkit/trunk ...
        M       WebCore/ChangeLog
        M       WebCore/dom/Position.cpp
        M       WebCore/dom/Position.h
Committed r42263

Removing review flag and marking obsolete so as to not to cause confusion with the other 2 remaining patches (one which needs review, and the other which needs landing).

[Darin Adler]

Comment on attachment 29276 [details]
Fix Position == operator to match our new fancy positions

I'm not sure that we want == to behave this way. We might instead want a named comparison that works this way, perhaps named equivalent() or something like that. I think reasonable to overload the built-in operators when the operation is a truly primitive low-level operation, but when two Position objects are == even though some of their accessors will return different values, I think we are creating a design problem.

[Eric Seidel (no email)]

Comment on attachment 29275 [details]
Pre-computed and store AnchorType and isLegacyEditingPosition on Position during construction

Committed this patch as:
Committing to http://svn.webkit.org/repository/webkit/trunk ...
	M	WebCore/ChangeLog
	M	WebCore/WebCore.base.exp
	M	WebCore/dom/Position.cpp
	M	WebCore/dom/Position.h
Committed r43075

I made corrections per your review comments.

[Maciej Stachowiak]

Comment on attachment 29276 [details]
Fix Position == operator to match our new fancy positions

Marking as r- per Darin's comment above.

[Eric Seidel (no email)]

I believe we have neighbor anchored positions these days, so I'm closing this.  CC'd ryosuke on the off chance he finds the old patch on this bug useful.