Hi Semyon, > On 5 Oct 2017, at 17:13, Semyon Sadetsky <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 at http://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 >>>>>>> <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. >>>> >>> >> >
