Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
212873002:
Remove Selection.containsNode assertion that is not valid. (Closed)
|
Created:
6 years, 9 months ago by Mads Ager (chromium) Modified:
6 years, 9 months ago CC:
blink-reviews Base URL:
svn://svn.chromium.org/blink/trunk Visibility:
Public. |
DescriptionRemove Selection.containsNode assertion that is not valid.
Align with Firefox and return false in the case where the
Range method leads to an exception. Previously, we returned
true.
The behavior of these selection APIs are not in any specs
and they are only implemented by Blink and Firefox.
R=haraken@chromium.org, nbarth@chromium.org
BUG=
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=170291
Patch Set 1 #
Total comments: 12
Patch Set 2 : Make test slightly smaller. #
Total comments: 7
Patch Set 3 : Fix spacing #
Messages
Total messages: 15 (0 generated)
Adding Kent as a reviewer since this is a (very minor) change is public API behavior.
https://codereview.chromium.org/212873002/diff/1/LayoutTests/editing/selectio... File LayoutTests/editing/selection/contains-node-cleared-document.html (right): https://codereview.chromium.org/212873002/diff/1/LayoutTests/editing/selectio... LayoutTests/editing/selection/contains-node-cleared-document.html:1: <!DOCTYPE html> You could make this test nicer by including js-test.js. https://codereview.chromium.org/212873002/diff/1/LayoutTests/editing/selectio... LayoutTests/editing/selection/contains-node-cleared-document.html:5: testRunner.dumpAsText(); You can remove this. https://codereview.chromium.org/212873002/diff/1/LayoutTests/editing/selectio... LayoutTests/editing/selection/contains-node-cleared-document.html:13: document.open(); I'm not sure if it's needed to open the document to test what you want to test. You might want to minimize the test case. https://codereview.chromium.org/212873002/diff/1/LayoutTests/editing/selectio... LayoutTests/editing/selection/contains-node-cleared-document.html:14: if (selection.containsNode(element,true)) You can just write: shouldBeFalse("selection.containsNode(element, true)"); Nit: One space needed before 'true'. https://codereview.chromium.org/212873002/diff/1/Source/core/page/DOMSelectio... File Source/core/page/DOMSelection.cpp (right): https://codereview.chromium.org/212873002/diff/1/Source/core/page/DOMSelectio... Source/core/page/DOMSelection.cpp:491: ASSERT(!exceptionState.hadException()); How about this ASSERT? Should we also fix this one? (You can do that in a follow-up if you like.)
lgtm
https://codereview.chromium.org/212873002/diff/1/LayoutTests/editing/selectio... File LayoutTests/editing/selection/contains-node-cleared-document.html (right): https://codereview.chromium.org/212873002/diff/1/LayoutTests/editing/selectio... LayoutTests/editing/selection/contains-node-cleared-document.html:1: <!DOCTYPE html> On 2014/03/26 13:06:30, haraken wrote: > > You could make this test nicer by including js-test.js. It seems that the way this test is messing with the document makes that not work. This test was copied from contains-node-crash.html which does something similar. https://codereview.chromium.org/212873002/diff/1/LayoutTests/editing/selectio... LayoutTests/editing/selection/contains-node-cleared-document.html:5: testRunner.dumpAsText(); On 2014/03/26 13:06:30, haraken wrote: > > You can remove this. Yes, but why not dump as text so that it is just a nice "PASS" text? https://codereview.chromium.org/212873002/diff/1/LayoutTests/editing/selectio... LayoutTests/editing/selection/contains-node-cleared-document.html:13: document.open(); On 2014/03/26 13:06:30, haraken wrote: > > I'm not sure if it's needed to open the document to test what you want to test. > You might want to minimize the test case. This is pretty minimal. It is needed to open the document. That is the point. That the old document gets removed here and therefore we are asking a selection of a detached document. https://codereview.chromium.org/212873002/diff/1/LayoutTests/editing/selectio... LayoutTests/editing/selection/contains-node-cleared-document.html:14: if (selection.containsNode(element,true)) On 2014/03/26 13:06:30, haraken wrote: > > You can just write: > > shouldBeFalse("selection.containsNode(element, true)"); > > Nit: One space needed before 'true'. Unfortunately, that does not work. Probably because I'm messing with the document here. I get null exceptions from the js-test.js code if I do that. https://codereview.chromium.org/212873002/diff/1/Source/core/page/DOMSelectio... File Source/core/page/DOMSelection.cpp (right): https://codereview.chromium.org/212873002/diff/1/Source/core/page/DOMSelectio... Source/core/page/DOMSelection.cpp:491: ASSERT(!exceptionState.hadException()); On 2014/03/26 13:06:30, haraken wrote: > > How about this ASSERT? Should we also fix this one? (You can do that in a > follow-up if you like.) I honestly don't know. I'm hoping that someone familiar with the selection code can take the test case that I have here and see if they can get this to trigger. I'm fixing this because I hit this with an unrelated oilpan change and noticed that the assert above is wrong as it is.
LGTM Needs an approval from an API owner. https://codereview.chromium.org/212873002/diff/1/LayoutTests/editing/selectio... File LayoutTests/editing/selection/contains-node-cleared-document.html (right): https://codereview.chromium.org/212873002/diff/1/LayoutTests/editing/selectio... LayoutTests/editing/selection/contains-node-cleared-document.html:1: <!DOCTYPE html> On 2014/03/26 13:18:00, Mads Ager (chromium) wrote: > On 2014/03/26 13:06:30, haraken wrote: > > > > You could make this test nicer by including js-test.js. > > It seems that the way this test is messing with the document makes that not > work. This test was copied from contains-node-crash.html which does something > similar. oh I see, thanks. https://codereview.chromium.org/212873002/diff/1/LayoutTests/editing/selectio... LayoutTests/editing/selection/contains-node-cleared-document.html:5: testRunner.dumpAsText(); On 2014/03/26 13:18:00, Mads Ager (chromium) wrote: > On 2014/03/26 13:06:30, haraken wrote: > > > > You can remove this. > > Yes, but why not dump as text so that it is just a nice "PASS" text? I just meant, if you include js-test.js, it calls dumpAsText for you :)
What the specification says about this behavior? How IE works? Adding editing people.
On 2014/03/27 00:36:51, tkent wrote: > What the specification says about this behavior? How IE works? > > Adding editing people. As far as I know, it's implemented in Firefox and Blink, but not yet speced. http://lists.w3.org/Archives/Public/public-webapps/2013OctDec/1018.html
https://codereview.chromium.org/212873002/diff/100001/LayoutTests/editing/sel... File LayoutTests/editing/selection/contains-node-cleared-document.html (right): https://codereview.chromium.org/212873002/diff/100001/LayoutTests/editing/sel... LayoutTests/editing/selection/contains-node-cleared-document.html:11: var parent = element.parentNode; |parent| is not used; is this necessary? https://codereview.chromium.org/212873002/diff/100001/LayoutTests/editing/sel... LayoutTests/editing/selection/contains-node-cleared-document.html:13: if (selection.containsNode(element,true)) nit: space after comma https://codereview.chromium.org/212873002/diff/100001/Source/core/page/DOMSel... File Source/core/page/DOMSelection.cpp (right): https://codereview.chromium.org/212873002/diff/100001/Source/core/page/DOMSel... Source/core/page/DOMSelection.cpp:484: if (exceptionState.hadException()) In what situation does this become true? I think it's better to add an error check above (along with lines 471-479) to avoid the DOM exception, and keep the ASSERT as is. Or better yet we could use ASSERT_NO_EXCEPTION.
On 2014/03/27 00:46:45, haraken wrote: > As far as I know, it's implemented in Firefox and Blink, but not yet speced. > http://lists.w3.org/Archives/Public/public-webapps/2013OctDec/1018.html Thanks. So, matching to Firefox is a good idea. lgtm. The CL description should mention such information.
https://codereview.chromium.org/212873002/diff/100001/LayoutTests/editing/sel... File LayoutTests/editing/selection/contains-node-cleared-document.html (right): https://codereview.chromium.org/212873002/diff/100001/LayoutTests/editing/sel... LayoutTests/editing/selection/contains-node-cleared-document.html:11: var parent = element.parentNode; On 2014/03/27 01:21:12, Yuta Kitamura wrote: > |parent| is not used; is this necessary? Yes, that is actually what is causing the issue here. In containsNode there is a check that if the parent node is null then the answer is false. The JavaScript variable |parent| keeps the parent alive here so that we get past that check. We then get into CompareBoundaryPoints in Range which figures out that the nodes are in different document fragments and therefore throws an exception. https://codereview.chromium.org/212873002/diff/100001/LayoutTests/editing/sel... LayoutTests/editing/selection/contains-node-cleared-document.html:13: if (selection.containsNode(element,true)) On 2014/03/27 01:21:12, Yuta Kitamura wrote: > nit: space after comma Thanks, done. https://codereview.chromium.org/212873002/diff/100001/Source/core/page/DOMSel... File Source/core/page/DOMSelection.cpp (right): https://codereview.chromium.org/212873002/diff/100001/Source/core/page/DOMSel... Source/core/page/DOMSelection.cpp:484: if (exceptionState.hadException()) On 2014/03/27 01:21:12, Yuta Kitamura wrote: > In what situation does this become true? > > I think it's better to add an error check above (along > with lines 471-479) to avoid the DOM exception, and keep > the ASSERT as is. Or better yet we could use > ASSERT_NO_EXCEPTION. This becomes true when the nodes that are compared are in separate document fragments of the same document (which can happen when the document is cleared as in the test). I don't know how to check for that in any other way that duplicating the code from compareBoundaryPoints. That does not seem desirable. :-/
LGTM, see below. https://codereview.chromium.org/212873002/diff/100001/Source/core/page/DOMSel... File Source/core/page/DOMSelection.cpp (right): https://codereview.chromium.org/212873002/diff/100001/Source/core/page/DOMSel... Source/core/page/DOMSelection.cpp:484: if (exceptionState.hadException()) On 2014/03/27 13:52:44, Mads Ager (chromium) wrote: > On 2014/03/27 01:21:12, Yuta Kitamura wrote: > > In what situation does this become true? > > > > I think it's better to add an error check above (along > > with lines 471-479) to avoid the DOM exception, and keep > > the ASSERT as is. Or better yet we could use > > ASSERT_NO_EXCEPTION. > > This becomes true when the nodes that are compared are in separate document > fragments of the same document (which can happen when the document is cleared as > in the test). I don't know how to check for that in any other way that > duplicating the code from compareBoundaryPoints. That does not seem desirable. > :-/ Ah, I tried running a debugger and I think I've got the point here. exceptionState's message is "The two ranges are in separate documents" (note that this is not "separate document fragments"), and probably this can be easily avoided by writing something like: if (parentNode->document() != selectedRange->startContainer()->document()) return false; But yes, I agree with your argument about code duplication, so I'm OK with landing your change as-is. (The test case is, erm, so smartly written, that I realized that parentNode and document.open() is necessary only after debugging...)
The CQ bit was checked by ager@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ager@chromium.org/212873002/120001
Message was sent while issue was closed.
Change committed as 170291 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
