Backspace/delete at start of table cell shouldn't step out of cell

[Julie Parent]

Repro steps:
1. Given a contentEditable table, like <div contentEditable><table border=1><tr><td>1</td><td>2</td></tr><t<tr><td>3</td><td>4</td></tr></table></div>
2. Put cursor at the start of a cell, like before the 2
3. Hit backspace/delete

Result:
Focus moves to previous cell.

Expected Result:
Nop.  This is consistent with IE, FF, Word, TextEdit.

Note: if in the first cell, steps out of the table.  This is also inconsistent with other editors, and also should be a nop.

[Shezan Baig]

Created attachment 192930 [details]
additional inconsistencies

Added attachment showing additional inconsistencies.  I think these issues can all be fixed if we adopted the "Backspacing at the start of a table cell does nothing" clause in https://dvcs.w3.org/hg/editing/raw-file/tip/editing.html#the-delete-command

[Shezan Baig]

Created attachment 192988 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 192988 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=192988&action=review

> Source/WebCore/editing/TypingCommand.cpp:470
> +        Node* enclosingTableCell = enclosingNodeOfType(visibleStart.deepEquivalent(), &isTableCell);
> +        if (enclosingTableCell && visibleStart == firstPositionInNode(enclosingTableCell))

This checks whether selection start is at the beginning of table cell or not, not whether the cell is empty or not.
With this, we're going to make select all + delete no-op.

[Shezan Baig]

Comment on attachment 192988 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=192988&action=review

>> Source/WebCore/editing/TypingCommand.cpp:470
>> +        if (enclosingTableCell && visibleStart == firstPositionInNode(enclosingTableCell))
> 
> This checks whether selection start is at the beginning of table cell or not, not whether the cell is empty or not.
> With this, we're going to make select all + delete no-op.

I don't quite follow.  If we select all, then the selection will be VisibleSelection::RangeSelection, and will be handled at line 436.  In our case, we have VisibleSelection::CaretSelection, so visibleStart == visibleEnd in this case

[Ryosuke Niwa]

Comment on attachment 192988 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=192988&action=review

> LayoutTests/editing/deleting/backspace-at-table-cell-beginning.html:11
> +                    + "a table-cell is a no-op.  The first dump verifies "
> +                    + "that the caret stays before 'abc', and doesn't select "
> +                    + "the nested table.  The second dump verifies that the "
> +                    + "caret stays before 'def'.");

Wrong indentation. + should be indented by exactly 4 spaces.
By the way, I usually prefer putting this description in p with id=description and then do:
Markup.description(document.getElementById('description').textContent);
There's an added benefit that we can automate this in the future.

> LayoutTests/editing/deleting/forward-delete-at-table-cell-ending.html:12
> +Markup.description("This test verifies that forward delete at the end of a "
> +                    + "table-cell is a no-op.  The first dump verifies that "
> +                    + "the caret stays after 'abc'.  The second dump verifies "
> +                    + "that the caret stays after 'def', and doesn't select "
> +                    + "the nested table.");

Ditto about the wrong indentation.

[Shezan Baig]

Created attachment 193227 [details]
Patch (with review comments)

[WebKit Review Bot]

Comment on attachment 193227 [details]
Patch (with review comments)

Clearing flags on attachment: 193227

Committed r145871: <http://trac.webkit.org/changeset/145871>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.