205485 - execCommand("backcolor") should work the same as execCommand("hilitecolor")

Status: RESOLVED FIXED

(Core :: DOM: Editor, defect)

Description

[Stefan Murariu]

User-Agent:       Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.4b) Gecko/20030507
Build Identifier: Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.4b) Gecko/20030507

The backcolor command will produce under I.E. the following code <FONT
style="BACKGROUND-COLOR:argument_color">Selected Text</FONT>. Mozilla
just change the background-color property of body for entire page, and cannot be
used getting the innerHTML of this. I see the best option to implement this just
as forecolor i.e. <span style="background-color:argument_color"> which is
working well.

Reproducible: Always

Steps to Reproduce:
1.
2.
3.

Comment 1

[Stefan Murariu]

Attachment: the main editor file

the example of midas use founded on devedge.netscape.com

Comment 2

[Stefan Murariu]

Attachment: the file to be edited

Initialy the file contain the "How must look" source

Comment 3

[Kathleen :Brade]

-->brade

Comment 4

[Kathleen :Brade]

This is invalid.  use "hilitecolor" in midas to set the hilitecolor (text
background color).  Background color in midas is specific to the background of
the page or a table/table cell.

Comment 5

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Gecko's current behavior seems to be to set bgcolor on the nearest td/body ancestor in non-CSS mode, and background-color on the nearest block ancestor in CSS mode.  This is undesirable for a whole bunch of reasons:

