Second and third execCommand() parameters should be optional

RESOLVED FIXED in mozilla14

Status

()

Core
Editor
--
minor
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: ayg, Assigned: ayg)

Tracking

({dev-doc-needed})

Trunk
mozilla14
dev-doc-needed
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

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.
Assignee: nobody → ayg
Status: NEW → ASSIGNED
Created attachment 605156 [details] [diff] [review]
Patch v1

This bug has actually annoyed me for months.
Attachment #605156 - Flags: review?(ehsan)
Whiteboard: [autoland]
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.
Attachment #605156 - Flags: review?(ehsan) → review+
Whiteboard: [autoland] → [autoland-try]

Updated

6 years ago
Whiteboard: [autoland-try] → [autoland-in-queue]

Comment 3

6 years ago
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
(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.
Keywords: dev-doc-needed
Whiteboard: [autoland-in-queue] → [autoland-try:-u all]

Updated

6 years ago
Whiteboard: [autoland-try:-u all] → [autoland-in-queue]

Comment 5

6 years ago
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
Indeed, yay for window.onerror
Keywords: checkin-needed
Whiteboard: [autoland-in-queue]
https://hg.mozilla.org/integration/mozilla-inbound/rev/5217bbf42aa7
Keywords: checkin-needed
Target Milestone: --- → mozilla14
https://hg.mozilla.org/mozilla-central/rev/5217bbf42aa7
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.