On Mon, 3 Mar 2025 21:10:39 GMT, Andy Goryachev <ango...@openjdk.org> wrote:

>> 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());
>> 
>> So, if I understand correctly, we can't directly do this `bind`:
>> 
>>     
>> symbol.focusTraversableProperty().bind(Platform.accessibilityActiveProperty())
>>     
>> Because on a background thread there may not yet be an initialized 
>> `Platform`?  Or perhaps, `Platform` would need to initialize and we don't 
>> want to do that yet on a background thread?
>
> The reason we `should not` be binding in this PR is because in part it's 
> **bad design(tm)**: we are creating a zillion of listeners (one per plot data 
> point); a better solution would be to create one listener and use (already 
> existing series) to toggle the data points.
> 
> The other reason is https://bugs.openjdk.org/browse/JDK-8351067 - the 
> `Platform::accessibilityActive` property getter is not thread safe, but even 
> if was, registering a listener with it may cause the change to come in 
> **before** the node becomes a part of the scene graph, leading to 
> unsynchronized multi-threaded access.

Was this solution considered:

    ObservableValue<Boolean> partOfSceneGraph = this.sceneProperty()
      .flatMap(Scene::windowProperty)
      .flatMap(Window::showingProperty)
      .orElse(false);
      
    symbol.focusTraversableProperty()
        .bind(Platform.accessibilityActiveProperty().when(partOfSceneGraph));
        
What the above code does is create a binding on the `when` statement (a 
listener is added on the `when` result).  However, the `when` property will not 
add a listener on `Platform.accessibilityActiveProperty()` until 
`partOfSceneGraph` is `true`.  As soon as it does though, a listener is 
installed on  `Platform.accessibilityActiveProperty()` and its latest value is 
set to `focusTraversableProperty` via the bind.

-------------

PR Review Comment: https://git.openjdk.org/jfx/pull/1697#discussion_r1978264839

Reply via email to