Bug 232420

Summary:

Selection extend() should trigger exception with no ranges

Product:

WebKit

Reporter:

Brandon <brandonstewart>

Component:

DOM

Assignee:

Nobody <webkit-unassigned>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

brandonstewart, cdumez, clopez, darin, ews-watchlist, ntim, webkit-bug-importer, youennf

Priority:

Keywords:

InRadar

Version:

Other

Hardware:

Unspecified

OS:

Unspecified

See Also:

https://github.com/web-platform-tests/wpt/pull/31460

Attachments:

Description	Flags
Patch	ews-feeder: commit-queue-
Patch	ews-feeder: commit-queue-
Patch	cdumez: review-
Patch	none
Patch	none
Patch	none
Patch	none

Brandon

Reported 2021-10-27 20:33:28 PDT

The Selection extend method should thrown an exception when no ranges are present. https://w3c.github.io/selection-api/#dom-selection-extend

Attachments
Patch (3.66 KB, patch) 2021-10-27 21:34 PDT, Brandon	ews-feeder: commit-queue-	Details Formatted Diff Diff
Patch (7.32 KB, patch) 2021-10-27 23:39 PDT, Brandon	ews-feeder: commit-queue-	Details Formatted Diff Diff
Patch (7.45 KB, patch) 2021-10-28 08:51 PDT, Brandon	cdumez: review-	Details Formatted Diff Diff
Patch (7.47 KB, patch) 2021-10-29 13:22 PDT, Brandon	no flags	Details Formatted Diff Diff
Patch (8.39 KB, patch) 2021-10-29 14:03 PDT, Brandon	no flags	Details Formatted Diff Diff
Patch (8.39 KB, patch) 2021-10-29 14:19 PDT, Brandon	no flags	Details Formatted Diff Diff
Patch (8.37 KB, patch) 2021-10-29 15:55 PDT, Brandon	no flags	Details Formatted Diff Diff
Show Obsolete (6) View All Add attachment proposed patch, testcase, etc.

Brandon

Comment 1 2021-10-27 20:38:00 PDT

<rdar://83931240>

Brandon

Comment 2 2021-10-27 21:34:37 PDT

Created attachment 442672 [details] Patch

Brandon

Comment 3 2021-10-27 23:39:02 PDT

Created attachment 442678 [details] Patch

Brandon

Comment 4 2021-10-28 08:51:42 PDT

Created attachment 442710 [details] Patch

Darin Adler

Comment 5 2021-10-28 13:34:16 PDT

Comment on attachment 442710 [details] Patch If this is important for interoperability why does this have no coverage in WPT?

Chris Dumez

Comment 6 2021-10-28 15:16:39 PDT

Comment on attachment 442710 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=442710&action=review > LayoutTests/fast/dom/Range/missingRange.html:3 > + <script src="../../../resources/js-test-pre.js"></script> If you don't end up converting this to a WPT test, you'll need to include this instead: <script src="/resources/testharness.js"></script> <script src="/resources/testharnessreport.js"></script> > LayoutTests/fast/dom/Range/missingRange.html:12 > + getSelection().extend(div1); And here: something like this: ``` test(function () { let div = document.createElement('div'); document.body.appendChild(div); assert_throws_dom("InvalidStateError", () => { getSelection().extend(div); }); }, 'Test extend method failure due to missing range'); ``` Then move it under LayoutTests/imported/w3c/web-platform-tests/selection/. Then later on submit a PR for that test to https://github.com/web-platform-tests/wpt.

Chris Dumez

Comment 7 2021-10-28 15:39:37 PDT

Comment on attachment 442710 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=442710&action=review > LayoutTests/fast/dom/Range/missingRange-expected.txt:7 > +FAIL successfullyParsed should be true (of type boolean). Was undefined (of type undefined). r- due to FAIL line in test output. >> LayoutTests/fast/dom/Range/missingRange.html:12 >> + getSelection().extend(div1); > > And here: > something like this: > ``` > test(function () { > let div = document.createElement('div'); > document.body.appendChild(div); > assert_throws_dom("InvalidStateError", () => { > getSelection().extend(div); > }); > }, 'Test extend method failure due to missing range'); > ``` > > Then move it under LayoutTests/imported/w3c/web-platform-tests/selection/. > > Then later on submit a PR for that test to https://github.com/web-platform-tests/wpt. And in the future, if writing such tests using js-test.js, please: - Include js-test.js instead of js-test-pre.js / js-test-post.js which are legacy - shouldThrowErrorName("getSelection().extend(div1);", "InvalidStateError") to properly PASS / FAIL based on the exception being thrown. Right now, the exception is throw and not caught and causes a FAIL line to be printed in the output.

Brandon

Comment 8 2021-10-29 13:22:26 PDT

Created attachment 442858 [details] Patch

Brandon

Comment 9 2021-10-29 13:34:56 PDT

