818835 - Merging table cells should insert a line break between merged contents when needed

Status: NEW

(Core :: DOM: Editor, defect)

Description

[Daniel Glazman (:glazou) (not active in Mozilla any more)]

The current code for nsHTMLEditor::MergeCells() doesn't check if
a <br> should be inserted between target cell's contents and
cellToMerge's content. If both cells contain for instance only
inline prose and no trailing <br>, the text nodes are glued
together, leading to a result requiring painful post-process
manual correction....

BlueGriffon users have complained about this behaviour, hence the
current bug.

Comment 1

[Daniel Glazman (:glazou) (not active in Mozilla any more)]

Attachment: proposed fix

Comment 2

[(no longer active)]

Comment on attachment 689141 [details] [diff] [review]
proposed fix

Review of attachment 689141 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good in general, but please address the comments below.  Also, this needs a test as well.  (And it doesn't need superreview.)

::: editor/libeditor/html/nsTableEditor.cpp
@@ +2348,5 @@
>        nsresult res = DeleteNode(cellChild->AsDOMNode());
>        NS_ENSURE_SUCCESS(res, res);
>        insertIndex = 0;
> +    }
> +    else {

Nit: please keep the else on the same line as the closing brace for the previous if.

@@ +2356,5 @@
> +      if (!isTargetCellEmpty) {
> +        // Let's look for the last child of the target cell that is not
> +        // an empty text node
> +        nsIContent* targetCellChild = targetCell->GetLastChild();
> +        nsCOMPtr<nsIDOMNode> targetCellChildNode = targetCellChild->AsDOMNode();

You shouldn't need to convert this into an nsIDOMNode.  You can just use the nsIContent*, I believe.

@@ +2360,5 @@
> +        nsCOMPtr<nsIDOMNode> targetCellChildNode = targetCellChild->AsDOMNode();
> +        bool isEmptyTextNode = false;
> +        while (targetCellChildNode
> +               && nsEditor::IsTextNode(targetCellChildNode)
> +               && (NS_SUCCEEDED(IsEmptyNode(targetCellChildNode, &isEmptyTextNode)) && isEmptyTextNode)) {

Nit: please place the and's at the end of the previous lines.

@@ +2373,5 @@
> +        if (!isBlock && !nsTextEditUtils::IsBreak(targetCellChildNode)) {
> +          // It's not, we may have to insert a break...
> +          // Let's look for the first child of the source cell that is not
> +          // an empty text node
> +          nsCOMPtr<nsIDOMNode> cellChildNode = cellToMerge->GetFirstChild()->AsDOMNode();

No need for nsIDOMNode here too.

@@ +2376,5 @@
> +          // an empty text node
> +          nsCOMPtr<nsIDOMNode> cellChildNode = cellToMerge->GetFirstChild()->AsDOMNode();
> +          while (cellChildNode
> +                 && nsEditor::IsTextNode(cellChildNode)
> +                 && (NS_SUCCEEDED(IsEmptyNode(cellChildNode, &isEmptyTextNode)) && isEmptyTextNode)) {

Ditto about the and's.

Comment 3

[(no longer active)]

(And please submit the patch to the try server as well with the test added for this fix.)

Comment 4

[[e-mail]]

Comment on attachment 689141 [details] [diff] [review]
proposed fix

>+        while (targetCellChildNode
[Can this ever be null?]

>+               && nsEditor::IsTextNode(targetCellChildNode)
Nit: &&s at ends of lines please

>+            // so both last visible node of target cell and first visible node of
>+            // source cell are not blocks or breaks. We need to insert
>+            // a break as first child of source cell so the merged contents
>+            // are separated by that break.
>+            nsCOMPtr<nsIDOMNode> brNode;
>+            res = CreateBR(aCellToMerge, 0, address_of(brNode));
Why add this to the cell to merge when you're immediately going to delete it and add it back to the target cell?

Comment 5

[Daniel Glazman (:glazou) (not active in Mozilla any more)]

(In reply to [e-mail] from comment #4)

> >+            res = CreateBR(aCellToMerge, 0, address_of(brNode));
> Why add this to the cell to merge when you're immediately going to delete it
> and add it back to the target cell?

Just because it was simpler than using the number of children already
present in the target cell ; but since we already computed len above,
you're right, I can change that code. Thanks for sharp eye.

Comment 6

[BugBot [:suhaib / :marco/ :calixte]]

The bug assignee is inactive on Bugzilla, so the assignee is being reset.