Selection API: collapseToStart() and collapseToEnd() should work for text fields. (issue 2869703004 by tkent@chromium.org)

8 views
Skip to first unread message

tk...@chromium.org

unread,
May 9, 2017, 12:28:28 AM5/9/17
to yo...@chromium.org, xiaoc...@chromium.org, blink-...@chromium.org, chromium...@chromium.org
Reviewers: yosin_UTC9, Xiaocheng
CL: https://codereview.chromium.org/2869703004/

Message:
yosin@ or xiaochengh@, would you review this please?


Description:
Selection API: collapseToStart() and collapseToEnd() should work for text
fields.

BUG=716725

Affected files (+71, -17 lines):
A third_party/WebKit/LayoutTests/editing/selection/collapseto_in_text_fields.html
M third_party/WebKit/Source/core/editing/DOMSelection.cpp


Index: third_party/WebKit/LayoutTests/editing/selection/collapseto_in_text_fields.html
diff --git a/third_party/WebKit/LayoutTests/editing/selection/collapseto_in_text_fields.html b/third_party/WebKit/LayoutTests/editing/selection/collapseto_in_text_fields.html
new file mode 100644
index 0000000000000000000000000000000000000000..5e2cbab69988923b3e5d0a8528d4f19eeec7a28d
--- /dev/null
+++ b/third_party/WebKit/LayoutTests/editing/selection/collapseto_in_text_fields.html
@@ -0,0 +1,36 @@
+<!DOCTYPE html>
+<title>Tests for crbug.com/716725</title>
+<body>
+<script src="../../resources/testharness.js"></script>
+<script src="../../resources/testharnessreport.js"></script>
+
+<input value="abcdef">
+<textarea>abcdef</textarea>
+
+<script>
+function testCollapseToStart(element) {
+ test(() => {
+ element.focus();
+ element.setSelectionRange(1, 5);
+ getSelection().collapseToStart();
+ assert_equals(element.selectionStart, 1);
+ assert_equals(element.selectionEnd, 1);
+ }, 'Selection.collapseToStart() should collapse text selection in ' + element.tagName);
+}
+
+function testCollapseToEnd(element) {
+ test(() => {
+ element.focus();
+ element.setSelectionRange(1, 5);
+ getSelection().collapseToEnd();
+ assert_equals(element.selectionStart, 5);
+ assert_equals(element.selectionEnd, 5);
+ }, 'Selection.collapseToEnd() should collapse text selection in ' + element.tagName);
+}
+
+testCollapseToStart(document.querySelector('input'));
+testCollapseToStart(document.querySelector('textarea'));
+testCollapseToEnd(document.querySelector('input'));
+testCollapseToEnd(document.querySelector('textarea'));
+</script>
+</body>
Index: third_party/WebKit/Source/core/editing/DOMSelection.cpp
diff --git a/third_party/WebKit/Source/core/editing/DOMSelection.cpp b/third_party/WebKit/Source/core/editing/DOMSelection.cpp
index dedd3cb035aa06c8e71fe668db4a4e111552962b..0dd8a65a4c64e7f55f9a88535db9b76dcd033656 100644
--- a/third_party/WebKit/Source/core/editing/DOMSelection.cpp
+++ b/third_party/WebKit/Source/core/editing/DOMSelection.cpp
@@ -294,15 +294,24 @@ void DOMSelection::collapseToEnd(ExceptionState& exception_state) {
return;
}

- // Otherwise, it must create a new range, set both its start and end to the
- // end of the context object's range,
- Range* new_range = getRangeAt(0, ASSERT_NO_EXCEPTION)->cloneRange();
- new_range->collapse(false);
-
- // and then set the context object's range to the newly-created range.
- SelectionInDOMTree::Builder builder;
- builder.Collapse(new_range->EndPosition());
- UpdateFrameSelection(builder.Build(), new_range);
+ if (Range* current_range = DocumentCachedRange()) {
+ // Otherwise, it must create a new range, set both its start and end to the
+ // end of the context object's range,
+ Range* new_range = current_range->cloneRange();
+ new_range->collapse(false);
+
+ // and then set the context object's range to the newly-created range.
+ SelectionInDOMTree::Builder builder;
+ builder.Collapse(new_range->EndPosition());
+ UpdateFrameSelection(builder.Build(), new_range);
+ } else {
+ // TODO(tkent): The Selection API doesn't define this behavior. We should
+ // discuss this on https://github.com/w3c/selection-api/issues/83.
+ SelectionInDOMTree::Builder builder;
+ builder.Collapse(
+ GetFrame()->Selection().GetSelectionInDOMTree().ComputeEndPosition());
+ UpdateFrameSelection(builder.Build(), nullptr);
+ }
}

// https://www.w3.org/TR/selection-api/#dom-selection-collapsetostart
@@ -318,15 +327,24 @@ void DOMSelection::collapseToStart(ExceptionState& exception_state) {
return;
}

- // Otherwise, it must create a new range, set both its start and end to the
- // start of the context object's range,
- Range* new_range = getRangeAt(0, ASSERT_NO_EXCEPTION)->cloneRange();
- new_range->collapse(true);
+ if (Range* current_range = DocumentCachedRange()) {
+ // Otherwise, it must create a new range, set both its start and end to the
+ // start of the context object's range,
+ Range* new_range = current_range->cloneRange();
+ new_range->collapse(true);

- // and then set the context object's range to the newly-created range.
- SelectionInDOMTree::Builder builder;
- builder.Collapse(new_range->StartPosition());
- UpdateFrameSelection(builder.Build(), new_range);
+ // and then set the context object's range to the newly-created range.
+ SelectionInDOMTree::Builder builder;
+ builder.Collapse(new_range->StartPosition());
+ UpdateFrameSelection(builder.Build(), new_range);
+ } else {
+ // TODO(tkent): The Selection API doesn't define this behavior. We should
+ // discuss this on https://github.com/w3c/selection-api/issues/83.
+ SelectionInDOMTree::Builder builder;
+ builder.Collapse(
+ GetFrame()->Selection().GetSelectionInDOMTree().ComputeStartPosition());
+ UpdateFrameSelection(builder.Build(), nullptr);
+ }
}

void DOMSelection::empty() {


xiaoc...@chromium.org

unread,
May 9, 2017, 2:09:03 PM5/9/17
to tk...@chromium.org, yo...@chromium.org, blink-...@chromium.org, chromium...@chromium.org

yosin@chromium.org via codereview.chromium.org

unread,
May 9, 2017, 8:59:46 PM5/9/17
to tk...@chromium.org, xiaoc...@chromium.org, blink-...@chromium.org, chromium...@chromium.org
lgtm

This change makes Selection#collapsTo{Start,End}() into two parts:
- Selection in Document tree, DocumentCachedRange() != nullptr
Using cached range and use start/end position
We could do without using Range but it doesn't work well when cached range
calculated from
VisibleSelection
- Selection in Shadow DOM Tree
Behavior of selection in shadow DOM tree isn't spec'ed. This patch does what
other browsers do.


https://codereview.chromium.org/2869703004/

commit-bot@chromium.org via codereview.chromium.org

unread,
May 9, 2017, 9:01:32 PM5/9/17
to tk...@chromium.org, yo...@chromium.org, xiaoc...@chromium.org, commi...@chromium.org, blink-...@chromium.org, chromium...@chromium.org

commit-bot@chromium.org via codereview.chromium.org

unread,
May 9, 2017, 11:06:47 PM5/9/17
to tk...@chromium.org, yo...@chromium.org, xiaoc...@chromium.org, commi...@chromium.org, blink-...@chromium.org, chromium...@chromium.org
Try jobs failed on following builders:
chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, build
has not started yet; builder either lacks capacity or does not exist
(misspelled?))

https://codereview.chromium.org/2869703004/

commit-bot@chromium.org via codereview.chromium.org

unread,
May 10, 2017, 6:48:35 PM5/10/17
to tk...@chromium.org, yo...@chromium.org, xiaoc...@chromium.org, commi...@chromium.org, blink-...@chromium.org, chromium...@chromium.org

tk...@chromium.org

unread,
May 10, 2017, 6:49:50 PM5/10/17
to yo...@chromium.org, xiaoc...@chromium.org, blink-...@chromium.org, chromium...@chromium.org
That's right. Ideally the |else| part of the code should work well in a case of
Document selection too, however dropping the cached range will cause Range
creation with VisibleSelection later, and cause many test failures in
wpt/selection/.


https://codereview.chromium.org/2869703004/

commit-bot@chromium.org via codereview.chromium.org

unread,
May 10, 2017, 9:08:31 PM5/10/17
to tk...@chromium.org, yo...@chromium.org, xiaoc...@chromium.org, commi...@chromium.org, blink-...@chromium.org, chromium...@chromium.org
Exhausted attempt retry quota accross all builders (set as global_retry_quota=2
in cq.cfg of this project)

https://codereview.chromium.org/2869703004/

commit-bot@chromium.org via codereview.chromium.org

unread,
May 11, 2017, 12:19:02 AM5/11/17
to tk...@chromium.org, yo...@chromium.org, xiaoc...@chromium.org, commi...@chromium.org, blink-...@chromium.org, chromium...@chromium.org

commit-bot@chromium.org via codereview.chromium.org

unread,
May 11, 2017, 1:37:59 AM5/11/17
to tk...@chromium.org, yo...@chromium.org, xiaoc...@chromium.org, commi...@chromium.org, blink-...@chromium.org, chromium...@chromium.org
Reply all
Reply to author
Forward
0 new messages