* It doesn't match any other browser.  IE and WebKit both treat backColor basically the same as Gecko's hiliteColor.  (WebKit supports both commands identically; IE doesn't support hiliteColor.)
* The dual behavior of setting bgcolor on the body or td in non-CSS mode is very confusing.
* Setting bgcolor on the body doesn't work at all in contenteditable, only designMode.  Gecko seems to set bgcolor on the body anyway, even if it's not editable, which is downright alarming.
* The behavior in CSS mode doesn't make sense from the user's perspective.  For instance, if each line is wrapped in a div, the background of the selected line will change; but if lines are separated by br, the whole editable area's background will change.  Worse, the background will be put on the contenteditable element instead of its contents, so if the HTML is submitted using innerHTML, the background won't be.  Even if the line's background is highlighted, it won't be the "whole" line if the element has a margin.  Etc.

I think the sanest path here is to follow WebKit, and make backColor an alias for hiliteColor.  This is what the editing spec currently requires:

http://dvcs.w3.org/hg/editing/raw-file/tip/editing.html#the-backcolor-command

I'll submit a patch shortly.

Comment 6

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Attachment: Patch v1

New richtext2 passes:

1003 ERROR TEST-UNEXPECTED-PASS | /tests/editor/libeditor/html/tests/browserscope/test_richtext2.html | [A, Proposed, BC:blue_TEXT-1_SI, dM] used to fail, but it just started passing - 1 should equal 1
1006 ERROR TEST-UNEXPECTED-PASS | /tests/editor/libeditor/html/tests/browserscope/test_richtext2.html | [A, Proposed, BC:blue_TEXT-1_SI, body] used to fail, but it just started passing - 1 should equal 1
1009 ERROR TEST-UNEXPECTED-PASS | /tests/editor/libeditor/html/tests/browserscope/test_richtext2.html | [A, Proposed, BC:blue_TEXT-1_SI, div] used to fail, but it just started passing - 1 should equal 1
1138 ERROR TEST-UNEXPECTED-PASS | /tests/editor/libeditor/html/tests/browserscope/test_richtext2.html | [AC, Proposed, BC:blue_TEXT-1_SI, dM] used to fail, but it just started passing - 1 should equal 1
1141 ERROR TEST-UNEXPECTED-PASS | /tests/editor/libeditor/html/tests/browserscope/test_richtext2.html | [AC, Proposed, BC:blue_TEXT-1_SI, body] used to fail, but it just started passing - 1 should equal 1
1144 ERROR TEST-UNEXPECTED-PASS | /tests/editor/libeditor/html/tests/browserscope/test_richtext2.html | [AC, Proposed, BC:blue_TEXT-1_SI, 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, dM] used to fail, but it just started passing - 1 should equal 1
1245 ERROR TEST-UNEXPECTED-PASS | /tests/editor/libeditor/html/tests/browserscope/test_richtext2.html | [C, Proposed, BC:842_FONTs:bc:fca-1_SW, dM] used to fail, but it just started passing - 1 should equal 1
1247 ERROR TEST-UNEXPECTED-PASS | /tests/editor/libeditor/html/tests/browserscope/test_richtext2.html | [C, Proposed, BC:842_FONTs:bc:fca-1_SW, body] 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:842_FONTs:bc:fca-1_SW, body] used to fail, but it just started passing - 1 should equal 1
1253 ERROR TEST-UNEXPECTED-PASS | /tests/editor/libeditor/html/tests/browserscope/test_richtext2.html | [C, Proposed, BC:00f_SPANs:bc:f00-1_SW, dM] used to fail, but it just started passing - 1 should equal 1
1255 ERROR TEST-UNEXPECTED-PASS | /tests/editor/libeditor/html/tests/browserscope/test_richtext2.html | [C, Proposed, BC:00f_SPANs:bc:f00-1_SW, dM] 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:00f_SPANs:bc:f00-1_SW, body] 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:00f_SPANs:bc:f00-1_SW, body] 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, BC:ace_FONT.ass.s:bc:rgb-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, BC:ace_FONT.ass.s:bc:rgb-1_SW, dM] 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, BC:ace_FONT.ass.s:bc:rgb-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, BC:ace_FONT.ass.s:bc:rgb-1_SW, body] 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, dM] used to fail, but it just started passing - 1 should equal 1
1437 ERROR TEST-UNEXPECTED-PASS | /tests/editor/libeditor/html/tests/browserscope/test_richtext2.html | [CC, Proposed, BC:gray_SPANs:bc:b-1_SW, dM] used to fail, but it just started passing - 1 should equal 1
1439 ERROR TEST-UNEXPECTED-PASS | /tests/editor/libeditor/html/tests/browserscope/test_richtext2.html | [CC, Proposed, BC:gray_SPANs:bc:b-1_SW, body] 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_SW, body] used to fail, but it just started passing - 1 should equal 1
1445 ERROR TEST-UNEXPECTED-PASS | /tests/editor/libeditor/html/tests/browserscope/test_richtext2.html | [CC, Proposed, BC:gray_SPANs:bc:b-1_SO, dM] used to fail, but it just started passing - 1 should equal 1
1447 ERROR TEST-UNEXPECTED-PASS | /tests/editor/libeditor/html/tests/browserscope/test_richtext2.html | [CC, Proposed, BC:gray_SPANs:bc:b-1_SO, dM] used to fail, but it just started passing - 1 should equal 1
1449 ERROR TEST-UNEXPECTED-PASS | /tests/editor/libeditor/html/tests/browserscope/test_richtext2.html | [CC, Proposed, BC:gray_SPANs:bc:b-1_SO, body] used to fail, but it just started passing - 1 should equal 1
1451 ERROR TEST-UNEXPECTED-PASS | /tests/editor/libeditor/html/tests/browserscope/test_richtext2.html | [CC, Proposed, BC:gray_SPANs:bc:b-1_SO, body] used to fail, but it just started passing - 1 should equal 1
1461 ERROR TEST-UNEXPECTED-PASS | /tests/editor/libeditor/html/tests/browserscope/test_richtext2.html | [CC, Proposed, BC:gray_P-SPANs:bc:b-1_SW, dM] used to fail, but it just started passing - 1 should equal 1
1463 ERROR TEST-UNEXPECTED-PASS | /tests/editor/libeditor/html/tests/browserscope/test_richtext2.html | [CC, Proposed, BC:gray_P-SPANs:bc:b-1_SW, dM] used to fail, but it just started passing - 1 should equal 1
1465 ERROR TEST-UNEXPECTED-PASS | /tests/editor/libeditor/html/tests/browserscope/test_richtext2.html | [CC, Proposed, BC:gray_P-SPANs:bc:b-1_SW, body] used to fail, but it just started passing - 1 should equal 1
1467 ERROR TEST-UNEXPECTED-PASS | /tests/editor/libeditor/html/tests/browserscope/test_richtext2.html | [CC, Proposed, BC:gray_P-SPANs:bc:b-1_SW, body] used to fail, but it just started passing - 1 should equal 1
1471 ERROR TEST-UNEXPECTED-PASS | /tests/editor/libeditor/html/tests/browserscope/test_richtext2.html | [CC, Proposed, BC:gray_P-SPANs:bc:b-2_SW, dM] used to fail, but it just started passing - 1 should equal 1
1473 ERROR TEST-UNEXPECTED-PASS | /tests/editor/libeditor/html/tests/browserscope/test_richtext2.html | [CC, Proposed, BC:gray_P-SPANs:bc:b-2_SW, dM] used to fail, but it just started passing - 1 should equal 1
1475 ERROR TEST-UNEXPECTED-PASS | /tests/editor/libeditor/html/tests/browserscope/test_richtext2.html | [CC, Proposed, BC:gray_P-SPANs:bc:b-2_SW, body] used to fail, but it just started passing - 1 should equal 1
1477 ERROR TEST-UNEXPECTED-PASS | /tests/editor/libeditor/html/tests/browserscope/test_richtext2.html | [CC, Proposed, BC:gray_P-SPANs:bc:b-2_SW, body] used to fail, but it just started passing - 1 should equal 1
1479 ERROR TEST-UNEXPECTED-PASS | /tests/editor/libeditor/html/tests/browserscope/test_richtext2.html | [CC, Proposed, BC:gray_P-SPANs:bc:b-2_SW, div] used to fail, but it just started passing - 1 should equal 1
1481 ERROR TEST-UNEXPECTED-PASS | /tests/editor/libeditor/html/tests/browserscope/test_richtext2.html | [CC, Proposed, BC:gray_P-SPANs:bc:b-2_SW, div] used to fail, but it just started passing - 1 should equal 1
1483 ERROR TEST-UNEXPECTED-PASS | /tests/editor/libeditor/html/tests/browserscope/test_richtext2.html | [CC, Proposed, BC:gray_P-SPANs:bc:b-3_SO, dM] used to fail, but it just started passing - 1 should equal 1
1485 ERROR TEST-UNEXPECTED-PASS | /tests/editor/libeditor/html/tests/browserscope/test_richtext2.html | [CC, Proposed, BC:gray_P-SPANs:bc:b-3_SO, dM] used to fail, but it just started passing - 1 should equal 1
1487 ERROR TEST-UNEXPECTED-PASS | /tests/editor/libeditor/html/tests/browserscope/test_richtext2.html | [CC, Proposed, BC:gray_P-SPANs:bc:b-3_SO, body] 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, BC:gray_P-SPANs:bc:b-3_SO, body] 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, BC:gray_P-SPANs:bc:b-3_SO, 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, BC:gray_P-SPANs:bc:b-3_SO, div] used to fail, but it just started passing - 1 should equal 1

