Range.surroundContents should throw InvalidStateError if a non-Text node is partially contained (issue 441853003 by kangil.han@samsung.com)

435 views
Skip to first unread message

kangi...@samsung.com

unread,
Aug 5, 2014, 2:25:01 AM8/5/14
to tk...@chromium.org, har...@chromium.org, yu...@chromium.org, blink-...@chromium.org, blink-re...@chromium.org, dglazko...@chromium.org, sigb...@opera.com, eae+bli...@chromium.org, rob....@samsung.com
Reviewers: tkent, haraken, Yuta Kitamura,

Message:
PTAL

fyi, trybot seems not working well at this moment.

Description:
Range.surroundContents should throw InvalidStateError if a non-Text node is
partially contained

SPEC: http://dom.spec.whatwg.org/#dom-range-surroundcontents

72 test cases will be passed with this CL in
http://w3c-test.org/dom/ranges/Range-surroundContents.html

Behavior in other browsers.
*)FF: PASS
*)IE: FAIL

TEST=LayoutTests/fast/dom/Range/surroundContents-for-invalid-state-error.html

Please review this at https://codereview.chromium.org/441853003/

SVN Base: https://chromium.googlesource.com/chromium/blink.git@master

Affected files (+40, -12 lines):
A LayoutTests/fast/dom/Range/surroundContents-for-invalid-state-error.html
A
LayoutTests/fast/dom/Range/surroundContents-for-invalid-state-error-expected.txt
M Source/core/dom/Range.cpp


Index:
LayoutTests/fast/dom/Range/surroundContents-for-invalid-state-error-expected.txt
diff --git
a/LayoutTests/fast/dom/Range/surroundContents-for-invalid-state-error-expected.txt
b/LayoutTests/fast/dom/Range/surroundContents-for-invalid-state-error-expected.txt
new file mode 100644
index
0000000000000000000000000000000000000000..ac1b547c901af89173ca816086c56e0d27f7b83e
--- /dev/null
+++
b/LayoutTests/fast/dom/Range/surroundContents-for-invalid-state-error-expected.txt
@@ -0,0 +1,5 @@
+PASS range.surroundContents(start) threw exception InvalidStateError:
Failed to execute 'surroundContents' on 'Range': The Range has partially
selected a non-Text node..
+PASS successfullyParsed is true
+
+TEST COMPLETE
+
Index:
LayoutTests/fast/dom/Range/surroundContents-for-invalid-state-error.html
diff --git
a/LayoutTests/fast/dom/Range/surroundContents-for-invalid-state-error.html
b/LayoutTests/fast/dom/Range/surroundContents-for-invalid-state-error.html
new file mode 100644
index
0000000000000000000000000000000000000000..a9da9ac3f23feca3109fa553fa9a0e74dab3beb5
--- /dev/null
+++
b/LayoutTests/fast/dom/Range/surroundContents-for-invalid-state-error.html
@@ -0,0 +1,23 @@
+<html>
+<head>
+<script src="../../../resources/js-test.js"></script>
+</head>
+<body>
+<div id='test'>
+ <p id='start'>start</p>
+ <p id='end'>end</p>
+</div>
+<script>
+var range = document.createRange();
+var start = document.getElementById('start');
+var end = document.getElementById('end');
+range.setStart(start.firstChild, 0);
+range.setEnd(end.firstChild, 0);
+
+shouldThrow("range.surroundContents(start)", '"InvalidStateError: Failed
to execute \'surroundContents\' on \'Range\': The Range has partially
selected a non-Text node."');
+
+if (window.testRunner)
+ document.getElementById('test').outerHTML = '';
+</script>
+</body>
+</html>
Index: Source/core/dom/Range.cpp
diff --git a/Source/core/dom/Range.cpp b/Source/core/dom/Range.cpp
index
0020d59410cc9e7d97e78678df1c14b96fb39b8b..17f2a4fe25afec1e81addb967926303c086cb174
100644
--- a/Source/core/dom/Range.cpp
+++ b/Source/core/dom/Range.cpp
@@ -1208,6 +1208,18 @@ void
Range::surroundContents(PassRefPtrWillBeRawPtr<Node> passNewParent, Excepti
return;
}

+ // BAD_BOUNDARYPOINTS_ERR: Raised if the Range partially selects a
non-Text node.
+ Node* startNonTextContainer = m_start.container();
+ if (startNonTextContainer->nodeType() == Node::TEXT_NODE)
+ startNonTextContainer = startNonTextContainer->parentNode();
+ Node* endNonTextContainer = m_end.container();
+ if (endNonTextContainer->nodeType() == Node::TEXT_NODE)
+ endNonTextContainer = endNonTextContainer->parentNode();
+ if (startNonTextContainer != endNonTextContainer) {
+ exceptionState.throwDOMException(InvalidStateError, "The Range has
partially selected a non-Text node.");
+ return;
+ }
+
// InvalidNodeTypeError: Raised if node is an Attr, Entity,
DocumentType, Notation,
// Document, or DocumentFragment node.
switch (newParent->nodeType()) {
@@ -1252,18 +1264,6 @@ void
Range::surroundContents(PassRefPtrWillBeRawPtr<Node> passNewParent, Excepti
// FIXME: Do we need a check if the node would end up with a child
node of a type not
// allowed by the type of node?

- // BAD_BOUNDARYPOINTS_ERR: Raised if the Range partially selects a
non-Text node.
- Node* startNonTextContainer = m_start.container();
- if (startNonTextContainer->nodeType() == Node::TEXT_NODE)
- startNonTextContainer = startNonTextContainer->parentNode();
- Node* endNonTextContainer = m_end.container();
- if (endNonTextContainer->nodeType() == Node::TEXT_NODE)
- endNonTextContainer = endNonTextContainer->parentNode();
- if (startNonTextContainer != endNonTextContainer) {
- exceptionState.throwDOMException(InvalidStateError, "The Range has
partially selected a non-Text node.");
- return;
- }
-
while (Node* n = newParent->firstChild()) {
toContainerNode(newParent)->removeChild(n, exceptionState);
if (exceptionState.hadException())


har...@chromium.org

unread,
Aug 5, 2014, 2:34:10 AM8/5/14
to kangi...@samsung.com, tk...@chromium.org, yu...@chromium.org, blink-...@chromium.org, blink-re...@chromium.org, dglazko...@chromium.org, sigb...@opera.com, eae+bli...@chromium.org, rob....@samsung.com
The change to core/ looks good, but yutak-san wants to take a look.



https://codereview.chromium.org/441853003/diff/1/LayoutTests/fast/dom/Range/surroundContents-for-invalid-state-error.html
File
LayoutTests/fast/dom/Range/surroundContents-for-invalid-state-error.html
(right):

https://codereview.chromium.org/441853003/diff/1/LayoutTests/fast/dom/Range/surroundContents-for-invalid-state-error.html#newcode20
LayoutTests/fast/dom/Range/surroundContents-for-invalid-state-error.html:20:
document.getElementById('test').outerHTML = '';

What is this code for?

https://codereview.chromium.org/441853003/

kangi...@samsung.com

unread,
Aug 5, 2014, 2:49:59 AM8/5/14
to tk...@chromium.org, har...@chromium.org, yu...@chromium.org, blink-...@chromium.org, blink-re...@chromium.org, dglazko...@chromium.org, sigb...@opera.com, eae+bli...@chromium.org, rob....@samsung.com

https://codereview.chromium.org/441853003/diff/1/LayoutTests/fast/dom/Range/surroundContents-for-invalid-state-error.html
File
LayoutTests/fast/dom/Range/surroundContents-for-invalid-state-error.html
(right):

https://codereview.chromium.org/441853003/diff/1/LayoutTests/fast/dom/Range/surroundContents-for-invalid-state-error.html#newcode20
LayoutTests/fast/dom/Range/surroundContents-for-invalid-state-error.html:20:
document.getElementById('test').outerHTML = '';
On 2014/08/05 06:34:10, haraken wrote:

> What is this code for?

This will clean 'start' and 'end' from the result.

https://codereview.chromium.org/441853003/

kangi...@samsung.com

unread,
Aug 7, 2014, 8:32:27 AM8/7/14
to tk...@chromium.org, har...@chromium.org, yu...@chromium.org, yo...@chromium.org, blink-...@chromium.org, blink-re...@chromium.org, dglazko...@chromium.org, sigb...@opera.com, eae+bli...@chromium.org, rob....@samsung.com
@yutak: PTAL :)

https://codereview.chromium.org/441853003/

yu...@chromium.org

unread,
Aug 8, 2014, 12:49:31 AM8/8/14
to kangi...@samsung.com, tk...@chromium.org, har...@chromium.org, yo...@chromium.org, blink-...@chromium.org, blink-re...@chromium.org, dglazko...@chromium.org, sigb...@opera.com, eae+bli...@chromium.org, rob....@samsung.com
I'm mildly confused by the change description. This patch
just corrects the order of error checking, right? Your
change description is not sufficiently explaining that.

Please revise the change description so that people can
better understand what the patch is actually trying to do.

The change itself looks good.


https://codereview.chromium.org/441853003/diff/1/Source/core/dom/Range.cpp
File Source/core/dom/Range.cpp (right):

https://codereview.chromium.org/441853003/diff/1/Source/core/dom/Range.cpp#newcode1211
Source/core/dom/Range.cpp:1211: // BAD_BOUNDARYPOINTS_ERR: Raised if the
Range partially selects a non-Text node.
BAD_BOUNDARYPOINTS_ERR is a completely wrong documentation
(I think it's the exception type which we previously threw).
Let's replace it with "InvalidStateError".

https://codereview.chromium.org/441853003/

kangi...@samsung.com

unread,
Aug 8, 2014, 1:04:48 AM8/8/14
to har...@chromium.org, yu...@chromium.org, yo...@chromium.org, blink-...@chromium.org, blink-re...@chromium.org, dglazko...@chromium.org, sigb...@opera.com, eae+bli...@chromium.org, rob....@samsung.com
PTAL


https://codereview.chromium.org/441853003/diff/1/Source/core/dom/Range.cpp
File Source/core/dom/Range.cpp (right):

https://codereview.chromium.org/441853003/diff/1/Source/core/dom/Range.cpp#newcode1211
Source/core/dom/Range.cpp:1211: // BAD_BOUNDARYPOINTS_ERR: Raised if the
Range partially selects a non-Text node.
On 2014/08/08 04:49:31, Yuta Kitamura wrote:
> BAD_BOUNDARYPOINTS_ERR is a completely wrong documentation
> (I think it's the exception type which we previously threw).
> Let's replace it with "InvalidStateError".

Done.

https://codereview.chromium.org/441853003/

yu...@chromium.org

unread,
Aug 8, 2014, 1:38:48 AM8/8/14
to kangi...@samsung.com, har...@chromium.org, yo...@chromium.org, blink-...@chromium.org, blink-re...@chromium.org, dglazko...@chromium.org, sigb...@opera.com, eae+bli...@chromium.org, rob....@samsung.com
lgtm



https://codereview.chromium.org/441853003/

commi...@chromium.org

unread,
Aug 8, 2014, 1:41:56 AM8/8/14
to kangi...@samsung.com, har...@chromium.org, yu...@chromium.org, yo...@chromium.org, blink-...@chromium.org, blink-re...@chromium.org, dglazko...@chromium.org, sigb...@opera.com, eae+bli...@chromium.org, rob....@samsung.com
CQ is trying da patch. Follow status at

https://chromium-status.appspot.com/cq/kangi...@samsung.com/441853003/20001


https://codereview.chromium.org/441853003/

commi...@chromium.org

unread,
Aug 8, 2014, 3:09:03 AM8/8/14
to kangi...@samsung.com, har...@chromium.org, yu...@chromium.org, yo...@chromium.org, blink-...@chromium.org, blink-re...@chromium.org, dglazko...@chromium.org, sigb...@opera.com, eae+bli...@chromium.org, rob....@samsung.com
Change committed as 179795

https://codereview.chromium.org/441853003/
Reply all
Reply to author
Forward
0 new messages