132837 - [PATCH]extra break inserted into new list item after using style

Status: VERIFIED FIXED

(Core :: DOM: Editor, defect)

Description

[sujay]

using 3/22 build on win

1) launch netscape
2) launch composer
3) make list
4) make bold
5) type "joe is sooo dreamy"
6) type return to make new list item
7) select "joe is sooo dreamy"
8) backspace
9) backspace again(this will delete 2nd bullet and keep caret in 1st bullet)
might have to hit backspace again one more time if 2nd bullet is still not
deleted.
10) click on bullet list icon

end result = 

*    O   <---  two different level bullets on the same line

Comment 1

[Joe Francis]

I've investigated this bug and determined the real problem (that casues the 
symptoms originally described).

Once you create the second list item in the original test case, the second list 
item is not empty.  Instead it has 2 breaks in it: one inside the bold and one 
not.  This gets you into the situation described here and in 132898 where 
clicking the list button makes you a new list (which in my opinion is correct).

The extra br is clearly wrong and if I fix that this bug goes away.

Comment 2

[Joe Francis]

Attachment: patch for nsHTMLEditor.cpp

The routine that copies style from the previous block into a new block is now
explicitly emptying the new block first.  This is because a block that passes
the IsEmptyNode() test is not neccessarily empty from a dom point of view.

Comment 3

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

Comment on attachment 77021 [details] [diff] [review]
patch for nsHTMLEditor.cpp

r=glazman

Comment 4

[kinmoz]

Comment on attachment 77021 [details] [diff] [review]
patch for nsHTMLEditor.cpp

sr=[e-mail]

I know we check for NS_SUCCEEDED in the |while| expression, but do we care if
the aNewBlock->GetFirstChild() call fails in the loop? That is if it fails
would we want to return instead of breaking out of the loop?

Comment 5

[Kevin McCluskey (gone)]

adt1.0.0

Comment 6

[Asa Dotzler [:asa]]

Comment on attachment 77021 [details] [diff] [review]
patch for nsHTMLEditor.cpp

a=asa (on behalf of drivers) for checkin to the 1.0 trunk

Comment 7

[Jaime Rodriguez, Jr.]

adt1.0.0+ (on ADT's behalf) approval for checkin into 1.0.

Comment 8

[Joe Francis]

fix landed on trunk

Comment 9

[sujay]

I can still reproduce this problem using 4/9 build using steps
originall described.

Comment 10

[Joe Francis]

moral: always test original testcase when fixing bug.  It turns out the original
testcase here was caused by two distinct bugs.  I fixed one and decalred
victory, but in fact had merely revealed the second.  Patch to fix second issue
is in 135337.

Comment 11

[Joe Francis]

fixed on trunk

Comment 12

[Jaime Rodriguez, Jr.]

ADT gave approval for checkin on 04.04. Why hasn't this landed on the branch?

Comment 13

[Kevin McCluskey (gone)]

Resolving as FIXED. Please add fixed1.0.0 keyword after the fix has been checked
into the 1.0.0 branch.

Comment 14

[sujay]

verified on 4/15 trunk build.

Comment 15

[Joe Francis]

fixed on branch

Comment 16

[sujay]

verified in 4/29 branch build.