Last Comment Bug 745701 - Strip existing styles more aggressively in execCommand()
: Strip existing styles more aggressively in execCommand()
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Editor (show other bugs)
: Trunk
: All All
: -- minor with 1 vote (vote)
: mozilla15
Assigned To: :Aryeh Gregor (away until August 15)
:
Mentors:
Depends on:
Blocks: richtext2
  Show dependency treegraph
 
Reported: 2012-04-16 02:36 PDT by :Aryeh Gregor (away until August 15)
Modified: 2012-04-24 17:56 PDT (History)
2 users (show)
ayg: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Part 1, v1 - Clean up nsHTMLEditor::RemoveStyleInside (9.96 KB, patch)
2012-04-16 03:21 PDT, :Aryeh Gregor (away until August 15)
no flags Details | Diff | Splinter Review
Part 2, v1 - Strip CSS styles for execCommand() even if styleWithCSS is false (88.66 KB, patch)
2012-04-16 03:32 PDT, :Aryeh Gregor (away until August 15)
no flags Details | Diff | Splinter Review
Patch part 1, v2 -- Clean up nsHTMLEditor::RemoveStyleInside (10.31 KB, patch)
2012-04-16 05:42 PDT, :Aryeh Gregor (away until August 15)
ehsan: review+
Details | Diff | Splinter Review
Patch part 2, v2 -- Strip existing styles more aggressively in execCommand() (116.11 KB, patch)
2012-04-16 05:47 PDT, :Aryeh Gregor (away until August 15)
ehsan: review+
Details | Diff | Splinter Review

Description :Aryeh Gregor (away until August 15) 2012-04-16 02:36:12 PDT
Example:

data:text/html,<!doctype html>
<div contenteditable><span style="color: blue">foo</span></div>
<script>
getSelection().selectAllChildren(document.body.firstChild);
document.execCommand("forecolor", false, "green");
document.body.textContent = document.body.firstChild.innerHTML;
</script>

The result is
  <font color="green"><span style="color: blue">foo</span></font>
but should be
  <font color="green">foo</font>

Chrome 19 dev gets this (more or less) right; IE 10 Developer Preview and Opera Next 12.00 alpha get it wrong too.  richtext2 tests for this.  fontSize has the same bug, but more complicated because we currently don't recognize any CSS equivalency for that at all (bug 480647).
Comment 1 :Aryeh Gregor (away until August 15) 2012-04-16 03:21:39 PDT
Created attachment 615288 [details] [diff] [review]
Part 1, v1 - Clean up nsHTMLEditor::RemoveStyleInside
Comment 2 :Aryeh Gregor (away until August 15) 2012-04-16 03:32:50 PDT
Created attachment 615290 [details] [diff] [review]
Part 2, v1 - Strip CSS styles for execCommand() even if styleWithCSS is false

This just makes some existing code unconditional, and removes redundant code.  It passes all mochitest-plain in editor/ and content/html/document/ on localhost, and crashtest in editor/.  richtext2 result changes are all unexpected passes:

