maxDeepOffset is confusing and should be removed I'll attach some patches here to try and do so.
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(-)
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(-)
+ 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
> 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.
> 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?
(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?
(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.
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.
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?
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(-)
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(-)
I've made the renames locally, and will squash these down into 2 patches.
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(-)
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 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!
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