Hi Sean,   

  
  
Updated the repository used in webrev from jdk8-tl to�http://hg.openjdk.java.net/jdk8/awt/jdk �.
  

  
  
new webrev:�http://cr.openjdk.java.net/~zhouyx/OJDK-61/webrev.01/�
  
I have the following comments about the fix:

1. When right-to-left orientation the Recent swatches inverts right and left button.
2. Could you please don't use package visibility when fileds/methods/inner classes can be private (e.g. field mainSwatchKeyListener)
3. I think you should uninstall the introduced listeners in the DefaultSwatchChooserPanel#uninstallChooserPanel method
4. Why new listeners are Serializable?
5. I recommend to use if condition instead of switch/case blocks with one branch
6. Could you please rename selrow (and similar variables) into selRow
7. Can we use Component#isFocusOwner instead of supporting new variable showcursor?
8. Could you please follow our code guide lines (spaces etc)

Regards, Pavel


  ---------- Forwarded message ----------
From: Sean Chou <zhouyx@linux.vnet.ibm.com>
Date: Thu, Aug 9, 2012 at 3:29 PM
Subject:  Add keyboard accessibility to JColorChooser swatch
To: swing-dev@openjdk.java.net
  
  
Hi all,   

  
  
� � This is a patch to add keyboard�accessibility to JColorChooser swatch, so when using TAB, the focus can move to�SwatchPanel, choose color with arrow keys and select color with space bar.�
  

  
  
In current implementation, it is not able to move the focus to�SwatchPanel with TAB, this can be seen in SwingSet2 demo.�
  
Steps:�
  
1. run SwingSet2 demo�
  
2. click on JColorChooser demo
  
3. click Background button and Swatches panel appears.
  
4. Press Tab key � � => � focus moves to OK button as shown in this image�http://cr.openjdk.java.net/~zhouyx/OJDK-61/screenshot_1.png
  

  
  
With this patch, in step4, focus moves to�SwatchPanel, as shown here�http://cr.openjdk.java.net/~zhouyx/OJDK-61/screenshot_2.png
  
  
Then, arrow keys can be used to choose color and select color by space bar.
  

  
  
The webrev is�http://cr.openjdk.java.net/~zhouyx/OJDK-61/webrev.00/ �.�
  

  
  
Please take a look.
     

  
  

  
-- 
Best Regards,
Sean Chou
  
  
     
     

  
-- 
Best Regards,
Sean Chou
  


">

(original) (raw)

Hi Pavel,


� � �I updated the patch according to your comments except No.1 and No.4, it is now at: �http://cr.openjdk.java.net/~zhouyx/OJDK-61/webrev.03/ �. �


� � �About comment No.1 (�When right-to-left orientation the Recent swatches inverts right and left button�), I tried to set the locale to ar\_AE, but didn't get a visual result about what I should do, please look at�http://cr.openjdk.java.net/\~zhouyx/OJDK-61/screenshot\_3.png� . �Can anyone give a little help on how to produce a right-to-left orientation in demo SwingSet2 or others?

� � �About comment No.4(Why new listeners are Serializable�), the original MouseListeners in this class are�Serializable �and I see javadoc for Component class says "Developers will need, as always, to consider the implications of making an object serializable" .

� � �Please take a look again, thanks.

On Thu, Aug 16, 2012 at 10:00 PM, Pavel Porvatov <pavel.porvatov@oracle.com> wrote:
Hi Sean,

Updated the repository used in webrev from jdk8-tl to�http://hg.openjdk.java.net/jdk8/awt/jdk �.


I have the following comments about the fix:

1\. When right-to-left orientation the Recent swatches inverts right and left button.
2\. Could you please don't use package visibility when fileds/methods/inner classes can be private (e.g. field mainSwatchKeyListener)
3\. I think you should uninstall the introduced listeners in the DefaultSwatchChooserPanel#uninstallChooserPanel method
4\. Why new listeners are Serializable?
5\. I recommend to use if condition instead of switch/case blocks with one branch
6\. Could you please rename selrow (and similar variables) into selRow
7\. Can we use Component#isFocusOwner instead of supporting new variable showcursor?
8\. Could you please follow our code guide lines (spaces etc)

Regards, Pavel



---------- Forwarded message ----------
From: Sean Chou <zhouyx@linux.vnet.ibm.com>
Date: Thu, Aug 9, 2012 at 3:29 PM
Subject: Add keyboard accessibility to JColorChooser swatch
To: swing-dev@openjdk.java.net


Hi all,

� � This is a patch to add keyboard�accessibility to JColorChooser swatch, so when using TAB, the focus can move to�SwatchPanel, choose color with arrow keys and select color with space bar.�

In current implementation, it is not able to move the focus to�SwatchPanel with TAB, this can be seen in SwingSet2 demo.�
Steps:�
1\. run SwingSet2 demo�
2\. click on JColorChooser demo
3\. click Background button and Swatches panel appears.
4\. Press Tab key � � => � focus moves to OK button as shown in this image�http://cr.openjdk.java.net/\~zhouyx/OJDK-61/screenshot\_1.png

With this patch, in step4, focus moves to�SwatchPanel, as shown here�http://cr.openjdk.java.net/\~zhouyx/OJDK-61/screenshot\_2.png
Then, arrow keys can be used to choose color and select color by space bar.


Please take a look.


\--
Best Regards,
Sean Chou




\--
Best Regards,
Sean Chou






--
Best Regards,
Sean Chou