1261 ERROR TEST-UNEXPECTED-PASS | /tests/editor/libeditor/html/tests/browserscope/test_richtext2.html | [C, Proposed, FC:g_SPANs:c:g-1_SW, dM] used to fail, but it just started passing - 1 should equal 1
1263 ERROR TEST-UNEXPECTED-PASS | /tests/editor/libeditor/html/tests/browserscope/test_richtext2.html | [C, Proposed, FC:g_SPANs:c:g-1_SW, dM] used to fail, but it just started passing - 1 should equal 1
1265 ERROR TEST-UNEXPECTED-PASS | /tests/editor/libeditor/html/tests/browserscope/test_richtext2.html | [C, Proposed, FC:g_SPANs:c:g-1_SW, body] used to fail, but it just started passing - 1 should equal 1
1267 ERROR TEST-UNEXPECTED-PASS | /tests/editor/libeditor/html/tests/browserscope/test_richtext2.html | [C, Proposed, FC:g_SPANs:c:g-1_SW, body] used to fail, but it just started passing - 1 should equal 1
1269 ERROR TEST-UNEXPECTED-PASS | /tests/editor/libeditor/html/tests/browserscope/test_richtext2.html | [C, Proposed, FC:g_SPANs:c:g-1_SW, div] used to fail, but it just started passing - 1 should equal 1
1271 ERROR TEST-UNEXPECTED-PASS | /tests/editor/libeditor/html/tests/browserscope/test_richtext2.html | [C, Proposed, FC:g_SPANs:c:g-1_SW, div] used to fail, but it just started passing - 1 should equal 1
1309 ERROR TEST-UNEXPECTED-PASS | /tests/editor/libeditor/html/tests/browserscope/test_richtext2.html | [C, Proposed, FN:c_SPANs:ff:a-1_SW, dM] used to fail, but it just started passing - 1 should equal 1
1311 ERROR TEST-UNEXPECTED-PASS | /tests/editor/libeditor/html/tests/browserscope/test_richtext2.html | [C, Proposed, FN:c_SPANs:ff:a-1_SW, dM] used to fail, but it just started passing - 1 should equal 1
1313 ERROR TEST-UNEXPECTED-PASS | /tests/editor/libeditor/html/tests/browserscope/test_richtext2.html | [C, Proposed, FN:c_SPANs:ff:a-1_SW, body] used to fail, but it just started passing - 1 should equal 1
1315 ERROR TEST-UNEXPECTED-PASS | /tests/editor/libeditor/html/tests/browserscope/test_richtext2.html | [C, Proposed, FN:c_SPANs:ff:a-1_SW, body] used to fail, but it just started passing - 1 should equal 1
1317 ERROR TEST-UNEXPECTED-PASS | /tests/editor/libeditor/html/tests/browserscope/test_richtext2.html | [C, Proposed, FN:c_SPANs:ff:a-1_SW, div] used to fail, but it just started passing - 1 should equal 1
1319 ERROR TEST-UNEXPECTED-PASS | /tests/editor/libeditor/html/tests/browserscope/test_richtext2.html | [C, Proposed, FN:c_SPANs:ff:a-1_SW, div] used to fail, but it just started passing - 1 should equal 1
1345 ERROR TEST-UNEXPECTED-PASS | /tests/editor/libeditor/html/tests/browserscope/test_richtext2.html | [C, Proposed, FN:c_SPANs:ff:v-FONTf:a-1_SW, dM] used to fail, but it just started passing - 1 should equal 1
1347 ERROR TEST-UNEXPECTED-PASS | /tests/editor/libeditor/html/tests/browserscope/test_richtext2.html | [C, Proposed, FN:c_SPANs:ff:v-FONTf:a-1_SW, dM] used to fail, but it just started passing - 1 should equal 1
1349 ERROR TEST-UNEXPECTED-PASS | /tests/editor/libeditor/html/tests/browserscope/test_richtext2.html | [C, Proposed, FN:c_SPANs:ff:v-FONTf:a-1_SW, body] used to fail, but it just started passing - 1 should equal 1
1351 ERROR TEST-UNEXPECTED-PASS | /tests/editor/libeditor/html/tests/browserscope/test_richtext2.html | [C, Proposed, FN:c_SPANs:ff:v-FONTf:a-1_SW, body] used to fail, but it just started passing - 1 should equal 1
1353 ERROR TEST-UNEXPECTED-PASS | /tests/editor/libeditor/html/tests/browserscope/test_richtext2.html | [C, Proposed, FN:c_SPANs:ff:v-FONTf:a-1_SW, div] used to fail, but it just started passing - 1 should equal 1
1355 ERROR TEST-UNEXPECTED-PASS | /tests/editor/libeditor/html/tests/browserscope/test_richtext2.html | [C, Proposed, FN:c_SPANs:ff:v-FONTf:a-1_SW, div] used to fail, but it just started passing - 1 should equal 1

It applies on top of a large stack of existing patches, because it changes richtext2's currentStatus.js.  All the previous patches passed a try run <https://tbpl.mozilla.org/?tree=Try&rev=0a582f0d148c>.  New try run with these patches added: <https://tbpl.mozilla.org/?tree=Try&rev=88006ac51b3b>.
Comment 3 :Aryeh Gregor (away until August 15) 2012-04-16 05:28:51 PDT
Comment on attachment 615288 [details] [diff] [review]
Part 1, v1 - Clean up nsHTMLEditor::RemoveStyleInside

