Bug 24586 – maxDeepOffset is confusing and should be removed

• Status: RESOLVED
• Resolution: FIXED
• Component: HTML Editing

[Eric Seidel (no email)]

maxDeepOffset is confusing and should be removed

I'll attach some patches here to try and do so.

[Eric Seidel (no email)]

Created attachment 28593 [details]
First attempt to kill maxDeepOffset

 WebCore/ChangeLog                           |   50 +++++++++++++++++++++++++++
 WebCore/dom/Position.cpp                    |   20 ++++++++--
 WebCore/dom/Position.h                      |    5 +++
 WebCore/editing/CompositeEditCommand.cpp    |    4 +-
 WebCore/editing/DeleteSelectionCommand.cpp  |    8 ++---
 WebCore/editing/Editor.cpp                  |    1 +
 WebCore/editing/InsertLineBreakCommand.cpp  |    3 +-
 WebCore/editing/InsertListCommand.cpp       |    4 +-
 WebCore/editing/ReplaceSelectionCommand.cpp |    5 ++-
 WebCore/editing/TypingCommand.cpp           |    2 +-
 WebCore/editing/VisibleSelection.cpp        |    6 ++--
 WebCore/editing/htmlediting.cpp             |   26 +++++++------
 WebCore/editing/visible_units.cpp           |    6 ++--
 WebCore/page/AccessibilityObject.cpp        |    4 +-
 WebCore/page/AccessibilityRenderObject.cpp  |   13 ++++---
 WebCore/rendering/RenderBox.cpp             |    9 ++---
 16 files changed, 118 insertions(+), 48 deletions(-)

[Eric Seidel (no email)]

Created attachment 28605 [details]
Further efforts to remove maxDeepOffset

 WebCore/ChangeLog                          |   57 ++++++++++++++++++++++++++++
 WebCore/dom/Position.cpp                   |   44 +++++++++++++--------
 WebCore/dom/Position.h                     |    7 ++-
 WebCore/dom/PositionIterator.cpp           |   18 ++++++---
 WebCore/editing/DeleteSelectionCommand.cpp |    9 ++--
 WebCore/editing/Editor.cpp                 |    4 +-
 WebCore/editing/VisiblePosition.cpp        |   12 +++--
 WebCore/editing/htmlediting.cpp            |   18 +++++----
 WebCore/editing/htmlediting.h              |    2 +-
 WebCore/editing/visible_units.cpp          |   21 +++++-----
 10 files changed, 135 insertions(+), 57 deletions(-)

[Justin Garcia]

+        Rename maxDeepOffset to lastOffsetInNode

I think that lastOffsetInNode is misleading because the offset returned isn't necessarily "in" the node given to the function.  I think it's a good description of what maxDeepOffset returns for text nodes and containers, but not so good for what it returns for tables, images, br, etc.  I think you could sp

[Eric Seidel (no email)]

> I think that lastOffsetInNode is misleading because the offset returned isn't
> necessarily "in" the node given to the function.  I think it's a good
> description of what maxDeepOffset returns for text nodes and containers, but
> not so good for what it returns for tables, images, br, etc.  I think you could
> sp

Looks like your comment got cut off.

The offset that lastOffsetInNode returns is actually *in* the node.  Or rather, the position is.  Even if the editing code doesn't interpret it as being inside the node.   [img, 1], that position is still "in" img, even though it's invalid, so the editing code treats it as after the img.

Anyway, very interested in seeing the rest of your comment.

[Justin Garcia]

> the editing code doesn't interpret it as being inside the node.

This was exactly my point.  What do you think about lastOffsetInNodeForEditing or lastEditingOffsetInNode?

[Eric Seidel (no email)]

