Summary: | collapse api in DOMSelection IDL does not match as per specification | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Shivakumar J M <shiva.jm> | ||||||
Component: | DOM | Assignee: | Nobody <webkit-unassigned> | ||||||
Status: | NEW --- | ||||||||
Severity: | Normal | CC: | ahmad.saleem792, ap, bfulgham, buildbot, cdumez, darin, gyuyoung.kim, rniwa, simon.fraser | ||||||
Priority: | P2 | ||||||||
Version: | 528+ (Nightly build) | ||||||||
Hardware: | Unspecified | ||||||||
OS: | Unspecified | ||||||||
Attachments: |
|
Description
Shivakumar J M
2014-11-06 22:08:15 PST
Created attachment 241165 [details]
Patch
collapse api in DOMSelection IDL does not match as per specification
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. 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. Created attachment 241341 [details]
Patch-Updated-Review1
Updated the patch with tests.
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. 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). (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. 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! |