jfx icon indicating copy to clipboard operation
jfx copied to clipboard

Published 20 hours ago • verified publisher icon openjdk

8319844 : Text/TextFlow.hitTest() is incorrect in RTL orientation

Open karthikpandelu opened this issue 1 year ago • 43 comments
trafficstars

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.

Progress

  • [x] Change must not contain extraneous whitespace
  • [x] Commit message must refer to an issue
  • [ ] Change must be properly reviewed (2 reviews required, with at least 1 Reviewer, 1 Author)

Issue

  • JDK-8319844: Text/TextFlow.hitTest() is incorrect in RTL orientation (Bug - P3)

Reviewers

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jfx.git pull/1323/head:pull/1323
$ git checkout pull/1323

Update a local copy of the PR:
$ git checkout pull/1323
$ git pull https://git.openjdk.org/jfx.git pull/1323/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 1323

View PR using the GUI difftool:
$ git pr show -t 1323

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jfx/pull/1323.diff

Webrev

Link to Webrev Comment

karthikpandelu avatar Jan 09 '24 07:01 karthikpandelu

:wave: Welcome back kpk! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

bridgekeeper[bot] avatar Jan 09 '24 07:01 bridgekeeper[bot]

Webrevs

mlbridge[bot] avatar Jan 09 '24 07:01 mlbridge[bot]

/reviewers 2

kevinrushforth avatar Jan 09 '24 13:01 kevinrushforth

@kevinrushforth The total number of required reviews for this PR (including the jcheck configuration and the last /reviewers command) is now set to 2 (with at least 1 Reviewer, 1 Author).

openjdk[bot] avatar Jan 09 '24 13:01 openjdk[bot]

MonkeyTester encounters an NPE on launch:

Exception in thread "JavaFX Application Thread" java.lang.NullPointerException: Cannot invoke "javafx.scene.Scene.getNodeOrientation()" because the return value of "javafx.scene.text.Text.getScene()" is null
	at javafx.graphics/javafx.scene.text.Text.findRunIndex(Text.java:1042)
	at javafx.graphics/javafx.scene.text.Text.hitTest(Text.java:1027)
	at javafx.controls/com.sun.javafx.scene.control.skin.Utils.computeTruncationIndex(Utils.java:208)
	at javafx.controls/com.sun.javafx.scene.control.skin.Utils.computeClippedText(Utils.java:281)
	at javafx.controls/javafx.scene.control.skin.LabeledSkinBase.updateDisplayedText(LabeledSkinBase.java:1136)
	at javafx.controls/javafx.scene.control.skin.LabeledSkinBase.layoutLabelInArea(LabeledSkinBase.java:552)
	at javafx.controls/javafx.scene.control.skin.LabeledSkinBase.layoutLabelInArea(LabeledSkinBase.java:475)
	at javafx.controls/javafx.scene.control.skin.TableCellSkinBase.layoutChildren(TableCellSkinBase.java:147)
	at javafx.controls/javafx.scene.control.Control.layoutChildren(Control.java:612)
	at javafx.controls/javafx.scene.control.Cell.layoutChildren(Cell.java:635)
	at javafx.controls/javafx.scene.control.TableCell.layoutChildren(TableCell.java:711)
	at javafx.graphics/javafx.scene.Parent.layout(Parent.java:1208)
	at javafx.graphics/javafx.scene.Parent.layout(Parent.java:1215)
	at javafx.graphics/javafx.scene.Parent.layout(Parent.java:1215)
	at javafx.graphics/javafx.scene.Parent.layout(Parent.java:1215)
	at javafx.graphics/javafx.scene.Parent.layout(Parent.java:1215)
	at javafx.graphics/javafx.scene.Parent.layout(Parent.java:1215)
	at javafx.graphics/javafx.scene.Parent.layout(Parent.java:1215)
	at javafx.graphics/javafx.scene.Parent.layout(Parent.java:1215)
	at javafx.graphics/javafx.scene.Parent.layout(Parent.java:1215)
	at javafx.graphics/javafx.scene.Parent.layout(Parent.java:1215)
	at javafx.graphics/javafx.scene.Parent.layout(Parent.java:1215)
	at javafx.graphics/javafx.scene.Parent.layout(Parent.java:1215)
	at javafx.graphics/javafx.scene.Parent.layout(Parent.java:1215)
	at javafx.graphics/javafx.scene.Parent.layout(Parent.java:1215)
	at javafx.graphics/javafx.scene.Parent.layout(Parent.java:1215)
	at javafx.graphics/javafx.scene.Parent.layout(Parent.java:1215)
	at javafx.graphics/javafx.scene.Parent.layout(Parent.java:1215)
	at javafx.graphics/javafx.scene.Scene.doLayoutPass(Scene.java:595)
	at javafx.graphics/javafx.scene.Scene$ScenePulseListener.pulse(Scene.java:2601)
	at javafx.graphics/com.sun.javafx.tk.Toolkit.lambda$2(Toolkit.java:401)
	at java.base/java.security.AccessController.doPrivileged(AccessController.java:400)
	at javafx.graphics/com.sun.javafx.tk.Toolkit.runPulse(Toolkit.java:400)
	at javafx.graphics/com.sun.javafx.tk.Toolkit.firePulse(Toolkit.java:430)
	at javafx.graphics/com.sun.javafx.tk.quantum.QuantumToolkit.pulse(QuantumToolkit.java:598)
	at javafx.graphics/com.sun.javafx.tk.quantum.QuantumToolkit.pulse(QuantumToolkit.java:578)
	at javafx.graphics/com.sun.javafx.tk.quantum.QuantumToolkit.pulseFromQueue(QuantumToolkit.java:571)
	at javafx.graphics/com.sun.javafx.tk.quantum.QuantumToolkit.lambda$11(QuantumToolkit.java:352)
	at javafx.graphics/com.sun.glass.ui.InvokeLaterDispatcher$Future.run(InvokeLaterDispatcher.java:95)

