Closed Bug 742240 Opened 12 years ago Closed 12 years ago

Handle unsupported commands per spec in execCommand/queryCommand*

Categories

(Core :: DOM: Editor, defect)

defect
Not set
minor

Tracking

()

RESOLVED FIXED
mozilla15

People

(Reporter: ayg, Assigned: ayg)

References

Details

Attachments

(1 file, 3 obsolete files)

Spec:

http://dvcs.w3.org/hg/editing/raw-file/tip/editing.html#methods-to-query-and-execute-commands

Test-case:

data:text/html,<!DOCTYPE html>
<div contenteditable>foo</div>
<script>
var s = "exec: ";
try { s += document.execCommand("sadsada") } catch(e) { s += e }
s += "\nenabled: ";
try { s += document.queryCommandEnabled("sadsada") } catch(e) { s += e }
s += "\nindeterm: ";
try { s += document.queryCommandIndeterm("sadsada") } catch(e) { s += e }
s += "\nstate: ";
try { s += document.queryCommandState("sadsada") } catch(e) { s += e }
s += "\nsupported: ";
try { s += document.queryCommandSupported("sadsada") } catch(e) { s += e }
s += "\nvalue: ";
try { s += document.queryCommandValue("sadsada") } catch(e) { s += e }
document.body.textContent = s;
document.body.style.whiteSpace = "pre";
</script>

Gecko:

"""
exec: false
enabled: false
indeterm: [Exception... "Component returned failure code: 0x80004001 (NS_ERROR_NOT_IMPLEMENTED) [nsIDOMHTMLDocument.queryCommandIndeterm]" nsresult: "0x80004001 (NS_ERROR_NOT_IMPLEMENTED)" location: "..." data: no]
state: [Exception... "Component returned failure code: 0x80004001 (NS_ERROR_NOT_IMPLEMENTED) [nsIDOMHTMLDocument.queryCommandState]" nsresult: "0x80004001 (NS_ERROR_NOT_IMPLEMENTED)" location: "..." data: no]
supported: false
value: [Exception... "Component returned failure code: 0x80004001 (NS_ERROR_NOT_IMPLEMENTED) [nsIDOMHTMLDocument.queryCommandValue]" nsresult: "0x80004001 (NS_ERROR_NOT_IMPLEMENTED)" location: "..." data: no]
"""

IE10 Developer Preview:

"""
exec: Error: Invalid argument.
enabled: Error: Invalid argument.
indeterm: Error: Invalid argument.
state: Error: Invalid argument.
supported: false
value: Error: Invalid argument.
"""

Chrome 19 dev:

"""
exec: false
enabled: false
indeterm: false
state: false
supported: false
value: false
"""

Opera Next 12.00 alpha:

"""
exec: DOMException: NOT_SUPPORTED_ERR
enabled: false
indeterm: TypeError: 'document.queryCommandIndeterm' is not a function
state: false
supported: false
value: 
"""

The spec matches IE -- if the command is unsupported, throw for everything except queryCommandSupported.  (Although the spec requires a DOMException NotSupportedError, not what IE throws.)  Changing to match this might incur some compat risk due to execCommand now throwing.  I doubt it will be a problem that queryCommandEnabled() throws, and of course there's nothing scary about changing the type of exception thrown by the other queryCommand*().

Also, all of these methods throw NS_ERROR_FAILURE if there's no command manager -- e.g., in the test-case, if I remove the <div contenteditable>.  We can tell whether a command is supported even if there's no command manager, so we should throw the proper exceptions in these cases.  queryCommandSupported() doesn't have to throw at all.
Attached patch Patch v1, causes test failures (obsolete) — Splinter Review
This causes unexpected richtext2 test failures.  Most of them are just failing in a different way than they did before -- unselect and forwardDelete are now throwing instead of returning false, but they were already failing (or should have been).  This also causes a queryCommandEnabled regression in richtext2, because it now throws instead of returning false, and richtext2 expects it to return false.

