Bug 55354 – Assert that 0 <= offset <= lastOffsetInNode in Position's constructor, moveToPosition, and moveToOffset

• Status: NEW
• Component: HTML Editing

[Ryosuke Niwa]

Right now, we have both legacy editing positions and new positions.  To make sure new types of positions aren't using legacy offsets (below 0 or above the maximum offset), we should assert the offset is between 0 and the last offset in the container node in Position::Position, Position::moveToOffset, and Position::moveToPosition.

[Ryosuke Niwa]

Created attachment 84023 [details]
work in progress

[Ryosuke Niwa]

Now that the bug 51389 has been fixed, the only thing that prevents me from posting a patch for this bug is TypingCommand::forwardDeleteKeyPressed:

        if (!startingSelection().isRange() || selectionToDelete.base() != startingSelection().start())
            selectionAfterUndo = selectionToDelete;
        else {
            // It's a little tricky to compute what the starting selection would have been in the original document.
            // We can't let the VisibleSelection class's validation kick in or it'll adjust for us based on
            // the current state of the document and we'll get the wrong result.
            Position extent = startingSelection().end();
            if (extent.deprecatedNode() != selectionToDelete.end().deprecatedNode())
                extent = selectionToDelete.extent();
            else {
                int extraCharacters;
                if (selectionToDelete.start().deprecatedNode() == selectionToDelete.end().deprecatedNode())
                    extraCharacters = selectionToDelete.end().deprecatedEditingOffset() - selectionToDelete.start().deprecatedEditingOffset();
                else
                    extraCharacters = selectionToDelete.end().deprecatedEditingOffset();
@                extent = Position(extent.deprecatedNode(), extent.deprecatedEditingOffset() + extraCharacters, Position::PositionIsOffsetInAnchor);
            }
            selectionAfterUndo.setWithoutValidation(startingSelection().start(), extent);
        }

In running editing/undo/undo-forward-delete-boundary.html, we forward delete each letter in "wo<b>rd</b> and undo it.  When we forward delete the first letter, we get the following values at @:

(gdb) p startingSelection().showTreeForThis()
BODY	0x129859dc0
	#text	0x1298617c0 "\n"
	DIV	0x129862a60 CLASS=editing
		#text	0x1298604e0 "\n"
		SPAN	0x129860540
SE			#text	0x129848440 "This o"
			B	0x12985d950
				#text	0x129868c60 "rd "
			#text	0x12985bae0 "should be selected, since the test deleted it a character at a time and then did an undo."
		#text	0x12985bbc0 "\n"
	#text	0x129858560 "\n\n"
	SCRIPT	0x1298585c0
		#text	0x1298586e0 "\nrunEditingTest();\n"
start: legacy, offset, offset:5
end: legacy, offset, offset:6
$1 = void
Current language:  auto; currently c++
(gdb) p selectionToDelete.showTreeForThis()
BODY	0x129859dc0
	#text	0x1298617c0 "\n"
	DIV	0x129862a60 CLASS=editing
		#text	0x1298604e0 "\n"
		SPAN	0x129860540
SE			#text	0x129848440 "This o"
			B	0x12985d950
				#text	0x129868c60 "rd "
			#text	0x12985bae0 "should be selected, since the test deleted it a character at a time and then did an undo."
		#text	0x12985bbc0 "\n"
	#text	0x129858560 "\n\n"
	SCRIPT	0x1298585c0
		#text	0x1298586e0 "\nrunEditingTest();\n"
start: legacy, offset, offset:5
end: legacy, offset, offset:6
$2 = void

And we end up creating a position with offset 7 to incorporate "w" that has already been deleted.  In various parts of editing code, we do need to store such "invalid" positions for undo/redo operations.  Maybe that we need a variant of Position (PositionForUndo or UndoablePosition?) that tolerates such positions.

[Ryosuke Niwa]

Created attachment 84529 [details]
work in progress 2