Bug 83993 - Unsupported commands in execCommand/queryCommand* should throw except queryCommandSupported
Summary: Unsupported commands in execCommand/queryCommand* should throw except queryCo...
Status: RESOLVED WONTFIX
Alias: None
Product: WebKit
Classification: Unclassified
Component: HTML Editing (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-04-15 00:09 PDT by Ryosuke Niwa
Modified: 2012-05-20 07:58 PDT (History)
7 users (show)

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Ryosuke Niwa 2012-04-15 00:09:32 PDT
Mozilla bug: https://bugzilla.mozilla.org/show_bug.cgi?id=742240

Virtually all versions of IE throws an exception and matches the spec, so there is probably a little risk that this causes a compat issue.
Comment 1 Alexey Proskuryakov 2012-04-16 09:20:04 PDT
Engines support different sets of commands, so raising an exception is particularly likely to completely break a page, as opposed to some minor stylistic difference.

I think that making execCommand raise an exception for unknown verb would be wrong.
Comment 2 Ryosuke Niwa 2012-04-16 09:25:12 PDT
(In reply to comment #1)
> Engines support different sets of commands, so raising an exception is particularly likely to completely break a page, as opposed to some minor stylistic difference.
> 
> I think that making execCommand raise an exception for unknown verb would be wrong.

But IE has been and will be throwing exceptions. If anything websites should be calling queryCommandSupported.
Comment 3 Alexey Proskuryakov 2012-04-16 09:59:58 PDT
Well, I guess IE will stop raising exceptions when overwhelmed with content that has only been tested with WebKit based browsers.

Historically, we have not seen site compatibility issues caused by not raising exceptions, but we've seen lots of show-stopping issues caused by raising them. I see very little reason to risk compatibility in this case.
Comment 4 Ehsan Akhgari [:ehsan] 2012-04-16 12:01:34 PDT
Arguably, raising an exception is the right behavior here, given the existence of queryCommandSupported.  I would really like Gecko and WebKit making this change, and reverting it in case we actually find web content which breaks because of this (which I would personally find surprising.)  If we see a Gecko bug about this, we will definitely let you guys know about it as well.
Comment 5 Alexey Proskuryakov 2012-04-16 12:28:30 PDT
I don't see how the existence queryCommandSupported makes this right. If anything, it makes raising an exception for a wrong verb unnecessary!

The right way to check for whether a verb is supported is with queryCommandSupported, not by checking for exceptions.
Comment 6 Ehsan Akhgari [:ehsan] 2012-04-16 12:33:52 PDT
(In reply to comment #5)
> I don't see how the existence queryCommandSupported makes this right. If anything, it makes raising an exception for a wrong verb unnecessary!
> 
> The right way to check for whether a verb is supported is with queryCommandSupported, not by checking for exceptions.

Sure.  Throwing exceptions from execCommand should not be used to detect what commands are supported, it should be used to notify the web application that the call they have made has not taken the desired effect.  Note that without throwing an exception, there is no way to communicate to the caller that they've used an unsupported verb, so the caller might incorrectly assume that the editing command has taken effect, and the failure will manifest itself in some possibly subtle way somewhere else in the program.
Comment 7 Alexey Proskuryakov 2012-04-16 12:50:46 PDT
This is a generic argument for raising exceptions for every error in every DOM function, and we are obviously not going there. Early error detection is a benefit that comes with draconian error handling, but the Web platform does not work like that. Browser users prefer subtle errors to completely dysfunctional pages.

What I'm saying is that execCommand is in less need of exceptions than an average DOM function.
Comment 8 Ehsan Akhgari [:ehsan] 2012-04-16 13:48:33 PDT
(In reply to comment #7)
> This is a generic argument for raising exceptions for every error in every DOM function, and we are obviously not going there. Early error detection is a benefit that comes with draconian error handling, but the Web platform does not work like that. Browser users prefer subtle errors to completely dysfunctional pages.

I think this is an over-generalization of my argument.

> What I'm saying is that execCommand is in less need of exceptions than an average DOM function.

While that may be true, how do you propose that web pages are expected to know when a command that they execute in order to have _some_ effect on the DOM not having any because it is not supported by the current UA?
Comment 9 Alexey Proskuryakov 2012-04-16 13:54:30 PDT
They would call queryCommandSupported first. One way or another, some code needs to be written, be it a check or an exception handler.
Comment 10 Ehsan Akhgari [:ehsan] 2012-04-16 14:01:26 PDT
OK, well, pages which use queryCommandSupported are not affected by the change requested here.  It is the set of pages which do not do that before calling execCommand that we're discussing here.
Comment 11 Alexey Proskuryakov 2012-04-16 14:14:19 PDT
Your question was "how do you propose that web pages are expected to know when a command that they execute in order to have _some_ effect on the DOM not having any because it is not supported by the current UA?"

A page that does not have any checks will not know that, period. Such page would not know that something wrong happened if an exception were to be raised. It would just be broken worse than before, infuriating users.

I think that we're making rounds here, once again discussing your argument that applies to programming in general, but contains no specifics relating to execCommand.
Comment 12 Ehsan Akhgari [:ehsan] 2012-04-16 14:26:45 PDT
My argument _is_ specific to execCommand, since it is an API which can potentially make complicated modifications to the DOM, and has no way of communicating the status of what happened back to the caller (short of throwing an exception, that is.)

I understand the web compat risk here, but I do not think this is likely to affect many real web pages (unless nobody has ever tested them with IE, which would be rather surprising.)  We're going to do this change in Gecko, and I guess I'll wait for Ryosuke to weigh in to see what will happen on the WebKit side, though I would really prefer if WebKit would make the same change.
Comment 13 Alexey Proskuryakov 2012-04-16 14:43:08 PDT
> unless nobody has ever tested them with IE, which would be rather surprising.

How would testing with IE tell developers that another engine doesn't support a certain verb? This is a flawed argument.

I think that this is a pretty obvious WONTFIX, as this suggestion is of no use to Web developers, and only carries site compatibility risks to users. 

Certain engineers may prefer draconian error handling, but this is no place to retrofit it as an exception to how the Web platform generally works. Doing this would be akin to dropping whole CSS stylesheets when any single rule is unknown.

There has been a long discussion already, without new ideas coming, so this seems safe to close.
Comment 14 Alexey Proskuryakov 2012-04-16 14:48:37 PDT
> is of no use to Web developers

I should qualify this statement. It's of no use to developers writing interoperable code. It is of use to developers who are tasked with figuring out why a page doesn't work in a particular browser.

In that case, it certainly helps to see an assertion fire in your developer tools. But there are other ways to debug such issues, so I don't think that this scenario should determine how APIs work.
Comment 15 Ryosuke Niwa 2012-04-16 17:43:19 PDT
(In reply to comment #13)
> > unless nobody has ever tested them with IE, which would be rather surprising.
> 
> How would testing with IE tell developers that another engine doesn't support a certain verb? This is a flawed argument.
> 
> I think that this is a pretty obvious WONTFIX, as this suggestion is of no use to Web developers, and only carries site compatibility risks to users.

I disagree. Making all browsers consistently throw exceptions would make authors who are mistakenly invoking execCommand with vendor-specific verbs be aware of the problem.

If the author was to write a website and only tests it on WebKit, then he may mistakenly think that unsupported commands will be ignored by execCommand, which isn't case on IE.

In general, I don't care what "the right behavior" is but I do care about the browser interoperability. If we think that throwing an exception here is a bad idea, then we need to convince Microsoft to stop throwing the exception and change the spec.
Comment 16 Alexey Proskuryakov 2012-04-16 18:02:36 PDT
So let me give you a specific example. If a site calls execCommand("LiveResize"), do we want this site to become completely unusable in WebKit, or do we want to ignore this? What about execCommand("ClearAuthenticationCache")?

If IE does the wrong thing that breaks sites in IE, it's their job to educate authors. But changing the spec would be useful is someone is up to it.
Comment 17 Aryeh Gregor 2012-04-17 03:48:56 PDT
IE has always thrown, but other browsers (except Opera) have always not.  What this means is that there are probably pages that use IE-specific commands, and therefore would throw in Gecko/WebKit if they changed to match IE, although they don't currently throw in any browser.  So on second thought, there is a real compat risk here.  IE supports all sorts of crazy commands that no one else will want to.

I like the idea of throwing, but it might be too late.  Instead, browsers can return false from execCommand if it's not supported.  This is what current Gecko/WebKit do.  Likewise for queryCommandEnabled.

But for queryCommandIndeterm/State/Value, we can still throw -- this is what both IE *and* Gecko have always done, so for web content it's only a compat risk for WebKit if there's some command that IE *and* Gecko support but WebKit doesn't.  For WebKit-specific content, why would anyone be running a command that WebKit doesn't support?

How does this sound to everyone?
Comment 18 Ehsan Akhgari [:ehsan] 2012-04-17 08:22:55 PDT
(In reply to comment #17)
> IE has always thrown, but other browsers (except Opera) have always not.  What this means is that there are probably pages that use IE-specific commands, and therefore would throw in Gecko/WebKit if they changed to match IE, although they don't currently throw in any browser.  So on second thought, there is a real compat risk here.  IE supports all sorts of crazy commands that no one else will want to.
> 
> I like the idea of throwing, but it might be too late.  Instead, browsers can return false from execCommand if it's not supported.  This is what current Gecko/WebKit do.  Likewise for queryCommandEnabled.
> 
> But for queryCommandIndeterm/State/Value, we can still throw -- this is what both IE *and* Gecko have always done, so for web content it's only a compat risk for WebKit if there's some command that IE *and* Gecko support but WebKit doesn't.  For WebKit-specific content, why would anyone be running a command that WebKit doesn't support?
> 
> How does this sound to everyone?

If WebKit is not willing to change the behavior, I'd be fine with this.
Comment 19 Alexey Proskuryakov 2012-04-17 09:19:12 PDT
> But for queryCommandIndeterm/State/Value, we can still throw -- this is what both IE *and* Gecko have always done

It appears that immediate compatibility risk is minimal for these. I'm not sure of usefulness of such exceptions though. Looking at the same example, who would want queryCommandState("LiveResize") to raise an exception? Inconsistency between closely related functions is not great, too.

Do you have an example where it would make more sense?

Looking at it from a different angle, I'm not sure what the frame of this discussion is. Are we talking about extending the set of execCommand verbs, so there will be a transition period for each new one? Because everyone already implements what makes sense, and silently ignoring other known verbs is the right answer. For instance, we'd likely need to add dummy implementations of LiveResize and ClearAuthenticationCache just to avoid possible exceptions.

And if we're talking about new verbs, then raising exceptions has major compatibility consequences for everyone, including engines that have always been raising exceptions. The risk is not about changing the wording in the spec, but about how badly IE would break on a site calling execCommand("ExperimentalHTML5MobileOnlyHotness"). The fact that IE always raised exceptions is irrelevant when sites actually start executing commands not supported by it.
Comment 20 Aryeh Gregor 2012-04-17 23:43:19 PDT
(In reply to comment #19)
> It appears that immediate compatibility risk is minimal for these. I'm not sure of usefulness of such exceptions though. Looking at the same example, who would want queryCommandState("LiveResize") to raise an exception? Inconsistency between closely related functions is not great, too.
> 
> Do you have an example where it would make more sense?

The issue is that WebKit's behavior means queryCommand*() will silently return bogus results for unsupported commands.

> Looking at it from a different angle, I'm not sure what the frame of this discussion is. Are we talking about extending the set of execCommand verbs, so there will be a transition period for each new one? Because everyone already implements what makes sense, and silently ignoring other known verbs is the right answer. For instance, we'd likely need to add dummy implementations of LiveResize and ClearAuthenticationCache just to avoid possible exceptions.
> 
> And if we're talking about new verbs, then raising exceptions has major compatibility consequences for everyone, including engines that have always been raising exceptions. The risk is not about changing the wording in the spec, but about how badly IE would break on a site calling execCommand("ExperimentalHTML5MobileOnlyHotness"). The fact that IE always raised exceptions is irrelevant when sites actually start executing commands not supported by it.

Most web APIs throw if given invalid input -- in particular, practically all DOM methods throw on failure.  You're right that this doesn't make as much sense if browsers implement different feature-sets.  But it's also bad that if an author writes document.execCommand("insertHoriznotalRule") it will silently fail and it will possibly take them a long time to figure out what's wrong.

So with respect to the legacy IE commands, we could always spec those commands as doing nothing rather than throwing.  That way authors can still catch typos quickly.  If we weren't introducing new commands, I think this would clearly be the best solution.

It's true that we're introducing new commands too, e.g., defaultParagraphSeparator.  If someone writes a page in a browser that supports it, they won't add try/catch, so it will break in other browsers.  But is this really any different from if we add new methods?  If authors call Selection.extend, it will work in Gecko and WebKit but fail in IE.  Does this mean that we should fail silently when authors call undefined methods?  This is not how the web platform works.  If authors use new features in a browser that doesn't support them, it normally throws.

So I'd say we should whitelist all the legacy IE commands and make them fail silently (return false).  But other unrecognized commands should throw, just like any new features that aren't implemented yet.  How does that sound to everyone?  IE already does something like that, so if Gecko or WebKit is willing to go with that, I'll spec it as the majority position.
Comment 21 Alexey Proskuryakov 2012-04-18 09:31:14 PDT
> But it's also bad that if an author writes document.execCommand("insertHoriznotalRule") it will silently fail and it will possibly take them a long time to figure out what's wrong.

I think that this argument is a generic one, subject to opinion. Web technologies in general don't make it easy to catch mistakes early. If we wanted to change this, we'd obsolete HTML for XML, because small typing errors can result in subtle and hard to catch problems with document structure.

I think that developer community has spoken very clearly that this is not what they want.

> This is not how the web platform works.

My idea of "how the platform works" comes more from what CSSStyleDeclaration.setProperty does. Making element.style.setProperty("-webkit-foo", "bar") raise an exception would clearly be a bad idea!

I think that the latest proposal (specify silent failure for legacy IE verbs) accurately describes what engines would do anyway if we move forward with raising the exceptions. However, there is non-trivial cognitive and implementation burden - a seemingly random set of verbs will behave differently than others.

How desirable adding new execCommand verbs is? Could we get away of only adding reasonably ignorable new ones, and implementing other APIs in different ways? I don't really think that this should be an API of choice for everything editing related - if nothing else, double dispatch makes performance worse. Where it makes good sense is commands that have state (can only paste if there is something on clipboard etc.)
Comment 22 Aryeh Gregor 2012-04-19 00:56:12 PDT
(In reply to comment #21)
> I think that this argument is a generic one, subject to opinion. Web technologies in general don't make it easy to catch mistakes early. If we wanted to change this, we'd obsolete HTML for XML, because small typing errors can result in subtle and hard to catch problems with document structure.
> 
> I think that developer community has spoken very clearly that this is not what they want.

XML is horrible because a) it has obscure corner-cases that are fatal errors even though they probably signal nothing wrong, like inclusion of invalid code-points; b) there's no way to isolate the errors so that they don't break the page; c) the content is commonly user-controlled (involving user-submitted content) and so it's hard to verify the input.

By contrast, running an unsupported command definitely signals that the page is not going to act as expected; you can easily stop such errors from breaking the page by using try/catch; and the author normally has full control over which commands are run, with no user input involved (usually just hard-coded strings).

> My idea of "how the platform works" comes more from what CSSStyleDeclaration.setProperty does. Making element.style.setProperty("-webkit-foo", "bar") raise an exception would clearly be a bad idea!

On the other hand, most DOM methods throw if given invalid input, such as: insertBefore, appendChild, replaceChild, removeChild, querySelector(All), splitText, (substring|insert|delete|replace)Data, (extract|clone)Contents, insertNode, surroundContents, isPointInRange, comparePoint, . . .

In some of these cases, it will throw even in cases where a browser just doesn't support the new feature.  querySelector() will throw if the selector is unsupported, but we introduce new selectors regularly.

I will grant that the platform is not entirely consistent about this, or anything.  :)

> How desirable adding new execCommand verbs is? Could we get away of only adding reasonably ignorable new ones, and implementing other APIs in different ways? I don't really think that this should be an API of choice for everything editing related - if nothing else, double dispatch makes performance worse. Where it makes good sense is commands that have state (can only paste if there is something on clipboard etc.)

execCommand() is a horrible API in all sorts of ways, but it's the way that all current editing features are implemented.  If we introduced some new system, we'd have to support all the (useful) existing commands in the new system, which gives two ways of doing the same thing and complicates the platform even more.  So unfortunately, I don't think this is feasible unless there are really compelling advantages to the new API.  And yes, we do want to introduce new commands that aren't reasonably ignorable -- e.g., fontSizePt <https://www.w3.org/Bugs/Public/show_bug.cgi?id=14253>.
Comment 23 Alexey Proskuryakov 2012-04-19 08:02:51 PDT
> On the other hand, most DOM methods throw if given invalid input, such as: insertBefore, appendChild, replaceChild, removeChild, querySelector(All), splitText, (substring|insert|delete|replace)Data, (extract|clone)Contents, insertNode, surroundContents, isPointInRange, comparePoint, . . .

What I see as a subtle, but conceptual difference is that exceptions in these cases are always indication of a programming mistake, not a side channel to inform developer of browser limitations.

> And yes, we do want to introduce new commands that aren't reasonably ignorable -- e.g., fontSizePt <https://www.w3.org/Bugs/Public/show_bug.cgi?id=14253>.

The consequence of ignoring this command is that font size won't change, which may or may not be important to the user, and should be caught in testing if it's a feature important to the developer. Using a wrong font size is superior to being completely broken due to uncaught exception.
Comment 24 Aryeh Gregor 2012-04-20 00:38:26 PDT
(In reply to comment #23)
> What I see as a subtle, but conceptual difference is that exceptions in these cases are always indication of a programming mistake, not a side channel to inform developer of browser limitations.

What about querySelector() with a selector that's not supported yet?  Or what about a method or interface that's just not supported at all?  Calling, say, history.pushState() (or any other method) will throw if the feature isn't supported.  Why should document.execCommand("defaultparagraphseparator") be different?  In fact, if I were designing this API from scratch, I'd make it look something like

  // document.execCommand("bold");
  document.editing.bold.exec();
  // document.queryCommandEnabled("bold");
  document.editing.bold.enabled;
  // document.queryCommandIndeterm("bold");
  document.editing.bold.indeterm;
  // document.queryCommandState("bold");
  document.editing.bold.state;
  // document.queryCommandSupported("bold");
  "bold" in document.editing;

and all of these but the last would throw if "bold" wasn't supported.

> The consequence of ignoring this command is that font size won't change, which may or may not be important to the user, and should be caught in testing if it's a feature important to the developer. Using a wrong font size is superior to being completely broken due to uncaught exception.

True, but the same could be said for almost anything -- authors who want to use new features will tend to get completely broken pages if they don't test in browsers they care about.  How many recently-introduced new features can actually be used without risk of completely breaking other browsers?  Looking at the last "Last week in WebKit":

http://www.webkit.org/blog/1983/last-week-in-webkit-css-khtml-and-apple-and-ancestororigins/

New features there include location.ancestorOrigins() and the Battery Status API.  Both will throw if you use them in browsers that don't support them.  CanvasRenderingContext2D.backingStorePixelRatio won't throw immediately in browsers that don't support it, but real-world use might well throw or otherwise break completely if it's treated as a number, because it will unexpectedly be zero (and authors might divide by it, etc.).

So I think throwing for unsupported new features is par for the course on the web platform, and execCommand() should be no different.
Comment 25 Alexey Proskuryakov 2012-04-20 08:30:55 PDT
I don't think that you explained how this is different from setProperty. The situation is pretty much the same - we have an extensible set of commands, and don't raise exceptions. Are you just saying that it's an outlier?

> How many recently-introduced new features can actually be used without risk of completely breaking other browsers?

That's how setProperty and execCommand are better :)
Comment 26 Ehsan Akhgari [:ehsan] 2012-04-20 12:04:02 PDT
(In reply to comment #25)
> I don't think that you explained how this is different from setProperty. The situation is pretty much the same - we have an extensible set of commands, and don't raise exceptions. Are you just saying that it's an outlier?
> 
> > How many recently-introduced new features can actually be used without risk of completely breaking other browsers?
> 
> That's how setProperty and execCommand are better :)

No, that's how they're "different".  The fact of the matter is that web APIs which silently accept invalid parameters and suddenly become no-op instead of throwing are very rare.  The majority of APIs just throw when passed invalid arguments.  So I don't think that giving an example of an API which does silently accept invalid arguments is a very strong case supporting that execCommand should also do the same.
Comment 27 Alexey Proskuryakov 2012-04-20 13:51:16 PDT
Invalid as in "programming error" is not the same as "unsupported by this particular engine".

Also, there is not a whole lot of APIs that take a string from an extensible set as argument, and I think that setProperty is by far most relevant example.

By the way, see bug 7296 for some context - we experimented with raising exceptions from setProperty before, despite a number of legitimate design concern raised back then.
Comment 28 Aryeh Gregor 2012-04-22 02:03:43 PDT
(In reply to comment #25)
> I don't think that you explained how this is different from setProperty. The situation is pretty much the same - we have an extensible set of commands, and don't raise exceptions. Are you just saying that it's an outlier?

Yes, the platform is inconsistent.  More examples:

* HTMLCanvasElement.getContext().  This returns null instead of an object if the canvas type is unsupported.  This means that if you don't check for errors, you'll get an exception as soon as you try to call any method on the returned canvas.  (To be fair, real silent failure here would be close to impossible given that calling an unknown method fails, so it's not much of an argument either way.)

* XMLHttpRequest.responseType: I'm told this silently ignores setting to invalid types.

* createElement(), setAttribute(), etc.: It's hard to say how to classify these.  They succeed but might not do what you expect, so maybe it should be called silent failure.  But you could also say that it's a deliberate feature that you can create elements/attributes with any name, so it's not failure at all if it does nothing special.

In #whatwg, annevk thought not throwing was the better option given a choice.  At this point I'm kind of ambivalent.  If we don't throw, execCommand() should still return false for an unsupported command or value (in cases like foreColor where weird values cause aborts).  I guess then queryCommand*() shouldn't throw either, for consistency, although neither IE nor Gecko behaves this way.
Comment 29 Aryeh Gregor 2012-05-19 23:41:01 PDT
Gecko now matches the spec, which is almost like WebKit (it returns "" instead of false from queryCommandValue()): https://bugzilla.mozilla.org/show_bug.cgi?id=742240#c18
Comment 30 Alexey Proskuryakov 2012-05-20 07:12:56 PDT
Thanks for the update! Could you please file a bug for us to change from false to ""?
Comment 31 Aryeh Gregor 2012-05-20 07:58:17 PDT
Bug 86964.