Last Comment Bug 745528 - execCommand() should apply style="" to only inline elements, not blocks
: execCommand() should apply style="" to only inline elements, not blocks
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Editor (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla15
Assigned To: :Aryeh Gregor (away until August 15)
:
Mentors:
: Pepi3008 (view as bug list)
Depends on:
Blocks: richtext2
  Show dependency treegraph
 
Reported: 2012-04-15 03:10 PDT by :Aryeh Gregor (away until August 15)
Modified: 2012-04-27 06:47 PDT (History)
3 users (show)
ayg: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch v1 (46.71 KB, patch)
2012-04-15 04:02 PDT, :Aryeh Gregor (away until August 15)
ehsan: review+
Details | Diff | Splinter Review

Description :Aryeh Gregor (away until August 15) 2012-04-15 03:10:07 PDT
Test-case:

data:text/html,<!doctype html>
<div contenteditable>foo</div>
<script>
getSelection().selectAllChildren(document.body.firstChild);
document.execCommand("hilitecolor", false, "lime");
getSelection().removeAllRanges();
</script>

Gecko adds style="background-color: lime" to the wrapping div, making the whole line green.  IE 10 Developer Preview, Chrome 19 dev, and Opera Next 12.00 alpha all add a <span> or <font> with style="background-color: lime".  Gecko's behavior has two disadvantages:

1) The whole line is highlighted, not just the selected text.  This is inconsistent with what happens if you only select "oo" instead of "foo": unselecting the "f" on the left makes the whole right part no longer highlighted.  This doesn't make a lot of sense.

2) If the author is using a script to submit the innerHTML of the contenteditable element, then any attributes added to the contenteditable element will be lost.

The spec requires the IE/WebKit/Opera behavior: if an element isn't an allowed child <http://dvcs.w3.org/hg/editing/raw-file/tip/editing.html#allowed-child> of span, it says to descend recursively into its children.

This is already tested by richtext2.
Comment 1 :Aryeh Gregor (away until August 15) 2012-04-15 04:02:22 PDT
Created attachment 615129 [details] [diff] [review]
Patch v1

New richtext2 passes:

1241 ERROR TEST-UNEXPECTED-PASS | /tests/editor/libeditor/html/tests/browserscope/test_richtext2.html | [C, Proposed, BC:842_FONTs:bc:fca-1_SW, div] used to fail, but it just started passing - 1 should equal 1
1243 ERROR TEST-UNEXPECTED-PASS | /tests/editor/libeditor/html/tests/browserscope/test_richtext2.html | [C, Proposed, BC:842_FONTs:bc:fca-1_SW, div] used to fail, but it just started passing - 1 should equal 1
1249 ERROR TEST-UNEXPECTED-PASS | /tests/editor/libeditor/html/tests/browserscope/test_richtext2.html | [C, Proposed, BC:00f_SPANs:bc:f00-1_SW, div] used to fail, but it just started passing - 1 should equal 1
1251 ERROR TEST-UNEXPECTED-PASS | /tests/editor/libeditor/html/tests/browserscope/test_richtext2.html | [C, Proposed, BC:00f_SPANs:bc:f00-1_SW, div] used to fail, but it just started passing - 1 should equal 1
1257 ERROR TEST-UNEXPECTED-PASS | /tests/editor/libeditor/html/tests/browserscope/test_richtext2.html | [C, Proposed, BC:ace_FONT.ass.s:bc:rgb-1_SW, div] used to fail, but it just started passing - 1 should equal 1
1259 ERROR TEST-UNEXPECTED-PASS | /tests/editor/libeditor/html/tests/browserscope/test_richtext2.html | [C, Proposed, BC:ace_FONT.ass.s:bc:rgb-1_SW, div] used to fail, but it just started passing - 1 should equal 1
1289 ERROR TEST-UNEXPECTED-PASS | /tests/editor/libeditor/html/tests/browserscope/test_richtext2.html | [C, Proposed, HC:g_FONTs:c:b-1_SW, div] used to fail, but it just started passing - 1 should equal 1
1291 ERROR TEST-UNEXPECTED-PASS | /tests/editor/libeditor/html/tests/browserscope/test_richtext2.html | [C, Proposed, HC:g_FONTs:c:b-1_SW, div] used to fail, but it just started passing - 1 should equal 1
1297 ERROR TEST-UNEXPECTED-PASS | /tests/editor/libeditor/html/tests/browserscope/test_richtext2.html | [C, Proposed, HC:g_SPANs:c:g-1_SW, div] used to fail, but it just started passing - 1 should equal 1
1299 ERROR TEST-UNEXPECTED-PASS | /tests/editor/libeditor/html/tests/browserscope/test_richtext2.html | [C, Proposed, HC:g_SPANs:c:g-1_SW, div] used to fail, but it just started passing - 1 should equal 1
1305 ERROR TEST-UNEXPECTED-PASS | /tests/editor/libeditor/html/tests/browserscope/test_richtext2.html | [C, Proposed, HC:g_SPAN.ass.s:c:rgb-1_SW, div] used to fail, but it just started passing - 1 should equal 1
1307 ERROR TEST-UNEXPECTED-PASS | /tests/editor/libeditor/html/tests/browserscope/test_richtext2.html | [C, Proposed, HC:g_SPAN.ass.s:c:rgb-1_SW, div] used to fail, but it just started passing - 1 should equal 1
1433 ERROR TEST-UNEXPECTED-PASS | /tests/editor/libeditor/html/tests/browserscope/test_richtext2.html | [CC, Proposed, BC:gray_SPANs:bc:b-1_SW, div] used to fail, but it just started passing - 1 should equal 1
1435 ERROR TEST-UNEXPECTED-PASS | /tests/editor/libeditor/html/tests/browserscope/test_richtext2.html | [CC, Proposed, BC:gray_SPANs:bc:b-1_SW, div] used to fail, but it just started passing - 1 should equal 1
1441 ERROR TEST-UNEXPECTED-PASS | /tests/editor/libeditor/html/tests/browserscope/test_richtext2.html | [CC, Proposed, BC:gray_SPANs:bc:b-1_SO, div] used to fail, but it just started passing - 1 should equal 1
1443 ERROR TEST-UNEXPECTED-PASS | /tests/editor/libeditor/html/tests/browserscope/test_richtext2.html | [CC, Proposed, BC:gray_SPANs:bc:b-1_SO, div] used to fail, but it just started passing - 1 should equal 1
1455 ERROR TEST-UNEXPECTED-PASS | /tests/editor/libeditor/html/tests/browserscope/test_richtext2.html | [CC, Proposed, BC:gray_P-SPANs:bc:b-1_SW, div] used to fail, but it just started passing - 1 should equal 1
1457 ERROR TEST-UNEXPECTED-PASS | /tests/editor/libeditor/html/tests/browserscope/test_richtext2.html | [CC, Proposed, BC:gray_P-SPANs:bc:b-1_SW, div] 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_SPANs:ff:a-1_SW, div] 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_SPANs:ff:a-1_SW, div] used to fail, but it just started passing - 1 should equal 1
1531 ERROR TEST-UNEXPECTED-PASS | /tests/editor/libeditor/html/tests/browserscope/test_richtext2.html | [CC, Proposed, FN:a_SPANs:ff:a-1_SI, div] used to fail, but it just started passing - 1 should equal 1
1533 ERROR TEST-UNEXPECTED-PASS | /tests/editor/libeditor/html/tests/browserscope/test_richtext2.html | [CC, Proposed, FN:a_SPANs:ff:a-1_SI, div] used to fail, but it just started passing - 1 should equal 1

Try run, applied on top of zillions of other patches: https://tbpl.mozilla.org/?tree=Try&rev=0a582f0d148c

Will request review when the try run passes, because I've gotten bitten by too many unexpected failures in the last few errors.
Comment 2 Mozilla RelEng Bot 2012-04-15 07:31:23 PDT
Try run for 0a582f0d148c is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=0a582f0d148c
Results (out of 227 total builds):
    success: 194
    warnings: 33
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/ayg@aryeh.name-0a582f0d148c
Comment 3 :Ehsan Akhgari (away Aug 1-5) 2012-04-16 12:09:22 PDT
Comment on attachment 615129 [details] [diff] [review]
Patch v1

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

::: editor/libeditor/html/nsHTMLEditorStyle.cpp
@@ +403,5 @@
> +          node = arrayOfNodes[j];
> +          res = SetInlinePropertyOnNode(node, aProperty, aAttribute, aValue);
> +          NS_ENSURE_SUCCESS(res, res);
> +        }
> +        arrayOfNodes.Clear();

arrayOfNodes will go out of scope here, so you shouldn't need to Clear() it explicitly.
Comment 4 :Aryeh Gregor (away until August 15) 2012-04-18 07:26:38 PDT
http://hg.mozilla.org/projects/birch/rev/b7037edf7fb3
Comment 5 :Ehsan Akhgari (away Aug 1-5) 2012-04-24 17:56:35 PDT
https://hg.mozilla.org/mozilla-central/rev/b7037edf7fb3
Comment 6 :Aryeh Gregor (away until August 15) 2012-04-27 06:47:31 PDT
*** Bug 747667 has been marked as a duplicate of this bug. ***

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