I found that these patches missed some cases.  Since you haven't reviewed yet, I'll submit new versions instead of extra patches.
Comment 4 :Aryeh Gregor (away until August 15) 2012-04-16 05:42:14 PDT
Created attachment 615309 [details] [diff] [review]
Patch part 1, v2 -- Clean up nsHTMLEditor::RemoveStyleInside

Same as the previous version, but also cleans up nsresult use.
Comment 5 :Aryeh Gregor (away until August 15) 2012-04-16 05:47:18 PDT
Created attachment 615310 [details] [diff] [review]
Patch part 2, v2 -- Strip existing styles more aggressively in execCommand()

In addition to fixing the richtext2 expected fails in comment 2, this also fixes the following:


1267 ERROR TEST-UNEXPECTED-PASS | /tests/editor/libeditor/html/tests/browserscope/test_richtext2.html | [C, Proposed, FC:g_FONTc:b.s:c:r-1_SW, dM] used to fail, but it just started passing - 1 should equal 1
1269 ERROR TEST-UNEXPECTED-PASS | /tests/editor/libeditor/html/tests/browserscope/test_richtext2.html | [C, Proposed, FC:g_FONTc:b.s:c:r-1_SW, dM] used to fail, but it just started passing - 1 should equal 1
1271 ERROR TEST-UNEXPECTED-PASS | /tests/editor/libeditor/html/tests/browserscope/test_richtext2.html | [C, Proposed, FC:g_FONTc:b.s:c:r-1_SW, body] used to fail, but it just started passing - 1 should equal 1
1273 ERROR TEST-UNEXPECTED-PASS | /tests/editor/libeditor/html/tests/browserscope/test_richtext2.html | [C, Proposed, FC:g_FONTc:b.s:c:r-1_SW, body] used to fail, but it just started passing - 1 should equal 1
1275 ERROR TEST-UNEXPECTED-PASS | /tests/editor/libeditor/html/tests/browserscope/test_richtext2.html | [C, Proposed, FC:g_FONTc:b.s:c:r-1_SW, div] used to fail, but it just started passing - 1 should equal 1
1277 ERROR TEST-UNEXPECTED-PASS | /tests/editor/libeditor/html/tests/browserscope/test_richtext2.html | [C, Proposed, FC:g_FONTc:b.s:c:r-1_SW, div] used to fail, but it just started passing - 1 should equal 1
1315 ERROR TEST-UNEXPECTED-PASS | /tests/editor/libeditor/html/tests/browserscope/test_richtext2.html | [C, Proposed, FN:c_FONTf:a.s:ff:v-1_SW, dM] used to fail, but it just started passing - 1 should equal 1
1317 ERROR TEST-UNEXPECTED-PASS | /tests/editor/libeditor/html/tests/browserscope/test_richtext2.html | [C, Proposed, FN:c_FONTf:a.s:ff:v-1_SW, dM] used to fail, but it just started passing - 1 should equal 1
1319 ERROR TEST-UNEXPECTED-PASS | /tests/editor/libeditor/html/tests/browserscope/test_richtext2.html | [C, Proposed, FN:c_FONTf:a.s:ff:v-1_SW, body] used to fail, but it just started passing - 1 should equal 1
1321 ERROR TEST-UNEXPECTED-PASS | /tests/editor/libeditor/html/tests/browserscope/test_richtext2.html | [C, Proposed, FN:c_FONTf:a.s:ff:v-1_SW, body] used to fail, but it just started passing - 1 should equal 1
1323 ERROR TEST-UNEXPECTED-PASS | /tests/editor/libeditor/html/tests/browserscope/test_richtext2.html | [C, Proposed, FN:c_FONTf:a.s:ff:v-1_SW, div] used to fail, but it just started passing - 1 should equal 1
1325 ERROR TEST-UNEXPECTED-PASS | /tests/editor/libeditor/html/tests/browserscope/test_richtext2.html | [C, Proposed, FN:c_FONTf:a.s:ff:v-1_SW, div] used to fail, but it just started passing - 1 should equal 1
1489 ERROR TEST-UNEXPECTED-PASS | /tests/editor/libeditor/html/tests/browserscope/test_richtext2.html | [CC, Proposed, FN:c_FONTf:a-1_SW, dM] used to fail, but it just started passing - 1 should equal 1
1491 ERROR TEST-UNEXPECTED-PASS | /tests/editor/libeditor/html/tests/browserscope/test_richtext2.html | [CC, Proposed, FN:c_FONTf:a-1_SW, dM] used to fail, but it just started passing - 1 should equal 1
1493 ERROR TEST-UNEXPECTED-PASS | /tests/editor/libeditor/html/tests/browserscope/test_richtext2.html | [CC, Proposed, FN:c_FONTf:a-1_SW, body] used to fail, but it just started passing - 1 should equal 1
1495 ERROR TEST-UNEXPECTED-PASS | /tests/editor/libeditor/html/tests/browserscope/test_richtext2.html | [CC, Proposed, FN:c_FONTf:a-1_SW, body] used to fail, but it just started passing - 1 should equal 1
1497 ERROR TEST-UNEXPECTED-PASS | /tests/editor/libeditor/html/tests/browserscope/test_richtext2.html | [CC, Proposed, FN:c_FONTf:a-1_SW, div] used to fail, but it just started passing - 1 should equal 1
1499 ERROR TEST-UNEXPECTED-PASS | /tests/editor/libeditor/html/tests/browserscope/test_richtext2.html | [CC, Proposed, FN:c_FONTf:a-1_SW, div] used to fail, but it just started passing - 1 should equal 1