I replaced the layout test with a new WPT test. From looking at the WPT extend tests they do have code that will handle a rangeCount of 0, but I am unable to find a specific test in which the exception is triggered.

EWS Watchlist

Comment 10 2021-10-29 13:36:50 PDT

This patch modifies the imported WPT tests. Please ensure that any changes on the tests (not coming from a WPT import) are exported to WPT. Please see https://trac.webkit.org/wiki/WPTExportProcess

Chris Dumez

Comment 11 2021-10-29 13:43:00 PDT

Comment on attachment 442858 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=442858&action=review > LayoutTests/imported/w3c/web-platform-tests/selection/extend-exception.html:2 > +<title>Selection extend() tests</title> Please provide a more specific title > LayoutTests/imported/w3c/web-platform-tests/selection/extend-exception.html:5 > +<body> Please close you <body> > LayoutTests/imported/w3c/web-platform-tests/selection/extend-exception.html:8 > +<script src=common.js></script> What is this used for? > LayoutTests/imported/w3c/web-platform-tests/selection/extend-exception.html:9 > +<script src=extend.js></script> What is this used for? > LayoutTests/imported/w3c/web-platform-tests/selection/extend-exception.html:10 > +<div id="log"></div> Ideally, the test() would create and remove the div: let div = document.createElement("div"); document.body.appendChild(div); > LayoutTests/imported/w3c/web-platform-tests/selection/extend-exception.html:14 > +var selection = document.getSelection(); Should be inside the test(). > LayoutTests/imported/w3c/web-platform-tests/selection/extend-exception.html:19 > + selection.removeAllRanges(); Which is this needed? How can there be a range already? > LayoutTests/imported/w3c/web-platform-tests/selection/extend-exception.html:22 > +}); We normally pass a string parameter as second parameter with a test description. > LayoutTests/imported/w3c/web-platform-tests/selection/extend-exception.html:24 > +testDiv.style.display = "none"; The test should do clean up itself. I'd recommend adding something like this inside your test(): ``` this.add_cleanup(function() { div.remove() }); ```

Brandon

Comment 12 2021-10-29 14:03:46 PDT

Created attachment 442859 [details] Patch

Brandon

Comment 13 2021-10-29 14:19:13 PDT

Created attachment 442862 [details] Patch

Chris Dumez

Comment 14 2021-10-29 14:31:21 PDT

Comment on attachment 442859 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=442859&action=review > LayoutTests/editing/execCommand/null_calc_primitive_value_for_css_property.html:16 > + document.getSelection().addRange(range); Wouldn't `document.getSelection().collapse(x, 0);` work? > LayoutTests/editing/execCommand/transpose-backslash-with-euc.html:13 > + getSelection().addRange(new Range()); May be better to call: getSelection().collapse(backslashDivElement, 0); before calling extend. > LayoutTests/editing/inserting/insert-html-crash-02.html:16 > + getSelection().addRange(new Range()); Similarly, would be better to call getSelection().collapse(document.body, 0) I think. > LayoutTests/imported/w3c/ChangeLog:9 > + contains no Range objects. Please provide a link to the specification (https://w3c.github.io/selection-api/#dom-selection-extend) since you are aligning with the spec. Also please mention in the change log the behavior of other browser engines. Matching the spec is good but only if other engines agree with the spec. > LayoutTests/imported/w3c/web-platform-tests/selection/extend-exception.html:2 > +<title>Selection extend() test thrown exceptions</title> Missing <html> tag. > LayoutTests/imported/w3c/web-platform-tests/selection/extend-exception.html:4 > +<meta name=timeout content=long> This is a trivial test, it doesn't need a long timeout. > Source/WebCore/page/DOMSelection.cpp:342 > + if (this->rangeCount() < 1) is this-> really needed? > Source/WebCore/page/DOMSelection.cpp:343 > + return Exception { InvalidStateError, "The 'extend' method requires a Range object to be added to the Selection object before calling this method." }; I think I'd prefer a message like: "extend() cannot be called on an empty selection"

Brandon

Comment 15 2021-10-29 15:54:59 PDT

Comment on attachment 442859 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=442859&action=review >> Source/WebCore/page/DOMSelection.cpp:343 >> + return Exception { InvalidStateError, "The 'extend' method requires a Range object to be added to the Selection object before calling this method." }; > > I think I'd prefer a message like: "extend() cannot be called on an empty selection" I am good with increasing the terseness of the current message, but we should still mention the missing range object. Other browsers make it a point to mention it too.

Brandon

Comment 16 2021-10-29 15:55:02 PDT

Brandon

Comment 17 2021-10-29 15:55:53 PDT

Created attachment 442877 [details] Patch

Chris Dumez

Comment 18 2021-10-29 15:59:53 PDT

Comment on attachment 442877 [details] Patch r=me if the bots are happy

EWS

Comment 19 2021-10-30 15:58:29 PDT

Committed r285084 (243726@main): <https://commits.webkit.org/243726@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 442877 [details].

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