Bug 24586

Summary: maxDeepOffset is confusing and should be removed
Product: WebKit Reporter: Eric Seidel (no email) <eric>
Component: HTML EditingAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: jparent, justin.garcia, kocienda, ojan
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Mac   
OS: OS X 10.5   
Attachments:
Description Flags
First attempt to kill maxDeepOffset
none
Further efforts to remove maxDeepOffset
none
Rename first/lastPositionInNode to first/lastDeepEditingPositionForNode
none
Rename lastOffsetInNode to lastOffsetForEditing and atStart/atEndOfNode to atFirst/atLastEditingPositionForNode
none
Squashed patch with renames eric: review+

Description Eric Seidel (no email) 2009-03-13 13:20:39 PDT
maxDeepOffset is confusing and should be removed

I'll attach some patches here to try and do so.
Comment 1 Eric Seidel (no email) 2009-03-13 13:21:41 PDT
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(-)
Comment 2 Eric Seidel (no email) 2009-03-13 15:23:24 PDT
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(-)
Comment 3 Justin Garcia 2009-03-15 02:36:22 PDT
+        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
Comment 4 Eric Seidel (no email) 2009-03-16 11:16:48 PDT
> 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.
Comment 5 Justin Garcia 2009-03-16 11:32:43 PDT
> 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?
Comment 6 Eric Seidel (no email) 2009-03-19 13:02:11 PDT
(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?
Comment 7 Justin Garcia 2009-03-19 13:12:22 PDT
(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.
Comment 8 Justin Garcia 2009-03-19 14:47:42 PDT
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.
Comment 9 Eric Seidel (no email) 2009-03-19 15:46:42 PDT
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?
Comment 10 Eric Seidel (no email) 2009-03-19 18:14:18 PDT
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(-)
Comment 11 Eric Seidel (no email) 2009-03-19 18:14:19 PDT
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(-)
Comment 12 Eric Seidel (no email) 2009-03-19 18:14:54 PDT
I've made the renames locally, and will squash these down into 2 patches.
Comment 13 Eric Seidel (no email) 2009-03-20 10:18:26 PDT
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(-)
Comment 14 Eric Seidel (no email) 2009-03-20 10:20:53 PDT
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.
Comment 15 Eric Seidel (no email) 2009-03-20 11:32:56 PDT
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!
Comment 16 Eric Seidel (no email) 2009-03-20 11:45:35 PDT
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