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`