854201 - nsHTMLEditor::IsSimpleModifiableNode does not check for _moz_dirty attribute

Status: RESOLVED WORKSFORME

(Core :: DOM: Editor, defect)

Description

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

I have found an error in nsHTMLEditor::IsSimpleModifiableNode(): that method looks for !aContent->GetAttrCount() but does not accept the case of an element having only the _moz_dirty attribute.

An immediate effect of that error is the following one:

 - markup is <p><i>foo</i><i>bar</i></p> 
 - user selects the whole paragraph
 - user hits the i button to remove italic
 - user hits back the i button to re-italicize the result

 EXPECTED RESULT: <p><i>foobar</i></p>
 ACTUAL RESULT:   <p><i>foo</i><i>bar</i></p> 

This happens because the first text node containing "foo" is turned into a <i _moz_dirty>foo</foo> and the code then does not accept to append "bar" at the end of that element instead of encapsulating it into a new <i> element...

I suspect there are other places in the editor where we should accept the presence of a lone _moz_dirty instead of looking only at a zeroed attribute count. This is a regression AFAIC, the original editor was always making sure to never count _moz_dirty in attribute counts.

Comment 1

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

Attachment: proposed fix

Comment 2

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

FWIW, this is BlueGriffon bug 359 http://bugzilla.bluegriffon.org/show_bug.cgi?id=539

Comment 3

[(no longer active)]

Comment on attachment 728740 [details] [diff] [review]
proposed fix

Review of attachment 728740 [details] [diff] [review]:
-----------------------------------------------------------------

Makes sense, but please add a test for it too, and make sure that it doesn't break any of the existing ones by pushing to the try server.  Thanks!

::: editor/libeditor/html/nsHTMLEditorStyle.cpp
@@ +267,5 @@
>    }
>  
> +  uint32_t attrCount = aContent->GetAttrCount();
> +  bool noAttr = !attrCount
> +                || (attrCount == 1 && aContent->GetAttrNameAt(0)->Equals(nsGkAtoms::mozdirty));

Nit: please put the or at the end of the previous line.

Comment 4

[(no longer active)]

https://tbpl.mozilla.org/?tree=Try&rev=ec6376c4f4dc

Comment 5

[sergeykish]

Can not reproduce

data:text/html,<p contenteditable="true"><i>foo</i><i>bar</i></p>

1. user selects the whole paragraph
2. document.execCommand('italic', false)

Nodes joined together <p contenteditable="true">foobar</p>

3. document.execCommand('italic', false)

Applied on joined text <p contenteditable="true"><i>foobar</i></p>