120000 - Indent list inside table causes table to split

Status: VERIFIED FIXED

(SeaMonkey :: Composer, defect)

Description

[selmer (gone)]

1/14 03 trunk build

Take a document that has a table inside it.
Create several rows and columns within the table.
Within one of the cells, create a bullet list and add items to it.
At the end of the list, indent the last bullet.
Notice that the remainder of the table disappears and the cursor is located in a
wacky place.

Comment 1

[selmer (gone)]

Oops, wrong component.

Comment 2

[selmer (gone)]

.

Comment 3

[Charles Manske]

I can't reproduce this. Please supply a sample page with a table and exact
instructions about which cell to add the list to, how many items to add, etc.,
show this problem.

Comment 4

[sujay]

I just tried this also and I cannot reproduce..

Steve, can you append an attachment of the page you have 
this problem with?

Is it the focal review document ?

Comment 5

[selmer (gone)]

It's the focal document.  The html in there tends to cause composer fits in
general, so maybe this is an edge case.  I took the base document and added
lists to most of the RHS cells in the topmost accomplishments table before
experiencing this.

Comment 6

[sujay]

Steve, I know about the composer fits you are talking about. We've captured
most of them due to illegal HTML in these bugs:

bug 113310
bug 119565

But we're still trying to reproduce the indent-bullet problem you mention.

Comment 7

[Joe Francis]

Attachment: aol 2001 focal document

this document is used with the following test case:
In the rightmost column of the table in the Key Accomplishments section (that
is the column with "Progress and Accomplishments" in the first row) click in
the second row.  Then create a list by clicking on the make list button.  Type
"aaa".	Then click indent.

This reproduces Steve's problem reliably for me.

Comment 8

[Joe Francis]

099.   Note that the rest of the table doest really disappear. It just gets
split off.  Still a bad bug though.  

Comment 9

[Joe Francis]

attaching patch later tonight (it's on a powerbook thats out of juice and my cord 
is at home)

Comment 10

[Joe Francis]

I've reworked this patch.  There are three problems with indention that are
fixed here.  Only one of the fixes is strictly needed to solve this problem, but
other similar problems could occur without the other fixes.

For reviewers: here's what's changed.
1) nsHTMLEditRules::WillHTMLIndent() now skips over non-editable nodes.
2) nsHTMLEditRules::WillHTMLIndent() makes sure there is only a "current list"
or "current blockquote" to place indented material.  Before there could be both,
which was erroneous.
3) nsHTMLEditRules::GetInnerContent() now takes an index, and inserts elements
into the supplied nsISupportsArray at the index location, rather than appending.    

1) is needed to prevent creating blockquotes or sublists for non-editable
content we encounter.
2) is needed to prevent content from being reordered incorrectly in cases where
we are blockquoting some nodes and sublisting others.
3) is needed to prevent indented content from becoming reordered when we are
acting on multiple table cells or list items.

removing dataloss and regression keywords.  I don't think this bug was either,
though it is pretty nasty.

Comment 11

[Joe Francis]

Attachment: patch for editor/libeditor/html/nsHTMLEditRules/cpp

Comment 12

[Joe Francis]

Attachment: simplified test case

Simplified test case.  Open in Composer and try to indent "aaa".  Note that if
you create a similar looking document in Composer, the bug won't exhibit
itself.  Bug is exposed by the various non-rendering whitespace formatting
nodes in the html source.

Comment 13

[kinmoz]

Comment on attachment 66974 [details] [diff] [review]
patch for editor/libeditor/html/nsHTMLEditRules/cpp

The changes look ok to me, with the exception of the logic
changes in RemoveEmptyNodes(). I think it should be more
like the following, otherwise some of our blocks (p, pre, etc)
will always be candidates, even when the selection is in
them. This new way also has the added benefit of doing
the Is<blocktype>() checks once.

Or am I not reading this right?

Here's what I think it should be:



@@ -6719,12 +6735,10 @@
       node = do_QueryInterface(content);
       if (!node) return NS_ERROR_FAILURE;

-      PRBool bIsEmptyNode;
-      res = mHTMLEditor->IsEmptyNode(node, &bIsEmptyNode, PR_FALSE, PR_TRUE);
-      if (NS_FAILED(res)) return res;
+      PRBool bIsCandidate = PR_FALSE;

       // only consider certain nodes to be empty for purposes of removal
-      if (!(
+      if (
	     nsTextEditUtils::NodeIsType(node, NS_LITERAL_STRING("a"))	    ||
	     nsTextEditUtils::NodeIsType(node, NS_LITERAL_STRING("b"))	    ||
	     nsTextEditUtils::NodeIsType(node, NS_LITERAL_STRING("i"))	    ||
@@ -6739,18 +6753,13 @@
	     nsTextEditUtils::NodeIsType(node, NS_LITERAL_STRING("sup"))    ||
	     nsTextEditUtils::NodeIsType(node, NS_LITERAL_STRING("font"))   ||
	     nsHTMLEditUtils::IsList(node)	||
-	     nsHTMLEditUtils::IsParagraph(node) ||
-	     nsHTMLEditUtils::IsHeader(node)	||
-	     nsHTMLEditUtils::IsListItem(node)	||
-	     nsHTMLEditUtils::IsBlockquote(node)||
	     nsHTMLEditUtils::IsDiv(node)	||
-	     nsHTMLEditUtils::IsPre(node)	||
-	     nsHTMLEditUtils::IsAddress(node) ) )
+	     nsHTMLEditUtils::IsAddress(node) )
       {
-	 bIsEmptyNode = PR_FALSE;
+	 bIsCandidate = PR_TRUE;
       }

-      if (bIsEmptyNode && !nsTextEditUtils::IsBody(node))
+      if (!bIsCandidate && !nsTextEditUtils::IsBody(node))
       {
	 if (nsHTMLEditUtils::IsParagraph(node) ||
	     nsHTMLEditUtils::IsHeader(node)	||


sr=[e-mail], with my changes above if they are correct?

Comment 14

[Joe Francis]

regarding kin's comments:
First off, this part of the diff made it in by accident and is part of an
unrelated bug fix.  But it still needs reviewing eventually, so no effort
wasted.  I think your changes are an improvement.  I don't see the correctness
issue you mention: it is true that the was the logic stands now a PRE would lite
the candidate flag at one point, but if the selection were in it then the
candidate flag would later be unlit.  However, your changes do avoid
unneccessary testing for certain block types twice.  thanks!

Comment 15

[Joe Francis]

fix landed on trunk

Comment 16

[michael wendell]

Verified on Win XP using trunk build 02-20.