<AWT Dev> [10] Review request for 8155197: Focus transition issue

[Alexey Ivanov]

Hi Dmitry,

Looks fine to me too.


Regards,
Alexey

On 05/10/2017 15:27, Dmitry Markov wrote:
> Hi Sergey,
>> On 4 Oct 2017, at 19:27, Sergey Bylokhov <sergey.bylokhov at oracle.com 
>> <mailto:sergey.bylokhov at oracle.com>> wrote:
>>
>> On 10/4/17 03:25, Dmitry Markov wrote:
>>> 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/>
>>
>> isn't the "restoreFocusTo = toFocus;" also redundant?
>> If we tried to restore the focus synchronously in Window1 (Actually 
>> we tried to post FocusEvent.Cause.ROLLBACK) and we found that the 
>> native window was changed to Window2 then why we should post ROLLBACK 
>> then the focus will be moved back to Window1? I assume that we never 
>> post ROLLBACK to the Window1 because when the Window2 will got the 
>> focus it will reset restoreFocusTo.
>> It is up to you to check it, it looks fine to me as-is.
> Most likely you are right. However if you do not mind I would prefer 
> to keep it as is.
>
> Thanks,
> Dmitry
>>
>>> Thanks,
>>> Dmitry
>>>> On 3 Oct 2017, at 18:30, Sergey Bylokhov 
>>>> <sergey.bylokhov at oracle.com 
>>>> <mailto:sergey.bylokhov at oracle.com><mailto:sergey.bylokhov at 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 locatedathttp://cr.openjdk.java.net/~dmarkov/8155197/webrev.02/ 
>>>>> <athttp://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.sadetsky at oracle.com <mailto:semyon.sadetsky at oracle.com> 
>>>>>> <mailto:semyon.sadetsky at oracle.com><mailto:semyon.sadetsky at 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.
>>
>>
>> --
>> Best regards, Sergey.
>