Bug 33509 - selection.modify() should throw an exception if it receives invalid parameters
Summary: selection.modify() should throw an exception if it receives invalid parameters
Status: NEW
Alias: None
Product: WebKit
Classification: Unclassified
Component: HTML Editing (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Windows XP
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-01-11 19:52 PST by Justin Lebar
Modified: 2017-07-18 08:27 PDT (History)
11 users (show)

See Also:


Attachments
patch v0 (7.37 KB, patch)
2010-03-10 22:09 PST, Hajime Morrita
no flags Details | Formatted Diff | Diff
patch v0 (8.35 KB, patch)
2010-03-11 00:31 PST, Hajime Morrita
no flags Details | Formatted Diff | Diff
v2; arrange to clarify test result (7.92 KB, patch)
2010-03-11 01:17 PST, Hajime Morrita
no flags Details | Formatted Diff | Diff
v3; add missing file listing to ChagneLog (8.05 KB, patch)
2010-03-11 01:41 PST, Hajime Morrita
no flags Details | Formatted Diff | Diff
remove garbage node from the testcase (8.10 KB, patch)
2010-03-16 00:04 PDT, Hajime Morrita
levin: review-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Justin Lebar 2010-01-11 19:52:02 PST
Currently, if you call selection.modify with nonsense arguments, the method silently fails.  Instead, the method should throw an exception, informing the programmer that the modify has failed.  This is important not only to inform the developer of typos, but also to help programmers who might provide the arguments to selection.modify() in the wrong order.

You can test this in attachment 46225 [details] (on bug 33435): https://bug-33435-attachments.webkit.org/attachment.cgi?id=46225
Comment 1 Hajime Morrita 2010-03-10 22:09:46 PST
Created attachment 50471 [details]
patch v0
Comment 2 Hajime Morrita 2010-03-10 22:11:26 PST
I'm not sure which exception we should throw. 
Although I chose INVALID_MODIFICATION_ERR, NOT_SUPPORTED_ERR would be possible, for example.
Comment 3 Kent Tamura 2010-03-10 22:30:46 PST
Comment on attachment 50471 [details]
patch v0

> +++ b/LayoutTests/editing/selection/modify-with-invalid-args.html

Please change this test for the script-tests format.
We already have shouldThrow() in fast/js/resources/js-test-pre.js.

> --- a/WebCore/page/DOMSelection.cpp
> +++ b/WebCore/page/DOMSelection.cpp
> @@ -271,8 +271,10 @@ void DOMSelection::modify(const String& alterString, const String& directionStri
>          alter = SelectionController::EXTEND;
>      else if (equalIgnoringCase(alterString, "move"))
>          alter = SelectionController::MOVE;
> -    else
> +    else {
> +        ec = INVALID_MODIFICATION_ERR;

I think NOT_SUPPORTED_ERR is better.
Comment 4 Hajime Morrita 2010-03-11 00:31:17 PST
Created attachment 50478 [details]
patch v0
Comment 5 Hajime Morrita 2010-03-11 00:39:06 PST
Thank you for reviewing!

> Please change this test for the script-tests format.
Fixed. I've overlooked this framework. Thank you for pointing it out.

> I think NOT_SUPPORTED_ERR is better.
Fixed. 

According to patch at https://bugzilla.mozilla.org/show_bug.cgi?id=496275 , 
Mozilla throws NS_ERROR_ILLEGAL_VALUE.  But DOMException has no equivalent for that error...
Comment 6 Kent Tamura 2010-03-11 00:53:19 PST
Comment on attachment 50478 [details]
patch v0


> diff --git a/LayoutTests/ChangeLog b/LayoutTests/ChangeLog
> index 9d47289..3538070 100644
> --- a/LayoutTests/ChangeLog
> +++ b/LayoutTests/ChangeLog
> @@ -1,3 +1,13 @@
> +2010-03-10  MORITA Hajime  <morrita@google.com>
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        Selection.modify() should throw an exception if it receives invalid parameters.
> +        https://bugs.webkit.org/show_bug.cgi?id=33509
> +
> +        * editing/selection/modify-with-invalid-args-expected.txt: Added.
> +        * editing/selection/modify-with-invalid-args.html: Added.

Please add script-tests/modify-with-invalid-args.js

> +function shouldSelectionKeepUnchanged() {
> +    shouldBeTrue("sel.baseNode == target");
> +    shouldBeTrue("sel.baseOffset == baseOffset");
> +    shouldBeTrue("sel.extentNode == target");
> +    shouldBeTrue("sel.extentOffset == extentOffset");
> +}
> +
> +shouldThrow("sel.modify('no-such-alter', 'forward', 'character');", "'Error: NOT_SUPPORTED_ERR: DOM Exception 9'");
> +shouldSelectionKeepUnchanged();

I prefer:
 - change shouldSelectionKeepUnchanged() to isSelectionUnchanged(), it returns just boolean and contains no shouldBe*()
 - replace shouldSelectionKeepUnchanged() calls with shouldBeTrue('isSelectionUnchanged()')
This improves readability of the test result.
Comment 7 Hajime Morrita 2010-03-11 01:17:49 PST
Created attachment 50480 [details]
v2; arrange to clarify test result
Comment 8 Hajime Morrita 2010-03-11 01:19:11 PST
Thank you for suggestion! I updated the patch for clarifying the test result.
Comment 9 Kent Tamura 2010-03-11 01:33:26 PST
Comment on attachment 50480 [details]
v2; arrange to clarify test result

> diff --git a/LayoutTests/ChangeLog b/LayoutTests/ChangeLog
> index 9d47289..3538070 100644
> --- a/LayoutTests/ChangeLog
> +++ b/LayoutTests/ChangeLog
> @@ -1,3 +1,13 @@
> +2010-03-10  MORITA Hajime  <morrita@google.com>
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        Selection.modify() should throw an exception if it receives invalid parameters.
> +        https://bugs.webkit.org/show_bug.cgi?id=33509
> +
> +        * editing/selection/modify-with-invalid-args-expected.txt: Added.
> +        * editing/selection/modify-with-invalid-args.html: Added.
> +

Please update the ChangeLog.  script-tests/modify-with-invalid-args.js is missing.
Comment 10 Hajime Morrita 2010-03-11 01:41:46 PST
Created attachment 50481 [details]
v3; add missing file listing to ChagneLog
Comment 11 Hajime Morrita 2010-03-11 01:42:43 PST
> Please update the ChangeLog.  script-tests/modify-with-invalid-args.js is
> missing.
Oops! Fixed. I'm sorry for that.
Comment 12 Kent Tamura 2010-03-11 01:49:44 PST
Comment on attachment 50481 [details]
v3; add missing file listing to ChagneLog

ok, I have no additional comments.  Please wait for an official reviewer.
Comment 13 Darin Adler 2010-03-12 10:00:23 PST
I am concerned that this change can break existing content.

We've had selection.modify for many years, and now we're going to make it start throwing exceptions for cases where previously it would silently do nothing. That's exactly the sort of change that results in broken websites.

Since selection.modify is WebKit-specific, I believe this would be in WebKit-specific code paths on websites, so sites that were specifically catering to Safari or Chrome or one of the other WebKit-based browsers.
Comment 14 Justin Lebar 2010-03-12 10:04:29 PST
(In reply to comment #13)
> I believe this would be in
> WebKit-specific code paths on websites, so sites that were specifically
> catering to Safari or Chrome or one of the other WebKit-based browsers.

Well, it's not going to be WebKit-specific for much longer.  :)
https://bugzilla.mozilla.org/show_bug.cgi?id=496275
Comment 15 Darin Adler 2010-03-12 10:06:43 PST
(In reply to comment #14)
> Well, it's not going to be WebKit-specific for much longer.  :)
> https://bugzilla.mozilla.org/show_bug.cgi?id=496275

That's great. It usually takes a while for websites to depend on features like this. There’s been more than 5 years for people to depend on the behavior because the function has been in Safari that long.
Comment 16 Justin Lebar 2010-03-12 10:21:01 PST
I wonder how many websites actually use selection.modify() and rely on WebKit not throwing when it receives bogus arguments.

For our part, we're not supporting some of the granularities (sentence, sentenceboundary, paragraph, paragraphboundary, documentboundary) in Firefox, so we're throwing an exception when we receive one of those.  If we're going to throw in that case (and I think we have to in order to inform developers that we don't support some of the granularities), I think it only makes sense for us to throw when we receive nonsense arguments.

It would be great if we all could agree on one interface.
Comment 17 Darin Adler 2010-03-12 14:11:03 PST
(In reply to comment #16)
> It would be great if we all could agree on one interface.

Many of the more serious compatibility problems we’ve seen over the years for Safari came from scripts that stopped; everything would be fine if the script had kept going even though some small part didn’t do exactly what the developer wanted. And exceptions stop scripts.

For a new function with no existing compatibility implications, an exception makes total sense. And for Firefox this is a new function. So I see why it makes sense to you.

We can add the exceptions and cross our fingers hoping it doesn’t break any existing sites.
Comment 18 Kent Tamura 2010-03-12 16:05:06 PST
How about printing a warning to JavaScript console instead of throwing an exception?
Comment 19 Justin Lebar 2010-03-12 16:07:30 PST
(In reply to comment #18)
> How about printing a warning to JavaScript console instead of throwing an
> exception?

I feel like throwing an exception is the right thing to do since it allows the application to handle the case when the modify() doesn't go through.  But feel free to suggest this on the Mozilla bug.
Comment 20 Shinichiro Hamaji 2010-03-15 23:14:29 PDT
Comment on attachment 50481 [details]
v3; add missing file listing to ChagneLog

I've just happened to find a style nitpick in your test.

> +TEST COMPLETE
> +hello

It's nice to have no messages after "TSET COMPLETE". I guess we can add document.body.removeChild(parent) before successfullyParsed.
Comment 21 Hajime Morrita 2010-03-16 00:04:22 PDT
Created attachment 50768 [details]
remove garbage node from the testcase
Comment 22 Hajime Morrita 2010-03-16 00:06:06 PDT
> It's nice to have no messages after "TSET COMPLETE". I guess we can add
> document.body.removeChild(parent) before successfullyParsed.
Thank you for pointing this out. I've updated the patch to fix  this.
Comment 23 David Levin 2010-03-25 13:53:43 PDT
Comment on attachment 50768 [details]
remove garbage node from the testcase

I'm r-'ing based on Darin's comment. This has potential compatibility issue for WebKit with existing web sites and hoping nothing breaks doesn't seem to be a good strategy to take here.

I think the core issue may be the bug itself. I think someone mentioned writing something to the console (when you get invalid parameters) and (while I'm no expert in this area) that seems reasonable to me.
Comment 24 Ryosuke Niwa 2010-09-24 11:10:42 PDT
Hi Morrita-san,

Would you like to be the assignee for this bug since you've been posting patches?  I'm trying to get rid of as many unconfirmed editing bugs as possible.
Comment 25 Hajime Morrita 2010-09-27 23:16:23 PDT
> Would you like to be the assignee for this bug since you've been posting patches?  I'm trying to get rid of as many unconfirmed editing bugs as possible.
Hi, thank you for the triage and I'm sorry for my slow response.
I'm assigning this to me.
Comment 26 Ryosuke Niwa 2010-10-28 00:00:51 PDT
(In reply to comment #18)
> How about printing a warning to JavaScript console instead of throwing an exception?

(In reply to comment #19)
> I feel like throwing an exception is the right thing to do since it allows the application to handle the case when the modify() doesn't go through.  But feel free to suggest this on the Mozilla bug.

I don't think printing a console message is necessary.  I'm inclined to close this bug as WONTFIX.
Comment 27 Jonas Sicking 2010-10-28 01:44:09 PDT
I really think that we want to throw an exception here. Putting something in the error console is very easy for developers to miss, and doesn't help at all unless the developer specifically goes to look for it.

It seems like a very small risk that developers would be relying on passing invalid values doing nothing. If it is a big concern maybe doing one release of just putting a warning in the error console, followed by then switching to throwing an exception might be worth it.

If we want this function standardized it seems to me that throwing an exception is the right way to go in the long run.

An alternative strategy would of course be to standardize a different function name, which webkit could safely implement to throw.
Comment 28 Ryosuke Niwa 2010-10-28 11:26:54 PDT
(In reply to comment #27)
> I really think that we want to throw an exception here. Putting something in the error console is very easy for developers to miss, and doesn't help at all unless the developer specifically goes to look for it.

Why?  I think it makes more sense for modify to return true/false given the method doesn't return any value right now.  Our internal SelectionController::modify is already a boolean function.

> It seems like a very small risk that developers would be relying on passing invalid values doing nothing. If it is a big concern maybe doing one release of just putting a warning in the error console, followed by then switching to throwing an exception might be worth it.

> An alternative strategy would of course be to standardize a different function name, which webkit could safely implement to throw.

On the other hand, why do you insist on throwing an exception?  Developer can easily verify that selection has changed by comparing selections before and after calling modify. Furthermore, selection is visible so if any developer tests his script on his own, he should realize that the selection isn't changing.

> If we want this function standardized it seems to me that throwing an exception is the right way to go in the long run.

I don't believe that throwing an exception is the only option.
Comment 29 Jonas Sicking 2010-10-28 12:34:13 PDT
(In reply to comment #28)
> (In reply to comment #27)
> > I really think that we want to throw an exception here. Putting something in the error console is very easy for developers to miss, and doesn't help at all unless the developer specifically goes to look for it.
> 
> Why?  I think it makes more sense for modify to return true/false given the method doesn't return any value right now.

Because of the precedence set by the vast majority of other DOM functions. When invalid values are used an exception is thrown.

The boolean value has the same problem as the console warning, it only helps if developers go look for it.

The goal here is to make bugs more visible and aid developers. Debugging tools are structured around exceptions being thrown for errors. Usually error consoles log exactly which line caused the exception to be thrown, and debuggers have the ability to break as soon as an exception is being thrown.

> > It seems like a very small risk that developers would be relying on passing invalid values doing nothing. If it is a big concern maybe doing one release of just putting a warning in the error console, followed by then switching to throwing an exception might be worth it.
> 
> > An alternative strategy would of course be to standardize a different function name, which webkit could safely implement to throw.
> 
> On the other hand, why do you insist on throwing an exception?  Developer can easily verify that selection has changed by comparing selections before and after calling modify. Furthermore, selection is visible so if any developer tests his script on his own, he should realize that the selection isn't changing.

The concern here isn't feature detection, but rather that bugs in pages will go silently unnoticed which is generally bad for everyone.
Comment 30 Hajime Morrita 2011-11-22 00:49:32 PST
Unassigning this myself since I have no idea how to proceed this.
Comment 31 Darin Adler 2011-11-30 10:05:53 PST
With Safari having shipped for about 8 years with the current behavior, changing the behavior could have a big effect on any sites already using this. If we want different behavior we may need to use a different name for the function.
Comment 32 Aryeh Gregor 2012-05-01 23:32:00 PDT
Well, Firefox isn't going to see any extra sites break -- it was already throwing exceptions on every call to modify() until it was implemented just recently!  I think an exception would be best ideally, but it does have potentially serious compat impact insofar as anything uses modify() to start with.

If you do throw an exception, IMO, it should be SYNTAX_ERR.
Comment 33 Ryosuke Niwa 2012-05-01 23:47:57 PDT
I think I agree with Darin here although it would have been nice if we had been throwing an error. Maybe we can output a console warning instead?