<Swing Dev> [11]RFR:JDK-8208640: [a11y][macosx] Unable to navigate between Radiobuttons in Radio group using keyboard

[Philip Race]

Ok. I've approved it. Please push.

-phil.

On 8/14/18, 1:07 AM, Krishna Addepalli wrote:
>
> Oops, my bad. I was trying to upload a webrev with rsync, and by 
> mistake wrong folder was selected. I have replaced 
> webrev02(http://cr.openjdk.java.net/~kaddepalli/8208640/webrev02/ 
> <http://cr.openjdk.java.net/%7Ekaddepalli/8208640/webrev02/>), with 
> the correct upload, with all the formatting issues fixed.
>
> I have also filed the “jdk11-fix-request” for the bug, and put in a 
> comment explaining the problem, fix and the tests that have been done.
>
> Thanks,
>
> Krishna.
>
> *From:*Phil Race
> *Sent:* Tuesday, August 14, 2018 12:43 AM
> *To:* Krishna Addepalli <krishna.addepalli at oracle.com>; Prasanta 
> Sadhukhan <prasanta.sadhukhan at oracle.com>
> *Cc:* swing-dev at openjdk.java.net
> *Subject:* Re: <Swing Dev> [11]RFR:JDK-8208640: [a11y][macosx] Unable 
> to navigate between Radiobuttons in Radio group using keyboard
>
>
>
> On 08/13/2018 12:08 AM, Krishna Addepalli wrote:
>
>     Hi Phil,
>
>     I have run the JCK tests for JRadioButton and also focus
>     traversal, and found no failures.
>
>     Here is the new webrev with additional formatting issues fixed:
>     http://cr.openjdk.java.net/~kaddepalli/8208640/webrev02/
>     <http://cr.openjdk.java.net/%7Ekaddepalli/8208640/webrev02/>
>
>
> You are referring to this comment of mine ?
>
> > You may have fixed the "){" pattern but I still see "if(" in v.01.
>
> But v02 still has these
>
> +        if(!isValidRadioButtonObj(eventSrc))
> +                if(!isValidRadioButtonObj(curElement))
> +                if(activeBtn != null) {
>   
> Comparing time stamps it looks like you just reposted v01 as v02
>   
> http://cr.openjdk.java.net/~kaddepalli/8208640/webrev01/  <http://cr.openjdk.java.net/%7Ekaddepalli/8208640/webrev01/>
> http://cr.openjdk.java.net/~kaddepalli/8208640/webrev02/  <http://cr.openjdk.java.net/%7Ekaddepalli/8208640/webrev02/>
>   
>   
> I am APPROVING the fix, and assuming that this will be taken care of in the push.
>   
> Go ahead and file the jdk11-fix-request paperwork ASAP.
>   
> -phil.
>   
>   
>
>     Thanks,
>
>     Krishna
>
>     *From:*Prasanta Sadhukhan
>     *Sent:* Monday, August 13, 2018 9:00 AM
>     *To:* Philip Race <philip.race at oracle.com>
>     <mailto:philip.race at oracle.com>
>     *Cc:* Krishna Addepalli <krishna.addepalli at oracle.com>
>     <mailto:krishna.addepalli at oracle.com>; swing-dev at openjdk.java.net
>     <mailto:swing-dev at openjdk.java.net>
>     *Subject:* Re: <Swing Dev> [11]RFR:JDK-8208640: [a11y][macosx]
>     Unable to navigate between Radiobuttons in Radio group using keyboard
>
>     On 8/12/2018 11:02 PM, Philip Race wrote:
>
>         I did notice the inner classes are not static, but this is the
>         same as
>         the class from which they were copied and it seems like they need
>         access to the enclosing instance.
>
>         Can you expand on the path to the problem you see ?
>
>         I don't think you will have a memory leak due to multiple
>         calls to installListeners()
>         which is how I read your statement. That would suggest you are
>         talking about
>         the SelectNextBtn instance leaking ?
>
>         First, the reference is one-way. Once the map entry is
>         over-written, the previous
>         SelectNextBtn() instance will become eligible to be gc'd since
>         there will be nothing
>         referencing it. Marginally inefficient, but that is all I see.
>
>         But what about just the single call to installListeners() ?
>         And if you mean the RadioButtonUI will
>         leak ? Multiple calls to installListeners() won't affect that
>         so far as I can see.
>
>     I was referring to AquaRadioButtonUI leak. I guess you are right,
>     if uninstallListeners is called, then it can be gced.
>
>     Regards
>     Prasanta
>
>
>         So long as uninstallListeners() is called then the map entry
>         will be cleared,
>         making the values referenced eligible for GC, in turn making
>         the RadioButtonUI
>         eligible for GC - which probably will typically happen only if
>         the app switches L&F so that is rare.
>
>         So I think there is only a bug if uninstallListeners() is not
>         called, and I presume
>         there has to be a pattern to make sure this happens across
>         Swing ...
>
>         -phil.
>
>         On 8/12/18, 9:13 AM, Prasanta Sadhukhan wrote:
>
>             One thing from 1st look...
>
>             Every time installListeners is called, it instantiates
>             SelectPreviousBtn and SelectNextBtn which is a inner class
>             (and not a static class) and it has a strong reference to
>             AquaButtonRadioUI so it may not be garbage collected and
>             can cause a leak.
>
>             Regards
>             Prasanta
>
>             On 8/11/2018 8:51 PM, Krishna Addepalli wrote:
>
>                 Hi All,
>
>                 Please review a fix for JDK-8208640:
>                 https://bugs.openjdk.java.net/browse/JDK-8208640
>
>                 Webrev:
>                 http://cr.openjdk.java.net/~kaddepalli/8208640/webrev00/
>                 <http://cr.openjdk.java.net/%7Ekaddepalli/8208640/webrev00/>
>
>                 The problem is that the arrow key navigation for Aqua
>                 L&F was not implemented, which is why neither the tab
>                 keys nor the arrow keys were allowing to navigate
>                 through the radio buttons through a button group.
>
>                 Proposed fix is to copy the implementation from Basic
>                 L&F into Aqua L&F. Also fixed the test that was
>                 written to include the Aqua L&F to be tested, and it
>                 passes on Mac successfully.
>
>                 Thanks,
>
>                 Krishna
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/swing-dev/attachments/20180814/ab8fbaea/attachment.html>