Closed Bug 745701 Opened 12 years ago Closed 12 years ago

Strip existing styles more aggressively in execCommand()

Categories

(Core :: DOM: Editor, defect)

Product:
Core
Component:
DOM: Editor
Type:
defect
Priority:
Not set
Severity:
minor

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla15

People

(Reporter: ayg, Assigned: ayg)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 2 obsolete files)

Patch part 1, v2 -- Clean up nsHTMLEditor::RemoveStyleInside
10.31 KB, patch
ehsan.akhgari
: review+
Details | Diff | Splinter Review
Patch part 2, v2 -- Strip existing styles more aggressively in execCommand()
116.11 KB, patch
ehsan.akhgari
: review+
Details | Diff | Splinter Review 
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).
Flags: in-testsuite+
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>.
Attachment #615290 - Flags: review?(ehsan)
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.
Attachment #615288 - Flags: review?(ehsan)
Attachment #615290 - Attachment is obsolete: true
Attachment #615290 - Flags: review?(ehsan)
Attachment #615288 - Attachment is obsolete: true
Summary: Strip CSS styles for execCommand("forecolor"/"fontname") → Strip existing styles more aggressively in execCommand()
Same as the previous version, but also cleans up nsresult use.
Attachment #615309 - Flags: review?(ehsan)
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
Attachment #615310 - Flags: review?(ehsan)
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 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.
Attachment #615309 - Flags: review?(ehsan) → review+
Attachment #615310 - Flags: review?(ehsan) → review+
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.
(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.
(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. ;)
https://hg.mozilla.org/mozilla-central/rev/7b7ef3f23f4c
https://hg.mozilla.org/mozilla-central/rev/25712ffe172a
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: