Bug 138494 – collapse api in DOMSelection IDL does not match as per specification

• Component: DOM
• Status: NEW

[Shivakumar J M]

collapse api in DOMSelection IDL does not match as per specification: http://www.w3.org/TR/selection-api/#widl-Selection-collapse-void-Node-node-unsigned-long-offset.
Specificaton does not mandate Node and offset to be optional, also node should not null, which is already covered in DOMSelection::isValidForPosition()

[Shivakumar J M]

Created attachment 241165 [details]
Patch

collapse api in DOMSelection IDL does not match as per specification

[Darin Adler]

Comment on attachment 241165 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=241165&action=review

Good change, but requires a test.

> Source/WebCore/ChangeLog:8
> +        No new tests, no behavior change.

That is not true. This is a behavior change. This change needs test coverage.

[Ryosuke Niwa]

Comment on attachment 241165 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=241165&action=review

> Source/WebCore/page/DOMSelection.idl:44
> -    [RaisesException] void collapse([Default=Undefined] optional Node node, 
> -                  [Default=Undefined] optional long index);
> +    [RaisesException] void collapse(Node node, long index);

I'm not certain if this is a backwards compatible change.
It may break contents that rely on these arguments to be optional in WebKit.

[Shivakumar J M]

Created attachment 241341 [details]
Patch-Updated-Review1

Updated the patch with tests.

[Chris Dumez]

Comment on attachment 241341 [details]
Patch-Updated-Review1

View in context: https://bugs.webkit.org/attachment.cgi?id=241341&action=review

> Source/WebCore/ChangeLog:10
> +        Chrome 38.

How about Firefox and IE?

> Source/WebCore/page/DOMSelection.idl:44
> +    [RaisesException] void collapse(Node node, [Default=Undefined] optional long index);

Please make sure calling collapse(null) still works (clears selection) as content apparently relies on this:
https://codereview.chromium.org/546703003/

We should add test coverage for this as well.

[Chris Dumez]

Comment on attachment 241341 [details]
Patch-Updated-Review1

r- because the patch seems to break several layout tests (yes, our layout tests call collapse() without argument).

[Shivakumar J M]

(In reply to comment #6)
> Comment on attachment 241341 [details]
> Patch-Updated-Review1
> 
> r- because the patch seems to break several layout tests (yes, our layout
> tests call collapse() without argument).

(In reply to comment #6)
> Comment on attachment 241341 [details]
> Patch-Updated-Review1
> 
> r- because the patch seems to break several layout tests (yes, our layout
> tests call collapse() without argument).


Dear Chris,

     Thanks for inputs, will modify all tests, also make selection content clears and add test coverage for the same.

[Ahmad Saleem]

I took the test case from the patch and changed it into JSFiddle:

Link - https://jsfiddle.net/xs17np58/show

and these are results across all browsers:

*** Safari 15.6 on macOS 12.5 ***

PASS selection.rangeCount is 1
PASS selection.collapse() threw exception TypeError: Not enough arguments.
PASS selection.rangeCount is 1
PASS selection.rangeCount is 1
PASS selection.rangeCount is 0
FAIL successfullyParsed should be true. Was false.
Some tests failed.

TEST COMPLETE

*** Chrome Canary 105 ***

PASS selection.rangeCount is 1
FAIL selection.collapse() should throw TypeError: Not enough arguments. Threw exception TypeError: Failed to execute 'collapse' on 'Selection': 1 argument required, but only 0 present..
PASS selection.rangeCount is 1
PASS selection.rangeCount is 1
PASS selection.rangeCount is 0
FAIL successfullyParsed should be true. Was false.
Some tests failed.

*** Firefox Nightly 105 ***

PASS selection.rangeCount is 1
FAIL selection.collapse() should throw TypeError: Not enough arguments. Threw exception TypeError: Selection.collapse: At least 1 argument required, but only 0 passed.
PASS selection.rangeCount is 1
PASS selection.rangeCount is 1
PASS selection.rangeCount is 0
FAIL successfullyParsed should be true. Was false.
Some tests failed.

TEST COMPLETE

_______________

As can be seen from the test results, all fail on "successfulyParsed" but Chrome Canary 106 and Firefox Nightly also fail on "selection.collapse()" compared to Safari, I don't know whether it is intentional to align with web-spec or not but just wanted to share updated status.

Please mark this bug accordingly. Thank!