322207 - merging two paragraphs using backspace/delete key leaves an unwanted BR element

Status: RESOLVED FIXED

(Core :: DOM: Editor, defect)

Description

[dpopa]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8) Gecko/20051111 Firefox/1.5
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8) Gecko/20051111 Firefox/1.5

When you have 2 adjacent paragraphs and delete characters from:
 - the second paragraph starting from the middle using the BACKSPACE key
     -- OR --
 - the first paragraph starting from the middle using the DELETE key

when there are no more characters to delete from the curent paragraph and attempt to delete one more the 2 adjacent paragraphs are joined, but an BR tag is inserted where it should not be any.

Reproducible: Always

Steps to Reproduce:
1.goto http://www.mozilla.org/editor/midasdemo/ in view HTML source mode
2. type the following HTML :
<p>first line</p>
<p>second line</p>
3. go back to rich text mode
4. click in the first paragraph and set focus between "n" and "e", press the DELETE key twice.
Actual Results:  
<p>first lin<br>second line</p>

Expected Results:  
<p>first linsecond line</p>

Comment 1

[Martijn Wargers (dead)]

Confirmed. I can reproduce this with the steps described below, and IE6 acts like described in the expected results. Also, I couldn't find an obvious duplicate.

Comment 2

[Alfonso Martinez]

Confirming according to comment 1

Comment 3

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

Taking bug.

I agree this bug is very annoying in Midas or Composer, but I don't want
to change the existing behavior for Thunderbird w/o a word from a mail peer.

Comment 4

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

Attachment: fix #1

Fixes the bug for all html editors but htmlmail.

Comment 5

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

Attachment: fix #2

better patch merging inlines inside blocks only when it can

Comment 6

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

This bug is not limited to Midas but affects all editors. Updating summary.

Comment 7

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

patch successfully tested in

  Seamonkey (trunk)
  Thunderbird (trunk), no effect as expected
  Firefox (trunk)
  Nvu 1.0

Comment 8

[[e-mail]]

Comment on attachment 236942 [details] [diff] [review]
fix #2

It does help if I apply the right patch, but instead I spent most of today verifying that attachment 236940 [details] [diff] [review] is no good, sigh...

Comment 9

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

Comment on attachment 236942 [details] [diff] [review]
fix #2

asking boris to sr

Comment 10

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 236942 [details] [diff] [review]
fix #2

>Index: nsHTMLEditRules.cpp
>-  if (nsHTMLEditUtils::IsParagraph(aNodeLeft))
>+  if (nsHTMLEditUtils::IsParagraph(aNodeLeft) && (mFlags & nsIPlaintextEditor::eEditorMailMask))

I don't think we should special-case mail here.  If you're worried about it, please just cc a mailnews peer on this bug and ask him to comment.  But I think you should just make this change and post to the relevant newsgroup to notify them of the change or something. If they have a problem with it, then we worry about it.

>     if (lastLeft && firstRight && mHTMLEditor->NodesSameType(lastLeft, firstRight))
>     {
>-      return JoinNodesSmart(lastLeft, firstRight, aOutMergeParent, aOutMergeOffset);
>+      if ((nsEditor::IsTextNode(lastLeft) && nsEditor::IsTextNode(firstRight)) ||

Why do we need the nsEditor::IsTextNode(firstRight) check?  The nodes passed NodesSameType(), no?

Comment 11

[Scott MacGregor]

I'm happy with this change for html mail compose. Thanks for checking with me!

Comment 12

[dpopa]

(In reply to comment #7)

Hello there, 

Guys, let's make sure that the we're doing the right thing here, that is the fix work even the selection is somewhere deep inside one of the 2 adjacent paragraphs, like in this case (posible selection places marked with pipe):

<p>first <b>line|</b></p>
<p><u>|second line</u></p>

> patch successfully tested in
> 
>   Seamonkey (trunk)
>   Thunderbird (trunk), no effect as expected
>   Firefox (trunk)
>   Nvu 1.0
> 

Comment 13

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

Attachment: in answer to bz's comments with scott's approval for email

Comment 14

[[e-mail]]

Comment on attachment 237094 [details] [diff] [review]
in answer to bz's comments with scott's approval for email

>-  else if (nsHTMLEditUtils::IsList(aNodeLeft)
>-           || mHTMLEditor->IsTextNode(aNodeLeft))
>+  if (nsHTMLEditUtils::IsList(aNodeLeft)
>+      || mHTMLEditor->IsTextNode(aNodeLeft))
Maybe editor likes to be different but the jst-review simulacrum will back me up when I say that we prefer to have the || at the end of the previous line ;-)

Comment 15

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 237094 [details] [diff] [review]
in answer to bz's comments with scott's approval for email

>Index: nsHTMLEditRules.cpp
>+  if (nsHTMLEditUtils::IsList(aNodeLeft)
>+      || mHTMLEditor->IsTextNode(aNodeLeft))

Generally speaking, what Neil said. Not sure what the module style is here, but operator at end of line is the general Gecko style at this point, I believe.

>     if (lastLeft && firstRight && mHTMLEditor->NodesSameType(lastLeft, firstRight))
>     {
>+      if (nsEditor::IsTextNode(lastLeft)
>+          || mHTMLEditor->mHTMLCSSUtils->ElementsSameStyle(lastLeft, firstRight))

Why not just add to the condition instead of creating a nested if?  I always find the nested ifs harder to read, but maybe it's just me.

sr=bzbarsky with the nits.

Comment 16

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

fixed and checked in (trunk). Code style updated as requested by bz.