Bug 25444

Created attachment 29841 [details] source

I think that we already have a bug about this somewhere - the issue looks familiar.

*** Bug 27797 has been marked as a duplicate of this bug. ***

*** Bug 33199 has been marked as a duplicate of this bug. ***

Created attachment 72830 [details] Proposed Patch Please check the attached patch.

I didn't think about this patch in depth, just some quick comments. + if (!indexPosition.node() + || (indexPosition.node()->rootEditableElement() != m_innerText && !isSelectableElement(indexPosition.node()))) There is no need to wrap this line. +This script should show: 5, 7 This comment seems obsolete - nothing shows "5, 7" here. + - Firefox: OK<br/> + - Opera: OK<br/> + - Konqueror: OK<br/> + - IE: (not supported) A comment in HTMLInputElement.idl says that this is a WinIE & FireFox extension. Is that wrong? Please fix either the comment or the test. \ No newline at end of file Please add one.

Comment on attachment 72830 [details] Proposed Patch View in context: https://bugs.webkit.org/attachment.cgi?id=72830&action=review > WebCore/ChangeLog:8 > + Only editable elements are available at RenderTextControl::indexForVisiblePosition. I don't get this comment. Could you elaborate more? > WebCore/rendering/RenderTextControl.cpp:264 > + if (node && node->isShadowNode() && (node->shadowParentNode()->hasTagName(inputTag) || node->shadowParentNode()->hasTagName(textareaTag))) Why do we exclude SVGInputElement here? > WebCore/rendering/RenderTextControl.cpp:298 > + if (!indexPosition.node() > + || (indexPosition.node()->rootEditableElement() != m_innerText && !isSelectableElement(indexPosition.node()))) Mn... we still do need to make sure the position is inside the form control though. what we might want to do is to check node()->contains(pos.node()). > LayoutTests/fast/forms/selection-start-end-readonly.html:14 > + test.setSelectionRange(5,7); > + > + if (test.selectionStart == 5 && test.selectionEnd == 7) { > + con.innerText = "PASS"; > + } else { > + con.innerText = "FAIL"; We probably want more than one case. At least, selecting all, all but the left end, all but the right end, in the middle, and collapsed caret in the middle, at the left end, and at the right end.