Plus the new richtext passes.  I checked comm-central for other users of this command and found none.

Try run: https://tbpl.mozilla.org/?tree=Try&rev=07bd16a5ec70

The patch applies on top of bug 279330 and bug 738385 to avoid conflicts in richtext2 updates.

Comment 7

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

(Try run passed; failures are due to bug 279330.  See also <https://tbpl.mozilla.org/?tree=Try&rev=23b3a61d4ca3>, <https://tbpl.mozilla.org/?tree=Try&rev=0a582f0d148c>.)

Comment 8

[Mozilla RelEng Bot]

Try run for 07bd16a5ec70 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=07bd16a5ec70
Results (out of 220 total builds):
    exception: 2
    success: 163
    warnings: 55
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-07bd16a5ec70

Comment 9

[(no longer active)]

Comment on attachment 615119 [details] [diff] [review]
Patch v1

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

r=me, assuming that you have looked to make sure comm-central is not using backcolor.

Comment 10

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Attachment: Patch v2, leaves cmd_backgroundColor untouched

Oops -- I found an apparent user:

http://mxr.mozilla.org/comm-central/source/editor/ui/composer/content/editor.js#1156

It uses cmd_backgroundColor directly, AFAICT, not execCommand("backcolor").  I found no uses of the latter.  So this patch just changes execCommand("backcolor") to use cmd_highlight and leaves all the cmd_backgroundColor use in place.  This is a significant change from the last patch, so I'll re-request review in case you want me to do something else (like still remove the code and get the comm-central people to remove the callers).

Comment 11

[Mozilla RelEng Bot]

Try run for 4c4306ac51c6 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=4c4306ac51c6
Results (out of 217 total builds):
    exception: 1
    success: 183
    warnings: 33
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-4c4306ac51c6

Comment 12

[(no longer active)]

Comment on attachment 615638 [details] [diff] [review]
Patch v2, leaves cmd_backgroundColor untouched

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

Please file a follow-up to fix the comm-central caller, and another one to do what your original patch did.  Thanks!

Comment 13

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8ddeb95d7650

Follow-ups: bug 746165, bug 746166.

Comment 14

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/8ddeb95d7650