I don't see how a method like `setFocused` could possibly work without
violating its specification. When a Scene is present, it could in
theory update the focusOwner, but when no Scene is present it probably
needs to remain `false` even when set to `true` (as otherwise you could
set multiple nodes to focused, unless you did a full scan of the entire
UI hierarchy to avoid that...).
Perhaps it is possible to make setFocused do something useful; let it
immediately change focus/focusOwner -- that could be tough as I think
focus changes can be veto'd; the purpose of requestFocus would then only
be to also focus the window. The setFocused method would then be a
useful feature. It would still need a Scene to work (but its
specification agrees with that part, so it can indeed remain false if
not attached to a scene). All nodes would need to clear focus status
when the Scene is null'd, and when calling setFocused on a Node, any
currently focused node in the same Scene needs to lose it (probably best
to remove focus first so those notifications happen before the new node
gains focus).
`setFocused(false)` would also need some special treatment; either it's
not allowed at all, or some other node gets picked as the new focus
owner (perhaps it switches to the first focusable node, sort of like a
focus reset as if the UI was freshly created).
If the above ain't realistically possible, then I guess deprecation is
the only option.
--John
On 11/10/2023 22:55, Michael Strauß wrote:
I think the cleanest solution would be to have the implementation of
Node.focused match its specification:
Indicates whether this {@code Node} currently has the input focus.
To have the input focus, a node must be the {@code Scene}'s focus
owner, and the scene must be in a {@code Stage} that is visible
and active.
Obviously, the Node.focused property is not implemented and used as
specified. But the problem is that Node.setFocused(boolean) is public
API. If we wanted the implementation to conform to its specification,
we would probably need to deprecate this API, and also make it
ineffective so that user code cannot change the focused property. Then
we need to fix all controls that use this API.
Maybe we can also just fix the controls without removing the
Node.setFocused API. But it begs the question why we would keep around
an API that is only useful in circumventing the specification of a
property. What do you think?
On Sun, Oct 8, 2023 at 10:11 AM John Hendrikx <john.hendr...@gmail.com> wrote:
I would be interested to know what use cases for these are, and what
would be then left of the use cases for the existing focusWithin and
focusVisible properties? The ticket referred doesn't give one and only
reports a bug.
Before implementing a second set of these types of properties, I would
like to know:
- What would be the distinct uses for focusOwnerWithin vs focusWithin,
are both actually needed?
- Isn't focusOwnerProperty basically a "better" focusedProperty now
because of how focusedProperty is abused by some controls?
- Why indeed is TableView doing this and should it really be doing this
instead of doing this another way? If it just wants to show nodes in
their focused style, that can (and should IMHO) be achieved differently.
There seems to be a need to have some controls retain their focused look
without them actually being the focus owner. TableView seems to be one
of them, and I personally have another (I would like Nodes to keep
looking focused even when their Stage has lost focus). The focused look
relies on the `focused` pseudo class, which is now strictly controlled
by the focusedProperty. It may be a good idea to look in another
direction; one could change the style sheet for example to also have
controls in focused look when they have a different pseudo class, say
`fake-focused`. TableView could then use this (and so could I).
With the TableView problem solved, the original focusWithin,
focusVisible and focused properties then can serve the purposes they
were intended for, instead of basically replacing them with a new set
that acts slightly differently to work around a problem with TableView.
If this does move forward though, I wonder why these need to be
properties specifically, given how hesistant we were to add properties
to Node? The `shownProperty` which IMHO has far more uses given the
essential role it can play in deterministic clean-up was rejected over
such an argument. Why isn't simply checking the psuedoClassState
sufficient for these properties:
`getPseudoClassStates().contains(FOCUS_OWNER_PSEUDOCLASS_STATE)`?
--John