andy-goryachev-oracle avatar Jan 10 '24 17:01 andy-goryachev-oracle

MonkeyTester encounters an NPE on launch:

@andy-goryachev-oracle did you find this NPE on any specific page? I couldn't see this issue

karthikpandelu avatar Jan 10 '24 17:01 karthikpandelu

Table View Page (TVP), I think. Select TVP, close the app, restart. Also seems to happen when navigating to the TVP.

In any case, we should be checking for null or maybe use the node's orientation rather than the scene's. What do you think?

andy-goryachev-oracle avatar Jan 10 '24 18:01 andy-goryachev-oracle

Preliminary testing looks ok, though it does suffer from effects of JDK-8318095 (and I did remove this.getScene(). in Text:1042)

I wonder if we should address JDK-8318095 first to be able to test this fix fully. Right now there are scenarios when things fail after resizing the split:

Screenshot 2024-01-10 at 11 48 39

andy-goryachev-oracle avatar Jan 10 '24 21:01 andy-goryachev-oracle

Preliminary testing looks ok, though it does suffer from effects of JDK-8318095 (and I did remove this.getScene(). in Text:1042)

Thanks @andy-goryachev-oracle for testing and reviewing the PR. Yes, to fully test this fix we need to address JDK-8318095. If you are planning to fix that issue, I'm ok to come back to this PR after the fix is done. However I do believe that we can review this in parallel as well.

karthikpandelu avatar Jan 11 '24 10:01 karthikpandelu

I wonder if we should address JDK-8318095 first to be able to test this fix fully.

We have another bug JDK-8319050 for the issue with careShape() and rangeShape(). Do you think it is better to address that issue after we complete JDK-8318095?

karthikpandelu avatar Jan 12 '24 06:01 karthikpandelu

Do you think it is better to address that issue after we complete JDK-8318095?

