Indent list inside table causes table to split

VERIFIED FIXED in mozilla0.9.9


17 years ago
4 years ago


(Reporter: selmer (gone), Assigned: Joe Francis)


Windows NT

Firefox Tracking Flags

(Not tracked)


(Whiteboard: EDITORBASE+; fixinhand; need r)


(2 attachments, 1 obsolete attachment)



17 years ago
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

17 years ago
Oops, wrong component.
Component: Compositor → Editor: Composer

Comment 2

17 years ago
Assignee: kmcclusk → syd
QA Contact: petersen → sujay


17 years ago
Keywords: dataloss, regression

Comment 3

17 years ago
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

17 years ago
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

17 years ago
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

17 years ago
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.

Assignee: syd → jfrancis

Comment 7

17 years ago
Created attachment 65431 [details]
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

17 years ago
099.   Note that the rest of the table doest really disappear. It just gets
split off.  Still a bad bug though.  
Severity: normal → major
Target Milestone: --- → mozilla0.9.9

Comment 9

17 years ago
attaching patch later tonight (it's on a powerbook thats out of juice and my cord 
is at home)
Whiteboard: EDITORBASE; fixinhand

Comment 10

17 years ago
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.
Keywords: dataloss, regression

Comment 11

17 years ago
Created attachment 66974 [details] [diff] [review]
patch for editor/libeditor/html/nsHTMLEditRules/cpp

Comment 12

17 years ago
Created attachment 66975 [details]
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.
Attachment #65431 - Attachment is obsolete: true


17 years ago
Whiteboard: EDITORBASE; fixinhand → EDITORBASE; fixinhand; need r, sr


17 years ago
Summary: Data loss: Indent list inside table, lose content → Indent list inside table causes table to split

Comment 13

17 years ago
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)	||, with my changes above if they are correct?
Attachment #66974 - Flags: superreview+


17 years ago
Whiteboard: EDITORBASE; fixinhand; need r, sr → EDITORBASE+; fixinhand; need r, sr

Comment 14

17 years ago
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!


17 years ago
Keywords: nsbeta1+


16 years ago
Whiteboard: EDITORBASE+; fixinhand; need r, sr → EDITORBASE+; fixinhand; need r

Comment 15

16 years ago
fix landed on trunk
Last Resolved: 16 years ago
Resolution: --- → FIXED

Comment 16

16 years ago
Verified on Win XP using trunk build 02-20.
Product: Browser → Seamonkey
You need to log in before you can comment on or make changes to this bug.