735059 - Second and third execCommand() parameters should be optional

Status: RESOLVED FIXED

(Core :: DOM: Editor, defect)

Description

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

It's nice to be able to do execCommand("bold") instead of execCommand("bold", false, "").  All the other browsers allow it, and the spec says it should be allowed too.

Comment 1

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

Attachment: Patch v1

This bug has actually annoyed me for months.

Comment 2

[(no longer active)]

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

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

r=me with the comment below.

::: editor/libeditor/html/tests/test_bug735059.html
@@ +12,5 @@
> +/** Test for Bug 735059 **/
> +
> +// Value defaults to the empty string, which evaluates to true, so this
> +// disables CSS styling
> +document.execCommand("usecss");

If we ever regress this, this call will throw, and the test will finish successfully with a message saying "no actual checks performed" or something similar to that.  The correct way to test is to wrap the entire js in a try/catch and explicitly fail the test in the catch block.

Comment 3

[Mozilla RelEng Bot]

Autoland Patchset:
	Patches: 605156
	Branch: mozilla-central => try
	Destination: http://hg.mozilla.org/try/pushloghtml?changeset=cb573ba079ca
Try run started, revision cb573ba079ca. To cancel or monitor the job, see: https://tbpl.mozilla.org/?tree=Try&rev=cb573ba079ca

Comment 4

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

(In reply to Ehsan Akhgari [:ehsan] from comment #2)
> If we ever regress this, this call will throw, and the test will finish
> successfully with a message saying "no actual checks performed" or something
> similar to that.  The correct way to test is to wrap the entire js in a
> try/catch and explicitly fail the test in the catch block.

I've confirmed that an uncaught exception will actually cause a failure.  See bug 365929.  If I revert the IDL patch but leave the test, the test fails with message

"""
an unexpected uncaught JS exception reported through window.onerror - uncaught exception: [Exception... "Not enough arguments [nsIDOMHTMLDocument.execCommand]" nsresult: "0x80570001 (NS_ERROR_XPC_NOT_ENOUGH_ARGS)" location: "JS frame :: http://mochi.test:8888/tests/editor/libeditor/html/tests/test_bug735059.html?autorun=1&closeWhenDone=1&logFile=%2Fmnt%2Fextra%2Fcheckouts%2Fmozilla-central%2Fobjdir-ff-release%2Fmochitest-plain.log&fileLevel=INFO&consoleLevel=INFO&failureFile=/mnt/extra/checkouts/mozilla-central/objdir-ff-release/_tests/testing/mochitest/makefailures.json :: <TOP_LEVEL> :: line 16" data: no] at :0
"""

So no changes needed to the patch, per instructions on IRC.

Comment 5

[Mozilla RelEng Bot]

Autoland Patchset:
	Patches: 605156
	Branch: mozilla-central => try
	Destination: http://hg.mozilla.org/try/pushloghtml?changeset=975dd82afb07
Try run started, revision 975dd82afb07. To cancel or monitor the job, see: https://tbpl.mozilla.org/?tree=Try&rev=975dd82afb07

Comment 6

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Indeed, yay for window.onerror

Comment 7

[(no longer active)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5217bbf42aa7

Comment 8

[Marco Bonardo [:mak]]

https://hg.mozilla.org/mozilla-central/rev/5217bbf42aa7