inline <br> does not get deleted when following some non-textual content

inline <br> does not get deleted when following some non-textual content

deleteSelectionCommand does not handle the case when a <br> element is inlined
after some non-textual content (input controls, image etc.).

When doing a back-delete at the start of a line following such a <br> the two
contiguous lines should merge and the <br> should get deleted.
Currently even though the <br> is deleted, another placeholder <br> is incorrectly
inserted at the same point, thus effectively there is no change.

We are incorrectly computing the <br> to be at the start of an empty line even
though the line is not empty.

In DeleteSelectionCommand::handleSpecialCaseBRDelete() added a check to verify
that the inline <br> is not on an empty line if the end node's previous sibling
is the <br> element.

WebKit changeset: http://trac.webkit.org/changeset/154479

R=eseidel@chromium.org

[leviw_travelin_and_unemployed, 2013-08-19 18:14:24]

https://codereview.chromium.org/23311004/diff/1/LayoutTests/editing/deleting/...
File LayoutTests/editing/deleting/delete-before-block-image-2-expected.txt
(right):

https://codereview.chromium.org/23311004/diff/1/LayoutTests/editing/deleting/...
LayoutTests/editing/deleting/delete-before-block-image-2-expected.txt:1: <img
id="img" style="display:block;" src="../resources/abe.png">
Is this related somehow?

https://codereview.chromium.org/23311004/diff/1/LayoutTests/editing/deleting/...
File LayoutTests/editing/deleting/delete-br-001.html (right):

https://codereview.chromium.org/23311004/diff/1/LayoutTests/editing/deleting/...
LayoutTests/editing/deleting/delete-br-001.html:26: testIt(1, 1, 2, 'abc<br
id="a"><br id="c">def');
Why did you make this change?

https://codereview.chromium.org/23311004/diff/1/LayoutTests/editing/deleting/...
File LayoutTests/editing/deleting/delete-inline-br.html (right):

https://codereview.chromium.org/23311004/diff/1/LayoutTests/editing/deleting/...
LayoutTests/editing/deleting/delete-inline-br.html:22: runEditingTest();
Why do we need the png results for this? Why not just dumpAsText?

[arpitab_, 2013-08-22 14:12:34]

Thanks for the review Levi! Shall upload another patch with the changes.

https://codereview.chromium.org/23311004/diff/1/LayoutTests/editing/deleting/...
File LayoutTests/editing/deleting/delete-before-block-image-2-expected.txt
(right):

https://codereview.chromium.org/23311004/diff/1/LayoutTests/editing/deleting/...
LayoutTests/editing/deleting/delete-before-block-image-2-expected.txt:1: <img
id="img" style="display:block;" src="../resources/abe.png">
On 2013/08/19 18:14:25, Levi wrote:
> Is this related somehow?
Because of the existing code, after deleting the given <br>, an extra
placeholder <br> is inserted using the InsertNodeBefore editing command which I
suspect causes the attributes rearrangement for the detached image element. (?)
But after the patch, the attributes remain the way they have been specified in
the test page.

https://codereview.chromium.org/23311004/diff/1/LayoutTests/editing/deleting/...
File LayoutTests/editing/deleting/delete-br-001.html (right):

https://codereview.chromium.org/23311004/diff/1/LayoutTests/editing/deleting/...
LayoutTests/editing/deleting/delete-br-001.html:26: testIt(1, 1, 2, 'abc<br
id="a"><br id="c">def');
On 2013/08/19 18:14:25, Levi wrote:
> Why did you make this change?
The sample html is: 'abc<br id="a"><br id="b"><br id="c">def'
For the given test we are supposed to place the caret after <br id="b"> and then
do a delete operation. This should result in: 'abc<br id="a"><br id="c">def'.
However, with the existing code, after removing the said <br>, an extra
placeholder <br> shall be inserted which obviously does not have the "id"
information, hence the change.

https://codereview.chromium.org/23311004/diff/1/LayoutTests/editing/deleting/...
File LayoutTests/editing/deleting/delete-inline-br.html (right):

https://codereview.chromium.org/23311004/diff/1/LayoutTests/editing/deleting/...
LayoutTests/editing/deleting/delete-inline-br.html:22: runEditingTest();
On 2013/08/19 18:14:25, Levi wrote:
> Why do we need the png results for this? Why not just dumpAsText?
Will change the layout testcase.

[tkent, 2013-09-05 06:07:38]

+yosin

[yosin_UTC9, 2013-09-05 06:35:28]

I have another patch for fixing BR related issues:
https://codereview.chromium.org/18799004/

I'll try whether my patch works for delete-inline-br.html or not.

[arpitab_, 2013-09-05 07:09:37]

On 2013/09/05 06:35:28, Yoshifumi Inoue wrote:
> I have another patch for fixing BR related issues:
> https://codereview.chromium.org/18799004/
> 
> I'll try whether my patch works for delete-inline-br.html or not.

Thanks for the update yosin.

[yosin_UTC9, 2013-09-05 07:24:35]

On 2013/09/05 07:09:37, arpitab_ wrote:
> On 2013/09/05 06:35:28, Yoshifumi Inoue wrote:
> > I have another patch for fixing BR related issues:
> > https://codereview.chromium.org/18799004/
> > 
> > I'll try whether my patch works for delete-inline-br.html or not.
> 
> Thanks for the update yosin.
My patch passes delete-inline-br.html.
It seems my patch covers more cases than this patch, because my patch fixes
editing/inserting/font-size-clears-from-typing-style.html.