The breakdown for throwing vs. returning false/"" is

* execCommand: IE/Opera throw, Gecko/WebKit return false
* enabled: IE throws, Gecko/WebKit/Opera return false
* indeterm/state/value: IE/Gecko throw, WebKit/Opera return false/"" (Opera doesn't support indeterm)
* supported: everyone returns false (duh)

So for queryCommandEnabled(), the majority return false rather than throwing, and browserscope's richtext2 tests for that, so I'm inclined to change the spec to require that.  Ehsan, Ryosuke, what do you think about that spec change?  (Not asking you for feedback about the patch, Ryosuke.  :) )
Attachment #612140 - Flags: feedback?(rniwa)
Attachment #612140 - Flags: feedback?(ehsan)
Do older versions of IE throw? If not, I don't think we can start throwing exceptions for the compatibility reasons.
I think that it makes sense for browsers to throw for a command which nobody supports.  The compat risk comes from throwing for a command which is supported by other engines, but then again, that's what most web APIs do.

I wouldn't worry at all for what BrowserScope tests for, we can easily change that (and most of what they test for is based on what the author of the test thought should happen anyway!).

I'd be fine with taking a patch to how Gecko handles this to match the current spec, and let it bake on the Beta channel, on the condition of backout if it proves to break a major website, however, I'd be surprised if it actually breaks a web site (because web authors are *supposed* to call queryCommandSupported if they want to do something fishy!).

Ryosuke, are you also willing to take this change in WebKit?  I think it's a good idea for us to do this around the same time, and let each other know if a regression is found in either engine.
Comment on attachment 612140 [details] [diff] [review]
Patch v1, causes test failures

