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

RESOLVED FIXED

Status

()

Core
Editor
RESOLVED FIXED
13 years ago
12 years ago

People

(Reporter: dpopa, Assigned: glazou)

Tracking

(Depends on: 1 bug)

Trunk
x86
Windows XP
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

13 years ago
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>
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.
Depends on: 322205

Comment 2

13 years ago
Confirming according to comment 1
Status: UNCONFIRMED → NEW
Ever confirmed: true
(Assignee)

Comment 3

12 years ago
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.
Assignee: mozeditor → daniel
(Assignee)

Comment 4

12 years ago
Created attachment 236940 [details] [diff] [review]
fix #1

Fixes the bug for all html editors but htmlmail.
Attachment #236940 - Flags: review?(neil)
(Assignee)

Comment 5

12 years ago
Created attachment 236942 [details] [diff] [review]
fix #2

better patch merging inlines inside blocks only when it can
Attachment #236940 - Attachment is obsolete: true
Attachment #236942 - Flags: review?(neil)
Attachment #236940 - Flags: review?(neil)
(Assignee)

Comment 6

12 years ago
This bug is not limited to Midas but affects all editors. Updating summary.
Status: NEW → ASSIGNED
Summary: [MIDAS] BACKSPACE when at the begining of an paragraph leaves an unwanted BR. → merging two paragraphs using backspace/delete key leaves an unwanted BR element
(Assignee)

Comment 7

12 years ago
patch successfully tested in

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

Comment 8

12 years ago
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...
Attachment #236942 - Flags: review?(neil) → review+
(Assignee)

Comment 9

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

asking boris to sr
Attachment #236942 - Flags: superreview?(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?
Attachment #236942 - Flags: superreview?(bzbarsky) → superreview-

Comment 11

12 years ago
I'm happy with this change for html mail compose. Thanks for checking with me!
(Reporter)

Comment 12

12 years ago
(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
> 

(Assignee)

Comment 13

12 years ago
Created attachment 237094 [details] [diff] [review]
in answer to bz's comments with scott's approval for email
Attachment #236942 - Attachment is obsolete: true
Attachment #237094 - Flags: superreview?(bzbarsky)
Attachment #237094 - Flags: review?(neil)

Comment 14

12 years ago
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 ;-)
Attachment #237094 - Flags: review?(neil) → review+
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.
Attachment #237094 - Flags: superreview?(bzbarsky) → superreview+
(Assignee)

Comment 16

12 years ago
fixed and checked in (trunk). Code style updated as requested by bz.
Status: ASSIGNED → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.