Last Comment Bug 742240 - Handle unsupported commands per spec in execCommand/queryCommand*
: Handle unsupported commands per spec in execCommand/queryCommand*
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Editor (show other bugs)
: Trunk
: All All
: -- minor (vote)
: mozilla15
Assigned To: :Aryeh Gregor (away until August 15)
:
Mentors:
Depends on:
Blocks: editingspectests
  Show dependency treegraph
 
Reported: 2012-04-04 02:29 PDT by :Aryeh Gregor (away until August 15)
Modified: 2012-05-20 23:42 PDT (History)
2 users (show)
ayg: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch v1, causes test failures (8.30 KB, patch)
2012-04-04 03:19 PDT, :Aryeh Gregor (away until August 15)
ehsan: feedback+
rniwa: feedback+
Details | Diff | Splinter Review
Patch v2 (236.21 KB, patch)
2012-04-15 02:54 PDT, :Aryeh Gregor (away until August 15)
no flags Details | Diff | Splinter Review
Patch v3, passes tests (240.02 KB, patch)
2012-04-15 07:25 PDT, :Aryeh Gregor (away until August 15)
ehsan: review+
Details | Diff | Splinter Review
Patch v4, updated for new spec (16.90 KB, patch)
2012-05-18 05:26 PDT, :Aryeh Gregor (away until August 15)
ehsan: review+
Details | Diff | Splinter Review

Description :Aryeh Gregor (away until August 15) 2012-04-04 02:29:15 PDT
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.
Comment 1 :Aryeh Gregor (away until August 15) 2012-04-04 03:19:33 PDT
Created attachment 612140 [details] [diff] [review]
Patch v1, causes test failures

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.  :) )
Comment 2 Ryosuke Niwa 2012-04-04 09:38:24 PDT
Do older versions of IE throw? If not, I don't think we can start throwing exceptions for the compatibility reasons.
Comment 3 :Ehsan Akhgari 2012-04-04 10:00:25 PDT
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 4 :Ehsan Akhgari 2012-04-04 10:00:55 PDT
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!
Comment 5 :Aryeh Gregor (away until August 15) 2012-04-04 10:07:55 PDT
(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!
Comment 6 :Ehsan Akhgari 2012-04-04 10:11:30 PDT
(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!  ;-)
Comment 7 :Aryeh Gregor (away until August 15) 2012-04-06 01:39:11 PDT
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.
Comment 8 Ryosuke Niwa 2012-04-15 00:10:11 PDT
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.
Comment 9 :Aryeh Gregor (away until August 15) 2012-04-15 02:54:33 PDT
Created attachment 615125 [details] [diff] [review]
Patch v2

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
Comment 10 :Aryeh Gregor (away until August 15) 2012-04-15 03:32:17 PDT
Comment on attachment 615125 [details] [diff] [review]
Patch v2

Canceling review request until I get a patch that passes the tryserver.
Comment 11 Mozilla RelEng Bot 2012-04-15 07:16:23 PDT
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
Comment 12 :Aryeh Gregor (away until August 15) 2012-04-15 07:25:36 PDT
Created attachment 615151 [details] [diff] [review]
Patch v3, passes tests

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/.
Comment 13 :Ehsan Akhgari 2012-04-16 11:59:24 PDT
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.)
Comment 14 :Aryeh Gregor (away until August 15) 2012-04-17 02:50:00 PDT
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 15 :Aryeh Gregor (away until August 15) 2012-04-17 08:30:06 PDT
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>.
Comment 16 :Aryeh Gregor (away until August 15) 2012-05-04 03:01:42 PDT
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.
Comment 17 :Ehsan Akhgari 2012-05-04 11:50:40 PDT
Sure, discussing this further doesn't benefit anybody.  :(
Comment 18 :Aryeh Gregor (away until August 15) 2012-05-18 05:26:36 PDT
Created attachment 625070 [details] [diff] [review]
Patch v4, updated for new spec

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.
Comment 19 :Aryeh Gregor (away until August 15) 2012-05-19 23:37:59 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/ad9940bdbd6a
Comment 20 Ed Morley [:emorley] 2012-05-20 23:42:43 PDT
https://hg.mozilla.org/mozilla-central/rev/ad9940bdbd6a

Note You need to log in before you can comment on or make changes to this bug.