On Fri, 7 Feb 2020 18:04:35 GMT, Kevin Rushforth <k...@openjdk.org> wrote:
>> This is a WIP PR. Requesting for comments. >> >> I could not replicate the test given at >> https://bugs.openjdk.java.net/browse/JDK-8176270 without TestFX. I >> nevertheless let my try to replicate the @Test things in here. >> >> The fix is just a wild guess without really understanding the side effects >> of `.addListener`. > > modules/javafx.controls/src/main/java/javafx/scene/control/TextInputControl.java > line 168: > >> 167: // Ensure that the last character to get is within the >> bounds of the txt string >> 168: if (end >= start + length) end = length-1; >> 169: // In case the start is after the whole txt, nothing >> valid is selected. Thus, return the default. > > First, I think the existing test is simply wrong: Why should the `start` > value matter when checking whether the `end` value needs to be clamped -- > it's an `end` point not a number of characters. I think the entire problem > might be the fact that it is comparing against `start + length`. Second, the > value to be clamped to was correct before your change. The `substring` method > to which `end` is passed is documented as subtracting 1. > > I think the test should be something like: > > if (end > length) end = length; on second thought: correct clamping or not, the start/end indices of selection are invalid whenever text was removed/added at an index before the selection - they are in coordinates of the _old_ text, not the new. If they point to an index > new textLength, incorrect clamping will throw. If they are both < new textLength, it'll fire an intermediate changeEvent with incorrect selection, the total sequence beining oldSelectedText -> incorrectly-shifted-selectedText incorrectly-shifted-selectedText -> empty The correct change notification would be, because at the end of the text change, the selection is cleared, oldSelectedText -> empty To me, it looks like using a binding here is not the best of options. ------------- PR: https://git.openjdk.java.net/jfx/pull/73