Thank you for your comment. I have some questions. (In reply to comment #7) > > WebCore/ChangeLog:8 > > + Only editable elements are available at RenderTextControl::indexForVisiblePosition. > > I don't get this comment. Could you elaborate more? Because rootEditableElement() is checked on RenderTextControl::indexForVisiblePosition, ReadOnly element is not editable element. So, it can't get correct selection information. The INPUT element on this test case has read-only property. I'll update ChangeLog as well. > > WebCore/rendering/RenderTextControl.cpp:264 > > + if (node && node->isShadowNode() && (node->shadowParentNode()->hasTagName(inputTag) || node->shadowParentNode()->hasTagName(textareaTag))) > > Why do we exclude SVGInputElement here? Please check 'http://www.w3.org/TR/html5/association-of-controls-and-forms.html#textFieldSelection' "4.11 APIs for the text field selections The input and textarea elements define the following members in their DOM interfaces for handling their selection:" Do I need to add SVGInputElement? > > WebCore/rendering/RenderTextControl.cpp:298 > > + if (!indexPosition.node() > > + || (indexPosition.node()->rootEditableElement() != m_innerText && !isSelectableElement(indexPosition.node()))) > > Mn... we still do need to make sure the position is inside the form control though. what we might want to do is to check node()->contains(pos.node()). In order to check element type, it should check shadow node. isSelectableElement() checks shadow node. If you look into 'DeleteSelectionCommand::doApply', It checks the type like Node* ancestorNode = startNode ? startNode->shadowAncestorNode() : 0; if (ancestorNode && ancestorNode->hasTagName(inputTag) I think I need to change the way to search shadow node on isSelectableElement() like the above. But, do I need 'node()->contains(pos.node())'? If I don't understand it well, please let me know. > > LayoutTests/fast/forms/selection-start-end-readonly.html:14 > > + test.setSelectionRange(5,7); > > + > > + if (test.selectionStart == 5 && test.selectionEnd == 7) { > > + con.innerText = "PASS"; > > + } else { > > + con.innerText = "FAIL"; > > We probably want more than one case. At least, selecting all, all but the left end, all but the right end, in the middle, and collapsed caret in the middle, at the left end, and at the right end. There is a test case for them already. Refer to 'selection-functions.html'. I think this issue is just related to read-only case. In spite of it, do I need to add those test cases? I'll upload patch again, after getting answer about the above items. Thanks.

(In reply to comment #8) > Because rootEditableElement() is checked on RenderTextControl::indexForVisiblePosition, > ReadOnly element is not editable element. So, it can't get correct selection information. > The INPUT element on this test case has read-only property. Yes, I understand that. But I couldn't infer any of this stuff from the original changelog comment. > Do I need to add SVGInputElement? Sorry. I misspoke. I meant WMLInputElement. I guess it's okay because WMLInputElement is also an input element. > In order to check element type, it should check shadow node. > isSelectableElement() checks shadow node. > If you look into 'DeleteSelectionCommand::doApply', > It checks the type like > Node* ancestorNode = startNode ? startNode->shadowAncestorNode() : 0; > if (ancestorNode && ancestorNode->hasTagName(inputTag) Yeah checking the shadow node is okay. But what I'm saying is that your check won't consider the case where the position is inside another input / textarea element since you just walk the DOM and see if the parent has a shadow node (no equality!). The original code checks equality between rootEditableElement and m_innerText and this prevents us from returning value when the root editable element isn't a descendent of "this" input / textarea element. > > We probably want more than one case. At least, selecting all, all but the left end, all but the right end, in the middle, and collapsed caret in the middle, at the left end, and at the right end. > > There is a test case for them already. Refer to 'selection-functions.html'. > I think this issue is just related to read-only case. > In spite of it, do I need to add those test cases? Please do. Those are very simple cases. I'm sure there are a lot more edge cases out there.

Created attachment 73054 [details] Propsed Patch updated. The patch was changed a little from the first patch according to reviewer's comment. Please look into it. Thanks.

I'm deferring to others for in-depth review, but have a few comments in passing: + if (Node* sNode = n->shadowAncestorNode()) { "sNode" is not a good name - if "s" prefix ever means anything, that's "static". > A comment in HTMLInputElement.idl says that this is a WinIE & FireFox extension. Is that wrong? > Please fix either the comment or the test. You addressed this comment, but without answering the question directly. Did you find out that selectionStart/selectionEnd work in IE, after all? Does the final test pass in both Firefox and IE? + if (elt.selectionStart == s1 && elt.selectionEnd == e1) + endResult = 1; + else + endResult = 0; Doesn't this mean that the overall PASS/FAIL result only depends on the last subtest? + if (window.layoutTestController) { + layoutTestController.waitUntilDone(); + } No need for waitUntilDone/notifyDone - the test runs from onload, and doesn't set any timeouts. I'd also suggest putting dumpAsText at the very beginning of the script, so that it's executed during parsing.

Sorry that I missed your comment. Please check my inline comment. If you have no concern about my comment, I'll update the patch according to my answer. Thanks. (In reply to comment #11) > "sNode" is not a good name - if "s" prefix ever means anything, that's "static". I'll change it to "shadowAncestor". > > A comment in HTMLInputElement.idl says that this is a WinIE & FireFox extension. Is that wrong? > > Please fix either the comment or the test. > > You addressed this comment, but without answering the question directly. Did you find out that selectionStart/selectionEnd work in IE, after all? Does the final test pass in both Firefox and IE? FF(I used FF3.6.11) works fine but IE(I used IE7) doesn't work. It's not only for IE and FF. 'http://www.w3.org/TR/html5/association-of-controls-and-forms.html#textFieldSelection' So, I think the comment should be removed. It you have no concern about that, I'll remove the comment in 'HTMLInputElement.idl'. > + if (elt.selectionStart == s1 && elt.selectionEnd == e1) > + endResult = 1; > + else > + endResult = 0; > > Doesn't this mean that the overall PASS/FAIL result only depends on the last subtest? I'll update it to check the previous test. So, if the previous test fails, the test will be stopped. > + if (window.layoutTestController) { > + layoutTestController.waitUntilDone(); > + } > > No need for waitUntilDone/notifyDone - the test runs from onload, and doesn't set any timeouts. I'd also suggest putting dumpAsText at the very beginning of the script, so that it's executed during parsing. Yes. I'll remove it.

Comment on attachment 73054 [details] Propsed Patch updated. View in context: https://bugs.webkit.org/attachment.cgi?id=73054&action=review > WebCore/rendering/RenderTextControl.cpp:262 > + if (!m_innerText || !m_innerText.get()->contains(n)) Not need to call get(). You can do m_innerText->contains(n) > WebCore/rendering/RenderTextControl.cpp:300 > + if (!indexPosition.node() || (indexPosition.node()->rootEditableElement() != m_innerText && !isSelectableElement(indexPosition.node()))) Can we also incorporate the condition indexPosition.node()->rootEditableElement() != m_innerText into isSelectableElement? I think that'll improve the clarity here. > LayoutTests/fast/forms/selection-start-end-readonly-expected.txt:1 > +https://bugs.webkit.org/show_bug.cgi?id=25444 I don't think we typically add a URL to bug because it's pretty clear which bug added this test if you see the svn log / trac. But I'd like to see some descriptions on what this test is testing. It's not clear at all from this output what output is expected. If this test fails in the future, how do we know what is the correct behavior? > LayoutTests/fast/forms/selection-start-end-readonly-expected.txt:9 > +Set Range: 0, 10 > +[Result] Start: 0, End: 10 > + > +Set Range: 0, 9 > +[Result] Start: 0, End: 9 Could you make this a script test and output PASS/FAIL?

Please check my inline comment. (In reply to comment #13) > (From update of attachment 73054 [details]) > Not need to call get(). You can do m_innerText->contains(n) Yes. I'll update it. > > WebCore/rendering/RenderTextControl.cpp:300 > > + if (!indexPosition.node() || (indexPosition.node()->rootEditableElement() != m_innerText && !isSelectableElement(indexPosition.node()))) > > Can we also incorporate the condition indexPosition.node()->rootEditableElement() != m_innerText into isSelectableElement? I think that'll improve the clarity here. Sure, I'll do. > > LayoutTests/fast/forms/selection-start-end-readonly-expected.txt:1 > > +https://bugs.webkit.org/show_bug.cgi?id=25444 > > I don't think we typically add a URL to bug because it's pretty clear which bug added this test if you see the svn log / trac. But I'd like to see some descriptions on what this test is testing. It's not clear at all from this output what output is expected. If this test fails in the future, how do we know what is the correct behavior? OK. I'll remove the link and add more detail description for test. > > LayoutTests/fast/forms/selection-start-end-readonly-expected.txt:9 > > +Set Range: 0, 10 > > +[Result] Start: 0, End: 10 > > + > > +Set Range: 0, 9 > > +[Result] Start: 0, End: 9 > > Could you make this a script test and output PASS/FAIL? You mean the result from each test part? For instance, +Set Range: 0, 10 +[Result] Start: 0, End: 10 +PASS +Set Range: 0, 9 +[Result] Start: 0, End: 9 +FAIL Could you explain what you expect? Thanks.

(In reply to comment #14) > > Could you make this a script test and output PASS/FAIL? > You mean the result from each test part? > For instance, > +Set Range: 0, 10 > +[Result] Start: 0, End: 10 > +PASS > +Set Range: 0, 9 > +[Result] Start: 0, End: 9 > +FAIL > Could you explain what you expect? Take a look at this one for example: http://trac.webkit.org/browser/trunk/LayoutTests/fast/forms/textarea-maxlength-expected.txt?rev=48698 http://trac.webkit.org/browser/trunk/LayoutTests/fast/forms/script-tests/textarea-maxlength.js?rev=48698

Comment on attachment 73054 [details] Propsed Patch updated. <http://trac.webkit.org/browser/trunk/LayoutTests/fast/speech/input-text-language-tag.html> is a better example - splitting tests into .html and .js only adds indirection, and wastes time of the person investigating future failures. Marking r-, as you intend to update the patch.

> I don't think we typically add a URL to bug because it's pretty clear which bug added this test if you see the svn log / trac. I usually add a line as below, but don't feel strongly about that: <p>Test for <a href="https://bugs.webkit.org/show_bug.cgi?id=25444">bug 25444</a>: SelectionStart, selectionEnd properties return wrong values when the selection is in a form input.</p>

Created attachment 73179 [details] Proposed Patch (3rd) I updated the proposed patch again. Please check. Thanks.

Comment on attachment 73179 [details] Proposed Patch (3rd) View in context: https://bugs.webkit.org/attachment.cgi?id=73179&action=review This patch looks much better but needs a little more polishing. > WebCore/rendering/RenderTextControl.cpp:260 > +bool RenderTextControl::isSelectableElement(Node* n) const Please do not abbreviate node as n. See Names section in http://webkit.org/coding/coding-style.html. > WebCore/rendering/RenderTextControl.cpp:275 > + if (Node* shadowAncestor = n->shadowAncestorNode()) { > + if ((shadowAncestor->hasTagName(inputTag) && static_cast<HTMLInputElement*>(shadowAncestor)->isTextField()) || shadowAncestor->hasTagName(textareaTag)) > + return true; > + } > + return false; I would write this as: Node* shadowAncestor = n->shadowAncestorNode(); return shadowAncestor && (shadowAncestor->hasTagName(textareaTag) || (shadowAncestor->hasTagName(inputTag) && static_cast<HTMLInputElement*>(shadowAncestor)->isTextField())); > LayoutTests/fast/forms/selection-start-end-readonly-expected.txt:10 > +PASS element.selectionStart is start > +PASS element.selectionEnd is end These outputs aren't as useful as I'd like to be. You might want to consider manually calling Passed / Failed to print out more useful information such as what offset was set and which offset was returned. But I'm fine with this output if you're reluctant to change it. > LayoutTests/fast/forms/selection-start-end-readonly.html:24 > + if (type == 1) > + element= document.getElementById('text'); > + else if (type ==2) > + element= document.getElementById('area'); Why don't you just pass element to startTest & testHandler instead of making if statement like this. i.e. testHandler(document.getElementById('text')); testHandler(document.getElementById('area')); And please put a space before and after == and =. Although this is JavaScript code, we'd like to keep it as clean as possible. > LayoutTests/fast/forms/selection-start-end-readonly.html:35 > + start = s; end = e; Why don't you name "s" and "e" "start" and "end" respectively in the first place? > LayoutTests/fast/forms/selection-start-end-readonly.html:44 > + var s = 0; > + var e = 10; > + startTest(type,s,e); There's no need to declare s and e. You can just do: startTest(type, 0, 10); If you follow my earlier comment, this should be: startTest(element, 0, 10); You can also make an array of offset pairs and use a for-loop to go through all of them as in: var offsets = [[0, 10], [0, 9], ...]; for (var i = 0; i < offsets.length; i++) startTest(element, offsets[i][0], offsets[i][1]);

Please check inline comment. (In reply to comment #19) > (From update of attachment 73179 [details]) > > WebCore/rendering/RenderTextControl.cpp:260 > > +bool RenderTextControl::isSelectableElement(Node* n) const > Please do not abbreviate node as n. See Names section in http://webkit.org/coding/coding-style.html. OK. I'll change it to 'node'. > > WebCore/rendering/RenderTextControl.cpp:275 > I would write this as: > Node* shadowAncestor = n->shadowAncestorNode(); > return shadowAncestor && (shadowAncestor->hasTagName(textareaTag) > || (shadowAncestor->hasTagName(inputTag) && static_cast<HTMLInputElement*>(shadowAncestor)->isTextField())); OK. I'll change it as you recommended. > > LayoutTests/fast/forms/selection-start-end-readonly-expected.txt:10 > > +PASS element.selectionStart is start > > +PASS element.selectionEnd is end > > These outputs aren't as useful as I'd like to be. You might want to consider manually calling Passed / Failed to print out more useful information such as what offset was set and which offset was returned. But I'm fine with this output if you're reluctant to change it. In order to display useful information, I have to send the second parameter of 'shouldbe' as a constant, not a variable. It means I can't use function. I have to just spread out them like 'textarea-maxlength.js'. If I have to do like that, I'll do it. > Why don't you just pass element to startTest & testHandler instead of making if statement like this. > i.e. > testHandler(document.getElementById('text')); > testHandler(document.getElementById('area')); Because 'eval' in 'shouldBe' throws exception like 'ReferenceError: element is not defined.' if I use local variable or just argument, I put global variable and assign node to it. As I told above, If I change it to spread out, not using function, This code will be removed. > And please put a space before and after == and =. Although this is JavaScript code, we'd like to keep it as clean as possible. > > > LayoutTests/fast/forms/selection-start-end-readonly.html:35 > > + start = s; end = e; > > Why don't you name "s" and "e" "start" and "end" respectively in the first place? Yes, I'll check it in test case as well. > > LayoutTests/fast/forms/selection-start-end-readonly.html:44 > > + var s = 0; > > + var e = 10; > > + startTest(type,s,e); > > There's no need to declare s and e. You can just do: > startTest(type, 0, 10); > If you follow my earlier comment, this should be: > startTest(element, 0, 10); > > You can also make an array of offset pairs and use a for-loop to go through all of them as in: > > var offsets = [[0, 10], [0, 9], ...]; > for (var i = 0; i < offsets.length; i++) > startTest(element, offsets[i][0], offsets[i][1]); It's same as the above. In conclusion, The 'eval' in 'shouldBe' throws exception if I use local variable or just argument. I have to use constant at 'shouldBe' in order to show more useful information. So, I'll change this test to spread out sentences like 'textarea-maxlength.js'. Do you approve of my idea?

(In reply to comment #20) > > Why don't you just pass element to startTest & testHandler instead of making if statement like this. > > i.e. > > testHandler(document.getElementById('text')); > > testHandler(document.getElementById('area')); > Because 'eval' in 'shouldBe' throws exception like 'ReferenceError: element is not defined.' if I use local variable or just argument, I put global variable and assign node to it. Just call testPassed / testFailed directly as in: http://trac.webkit.org/browser/trunk/LayoutTests/editing/execCommand/script-tests/query-command-state.js?rev=70810

Created attachment 73369 [details] Proposed patch(4th) I've attached proposed patch again. Please check. Thanks.

Comment on attachment 73369 [details] Proposed patch(4th) Great! LGTM. Wait for the formal review.

Created attachment 73513 [details] new testcase

Comment on attachment 73369 [details] Proposed patch(4th) View in context: https://bugs.webkit.org/attachment.cgi?id=73369&action=review > WebCore/ChangeLog:5 > + SelectionStart, selectionEnd properties return wrong values when the selection is in a form input This summary is not good. "in a form input" -> "in a read-only input or textarea element"?

Thank you for your review. I'll change and update it by tomorrow. If there is any other thing which I have to change, please comment. (In reply to comment #25) > (From update of attachment 73369 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=73369&action=review > > > WebCore/ChangeLog:5 > > + SelectionStart, selectionEnd properties return wrong values when the selection is in a form input > > This summary is not good. "in a form input" -> "in a read-only input or textarea element"?

(In reply to comment #26) > Thank you for your review. > I'll change and update it by tomorrow. > If there is any other thing which I have to change, please comment. Great. I have no other comments.

Created attachment 73602 [details] Proposed patch(5th) I changed summery as reviewer recommended. Please check. Thanks.

Comment on attachment 73369 [details] Proposed patch(4th) Cleared Kent Tamura's review+ from obsolete attachment 73369 [details] so that this bug does not appear in http://webkit.org/pending-commit.

Comment on attachment 73602 [details] Proposed patch(5th) Clearing flags on attachment: 73602 Committed r71880: <http://trac.webkit.org/changeset/71880>

All reviewed patches have been landed. Closing bug.