I haven't actually reviewed this patch, let me know if you want me to!
Attachment #612140 - Flags: feedback?(ehsan) → feedback+
(In reply to Ryosuke Niwa from comment #2)
> Do older versions of IE throw? If not, I don't think we can start throwing
> exceptions for the compatibility reasons.

In IE6, courtesy of ies4linux, with .textContent changed to .innerText:

"""
exec: [object Error]
enabled: [object Error]
indeterm: [object Error]
state: [object Error]
supported: false
value: [object Error]
"""

So it's basically the same as IE10.

(In reply to Ehsan Akhgari [:ehsan] from comment #4)
> Comment on attachment 612140 [details] [diff] [review]
> Patch v1, causes test failures
> 
> I haven't actually reviewed this patch, let me know if you want me to!

I need to update the tests.  Then I'll submit a patch with r?.  Thanks!
(In reply to Aryeh Gregor from comment #5)
> (In reply to Ryosuke Niwa from comment #2)
> > Do older versions of IE throw? If not, I don't think we can start throwing
> > exceptions for the compatibility reasons.
> 
> In IE6, courtesy of ies4linux, with .textContent changed to .innerText:
> 
> """
> exec: [object Error]
> enabled: [object Error]
> indeterm: [object Error]
> state: [object Error]
> supported: false
> value: [object Error]
> """
> 
> So it's basically the same as IE10.

That's a compelling reason to switch the behavior in Gecko and WebKit.  Because any sites which would break because of this chnage have been broken for the past 10 years!  ;-)
I'll resume work on this the week after next.  It needs to be based on the patches on bug 738385 so that the changes to test_bug408231.html don't conflict.
Status: NEW → ASSIGNED
Comment on attachment 612140 [details] [diff] [review]
Patch v1, causes test failures

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

Okay, l filed https://bugs.webkit.org/show_bug.cgi?id=83993.
Attachment #612140 - Flags: feedback?(rniwa) → feedback+
Attached patch Patch v2 (obsolete) — Splinter Review
New richtext2 expected failures:

241 ERROR TEST-UNEXPECTED-FAIL | /tests/editor/libeditor/html/tests/browserscope/test_richtext2.html | [S, Proposed, UNSEL_TEXT-1_SI, dM] has regressed - got 0, expected 1
244 ERROR TEST-UNEXPECTED-FAIL | /tests/editor/libeditor/html/tests/browserscope/test_richtext2.html | [S, Proposed, UNSEL_TEXT-1_SI, body] has regressed - got 0, expected 1
247 ERROR TEST-UNEXPECTED-FAIL | /tests/editor/libeditor/html/tests/browserscope/test_richtext2.html | [S, Proposed, UNSEL_TEXT-1_SI, div] has regressed - got 0, expected 1
2782 ERROR TEST-UNEXPECTED-FAIL | /tests/editor/libeditor/html/tests/browserscope/test_richtext2.html | [FD, Proposed, TABLE-1_SB, dM] has regressed - got 0, expected 1
2784 ERROR TEST-UNEXPECTED-FAIL | /tests/editor/libeditor/html/tests/browserscope/test_richtext2.html | [FD, Proposed, TABLE-1_SB, dM] has regressed - got 0, expected 1
2786 ERROR TEST-UNEXPECTED-FAIL | /tests/editor/libeditor/html/tests/browserscope/test_richtext2.html | [FD, Proposed, TABLE-1_SB, body] has regressed - got 0, expected 1
2788 ERROR TEST-UNEXPECTED-FAIL | /tests/editor/libeditor/html/tests/browserscope/test_richtext2.html | [FD, Proposed, TABLE-1_SB, body] has regressed - got 0, expected 1
2790 ERROR TEST-UNEXPECTED-FAIL | /tests/editor/libeditor/html/tests/browserscope/test_richtext2.html | [FD, Proposed, TABLE-1_SB, div] has regressed - got 0, expected 1
2792 ERROR TEST-UNEXPECTED-FAIL | /tests/editor/libeditor/html/tests/browserscope/test_richtext2.html | [FD, Proposed, TABLE-1_SB, div] has regressed - got 0, expected 1
2794 ERROR TEST-UNEXPECTED-FAIL | /tests/editor/libeditor/html/tests/browserscope/test_richtext2.html | [FD, Proposed, TD-1_SE, dM] has regressed - got 0, expected 1
2796 ERROR TEST-UNEXPECTED-FAIL | /tests/editor/libeditor/html/tests/browserscope/test_richtext2.html | [FD, Proposed, TD-1_SE, dM] has regressed - got 0, expected 1
2798 ERROR TEST-UNEXPECTED-FAIL | /tests/editor/libeditor/html/tests/browserscope/test_richtext2.html | [FD, Proposed, TD-1_SE, body] has regressed - got 0, expected 1
2800 ERROR TEST-UNEXPECTED-FAIL | /tests/editor/libeditor/html/tests/browserscope/test_richtext2.html | [FD, Proposed, TD-1_SE, body] has regressed - got 0, expected 1
2802 ERROR TEST-UNEXPECTED-FAIL | /tests/editor/libeditor/html/tests/browserscope/test_richtext2.html | [FD, Proposed, TD-1_SE, div] has regressed - got 0, expected 1
2804 ERROR TEST-UNEXPECTED-FAIL | /tests/editor/libeditor/html/tests/browserscope/test_richtext2.html | [FD, Proposed, TD-1_SE, div] has regressed - got 0, expected 1
2806 ERROR TEST-UNEXPECTED-FAIL | /tests/editor/libeditor/html/tests/browserscope/test_richtext2.html | [FD, Proposed, TD2-1_SE1, dM] has regressed - got 0, expected 1
2808 ERROR TEST-UNEXPECTED-FAIL | /tests/editor/libeditor/html/tests/browserscope/test_richtext2.html | [FD, Proposed, TD2-1_SE1, dM] has regressed - got 0, expected 1
2810 ERROR TEST-UNEXPECTED-FAIL | /tests/editor/libeditor/html/tests/browserscope/test_richtext2.html | [FD, Proposed, TD2-1_SE1, body] has regressed - got 0, expected 1
2812 ERROR TEST-UNEXPECTED-FAIL | /tests/editor/libeditor/html/tests/browserscope/test_richtext2.html | [FD, Proposed, TD2-1_SE1, body] has regressed - got 0, expected 1
2814 ERROR TEST-UNEXPECTED-FAIL | /tests/editor/libeditor/html/tests/browserscope/test_richtext2.html | [FD, Proposed, TD2-1_SE1, div] has regressed - got 0, expected 1
2816 ERROR TEST-UNEXPECTED-FAIL | /tests/editor/libeditor/html/tests/browserscope/test_richtext2.html | [FD, Proposed, TD2-1_SE1, div] has regressed - got 0, expected 1
2878 ERROR TEST-UNEXPECTED-FAIL | /tests/editor/libeditor/html/tests/browserscope/test_richtext2.html | [FD, Proposed, DIV:ce:false-1_SB, dM] has regressed - got 0, expected 1
2880 ERROR TEST-UNEXPECTED-FAIL | /tests/editor/libeditor/html/tests/browserscope/test_richtext2.html | [FD, Proposed, DIV:ce:false-1_SB, dM] has regressed - got 0, expected 1
2882 ERROR TEST-UNEXPECTED-FAIL | /tests/editor/libeditor/html/tests/browserscope/test_richtext2.html | [FD, Proposed, DIV:ce:false-1_SB, body] has regressed - got 0, expected 1
2884 ERROR TEST-UNEXPECTED-FAIL | /tests/editor/libeditor/html/tests/browserscope/test_richtext2.html | [FD, Proposed, DIV:ce:false-1_SB, body] has regressed - got 0, expected 1
2886 ERROR TEST-UNEXPECTED-FAIL | /tests/editor/libeditor/html/tests/browserscope/test_richtext2.html | [FD, Proposed, DIV:ce:false-1_SB, div] has regressed - got 0, expected 1
2888 ERROR TEST-UNEXPECTED-FAIL | /tests/editor/libeditor/html/tests/browserscope/test_richtext2.html | [FD, Proposed, DIV:ce:false-1_SB, div] has regressed - got 0, expected 1
2902 ERROR TEST-UNEXPECTED-FAIL | /tests/editor/libeditor/html/tests/browserscope/test_richtext2.html | [FD, Proposed, DIV:ce:false-1_SI, dM] has regressed - got 0, expected 1
2904 ERROR TEST-UNEXPECTED-FAIL | /tests/editor/libeditor/html/tests/browserscope/test_richtext2.html | [FD, Proposed, DIV:ce:false-1_SI, dM] has regressed - got 0, expected 1
2906 ERROR TEST-UNEXPECTED-FAIL | /tests/editor/libeditor/html/tests/browserscope/test_richtext2.html | [FD, Proposed, DIV:ce:false-1_SI, body] has regressed - got 0, expected 1
2908 ERROR TEST-UNEXPECTED-FAIL | /tests/editor/libeditor/html/tests/browserscope/test_richtext2.html | [FD, Proposed, DIV:ce:false-1_SI, body] has regressed - got 0, expected 1
2910 ERROR TEST-UNEXPECTED-FAIL | /tests/editor/libeditor/html/tests/browserscope/test_richtext2.html | [FD, Proposed, DIV:ce:false-1_SI, div] has regressed - got 0, expected 1
2912 ERROR TEST-UNEXPECTED-FAIL | /tests/editor/libeditor/html/tests/browserscope/test_richtext2.html | [FD, Proposed, DIV:ce:false-1_SI, div] has regressed - got 0, expected 1
3736 ERROR TEST-UNEXPECTED-FAIL | /tests/editor/libeditor/html/tests/browserscope/test_richtext2.html | [QE, Proposed, garbage-1_TEXT-1, dM] has regressed - got 0, expected 1
3739 ERROR TEST-UNEXPECTED-FAIL | /tests/editor/libeditor/html/tests/browserscope/test_richtext2.html | [QE, Proposed, garbage-1_TEXT-1, body] has regressed - got 0, expected 1
3742 ERROR TEST-UNEXPECTED-FAIL | /tests/editor/libeditor/html/tests/browserscope/test_richtext2.html | [QE, Proposed, garbage-1_TEXT-1, div] has regressed - got 0, expected 1

All of these are either unselect tests, forwardDelete tests, or incorrect expectations for queryCommandEnabled.

Try run: <https://tbpl.mozilla.org/?tree=Try&rev=23b3a61d4ca3>.  This patch only applies on top of a boatload of other patches, not all of which I've posted yet, because everything I'm working on changes richtext2's currentStatus.js, and I don't want to have to resolve millions of merge conflicts.

I filed a bug against richtext2 to get them to expect exceptions here: http://code.google.com/p/browserscope/issues/detail?id=328
Attachment #612140 - Attachment is obsolete: true
Attachment #615125 - Flags: review?(ehsan)
Comment on attachment 615125 [details] [diff] [review]
Patch v2

Canceling review request until I get a patch that passes the tryserver.
Attachment #615125 - Flags: review?(ehsan)
Try run for 23b3a61d4ca3 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=23b3a61d4ca3
Results (out of 230 total builds):
    success: 186
    warnings: 44
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/ayg@aryeh.name-23b3a61d4ca3
Attached patch Patch v3, passes tests (obsolete) — Splinter Review
Passes try run: https://tbpl.mozilla.org/?tree=Try&rev=0a582f0d148c

This updates test_bug408231.html, which failed in v2.  I missed it when running tests locally because it was in content/html/content/test/ for some reason, and I didn't run the tests from there locally.  I've moved it to content/html/document/test/.
Attachment #615125 - Attachment is obsolete: true
Attachment #615151 - Flags: review?(ehsan)
Comment on attachment 615151 [details] [diff] [review]
Patch v3, passes tests

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

(FWIW, I don't really review changes to currentStatus.js, I just assume that they're auto-generated.  As long as we understand why the output of richtext{,2} tests change, changes in their results are not that important.)
Attachment #615151 - Flags: review?(ehsan) → review+
currentStatus.js for richtext2 is impossible to understand, in my experience, particularly diffs.  I just post the output to the bug for reference so that we know which tests it's supposed to be fixing.  The diffs to currentStatus.js are self-explanatory -- see bug 745723.
Comment on attachment 615151 [details] [diff] [review]
Patch v3, passes tests

This is no longer an approach we want to pursue; see <https://bugs.webkit.org/show_bug.cgi?id=83993#c17>.
Attachment #615151 - Attachment is obsolete: true
Ehsan, do we just want to match WebKit here?  That would mean changing queryCommand(Indeterm|State|Value) to not throw; we already match for the other functions.  If that's the way we want to go -- it makes the most sense to me at this point -- I'll update the spec and official tests.
Sure, discussing this further doesn't benefit anybody.  :(
This reduces test_runtest.html.json from 4.7M to 2.2M, mostly because of commands that are specced to return false/false/"" anyway -- like forwardDelete or insertText.  Now we return false/false/"" because we don't support them, so the tests pass.  I also updated the spec's tests to test an unsupported command ("quasit") explicitly, and then re-imported them, in case you're wondering why those files are modified.

I omitted most of the test_runtest.html.json changes, because the diff was too large again (>4M).  The try run is at <https://tbpl.mozilla.org/?tree=Try&rev=b4e113a2932b>, and interested parties can get the actual patch from there.
Attachment #625070 - Flags: review?(ehsan)
Attachment #625070 - Flags: review?(ehsan) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/ad9940bdbd6a
No longer blocks: editingspectests
Flags: in-testsuite+
Target Milestone: --- → mozilla15
https://hg.mozilla.org/mozilla-central/rev/ad9940bdbd6a
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.