RFR: 8319844 : Text/TextFlow.hitTest() is incorrect in RTL orientation [v13]

[John Hendrikx]

On Wed, 21 Feb 2024 10:01:37 GMT, Karthik P K <kpk at 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