Closed Bug 745528 Opened 13 years ago Closed 13 years ago

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

Categories

(Core :: DOM: Editor, defect)

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

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla15

People

(Reporter: ayg, Assigned: ayg)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

Patch v1
(deleted), patch
ehsan.akhgari
: review+
Details | Diff | Splinter Review
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+
Attached patch Patch v1 (deleted) — — Splinter Review
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)
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+
https://hg.mozilla.org/mozilla-central/rev/b7037edf7fb3
Status: ASSIGNED → RESOLVED
Closed: 13 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: