Closed
Bug 742240
Opened 13 years ago
Closed 13 years ago
Handle unsupported commands per spec in execCommand/queryCommand*
Categories
(Core :: DOM: Editor, defect)
Core
DOM: Editor
Tracking
()
RESOLVED
FIXED
mozilla15
People
(Reporter: ayg, Assigned: ayg)
References
Details
Attachments
(1 file, 3 obsolete files)
(deleted),
patch
|
ehsan.akhgari
:
review+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•13 years ago
|
||
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)
Comment 2•13 years ago
|
||
Do older versions of IE throw? If not, I don't think we can start throwing exceptions for the compatibility reasons.
Comment 3•13 years ago
|
||
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•13 years ago
|
||
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+
Assignee | ||
Comment 5•13 years ago
|
||
(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•13 years ago
|
||
(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! ;-)
Assignee | ||
Comment 7•13 years ago
|
||
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 8•13 years ago
|
||
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+
Assignee | ||
Comment 9•13 years ago
|
||
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)
Assignee | ||
Comment 10•13 years ago
|
||
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)
Comment 11•13 years ago
|
||
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
Assignee | ||
Comment 12•13 years ago
|
||
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 13•13 years ago
|
||
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+
Assignee | ||
Comment 14•13 years ago
|
||
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.
Assignee | ||
Comment 15•13 years ago
|
||
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
Assignee | ||
Comment 16•13 years ago
|
||
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•13 years ago
|
||
Sure, discussing this further doesn't benefit anybody. :(
Assignee | ||
Updated•13 years ago
|
Blocks: editingspectests
Assignee | ||
Comment 18•13 years ago
|
||
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)
Updated•13 years ago
|
Attachment #625070 -
Flags: review?(ehsan) → review+
Assignee | ||
Comment 19•13 years ago
|
||
Assignee | ||
Updated•13 years ago
|
Blocks: editingspectests
Comment 20•13 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•