14430 - [blocker] IsEmptyNode misbehaving on <HR> (was: Making italic clobbers <HR>)

Status: VERIFIED FIXED

(Core :: DOM: Editor, defect, P1)

Description

[Simon Fraser [no longer active]]

Select across the <HR> in the test page, from withing the paragraph before to in
the para after, and hit the Italic button. The <HR> disappears.

Comment 1

[buster]

Here's the root cause:
nsHTMLEditRules::CleanUpSelection() scans the selection doing garbage
collection on empty nodes that result from an edit operation.  It
uses nsHTMLEditRules::IsEmptyNode() to determine if a node is garbage or
content.  Unfortunately, IsEmptyNode() returns true on any node that has no
children, such as <HR>.  I'm guessing this wouldn't work for <OBJECT> or a bunch
of other legit tags either.  This really needs to get fixed!
This should get fixed right away, it'll cause a lot of heartache in a lot of
places.
Note to Joe:  you won't be able to test this particular case without code
sitting in my tree, to be checked in today if at all possible.  I'll include the
(very incomplete) diff in case you want to get to it before I can check in:

Index: base/nsHTMLEditor.cpp
===================================================================
RCS file: /cvsroot/mozilla/editor/base/nsHTMLEditor.cpp,v
retrieving revision 1.155
diff -r1.155 nsHTMLEditor.cpp
5421a5422,5430
> NS_IMETHODIMP nsHTMLEditor::IsLeafThatTakesInlineStyle(const nsString *aTag,
>                                                        PRBool &aResult)
> {
>   if (!aTag) { return NS_ERROR_NULL_POINTER; }
>
>   aResult = (PRBool)((PR_FALSE==aTag->Equals("br")) &&
>                      (PR_FALSE==aTag->Equals("hr")) );
>   return NS_OK;
> }
5524,5525c5533,5537
<           if (tag != "br")  // skip <BR>, even though it's a leaf
<           { // only want to wrap the text node in a new style node if it doesn
't already have that style
---
>           PRBool processLeaf;
>           IsLeafThatTakesInlineStyle(&tag, processLeaf);
>           if (processLeaf)  // skip leaf tags that don't take inline style
>           {
>             // only want to wrap the node in a new style node if it doesn't al
ready have that style
Index: base/nsHTMLEditor.h
===================================================================
RCS file: /cvsroot/mozilla/editor/base/nsHTMLEditor.h,v
retrieving revision 1.71
diff -r1.71 nsHTMLEditor.h
294a295,296
>   NS_IMETHOD IsLeafThatTakesInlineStyle(const nsString *aTag, PRBool &aResult)
;
>

Comment 2

[Joe Francis]

accepting bug...

Comment 3

[buster]

can slip to M12.

Comment 4

[buster]

ok, joe, my portion of this is in.

Comment 5

[Joe Francis]

this should be a pretty easy fix.  I only want to nuke empty container nodes.  I
can't remember if our current level of dtd access lets us ask that question, but
if it doesn't we can just use a temporary utility function with a hardcoded list
of container nodes for now.

Comment 6

[Joe Francis]

fixed a while ago; forgot to mark

Comment 7

[sujay]

verified in 11/4 build.