Hi Sergey, > On 4 Oct 2017, at 19:27, Sergey Bylokhov <sergey.bylok...@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/~dmarkov/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.bylok...@oracle.com >>> <mailto:sergey.bylok...@oracle.com> <mailto: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/ >>>> <athttp://cr.openjdk.java.net/~dmarkov/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. > > > -- > Best regards, Sergey.
