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

[Semyon Sadetsky]

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.sadetsky at oracle.com 
>> <mailto:semyon.sadetsky at 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.sadetsky at oracle.com <mailto:semyon.sadetsky at 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.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 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.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.
>>>>>
>>>>
>>>
>>
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/awt-dev/attachments/20171005/d8f07b8e/attachment-0001.html>