On Thu, 20 Feb 2025 23:04:15 GMT, Andy Goryachev <ango...@openjdk.org> wrote:
>> Root Cause: >> (Multiple) properties are getting bound to the global >> `Platform.accessibilityActive` property. Binding (and I say, accessing) of >> properties is not thread-safe. >> >> I also changed the design a bit. Originally, every symbol in a chart had >> its `focusTraversableProperty` bound to `Platform.accessibilityActive` in >> order to enable the accessibility navigation across the chart data points. >> This is rather inefficient, as the property has to manage (thousands?) of >> listeners. >> >> Instead, a single boolean property is added to each chart, with a listener >> added to it which iterates over data symbols to toggle the >> `focusTraversableProperty` directly. >> >> The exact moment when the new property gets bound is also important, and has >> to happen in the FX application thread. >> >> With this change, it is safe to create and populate charts with data in a >> background thread. >> >> --- >> >> ## NOTES >> >> 1. It looks like the `Platform.accessibilityActive` property never >> transitions back to false after it transitioned to true. Some say it is >> because there is no mechanism in the platform to get notified which cannot >> possibly be true. >> 2. The javadoc for `Platform.accessibilityActiveProperty()` method >> stipulates that "_This method may be called from any thread_" which is >> patently not true as the current implementation is simply not thread-safe. >> >> ## Note to the Reviewers >> >> To avoid merge conflicts, the preferred order of integrations: >> >> #1697 >> #1713 >> #1717 > > Andy Goryachev has updated the pull request with a new target base due to a > merge or a rebase. The pull request now contains 22 commits: > > - Merge remote-tracking branch 'origin/master' into > 8349091.charts.thread.safety > - enabled pie chart test > - Merge branch 'master' into 8349091.charts.thread.safety > - Merge branch 'master' into 8349091.charts.thread.safety > - whitespace > - Merge remote-tracking branch 'origin/master' into > 8349091.charts.thread.safety > - cleanup > - tests pass > - chart tests only > - more jitter > - ... and 12 more: https://git.openjdk.org/jfx/compare/307e3087...e60c027b This is heading in the right direction, but has some bugs that will need to be fixed. modules/javafx.controls/src/main/java/javafx/scene/chart/AreaChart.java line 546: > 544: symbol.setAccessibleRole(AccessibleRole.TEXT); > 545: symbol.setAccessibleRoleDescription("Point"); > 546: symbol.setFocusTraversable(isAccessibilityActive()); It looks like you are mixing the setting up of the listeners with setting the focus traversable of this symbol to the current state of accessibilityActive, which will be `Platform.accessibilityActiveProperty` for the FX app thread) and "false" (for background thread. As a result, this method, which is called once per symbol, will set up a new listener each time it is called. You might consider refactoring this a bit to only ever set up the listeners when the object is constructed. Either that or ensure that you guard against repeatedly recreating the listener. modules/javafx.controls/src/main/java/javafx/scene/chart/Chart.java line 536: > 534: */ > 535: // package protected: custom charts must handle accessbility on > their own > 536: boolean isAccessibilityActive() { This can be final, although as I mentioned in an earlier comment, it could probably use to be refactored. modules/javafx.controls/src/main/java/javafx/scene/chart/Chart.java line 551: > 549: ChangeListener<Scene> li = new ChangeListener<>() { > 550: @Override > 551: public void changed(ObservableValue<? extends Scene> > src, Scene prev, Scene scene) { This is insufficient. It will miss the case where both the node and the scene are created in the background thread, and the scene is later added to a window on the FX app thread. I tested this case and verified my assertion. This also means that in some cases, if this listener is called from a background thread, you will create a new scene listener from the current scene listener. Consider instead using `TreeShowingProperty`, which is used by `ProgressIndicatorSkin` to set up its animation.`TreeShowingProperty` sets up a listener chain that fires when the the "tree showing" status of a node changed (tree showing means the node and all ancestors are visible and is attached to a scene that is attached to a window that is showing. Alternatively, since you might not care whether the node is visible, you can just set up a binding using `ObservableValue::flatMap`. The code snippet of the docs: ObservableValue<Boolean> isShowing = sceneProperty() .flatMap(Scene::windowProperty) .flatMap(Window::showingProperty) .orElse(false); And then you can then add a listener (or a value subscription might be better) on `isShowing` -- when it becomes true, you can setup the binding or listener to `Platform.accessibilityActiveProperty`. With either of the above two solutions, the showing state will only become true on the FX app thread. ------------- PR Review: https://git.openjdk.org/jfx/pull/1697#pullrequestreview-2634906931 PR Review Comment: https://git.openjdk.org/jfx/pull/1697#discussion_r1968581638 PR Review Comment: https://git.openjdk.org/jfx/pull/1697#discussion_r1968583768 PR Review Comment: https://git.openjdk.org/jfx/pull/1697#discussion_r1968591700
