OK, looks good to me, thanks.
--Semyon
On 10/05/2017 02:15 PM, Dmitry Markov wrote:
Hi Semyon,
On 5 Oct 2017, at 17:13, Semyon Sadetsky <semyon.sadet...@oracle.com
<mailto:semyon.sadet...@oracle.com>> wrote:
Hi Dmitriy,
On 10/05/2017 07:25 AM, Dmitry Markov wrote:
Hi Semyon,
Actually the most recent focus owner is updated during invocation of
setEnabled(), setFocusable() and setVisible(). Please find the
details below:
- Component.setEnabled(false), (i.e. disable a component) -
setEnabled(false) -> Component.this.setEnabled(false) ->
enable(false) -> disable() ->
KeyboardFocusManager.clearMostRecentFocusOwner()
- Component.setFocusable(false), (i.e. make a component
non-focusable) - setFocusable(false) -> transferFocus()
and KeyboardFocusManager.clearMostRecentFocusOwner()
- Component.setVisible(false), (i.e. make a component invisible) -
setVisible(false) -> Component.this.setVisible(false) -> show(false)
-> hide() -> clearMostRecentFocusOwnerOnHide() and transferFocus()
So the most recent focus owner is always up to date. I am sorry, but
looks like it is not necessary to perform checks such as isShowing()
and canBeFocusOwner() before we set some value to restoreFocusTo.
These checks are implicitly performed before usage of restoreFocusTo
and I believe that is enough even for concurrent execution. What do
you think?
Yes, this makes sense. I'm only not sure about the case when a parent
component is made invisible and the most recent focus owner is
cleared for the parent component but not for its child components. So
all the child components will start to return isShowing() false (e.g.
non-focusable) but any of them still may be set as most recent focus
owner. This makes the isShowing() check mandatory for component
returned by getMostRecentFocusOwner().
If a parent component, (i.e. container) is made invisible, the most
recent focus owner is cleared for its child components too, see
implementation of clearMostRecentFocusOwnerOnHide() in Container class
for details. So I do not think that it is really necessary to invoke
isShowing() for the component returned by getMostRecentFocusOwner().
Thanks,
Dmitry
--Semyon
Thanks,
Dmitry
On 4 Oct 2017, at 17:23, Semyon Sadetsky
<semyon.sadet...@oracle.com <mailto:semyon.sadet...@oracle.com>> wrote:
Hi Dmitry,
If you make a detailed look on the Component class methods like
setEnables(), setFocusable() and setVisible() the most recent focus
owner is cleared or transferred only after the component is made
non-focusable. Since the fix is aimed for concurrent scenarios the
recheck actually makes sens.
Also it would be more correct always trying to determine the first
focusable component in traversal order before before the attempt to
set focus owner as it is done in restoreFocus(Window aWindow,
Component boolean clearOnFailure):
if (!toFocus.isShowing() ||
!toFocus.canBeFocusOwner()) {
toFocus = toFocus.getNextFocusCandidate();
}
otherwise when focus is restored to actually non-focusable
component the final focus owner may become null while the next
component in container traversal order is expected to have the
input focus.
--Semyon
On 10/04/2017 03:25 AM, Dmitry Markov wrote:
Hi Sergey,
Good catch!
Actually we check restoreFocusTo before its usage, (i.e. we
compare restoreFocusTo with most resent focus owner for the
window; most recent focus owner is visible and focusable,
otherwise it is removed from recent focus owners map).
So the checks before assignment are not really necessary. I
updated the fix (reverted it to the previous version). Please find
webrev here:
http://cr.openjdk.java.net/~dmarkov/8155197/webrev.03/
<http://cr.openjdk.java.net/%7Edmarkov/8155197/webrev.03/>
Thanks,
Dmitry
On 3 Oct 2017, at 18:30, Sergey Bylokhov
<sergey.bylok...@oracle.com <mailto:sergey.bylok...@oracle.com>>
wrote:
Hi, Dmitry.
If this check is valid then probably the same check should be
added to restoreFocus()? before:
#173 restoreFocusTo = toFocus;
But as it was mentioned before, the component can become
non-visible/non-focusable at any point, for example after this
check but before the assignment. And instead of data validation
before assignment we should check them before use. As far as I
understand we already do this, all usages of restoreFocusTo are a
checks == or != to some other component which is visible and
focusable, isn't it?
On 10/3/17 08:23, Dmitry Markov wrote:
Hi Semyon,
I have updated the fix based on your suggestion. The new version
is located
athttp://cr.openjdk.java.net/~dmarkov/8155197/webrev.02/
<http://cr.openjdk.java.net/%7Edmarkov/8155197/webrev.02/>
Also I slightly modified the test to simplify it.
Thanks,
Dmitry
On 2 Oct 2017, at 18:32, Semyon Sadetsky
<semyon.sadet...@oracle.com
<mailto:semyon.sadet...@oracle.com><mailto:semyon.sadet...@oracle.com>>
wrote:
Hi Dmitry,
Actually the parent frame doesn't own the input focus when
the issue happens. The focus is on the dialog already and
when FOCUS_GAINED event comes for the dialog the KFM
discovers that the dialog should not have the focus and
rejects it in line 588 of the DefaultKeyboardFocusManager:
restoreFocus(fe, newFocusedWindow);
This happens when the dialog became non-focusable
(non-visible) after the focus request is sent, but before the
corresponding FOCUS_GAINED event is handled on the EDT. In
this case the focus is directly restored to the previously
focused window which doesn't have the focus at this moment
and input focus cannot be requested to one of its components
synchronously.
Please confirm whether you agree on the root cause of the bug.
You are right. I agree with your evaluation.
Thanks.
Before setting the restoreFocusTo to toFocus in line 190 I
would recheck for toFocus.isShowing() &&
toFocus.canBeFocusOwner() once again because the component can
be made non-focusable concurrently.
--Semyon
--
Best regards, Sergey.
