388655 - Editor br gets added to empty table cells if contenteditable node is in document

Status: VERIFIED FIXED

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

Description

[Martijn Wargers (dead)]

Attachment: testcase

See testcase, when a contenteditable element is in the document, all the empty table cells get <br> added to the dom.
(In the dom inspector, I see the <br> has _moz_dirty="" and type="_moz" as attributes, but innerHTML doesn't reflect that, which seems odd to me. Anyhow, unrelated to this bug, I guess)

Comment 1

[Peter Van der Beken [:peterv]]

(In reply to comment #0)
> See testcase, when a contenteditable element is in the document, all the empty
> table cells get <br> added to the dom.

Is that different from what happens in designMode? If not, this isn't really a contentEditable bug but a general editor bug.

Comment 2

[Martijn Wargers (dead)]

Yes, it happens in designMode, but I guess it should happen in designMode (does seem like a hack, though)
IE7 does paint a border around empty table cells when in designMode or when the table is contenteditable, see url.

But the issue is that the table is _not_ in contenteditable mode, so it should not get these weird <br>'s added anyway.

Comment 3

[spam]

Will this bug be fixed for the Firefox 3.0 final? Shouldn't this one be blocking?

I think this one is important since contentEditable mode should never modify any contents outside it's scope it's kind of the whole point with contentEditable vs designMode.

Comment 4

[Martijn Wargers (dead)]

If you think a bug is really bad (and it's a regression from branch), you can set the blocking1.9? flag (which I just did for you).

Comment 5

[spam]

Thanks, I didn't know that. Some extra information for this bug, the BR elements gets added at the end of all table cells not only empty ones.

Comment 6

[Chris Pearce [:cpearce (Not reading bugmail)]]

Taking bug. The problem is that we're inserting <br> tags in from the following stack trace:


gklayout.dll!nsHTMLEditor::CreateBR(nsIDOMNode * aNode=0x079311fc, int aOffset=0, nsCOMPtr<nsIDOMNode> * outBRNode=0x0012f0b4, short aSelect=0)  Line 1535	C++
gklayout.dll!nsTextEditRules::CreateMozBR(nsIDOMNode * inParent=0x079311fc, int inOffset=0, nsCOMPtr<nsIDOMNode> * outBRNode=0x0012f0b4)  Line 1462 + 0x25 bytes	C++
gklayout.dll!nsHTMLEditRules::AdjustSpecialBreaks(int aSafeToAskFrames=0)  Line 7400 + 0x25 bytes	C++
gklayout.dll!nsHTMLEditRules::Init(nsPlaintextEditor * aEditor=0x03cf67e0, unsigned int aFlags=2048)  Line 290 + 0xd bytes	C++
gklayout.dll!nsHTMLEditor::InitRules()  Line 436 + 0x2a bytes	C++
gklayout.dll!nsPlaintextEditor::EndEditorInit()  Line 179 + 0x11 bytes	C++
gklayout.dll!nsAutoEditInitRulesTrigger::~nsAutoEditInitRulesTrigger()  Line 134 + 0x12 bytes	C++
gklayout.dll!nsHTMLEditor::Init(nsIDOMDocument * aDoc=0x073e0494, nsIPresShell * aPresShell=0x07957d50, nsIContent * aRoot=0x00000000, nsISelectionController * aSelCon=0x07957dd8, unsigned int aFlags=2048)  Line 345	C++
composer.dll!nsEditingSession::SetupEditorOnWindow(nsIDOMWindow * aWindow=0x063bc0f8)  Line 499 + 0x49 bytes	C++
composer.dll!nsEditingSession::MakeWindowEditable(nsIDOMWindow * aWindow=0x063bc0f8, const char * aEditorType=0x02ea6f6c, int aDoAfterUriLoad=0, int aMakeWholeDocumentEditable=0, int aInteractive=1)  Line 208 + 0x12 bytes	C++
gklayout.dll!nsHTMLDocument::EditingStateChanged()  Line 4002 + 0x31 bytes	C++
gklayout.dll!nsHTMLDocument::EndUpdate(unsigned int aUpdateType=1)  Line 3747	C++
gklayout.dll!mozAutoDocUpdate::~mozAutoDocUpdate()  Line 1015	C++
gklayout.dll!SinkContext::FlushTags()  Line 1407	C++
gklayout.dll!HTMLContentSink::CloseBody()  Line 2130	C++
gklayout.dll!HTMLContentSink::CloseContainer(nsHTMLTag aTag=eHTMLTag_body)  Line 2382 + 0xe bytes	C++
gkparser.dll!CNavDTD::CloseContainer(nsHTMLTag aTag=eHTMLTag_body, int aMalformed=0)  Line 2686 + 0x1e bytes	C++
gkparser.dll!CNavDTD::CloseContainersTo(int anIndex=1, nsHTMLTag aTarget=eHTMLTag_body, int aClosedByStartTag=0)  Line 2734 + 0xe bytes	C++
gkparser.dll!CNavDTD::CloseContainersTo(nsHTMLTag aTag=eHTMLTag_body, int aClosedByStartTag=0)  Line 2878 + 0x14 bytes	C++
gkparser.dll!CNavDTD::DidBuildModel(unsigned int anErrorCode=0, int aNotifySink=1, nsIParser * aParser=0x062e9ad0, nsIContentSink * aSink=0x07958704)  Line 433 + 0x16 bytes	C++
gkparser.dll!nsParser::DidBuildModel(unsigned int anErrorCode=0)  Line 953 + 0x35 bytes	C++
gkparser.dll!nsParser::ResumeParse(int allowIteration=1, int aIsFinalChunk=1, int aCanInterrupt=1)  Line 1656	C++

When we init the html editor for either contenteditable or designMode we pass the document's root node as a limiter. When the html edit rules are inited, they traverse from the root and add <br>s to any <td> and <li> tags which have no content.

So the fix is to change nsEmptyFunctor which is used bynsHTMLEditRules::AdjustSpecialBreaks() to only select nodes which are mozReadWrite, e.g. not contenteditable.

Comment 7

[Chris Pearce [:cpearce (Not reading bugmail)]]

(In reply to comment #6)
> So the fix is to change nsEmptyFunctor which is used
> bynsHTMLEditRules::AdjustSpecialBreaks() to only select nodes which are
> mozReadWrite, e.g. not contenteditable.
> 

I think I meant -moz-user-modify: read-write.

Comment 8

[Damon Sicore (:damons)]

+'ing and marking P3.

Comment 9

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Patch v1 - Only add mozbr to editable <li> and <td>

This patch renames nsEmptyFunctor to nsEmptyEditableFunctor, and changes it to only return true on editable nodes. This functor is used to filter nodes to add mozbr's to, and we only want to add them to <li>/<td> that we're editing anyway, so with this patch we do.

Comment 10

[Peter Van der Beken [:peterv]]

Comment on attachment 291359 [details] [diff] [review]
Patch v1 - Only add mozbr to editable <li> and <td>

>Index: editor/libeditor/html/nsHTMLEditRules.cpp
>===================================================================

>       if (nsHTMLEditUtils::IsListItem(aNode) || nsHTMLEditUtils::IsTableCellOrCaption(aNode))
>       {
>         PRBool bIsEmptyNode;
>         nsresult res = mHTMLEditor->IsEmptyNode(aNode, &bIsEmptyNode, PR_FALSE, PR_FALSE);
>         if (NS_FAILED(res)) return PR_FALSE;
>-        if (bIsEmptyNode) 
>+        if (bIsEmptyNode && mHTMLEditor->IsEditable(aNode))

I'd add the IsEditable check to the IsListItem/IsTableCellOrCaption checks above:

      if (mHTMLEditor->IsEditable(aNode) &&
          (nsHTMLEditUtils::IsListItem(aNode) || nsHTMLEditUtils::IsTableCellOrCaption(aNode)))
      {
        PRBool bIsEmptyNode;
        nsresult res = mHTMLEditor->IsEmptyNode(aNode, &bIsEmptyNode, PR_FALSE, PR_FALSE);
        if (NS_FAILED(res)) return PR_FALSE;
        if (bIsEmptyNode)

Comment 11

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Patch v1 with PeterV's suggestion

This is patch v1 with PeterV's suggestion implemented.

Comment 12

[Reed Loden [:reed]]

Checking in editor/libeditor/html/nsHTMLEditRules.cpp;
/cvsroot/mozilla/editor/libeditor/html/nsHTMLEditRules.cpp,v  <--  nsHTMLEditRules.cpp
new revision: 1.340; previous revision: 1.339
done

Comment 13

[Marcia Knous [:marcia]]

verified fixed using Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9b2pre) Gecko/2007120505 Minefield/3.0b2pre. I verified using the testcase in Comment 0.