And five richtext expected fails.  It adds correct handling of cases like
  <font color=blue style="color: red">
where we'd previously make it something like
  <font color=green style="color: red">
or
  <font color=blue style="color: green">
when setting foreColor to green.  We were overriding CSS styles in CSS mode and HTML styles in HTML mode, but not vice versa.

New try run (ignore the old one's results when they post): https://tbpl.mozilla.org/?tree=Try&rev=61e3116cbcd4
Comment 6 Mozilla RelEng Bot 2012-04-16 07:30:56 PDT
Try run for 88006ac51b3b is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=88006ac51b3b
Results (out of 219 total builds):
    success: 183
    warnings: 36
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/ayg@aryeh.name-88006ac51b3b
Comment 7 :Ehsan Akhgari (away Aug 1-5) 2012-04-16 12:19:18 PDT
Comment on attachment 615309 [details] [diff] [review]
Patch part 1, v2 -- Clean up nsHTMLEditor::RemoveStyleInside

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

::: editor/libeditor/html/nsHTMLEditorStyle.cpp
@@ +651,5 @@
> +        (aProperty == nsEditProperty::href && nsHTMLEditUtils::IsLink(aNode)) ||
> +        // and for named anchors
> +        (aProperty == nsEditProperty::name && nsHTMLEditUtils::IsNamedAnchor(aNode)))) ||
> +        // or node is any prop and we asked for that
> +        (!aProperty && NodeIsProperty(aNode)))

Nit: please fix the indentation.

@@ +660,4 @@
>        NS_NAMED_LITERAL_STRING(styleAttr, "style");
>        NS_NAMED_LITERAL_STRING(classAttr, "class");
> +      const bool hasStyleAttr = HasAttr(aNode, &styleAttr);
> +      const bool hasClassAttr = HasAttr(aNode, &classAttr);

Although I personally like to use const to catch mistakes, this violates our code style, so please take the consts out.

@@ +701,4 @@
>            nsCOMPtr<nsIDOMElement> elem = do_QueryInterface(aNode);
>            NS_ENSURE_TRUE(elem, NS_ERROR_NULL_POINTER);
>            res = RemoveAttribute(elem, *aAttribute);
> +          NS_ENSURE_SUCCESS(res, res);

Please move the NS_ENSURE_SUCCESS call to after the conditional statement.