I think the solution for JDK-8318095 is just to trigger a layout, so we could do all these in parallel (as long as we don't resize the container). I was hoping to finish this review today.

andy-goryachev-oracle avatar Jan 12 '24 15:01 andy-goryachev-oracle

There seems to be a weird problem with Text (tested on macOS) in the Monkey Tester. 'Writing Systems' is a multi-line text with a tricky font (which does not get rendered correctly in LTR mode for some reason, but does in RTL). So if you try to hover over Aramaic line, the hit test info does not get updated:

Screenshot 2024-01-12 at 08 54 08

hit test is also not updated over some other areas, so you may want to research this.

andy-goryachev-oracle avatar Jan 12 '24 16:01 andy-goryachev-oracle

There seems to be a weird problem with Text (tested on macOS) in the Monkey Tester. 'Writing Systems' is a multi-line text with a tricky font (which does not get rendered correctly in LTR mode for some reason, but does in RTL). So if you try to hover over Aramaic line, the hit test info does not get updated:

I do not see this problem when I tested in a separate app. I will check more on this issue.

karthikpandelu avatar Jan 16 '24 12:01 karthikpandelu

There seems to be a weird problem with Text (tested on macOS) in the Monkey Tester.

Fixed this issue. Please check.

I observed one more issue. When I scroll the Text window around 70% or more with Writing Systems selected and then hover over the text, flickering is observed. Attaching the video for reference.

https://github.com/openjdk/jfx/assets/26969459/601aac43-c8d8-4402-9f15-bc23e63572f9

karthikpandelu avatar Jan 17 '24 10:01 karthikpandelu

flickering is observed

This is not an issue: the hit info label gets wider than available space, causing a layout (there is no scroll bar).

andy-goryachev-oracle avatar Jan 17 '24 16:01 andy-goryachev-oracle

Noticed a problem - on Windows 11 and on macOS 14.2.1 hit into shows different values for Text and TextFlow. This issue can be seen with pretty much every text, even "aaa\nbbb\ncccc". In this screenshot, the line corresponds to the mouse position:

Screenshot 2024-01-17 at 11 42 03

andy-goryachev-oracle avatar Jan 17 '24 19:01 andy-goryachev-oracle

Noticed a problem - on Windows 11 and on macOS 14.2.1 hit into shows different values for Text and TextFlow. This issue can be seen with pretty much every text, even "aaa\nbbb\ncccc". In this screenshot, the line corresponds to the mouse position:

I see this problem as well. I will check on this issue.

karthikpandelu avatar Jan 18 '24 07:01 karthikpandelu

Noticed a problem - on Windows 11 and on macOS 14.2.1 hit into shows different values for Text and TextFlow.

Fixed this issue. Please check.

karthikpandelu avatar Jan 29 '24 07:01 karthikpandelu

@karthikpandelu it looks worse than before: Screenshot 2024-01-29 at 09 11 06

could you please pull the latest MonkeyTester and re-test Text and TextFlow pages please? https://github.com/andy-goryachev-oracle/MonkeyTest

andy-goryachev-oracle avatar Jan 29 '24 17:01 andy-goryachev-oracle

The issue should be fixed now. I tested using monkey tester, my local standalone tests and system tests. Let me know if you find issue in any case.

karthikpandelu avatar Jan 31 '24 10:01 karthikpandelu

Inline Nodes mess up computation. In this example, the Text.hitInfo shows bogus values anywhere in the word "trailing":

Screenshot 2024-01-31 at 13 25 10

andy-goryachev-oracle avatar Jan 31 '24 21:01 andy-goryachev-oracle

... and got an exception hovering over "Rich Text" text on the TextFlow page:

Exception in thread "JavaFX Application Thread" java.lang.IllegalArgumentException: offset is out of bounds: 7
	at java.base/sun.util.locale.provider.BreakIteratorProviderImpl$GraphemeBreakIterator.following(BreakIteratorProviderImpl.java:255)
	at javafx.graphics/com.sun.javafx.text.PrismTextLayout.getHitInfo(PrismTextLayout.java:582)
	at javafx.graphics/javafx.scene.text.Text.hitTest(Text.java:1035)
	at monkey_tester/com.oracle.tools.fx.monkey.pages.TextFlowPage.handleMouseEvent(TextFlowPage.java:252)
	at javafx.base/com.sun.javafx.event.CompositeEventHandler$NormalEventHandlerRecord.handleBubblingEvent(CompositeEventHandler.java:247)
	at javafx.base/com.sun.javafx.event.CompositeEventHandler.dispatchBubblingEvent(CompositeEventHandler.java:80)
	at javafx.base/com.sun.javafx.event.EventHandlerManager.dispatchBubblingEvent(EventHandlerManager.java:232)
	at javafx.base/com.sun.javafx.event.EventHandlerManager.dispatchBubblingEvent(EventHandlerManager.java:189)
	at javafx.base/com.sun.javafx.event.CompositeEventDispatcher.dispatchBubblingEvent(CompositeEventDispatcher.java:59)
	at javafx.base/com.sun.javafx.event.BasicEventDispatcher.dispatchEvent(BasicEventDispatcher.java:58)
	at javafx.base/com.sun.javafx.event.EventDispatchChainImpl.dispatchEvent(EventDispatchChainImpl.java:114)
	at javafx.base/com.sun.javafx.event.BasicEventDispatcher.dispatchEvent(BasicEventDispatcher.java:56)
	at javafx.base/com.sun.javafx.event.EventDispatchChainImpl.dispatchEvent(EventDispatchChainImpl.java:114)
	at javafx.base/com.sun.javafx.event.BasicEventDispatcher.dispatchEvent(BasicEventDispatcher.java:56)
	at javafx.base/com.sun.javafx.event.EventDispatchChainImpl.dispatchEvent(EventDispatchChainImpl.java:114)
	at javafx.base/com.sun.javafx.event.BasicEventDispatcher.dispatchEvent(BasicEventDispatcher.java:56)
	at javafx.base/com.sun.javafx.event.EventDispatchChainImpl.dispatchEvent(EventDispatchChainImpl.java:114)
	at javafx.base/com.sun.javafx.event.BasicEventDispatcher.dispatchEvent(BasicEventDispatcher.java:56)
	at javafx.base/com.sun.javafx.event.EventDispatchChainImpl.dispatchEvent(EventDispatchChainImpl.java:114)
	at javafx.base/com.sun.javafx.event.BasicEventDispatcher.dispatchEvent(BasicEventDispatcher.java:56)
	at javafx.base/com.sun.javafx.event.EventDispatchChainImpl.dispatchEvent(EventDispatchChainImpl.java:114)
	at javafx.base/com.sun.javafx.event.EventUtil.fireEventImpl(EventUtil.java:74)
	at javafx.base/com.sun.javafx.event.EventUtil.fireEvent(EventUtil.java:54)
	at javafx.base/javafx.event.Event.fireEvent(Event.java:198)
	at javafx.base/com.sun.javafx.event.EventQueue.fire(EventQueue.java:48)
	at javafx.graphics/javafx.scene.Scene$MouseHandler.handleEnterExit(Scene.java:3901)
	at javafx.graphics/javafx.scene.Scene$MouseHandler.process(Scene.java:3972)
	at javafx.graphics/javafx.scene.Scene.processMouseEvent(Scene.java:1893)
	at javafx.graphics/javafx.scene.Scene$ScenePeerListener.mouseEvent(Scene.java:2711)
	at javafx.graphics/com.sun.javafx.tk.quantum.GlassViewEventHandler$MouseEventNotification.run(GlassViewEventHandler.java:411)
	at javafx.graphics/com.sun.javafx.tk.quantum.GlassViewEventHandler$MouseEventNotification.run(GlassViewEventHandler.java:1)
	at java.base/java.security.AccessController.doPrivileged(AccessController.java:400)
	at javafx.graphics/com.sun.javafx.tk.quantum.GlassViewEventHandler.lambda$2(GlassViewEventHandler.java:450)
	at javafx.graphics/com.sun.javafx.tk.quantum.QuantumToolkit.runWithoutRenderLock(QuantumToolkit.java:430)
	at javafx.graphics/com.sun.javafx.tk.quantum.GlassViewEventHandler.handleMouseEvent(GlassViewEventHandler.java:449)
	at javafx.graphics/com.sun.glass.ui.View.handleMouseEvent(View.java:551)
	at javafx.graphics/com.sun.glass.ui.View.notifyMouse(View.java:937)
	at javafx.graphics/com.sun.glass.ui.mac.MacView.notifyMouse(MacView.java:127)

Screenshot 2024-01-31 at 13 30 14

andy-goryachev-oracle avatar Jan 31 '24 21:01 andy-goryachev-oracle

I will work on the inline node issue and the issue in Rich text area. The rich text area issue is basically because of the repeated text node BOLD. Im not really sure if the increased scale is causing any issue, I will get into it.

karthikpandelu avatar Feb 01 '24 08:02 karthikpandelu

Fixed the above issues. I have simplified the logic little bit so that it doesn't get confusing as I add more conditions. I tested using the monkey tester and automated tests. Please let me know if you find issue in any scenario.

karthikpandelu avatar Feb 06 '24 10:02 karthikpandelu

Fails with "Emojis" data set (magenta line indicates where):

Screenshot 2024-02-06 at 10 07 45

andy-goryachev-oracle avatar Feb 06 '24 18:02 andy-goryachev-oracle

Also fails with "Tabs":

Screenshot 2024-02-06 at 10 14 34

andy-goryachev-oracle avatar Feb 06 '24 18:02 andy-goryachev-oracle

Fails with "Emojis" data set (magenta line indicates where):

I forgot to mention that I didn't test with emojis. I wanted to make it work with text with all scenario and then move to emojis. Now that we have text related issues fixed, I will check this. With tabs it works properly on the text and on the tab spaces it fails now. I will look into this as well.

karthikpandelu avatar Feb 07 '24 05:02 karthikpandelu

The issue with emojis and tabs are fixed. In Monkey tester with RichText option selected, I see issue in the last line shown in the screenshot below. Screenshot 2024-02-07 at 3 53 02 PM

Since the emoji itself is not rendered properly I don't think it is an issue relevant to this PR.

karthikpandelu avatar Feb 07 '24 10:02 karthikpandelu

Since the emoji itself is not rendered properly I don't think it is an issue relevant to this PR.

Quite possibly, these are grapheme clusters which are currently not supported: ☝🏿☝🏿☝🏿🤦🏼‍♂️

I see the emoji and tab text options work now, but inline nodes still fail.

andy-goryachev-oracle avatar Feb 07 '24 23:02 andy-goryachev-oracle

I see the emoji and tab text options work now, but inline nodes still fail.

I don't see issue with inline nodes. Please let me know in which case you are facing issue. Screenshot 2024-02-08 at 11 37 36 AM

karthikpandelu avatar Feb 08 '24 06:02 karthikpandelu