Bug 33509 – selection.modify() should throw an exception if it receives invalid parameters

• Component: HTML Editing
• Status: NEW

[Justin Lebar]

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

[Hajime Morrita]

Created attachment 50471 [details]
patch v0

[Hajime Morrita]

I'm not sure which exception we should throw. 
Although I chose INVALID_MODIFICATION_ERR, NOT_SUPPORTED_ERR would be possible, for example.

[Kent Tamura]

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.

[Hajime Morrita]

Created attachment 50478 [details]
patch v0

[Hajime Morrita]

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...

[Kent Tamura]

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 @@
> [phone]  MORITA Hajime  <[e-mail]>
> +
> +        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.

[Hajime Morrita]

Created attachment 50480 [details]
v2; arrange to clarify test result

[Hajime Morrita]

Thank you for suggestion! I updated the patch for clarifying the test result.

[Kent Tamura]

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 @@
> [phone]  MORITA Hajime  <[e-mail]>
> +
> +        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.

[Hajime Morrita]

Created attachment 50481 [details]
v3; add missing file listing to ChagneLog

[Hajime Morrita]

> Please update the ChangeLog.  script-tests/modify-with-invalid-args.js is
> missing.
Oops! Fixed. I'm sorry for that.

[Kent Tamura]

Comment on attachment 50481 [details]
v3; add missing file listing to ChagneLog

ok, I have no additional comments.  Please wait for an official reviewer.

[Darin Adler]

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.

[Justin Lebar]

(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

[Darin Adler]

(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.

[Justin Lebar]

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.

[Darin Adler]

(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.

[Kent Tamura]

How about printing a warning to JavaScript console instead of throwing an exception?

[Justin Lebar]

(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.

[Shinichiro Hamaji]

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.

[Hajime Morrita]

Created attachment 50768 [details]
remove garbage node from the testcase

[Hajime Morrita]

> 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.

[David Levin]

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.

[Ryosuke Niwa]

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.

[Hajime Morrita]

> 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.

[Ryosuke Niwa]

(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.

[Jonas Sicking]

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.

[Ryosuke Niwa]

(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.

[Jonas Sicking]

(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.

[Hajime Morrita]

Unassigning this myself since I have no idea how to proceed this.

[Darin Adler]

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.

[Aryeh Gregor]

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.

[Ryosuke Niwa]

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?

[Ahmad Saleem]

It is easy to do but I don't see even Blink doing it here:

https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/editing/dom_selection.cc;l=387;drc=94ebc1b26bcbaec9152004e8f1ff5823cd676492

Even couldn't find WPT as well.

@Ryosuke / Wenson - should we keep it?