@@ +705,5 @@
>          }
>        }
>      }
> +  } else if (!aChildrenOnly && IsCSSEnabled() &&
> +      mHTMLCSSUtils->IsCSSEditableProperty(aNode, aProperty, aAttribute)) {

Nit: indentation.
Comment 8 Mozilla RelEng Bot 2012-04-16 18:02:31 PDT
Try run for 61e3116cbcd4 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=61e3116cbcd4
Results (out of 221 total builds):
    exception: 1
    success: 184
    warnings: 34
    other: 2
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/ayg@aryeh.name-61e3116cbcd4
 Timed out after 12 hours without completing.
Comment 9 :Aryeh Gregor (away until August 15) 2012-04-17 03:02:42 PDT
(In reply to Ehsan Akhgari [:ehsan] from comment #7)
> Nit: please fix the indentation.

Okay.  I didn't even realize how crazy the indentation here was.  :)  I don't know exactly how you want it, but I changed it to something that at least lets you follow the structure:

  if (
    (!aChildrenOnly &&
      (
        // node is prop we asked for
        (aProperty && NodeIsType(aNode, aProperty)) ||
        // but check for link (<a href=...)
        (aProperty == nsEditProperty::href && nsHTMLEditUtils::IsLink(aNode)) ||
        // and for named anchors
        (aProperty == nsEditProperty::name && nsHTMLEditUtils::IsNamedAnchor(aNode))
      )
    ) ||
    // or node is any prop and we asked for that
    (!aProperty && NodeIsProperty(aNode))
  ) {

> Although I personally like to use const to catch mistakes, this violates our
> code style, so please take the consts out.

I looked for guidance on use of const but couldn't find it, e.g. at <https://developer.mozilla.org/En/Mozilla_Coding_Style_Guide>.  Where's the code style policy for that?

> Please move the NS_ENSURE_SUCCESS call to after the conditional statement.

Okay.

> > +  } else if (!aChildrenOnly && IsCSSEnabled() &&
> > +      mHTMLCSSUtils->IsCSSEditableProperty(aNode, aProperty, aAttribute)) {
> 
> Nit: indentation.

This should be:

  } else if (!aChildrenOnly && IsCSSEnabled() &&
             mHTMLCSSUtils->IsCSSEditableProperty(aNode, aProperty, aAttribute)) {

with the two lines aligned, right?  That's what I've mostly seen in Gecko, but <https://developer.mozilla.org/En/Mozilla_Coding_Style_Guide> doesn't mention it.
Comment 11 :Ehsan Akhgari (away Aug 1-5) 2012-04-18 18:22:02 PDT
(In reply to Aryeh Gregor from comment #9)
> (In reply to Ehsan Akhgari [:ehsan] from comment #7)
> > Nit: please fix the indentation.
> 
> Okay.  I didn't even realize how crazy the indentation here was.  :)  I
> don't know exactly how you want it, but I changed it to something that at
> least lets you follow the structure:
> 
>   if (
>     (!aChildrenOnly &&
>       (
>         // node is prop we asked for
>         (aProperty && NodeIsType(aNode, aProperty)) ||
>         // but check for link (<a href=...)
>         (aProperty == nsEditProperty::href &&
> nsHTMLEditUtils::IsLink(aNode)) ||
>         // and for named anchors
>         (aProperty == nsEditProperty::name &&
> nsHTMLEditUtils::IsNamedAnchor(aNode))
>       )
>     ) ||
>     // or node is any prop and we asked for that
>     (!aProperty && NodeIsProperty(aNode))
>   ) {

That's not my personal style, but it's not razy, cso it's OK!

> > Although I personally like to use const to catch mistakes, this violates our
> > code style, so please take the consts out.
> 
> I looked for guidance on use of const but couldn't find it, e.g. at
> <https://developer.mozilla.org/En/Mozilla_Coding_Style_Guide>.  Where's the
> code style policy for that?

We may not have it written down in the coding style guide (we should) but this is the common practice.

> > Please move the NS_ENSURE_SUCCESS call to after the conditional statement.
> 
> Okay.
> 
> > > +  } else if (!aChildrenOnly && IsCSSEnabled() &&
> > > +      mHTMLCSSUtils->IsCSSEditableProperty(aNode, aProperty, aAttribute)) {
> > 
> > Nit: indentation.
> 
> This should be:
> 
>   } else if (!aChildrenOnly && IsCSSEnabled() &&
>              mHTMLCSSUtils->IsCSSEditableProperty(aNode, aProperty,
> aAttribute)) {
> 
> with the two lines aligned, right?  That's what I've mostly seen in Gecko,
> but <https://developer.mozilla.org/En/Mozilla_Coding_Style_Guide> doesn't
> mention it.

Yeah.  FWIW, while that document is the official reference for this kind of stuff, it's not exhaustive.  My usual way of writing patches in various parts of Gecko is to maintain consistency with the surrounding code, and not being evil (no tabs, no trailing spaces, etc. ;)

Note You need to log in before you can comment on or make changes to this bug.