The Selection extend method should thrown an exception when no ranges are present. https://w3c.github.io/selection-api/#dom-selection-extend
<rdar://83931240>
Created attachment 442672 [details] Patch
Created attachment 442678 [details] Patch
Created attachment 442710 [details] Patch
Comment on attachment 442710 [details] Patch If this is important for interoperability why does this have no coverage in WPT?
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.
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.
Created attachment 442858 [details] Patch
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.
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
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() }); ```
Created attachment 442859 [details] Patch
Created attachment 442862 [details] Patch
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"
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.
Created attachment 442877 [details] Patch
Comment on attachment 442877 [details] Patch r=me if the bots are happy
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].