On Wed, 21 Feb 2024 10:01:37 GMT, Karthik P K <k...@openjdk.org> wrote:
>> In the `getHitInfo()` method of PrismTextLayout, RTL node orientation >> conditions were not considered, hence hit test values such as character >> index and insertion index values were incorrect. >> >> Added checks for RTL orientation of nodes and fixed the issue in >> `getHitInfo()` to calculate correct hit test values. >> >> Added system tests to validate the changes. > > Karthik P K has updated the pull request with a new target base due to a > merge or a rebase. The incremental webrev excludes the unrelated changes > brought in by the merge/rebase. The pull request contains 13 additional > commits since the last revision: > > - Merge branch 'master' into rtl_hittest_issue > - Code refactoring > - Review comments > - Fix emoji issue > - Inline node issue fix > - Code review changes > - Fix issue with multiline text > - Fix issue with RTL text within LTR text > - Review changes > - Fix multiline text insertion index calculation issue > - ... and 3 more: https://git.openjdk.org/jfx/compare/02e8b6ba...e3812732 I'm confused by this part: > we will not have information to decide if the character index should be > calculated relative to Text node or TextFlow The character index, even in your example image above, seems to be relative to the `Text`, not to the `TextFlow` (it is 0, even though there are clearly 10 characters before it belonging to another `Text`), so what exactly do you mean with this? What I find it confusing further more is that if I request the `HitInfo` of the `TextFlow` container (which I think would make more sense to do) that these values are subtly different. Take this modification to your test program: if (n instanceof Text t) { Point3D p3 = pick.getIntersectedPoint(); Point2D p = new Point2D(p3.getX(), p3.getY()); HitInfo h = t.getParent() instanceof TextFlow tf ? tf.hitTest(p) : t.hitTest(p); hitInfo2.setText("TextFlow: " + h + "\nText: " + t.hitTest(p)); } When used with these two texts: t("Arabic**"), t("ABCD") You'll find that the values perfectly match for the first `Text` in the flow, but for the 2nd `Text` in the flow they're subtly different. Somehow I think that's already very strange. The reported character indices are always relative to the `Text`s though, which is why I'm confused about your earlier statement. Your fix however does mean that the `HitInfo` for `Text` seems to be more accurate, but it doesn't match with what `TextFlow` reports (in other words, the `hitTest` of `TextFlow` was and is broken still, your fix didn't change that). I'm still looking into why `PrismTextLayout` needs to be "aware" of texts being nested into `TextFlow` -- this seems like such an odd thing to me that I find it tough to let it go. I think that if we know why `hitTest` of `TextFlow` is broken, then that it will also make clear why it currently must be aware of the nesting; and I think once fixed, it won't need to know this anymore. In its current form, the `hitTest` method on `TextFlow` seems completely useless. The reported `HitInfo` can't be made sense of without knowing which child we're talking about. I think it may make sense to extend `HitInfo` with the `Node` involved. ------------- PR Comment: https://git.openjdk.org/jfx/pull/1323#issuecomment-1962749265
