execCommand() should apply style="" to only inline elements, not blocks

RESOLVED FIXED in mozilla15

Status

()

Core
Editor
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: ayg, Assigned: ayg)

Tracking

(Blocks: 1 bug)

Trunk
mozilla15
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

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.
Flags: in-testsuite+
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.
Attachment #615129 - Flags: review?(ehsan)
Blocks: 685931

Comment 2

5 years ago
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 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.
Attachment #615129 - Flags: review?(ehsan) → review+
http://hg.mozilla.org/projects/birch/rev/b7037edf7fb3
Target Milestone: --- → mozilla15
https://hg.mozilla.org/mozilla-central/rev/b7037edf7fb3
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Duplicate of this bug: 747667
You need to log in before you can comment on or make changes to this bug.