(In reply to comment #5)
> > the editing code doesn't interpret it as being inside the node.
> 
> This was exactly my point.  What do you think about lastOffsetInNodeForEditing
> or lastEditingOffsetInNode?

Maybe lastNodeOffsetForEditing?

[Justin Garcia]

(In reply to comment #6)
> (In reply to comment #5)
> > > the editing code doesn't interpret it as being inside the node.
> > 
> > This was exactly my point.  What do you think about lastOffsetInNodeForEditing
> > or lastEditingOffsetInNode?
> 
> Maybe lastNodeOffsetForEditing?

Sounds good to me.

[Justin Garcia]

This is definitely the right direction.

-    if (startNode->renderer()
-        && ((startNode->renderer()->isTable() && !startNode->renderer()->isInline())
-            || startNode->renderer()->isHR())
-        && p.m_offset == maxDeepOffset(startNode))
-        return VisiblePosition(Position(startNode, 0));
+    if (renderedAsNonInlineTableOrHR(startNode->renderer()) && p.atEndOfNode())
+        return firstPositionInNode(startNode);

That said, I think that  "atEndOfNode" is inaccurate.  [hr, 1] is demonstrative of this.  [hr, 1] isn't at the "end of" the hr, it's after it.  Same for a table, an image, etc.

[Eric Seidel (no email)]

Ok, my current thinking for new names are:

firstPositionInNode and lastPositionInNode become:
// These functions match the 0/maxDeepOffset behavior,
// "deep" is intended to convey that they will prefer the deeper
// of two editing positions corresponding to a node
lastDeepEditingPositionForNode(node)
firstDeepEditingPositionForNode(node)


// lastOffsetInNode will become
int lastNodeOffsetForEditing(node)

// the new functions atStartOfNode and atEndOfNode will become:
atFirstEditingPositionForNode()
atLastEditingPositionForNode()

Once we have the ability to tell if a position is container relative or neighbor relative, we can add more specific check functions for just before the node, just inside the node, etc.

Sound good?

[Eric Seidel (no email)]

Created attachment 28775 [details]
Rename first/lastPositionInNode to first/lastDeepEditingPositionForNode

 WebCore/dom/Position.cpp                    |   15 ++++++++-------
 WebCore/dom/Position.h                      |    9 +++++----
 WebCore/dom/PositionIterator.cpp            |    2 +-
 WebCore/editing/CompositeEditCommand.cpp    |    4 ++--
 WebCore/editing/DeleteSelectionCommand.cpp  |    6 +++---
 WebCore/editing/InsertLineBreakCommand.cpp  |    2 +-
 WebCore/editing/InsertListCommand.cpp       |    4 ++--
 WebCore/editing/ReplaceSelectionCommand.cpp |    4 ++--
 WebCore/editing/TypingCommand.cpp           |    2 +-
 WebCore/editing/VisibleSelection.cpp        |    6 +++---
 WebCore/editing/htmlediting.cpp             |   16 ++++++++--------
 WebCore/editing/visible_units.cpp           |    8 ++++----
 WebCore/page/AccessibilityObject.cpp        |    2 +-
 WebCore/page/AccessibilityRenderObject.cpp  |    4 ++--
 WebCore/rendering/RenderBox.cpp             |    8 ++++----
 15 files changed, 47 insertions(+), 45 deletions(-)

[Eric Seidel (no email)]

Created attachment 28776 [details]
Rename lastOffsetInNode to lastOffsetForEditing and atStart/atEndOfNode to atFirst/atLastEditingPositionForNode

 WebCore/dom/Position.cpp                   |   18 +++++++++---------
 WebCore/dom/Position.h                     |    6 ++++--
 WebCore/dom/PositionIterator.cpp           |   10 +++++-----
 WebCore/editing/DeleteSelectionCommand.cpp |    6 +++---
 WebCore/editing/Editor.cpp                 |    4 ++--
 WebCore/editing/htmlediting.cpp            |    8 ++++----
 WebCore/editing/htmlediting.h              |    2 +-
 WebCore/editing/visible_units.cpp          |    6 +++---
 8 files changed, 31 insertions(+), 29 deletions(-)

[Eric Seidel (no email)]

I've made the renames locally, and will squash these down into 2 patches.

[Eric Seidel (no email)]

Created attachment 28788 [details]
Squashed patch with renames

 WebCore/ChangeLog                           |   98 +++++++++++++++++++++++++++
 WebCore/dom/Position.cpp                    |   63 ++++++++++++------
 WebCore/dom/Position.h                      |   15 ++++-
 WebCore/dom/PositionIterator.cpp            |   18 +++--
 WebCore/editing/CompositeEditCommand.cpp    |    4 +-
 WebCore/editing/DeleteSelectionCommand.cpp  |   17 ++---
 WebCore/editing/Editor.cpp                  |    3 +-
 WebCore/editing/InsertLineBreakCommand.cpp  |    3 +-
 WebCore/editing/InsertListCommand.cpp       |    4 +-
 WebCore/editing/ReplaceSelectionCommand.cpp |    5 +-
 WebCore/editing/TypingCommand.cpp           |    2 +-
 WebCore/editing/VisiblePosition.cpp         |   12 ++--
 WebCore/editing/VisibleSelection.cpp        |    6 +-
 WebCore/editing/htmlediting.cpp             |   44 +++++++------
 WebCore/editing/htmlediting.h               |    2 +-
 WebCore/editing/visible_units.cpp           |   27 ++++----
 WebCore/page/AccessibilityObject.cpp        |    4 +-
 WebCore/page/AccessibilityRenderObject.cpp  |   13 ++--
 WebCore/rendering/RenderBox.cpp             |    9 +--
 19 files changed, 246 insertions(+), 103 deletions(-)

[Eric Seidel (no email)]

The last patch is the combination of the previous 4 patches.  I tried squashing it down into 2 and it ended up more trouble than it was worth.  I updated the ChangeLog when combining the two original patches into one.  This is ready for review again. :)  Once this lands I will get to work on removing the use of Position::offset() from our code, and making actual position offsets opaque to the editing code.

[Eric Seidel (no email)]

Comment on attachment 28788 [details]
Squashed patch with renames

Justin reviewed this over IRC (from the shuttle).  

His comments:

some weird formatting in the change log

FIXED.

in the second line of the section that starts with "editing positions"
don't really need "deep"

I'm going to leave it for now.  The point was to identify that it was inside the node if possible.  We can change it later (since this code is still very in flux.)

the rest of the editing code will treat [img, 0]
+// as "the first position before the image"
just "the position before the image" i think

Will fix.

Thanks again Justin!

[Eric Seidel (no email)]

Committing to http://svn.webkit.org/repository/webkit/trunk ...
	M	WebCore/ChangeLog
	M	WebCore/dom/Position.cpp
	M	WebCore/dom/Position.h
	M	WebCore/dom/PositionIterator.cpp
	M	WebCore/editing/CompositeEditCommand.cpp
	M	WebCore/editing/DeleteSelectionCommand.cpp
	M	WebCore/editing/Editor.cpp
	M	WebCore/editing/InsertLineBreakCommand.cpp
	M	WebCore/editing/InsertListCommand.cpp
	M	WebCore/editing/ReplaceSelectionCommand.cpp
	M	WebCore/editing/TypingCommand.cpp
	M	WebCore/editing/VisiblePosition.cpp
	M	WebCore/editing/VisibleSelection.cpp
	M	WebCore/editing/htmlediting.cpp
	M	WebCore/editing/htmlediting.h
	M	WebCore/editing/visible_units.cpp
	M	WebCore/page/AccessibilityObject.cpp
	M	WebCore/page/AccessibilityRenderObject.cpp
	M	WebCore/rendering/RenderBox.cpp
Committed r41863