Bug 138494 - collapse api in DOMSelection IDL does not match as per specification
Summary: collapse api in DOMSelection IDL does not match as per specification
Status: NEW
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2014-11-06 22:08 PST by Shivakumar J M
Modified: 2014-11-11 19:33 PST (History)
7 users (show)

See Also:


Attachments
Patch (1.23 KB, patch)
2014-11-07 00:34 PST, Shivakumar J M
darin: review-
darin: commit-queue-
Details | Formatted Diff | Diff
Patch-Updated-Review1 (4.22 KB, patch)
2014-11-11 00:40 PST, Shivakumar J M
cdumez: review-
cdumez: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Shivakumar J M 2014-11-06 22:08:15 PST
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()
Comment 1 Shivakumar J M 2014-11-07 00:34:22 PST
Created attachment 241165 [details]
Patch

collapse api in DOMSelection IDL does not match as per specification
Comment 2 Darin Adler 2014-11-07 15:23:09 PST
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 3 Ryosuke Niwa 2014-11-07 15:42:27 PST
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.
Comment 4 Shivakumar J M 2014-11-11 00:40:55 PST
Created attachment 241341 [details]
Patch-Updated-Review1

Updated the patch with tests.
Comment 5 Chris Dumez 2014-11-11 09:59:20 PST
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 6 Chris Dumez 2014-11-11 10:08:16 PST
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).
Comment 7 Shivakumar J M 2014-11-11 19:33:59 PST
(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.