RFR: 8309374: Accessibility Focus Rectangle on ListItem is not drawn when ListView is shown for first time
This is accessibility specific fix. **Issue**: When a ListView is shown for first time then accessibility focus rectangle is not drawn around the focused ListIem **Cause:** The ListView takes a little time to create it's skin(ListViewSkin) and the skins for ListItems(ListCellSkin) If the Accessibility client application requests focused item before ListView is completely ready then JavaFX return null. **Fix**: Send a focus item change notification to accessibility client application after the ListIteam is ready **Verification:** - On Windows machine, launch Narrator. Issue is observed only with Narrator - Execute the [program](https://bugs.openjdk.org/secure/attachment/104180/Main.java) attached to the JBS issue [JDK-8309374](https://bugs.openjdk.org/browse/JDK-8309374) Move through the TextFields and press `Alt+down`, observe that focus rectangle is drawn correctly. Once the ListView is showing Press and hold `Up/Down` or `Ctrl + Up/Down` keys, observe that focus rectangle is always drawn. - Commit messages: - a11y ListView focused item fix Changes: https://git.openjdk.org/jfx/pull/1363/files Webrev: https://webrevs.openjdk.org/?repo=jfx&pr=1363&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8309374 Stats: 31 lines in 1 file changed: 31 ins; 0 del; 0 mod Patch: https://git.openjdk.org/jfx/pull/1363.diff Fetch: git fetch https://git.openjdk.org/jfx.git pull/1363/head:pull/1363 PR: https://git.openjdk.org/jfx/pull/1363
Re: RFR: JDK-8324182 Deprecate for removal SimpleSelector and CompoundSelector classes [v2]
On Sat, 10 Feb 2024 15:13:44 GMT, Kevin Rushforth wrote: >>> > need 2 passes, because there already is a method named like that: >>> >>> oh yeah, sorry. `getStyleClassesSet()` ? >> >> That one is still available yes :) > > @hjohn I'm now leaning towards either `getStyleClassNames` or > `getClassNames`, but it isn't a strong preference. Why don't you pick one, > update the PR, and create a Draft CSR. @kevinrushforth CSR was created - PR Comment: https://git.openjdk.org/jfx/pull/1340#issuecomment-1941529341
Re: RFR: 8278021: Fix warnings in macOS glass native code and treat warnings as errors [v5]
On Mon, 12 Feb 2024 20:48:22 GMT, Kevin Rushforth wrote: >> Martin Fox has updated the pull request incrementally with two additional >> commits since the last revision: >> >> - Work-around for using a call not present in the Xcode 13.3 SDK. >> - Need correctly formed NSNotification when running tests > > modules/javafx.graphics/src/main/native-glass/mac/GlassApplication.m line 655: > >> 653: // try again using Java generic icon (this icon >> might go away eventually ?) >> 654: iconPath = [NSString stringWithFormat:@"%s", >> "/System/Library/Frameworks/JavaVM.framework/Resources/GenericApp.icns"]; >> 655: } > > I presume you removed this because it is obsolete code? I do note that the > file in question is no longer there, so this seems fine. Yes, I think it's obsolete. I haven't seen an icon file in the JavaVM framework since I started working on JavaFX years ago. - PR Review Comment: https://git.openjdk.org/jfx/pull/687#discussion_r1488159498
RFR: 8323880: Caret rendered at wrong position in case of a click event on RTL text
Issue: The current implementation of complex text rendering paths on the Java platform is experiencing side effects. Solution: We need to align with WebKit 616.1 standards. The patch supports two paths simple rendering path and complex rendering path based on text rendering mode. - Commit messages: - 8323880: Caret rendered at wrong position in case of a click event on RTL text Changes: https://git.openjdk.org/jfx/pull/1364/files Webrev: https://webrevs.openjdk.org/?repo=jfx&pr=1364&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8323880 Stats: 12 lines in 1 file changed: 12 ins; 0 del; 0 mod Patch: https://git.openjdk.org/jfx/pull/1364.diff Fetch: git fetch https://git.openjdk.org/jfx.git pull/1364/head:pull/1364 PR: https://git.openjdk.org/jfx/pull/1364
Re: RFR: 8323880: Caret rendered at wrong position in case of a click event on RTL text
On Tue, 13 Feb 2024 16:57:37 GMT, Jay Bhaskar wrote: > Issue: The current implementation of complex text rendering paths on the Java > platform is experiencing side effects. > Solution: We need to align with WebKit 616.1 standards. The patch supports > two paths simple rendering path and complex rendering path based on text > rendering mode. Reviewers: @kevinrushforth @HimaBinduMeda - PR Comment: https://git.openjdk.org/jfx/pull/1364#issuecomment-1942127715
Re: RFR: 8309374: Accessibility Focus Rectangle on ListItem is not drawn when ListView is shown for first time
On Tue, 13 Feb 2024 12:29:49 GMT, Ambarish Rapte wrote: > This is accessibility specific fix. > > **Issue**: When a ListView is shown for first time then accessibility focus > rectangle is not drawn around the focused ListIem > > **Cause:** >The ListView takes a little time to create it's skin(ListViewSkin) and the > skins for ListItems(ListCellSkin) >If the Accessibility client application requests focused item before > ListView is completely ready then JavaFX return null. > > **Fix**: Send a focus item change notification to accessibility client > application after the ListIteam is ready > > **Verification:** > - On Windows machine, launch Narrator. Issue is observed only with Narrator > - Execute the > [program](https://bugs.openjdk.org/secure/attachment/104180/Main.java) > attached to the JBS issue > [JDK-8309374](https://bugs.openjdk.org/browse/JDK-8309374) > Move through the TextFields and press `Alt+down`, observe that focus > rectangle is drawn correctly. > Once the ListView is showing Press and hold `Up/Down` or `Ctrl + > Up/Down` keys, observe that focus rectangle is always drawn. Reviewers: @kevinrushforth @azuev-java - PR Comment: https://git.openjdk.org/jfx/pull/1363#issuecomment-1942130364
Re: [External] : Re: Platform preferences theme detection
The reason why the values are wrong might be that in PlatformSupport.m L105, we query [NSApp effectiveApperance], but this always seems to return the light appearance if JavaFX doesn't own the NSApplication. On Mon, Feb 12, 2024 at 6:26 PM Kevin Rushforth wrote: > > Actually, it's worse than not detecting changes, it's simply not getting the > right values at all. If I run the program when the system appearance is > already in Dark mode, it doesn't get the correct values at start up. > > -- Kevin
RFR: JDK-8325798: Spinner throws uncatchable exception on tab out from garbled text
When a `Spinner` is configured with e.g. Integers (`IntegerSpinnerValueFactory`) and the user types in a `String`, e.g. 'abc' and focuses another `Node`, an exception is thrown (`NumberFormatException`). This exception is literally uncatchable, as it happens on focus lost. The issue is the same as for the `DatePicker` component described in [JDK-8303478](https://bugs.openjdk.org/browse/JDK-8303478), which was fixed in PR: https://github.com/openjdk/jfx/pull/1274. I did the exact same fix in this PR. Also added the same test. - Commit messages: - JDK-8325798: Spinner throws uncatchable exception on tab out from garbled text Changes: https://git.openjdk.org/jfx/pull/1365/files Webrev: https://webrevs.openjdk.org/?repo=jfx&pr=1365&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8325798 Stats: 42 lines in 2 files changed: 39 ins; 0 del; 3 mod Patch: https://git.openjdk.org/jfx/pull/1365.diff Fetch: git fetch https://git.openjdk.org/jfx.git pull/1365/head:pull/1365 PR: https://git.openjdk.org/jfx/pull/1365
Re: RFR: JDK-8324182 Deprecate for removal SimpleSelector and CompoundSelector classes [v5]
On Sat, 10 Feb 2024 17:53:27 GMT, John Hendrikx wrote: >> The SimpleSelector and CompoundSelector classes are public classes in an >> exported package, javafx.css, but they are not intended to be used by >> applications. They are implementation details. They cannot be constructed >> directly and no other JavaFX API accepts or returns a SimpleSelector or >> CompoundSelector. >> >> We should deprecate them for removal so we can move them to a non-exported >> package, removing them from the public API. > > John Hendrikx has updated the pull request incrementally with one additional > commit since the last revision: > > Use getStyleClassNames Looks good and reasonable to me as well. I can also confirm that I have never used both `Selector` classes or seen any use for them. Also +1 for `getStyleClassNames`, feels the most natural to me. - Marked as reviewed by mhanl (Committer). PR Review: https://git.openjdk.org/jfx/pull/1340#pullrequestreview-1878881707
Re: RFR: 8323880: Caret rendered at wrong position in case of a click event on RTL text
On Tue, 13 Feb 2024 16:57:37 GMT, Jay Bhaskar wrote: > Issue: The current implementation of complex text rendering paths on the Java > platform is experiencing side effects. > Solution: We need to align with WebKit 616.1 standards. The patch supports > two paths simple rendering path and complex rendering path based on text > rendering mode. Looks good. Tested on macOS. - Marked as reviewed by kcr (Lead). PR Review: https://git.openjdk.org/jfx/pull/1364#pullrequestreview-1878933440
Re: RFR: 8309374: Accessibility Focus Rectangle on ListItem is not drawn when ListView is shown for first time
On Tue, 13 Feb 2024 12:29:49 GMT, Ambarish Rapte wrote:
> This is accessibility specific fix.
>
> **Issue**: When a ListView is shown for first time then accessibility focus
> rectangle is not drawn around the focused ListIem
>
> **Cause:**
>The ListView takes a little time to create it's skin(ListViewSkin) and the
> skins for ListItems(ListCellSkin)
>If the Accessibility client application requests focused item before
> ListView is completely ready then JavaFX return null.
>
> **Fix**: Send a focus item change notification to accessibility client
> application after the ListIteam is ready
>
> **Verification:**
> - On Windows machine, launch Narrator. Issue is observed only with Narrator
> - Execute the
> [program](https://bugs.openjdk.org/secure/attachment/104180/Main.java)
> attached to the JBS issue
> [JDK-8309374](https://bugs.openjdk.org/browse/JDK-8309374)
> Move through the TextFields and press `Alt+down`, observe that focus
> rectangle is drawn correctly.
> Once the ListView is showing Press and hold `Up/Down` or `Ctrl +
> Up/Down` keys, observe that focus rectangle is always drawn.
The fix looks good to me, and my initial testing shows that it works as
expected. I left a few suggestions on minor wording changes inline. I'll
reapprove if you make the changes.
modules/javafx.controls/src/main/java/javafx/scene/control/ListCell.java line
351:
> 349:
> 350: /*
> 351: * The layoutChildren() method is overloaded to address a specific
> accessibility issue: JBS-8309374
Minor typos: "overloaded" --> "overridden"; "JBS" --> "JDK"
modules/javafx.controls/src/main/java/javafx/scene/control/ListCell.java line
366:
> 364: if (listView != null) {
> 365: /*
> 366: * The notifyAccessibleAttributeChanged() call is
> submitted on the Application thread, because:
This method is already being run on the application thread.
Suggestion: "is submitted via runLater to defer it until after the layout
completes, because:"
modules/javafx.controls/src/main/java/javafx/scene/control/ListCell.java line
371:
> 369: * notification from here.
> 370: * We observed that this scenario occurs when client
> application is trying to get the
> 371: * UIA_HasKeyboardFocusPropertyId property of a focused
> ListItem's parent(ListView).
I might replace `UIA_HasKeyboardFocusPropertyId`, which is Windows-specific,
with something more generic like "the focus item".
modules/javafx.controls/src/main/java/javafx/scene/control/ListCell.java line
372:
> 370: * We observed that this scenario occurs when client
> application is trying to get the
> 371: * UIA_HasKeyboardFocusPropertyId property of a focused
> ListItem's parent(ListView).
> 372: * This scenario is avoided by submitting the call
> notifyAccessibleAttributeChanged() on Application thread,
Similar comment to the earlier one about the thread. I suggest something like:
"This scenario is avoided by submitting the call via runLater,"
-
Marked as reviewed by kcr (Lead).
PR Review: https://git.openjdk.org/jfx/pull/1363#pullrequestreview-1878722288
PR Review Comment: https://git.openjdk.org/jfx/pull/1363#discussion_r1488437475
PR Review Comment: https://git.openjdk.org/jfx/pull/1363#discussion_r1488442179
PR Review Comment: https://git.openjdk.org/jfx/pull/1363#discussion_r1488445070
PR Review Comment: https://git.openjdk.org/jfx/pull/1363#discussion_r1488447524
Re: RFR: JDK-8325798: Spinner throws uncatchable exception on tab out from garbled text
On Tue, 13 Feb 2024 20:42:08 GMT, Marius Hanl wrote: > When a `Spinner` is configured with e.g. Integers > (`IntegerSpinnerValueFactory`) and the user types in a `String`, e.g. 'abc' > and focuses another `Node`, an exception is thrown (`NumberFormatException`). > This exception is literally uncatchable, as it happens on focus lost. > > The issue is the same as for the `DatePicker` component described in > [JDK-8303478](https://bugs.openjdk.org/browse/JDK-8303478), which was fixed > in PR: https://github.com/openjdk/jfx/pull/1274. > > I did the exact same fix in this PR. Also added the same test. looks good. thank you for referring to the other bug! - Marked as reviewed by angorya (Reviewer). PR Review: https://git.openjdk.org/jfx/pull/1365#pullrequestreview-1878983533
Re: RFR: JDK-8325798: Spinner throws uncatchable exception on tab out from garbled text
On Tue, 13 Feb 2024 20:42:08 GMT, Marius Hanl wrote:
> When a `Spinner` is configured with e.g. Integers
> (`IntegerSpinnerValueFactory`) and the user types in a `String`, e.g. 'abc'
> and focuses another `Node`, an exception is thrown (`NumberFormatException`).
> This exception is literally uncatchable, as it happens on focus lost.
>
> The issue is the same as for the `DatePicker` component described in
> [JDK-8303478](https://bugs.openjdk.org/browse/JDK-8303478), which was fixed
> in PR: https://github.com/openjdk/jfx/pull/1274.
>
> I did the exact same fix in this PR. Also added the same test.
LGTM
modules/javafx.controls/src/test/java/test/javafx/scene/control/SpinnerTest.java
line 1588:
> 1586:
> 1587: /**
> 1588: * When Spinner looses focus with misformatted text in the editor,
typo: loses
modules/javafx.controls/src/test/java/test/javafx/scene/control/SpinnerTest.java
line 1609:
> 1607: spinner.getEditor().setText("2abc");
> 1608:
> 1609: // loosing focus triggers cancelEdit() because the text cannot
> be parsed
typo: losing
-
Marked as reviewed by mstrauss (Committer).
PR Review: https://git.openjdk.org/jfx/pull/1365#pullrequestreview-1879077791
PR Review Comment: https://git.openjdk.org/jfx/pull/1365#discussion_r1488678419
PR Review Comment: https://git.openjdk.org/jfx/pull/1365#discussion_r1488678809
Re: RFR: 8309374: Accessibility Focus Rectangle on ListItem is not drawn when ListView is shown for first time [v2]
> This is accessibility specific fix. > > **Issue**: When a ListView is shown for first time then accessibility focus > rectangle is not drawn around the focused ListIem > > **Cause:** >The ListView takes a little time to create it's skin(ListViewSkin) and the > skins for ListItems(ListCellSkin) >If the Accessibility client application requests focused item before > ListView is completely ready then JavaFX return null. > > **Fix**: Send a focus item change notification to accessibility client > application after the ListIteam is ready > > **Verification:** > - On Windows machine, launch Narrator. Issue is observed only with Narrator > - Execute the > [program](https://bugs.openjdk.org/secure/attachment/104180/Main.java) > attached to the JBS issue > [JDK-8309374](https://bugs.openjdk.org/browse/JDK-8309374) > Move through the TextFields and press `Alt+down`, observe that focus > rectangle is drawn correctly. > Once the ListView is showing Press and hold `Up/Down` or `Ctrl + > Up/Down` keys, observe that focus rectangle is always drawn. Ambarish Rapte has updated the pull request incrementally with one additional commit since the last revision: review corrections-1 - Changes: - all: https://git.openjdk.org/jfx/pull/1363/files - new: https://git.openjdk.org/jfx/pull/1363/files/438f7946..938c6be6 Webrevs: - full: https://webrevs.openjdk.org/?repo=jfx&pr=1363&range=01 - incr: https://webrevs.openjdk.org/?repo=jfx&pr=1363&range=00-01 Stats: 5 lines in 1 file changed: 1 ins; 0 del; 4 mod Patch: https://git.openjdk.org/jfx/pull/1363.diff Fetch: git fetch https://git.openjdk.org/jfx.git pull/1363/head:pull/1363 PR: https://git.openjdk.org/jfx/pull/1363
Re: RFR: 8309374: Accessibility Focus Rectangle on ListItem is not drawn when ListView is shown for first time [v2]
On Tue, 13 Feb 2024 21:15:16 GMT, Kevin Rushforth wrote: > The fix looks good to me, and my initial testing shows that it works as > expected. I left a few suggestions on minor wording changes inline. I'll > reapprove if you make the changes. Thank you Kevin, Please have a re-look, I made the changes as per the comments. - PR Comment: https://git.openjdk.org/jfx/pull/1363#issuecomment-1943009421
Re: RFR: 8319844 : Text/TextFlow.hitTest() is incorrect in RTL orientation [v11]
On Fri, 9 Feb 2024 07:59:47 GMT, Karthik P K 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 incrementally with one additional
> commit since the last revision:
>
> Review comments
I think we should simplify the `getHitInfo` method in the `TextLayout`
interface.
The code currently seems to be making distinctions where there are none.
`TextFlow`s provide spans, while `Text`s provide a text. `getHitInfo` can take
advantage of this to avoid the `text` and `forTextFlow` parameters altogether.
This will reduce confusion (as there is no distinction) and also avoids cases
that are non-sensical (providing `text` while `forTextFlow` is `true` or vice
versa).
Previous changes to this code (when the parameter `text` was introduced to
`getHitInfo`) should probably be partially undone and simplified with this new
knowledge.
modules/javafx.graphics/src/main/java/com/sun/javafx/scene/text/TextLayout.java
line 213:
> 211: * @param textRunStart Start position of first Text run where hit
> info is requested.
> 212: * @param curRunStart Start position of text run where hit info is
> requested.
> 213: * @param forTextFlow Indicates if the hit info is requested for
> TextFlow or Text node. {@code true} for TextFlow and {@code false} for Text
> node.
I have the impression that we're overcomplicating things here. There is a flag
(`forTextFlow`) for Text/TextFlow, and a String (`text`) for Text/TextFlow, and
there are already `setContent` methods for Text/TextFlow.
I don't see a need for any of these changes to `getHitInfo` at all.
If the content was set with `setContent(TextSpan[] spans)`, then we know it is
a TextFlow (actually we don't care, we have spans which is the difference we're
interested in). This fact can be checked at any time with:
boolean isThisForTextFlow = this.spans != null;
See how the `setContent` methods work; they either set `spans` or they don't.
The rest of the code is already full of alternate paths with `if (spans ==
null) { /* do Text stuff */ } else { /* do TextFlow stuff */ }`
The `text` parameter is also already known, because this is explicitely set for
`Text` nodes because they use `setContent(String text, Object font)`. However,
before using it (specifically for `Text`), make sure that check `spans == null`
as it is filled for `TextFlow` as well at a later point.
So, I think:
- the `text` parameter should never have been added (it wasn't until recently)
- `forTextFlow` flag is unnecessary, just check `spans != null`.
modules/javafx.graphics/src/main/java/com/sun/javafx/text/PrismTextLayout.java
line 424:
> 422:
> 423: @Override
> 424: public Hit getHitInfo(float x, float y, String text, int
> textRunStart, int curRunStart, boolean forTextFlow) {
This method has become huge, with I think upto 7 levels of nesting. It would
benefit from some splitting.
Suggestions in that area:
- Split it in one that handles RTL and one for LTR **or**
- Split it in one that handles `spans != null` (`TextFlow`) and one that
handles `Text`
You can also reduce the nesting of the first `if/else` by returning early:
if (lineIndex >= getLineCount()) {
charIndex = getCharCount();
insertionIndex = charIndex + 1;
return new Hit(charIndex, insertionIndex, leading); // return
early here
} else { // no need to nest 150+ lines of code
More suggestions inline below
modules/javafx.graphics/src/main/java/com/sun/javafx/text/PrismTextLayout.java
line 432:
> 430: int ltrIndex = 0;
> 431: int textWidthPrevLine = 0;
> 432: float xHitPos = x;
This variable seems important, yet is only used in the RTL+Text branches.
It seems that this variable can also be easily calculated as:
originalX - (getMirroringWidth() - x) + bounds.getMinX
This calculation can be done in the final RTL+Text branch near the end, no need
to precalculate it IMHO
modules/javafx.graphics/src/main/java/com/sun/javafx/text/PrismTextLayout.java
line 438:
> 436: if (lineIndex >= getLineCount()) {
> 437: charIndex = getCharCount();
> 438: insertionIndex = charIndex + 1;
By returning early here, you can avoid the `else` and save a lot of nesting.
modules/javafx.graphics/src/main/java/com/sun/javafx/text/PrismTextLayout.java
line 440:
> 438: insertionIndex = charIndex + 1;
> 439: } else {
> 440: if (isMirrored && (forTextFlow || spans == null)) {
The extra condition here adds nothing of value, the original `if` was correct.
Because `spans == null`
