<html><head></head><body><div style="font-family: Verdana;font-size: 12.0px;"><div>I'm also wondering whether we should check and may improve the focus code rather than introducing two new properties.<br/>
I did not yet checked the details.</div>
<div><br/>
But I was checking if we could remove the hacky focus stuff/css, e.g. ControlUtils.requestFocusOnControlOnlyIfCurrentFocusOwnerIsChild(..) with the new focusWithin. Basically what we want is that the table has the focus border when selecting (and therefore focusing) rows or cells.<br/>
But the weird table/row focus behaviour as written in https://bugs.openjdk.org/browse/JDK-8317426 makes that impossible for now. Everything else works actually, already using it for our tables.</div>
<div>
<div> </div>
<div>-- Marius</div>
<div>
<div name="quote" style="margin:10px 5px 5px 10px; padding: 10px 0 10px 10px; border-left:2px solid #C3D9E5; word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;">
<div style="margin:0 0 10px 0;"><b>Gesendet:</b> Donnerstag, 12. Oktober 2023 um 00:15 Uhr<br/>
<b>Von:</b> "John Hendrikx" <john.hendrikx@gmail.com><br/>
<b>An:</b> openjfx-dev@openjdk.org<br/>
<b>Betreff:</b> Re: New properties: focusOwner and focusOwnerWithin</div>
<div name="quoted-content">I don't see how a method like `setFocused` could possibly work without<br/>
violating its specification. When a Scene is present, it could in<br/>
theory update the focusOwner, but when no Scene is present it probably<br/>
needs to remain `false` even when set to `true` (as otherwise you could<br/>
set multiple nodes to focused, unless you did a full scan of the entire<br/>
UI hierarchy to avoid that...).<br/>
<br/>
Perhaps it is possible to make setFocused do something useful; let it<br/>
immediately change focus/focusOwner -- that could be tough as I think<br/>
focus changes can be veto'd; the purpose of requestFocus would then only<br/>
be to also focus the window. The setFocused method would then be a<br/>
useful feature. It would still need a Scene to work (but its<br/>
specification agrees with that part, so it can indeed remain false if<br/>
not attached to a scene). All nodes would need to clear focus status<br/>
when the Scene is null'd, and when calling setFocused on a Node, any<br/>
currently focused node in the same Scene needs to lose it (probably best<br/>
to remove focus first so those notifications happen before the new node<br/>
gains focus).<br/>
<br/>
`setFocused(false)` would also need some special treatment; either it's<br/>
not allowed at all, or some other node gets picked as the new focus<br/>
owner (perhaps it switches to the first focusable node, sort of like a<br/>
focus reset as if the UI was freshly created).<br/>
<br/>
If the above ain't realistically possible, then I guess deprecation is<br/>
the only option.<br/>
<br/>
--John<br/>
<br/>
<br/>
On 11/10/2023 22:55, Michael Strauß wrote:<br/>
> I think the cleanest solution would be to have the implementation of<br/>
> Node.focused match its specification:<br/>
><br/>
> Indicates whether this {@code Node} currently has the input focus.<br/>
> To have the input focus, a node must be the {@code Scene}'s focus<br/>
> owner, and the scene must be in a {@code Stage} that is visible<br/>
> and active.<br/>
><br/>
> Obviously, the Node.focused property is not implemented and used as<br/>
> specified. But the problem is that Node.setFocused(boolean) is public<br/>
> API. If we wanted the implementation to conform to its specification,<br/>
> we would probably need to deprecate this API, and also make it<br/>
> ineffective so that user code cannot change the focused property. Then<br/>
> we need to fix all controls that use this API.<br/>
><br/>
> Maybe we can also just fix the controls without removing the<br/>
> Node.setFocused API. But it begs the question why we would keep around<br/>
> an API that is only useful in circumventing the specification of a<br/>
> property. What do you think?<br/>
><br/>
><br/>
> On Sun, Oct 8, 2023 at 10:11 AM John Hendrikx <john.hendrikx@gmail.com> wrote:<br/>
>> I would be interested to know what use cases for these are, and what<br/>
>> would be then left of the use cases for the existing focusWithin and<br/>
>> focusVisible properties? The ticket referred doesn't give one and only<br/>
>> reports a bug.<br/>
>><br/>
>> Before implementing a second set of these types of properties, I would<br/>
>> like to know:<br/>
>><br/>
>> - What would be the distinct uses for focusOwnerWithin vs focusWithin,<br/>
>> are both actually needed?<br/>
>> - Isn't focusOwnerProperty basically a "better" focusedProperty now<br/>
>> because of how focusedProperty is abused by some controls?<br/>
>> - Why indeed is TableView doing this and should it really be doing this<br/>
>> instead of doing this another way? If it just wants to show nodes in<br/>
>> their focused style, that can (and should IMHO) be achieved differently.<br/>
>><br/>
>> There seems to be a need to have some controls retain their focused look<br/>
>> without them actually being the focus owner. TableView seems to be one<br/>
>> of them, and I personally have another (I would like Nodes to keep<br/>
>> looking focused even when their Stage has lost focus). The focused look<br/>
>> relies on the `focused` pseudo class, which is now strictly controlled<br/>
>> by the focusedProperty. It may be a good idea to look in another<br/>
>> direction; one could change the style sheet for example to also have<br/>
>> controls in focused look when they have a different pseudo class, say<br/>
>> `fake-focused`. TableView could then use this (and so could I).<br/>
>><br/>
>> With the TableView problem solved, the original focusWithin,<br/>
>> focusVisible and focused properties then can serve the purposes they<br/>
>> were intended for, instead of basically replacing them with a new set<br/>
>> that acts slightly differently to work around a problem with TableView.<br/>
>><br/>
>> If this does move forward though, I wonder why these need to be<br/>
>> properties specifically, given how hesistant we were to add properties<br/>
>> to Node? The `shownProperty` which IMHO has far more uses given the<br/>
>> essential role it can play in deterministic clean-up was rejected over<br/>
>> such an argument. Why isn't simply checking the psuedoClassState<br/>
>> sufficient for these properties:<br/>
>> `getPseudoClassStates().contains(FOCUS_OWNER_PSEUDOCLASS_STATE)`?<br/>
>><br/>
>> --John<br/>
>></div>
</div>
</div>
</div></div></body></html>