<Swing Dev> Fwd: Add keyboard accessibility to JColorChooser swatch
Alexander Scherbatiy
alexandr.scherbatiy at oracle.com
Thu Sep 20 13:43:10 UTC 2012
On 9/20/2012 11:40 AM, Sean Chou wrote:
> Hi Alexander,
>
> May I find some one to commit it ?
I have pushed your fix to the JDK 8:
http://hg.openjdk.java.net/jdk8/awt/jdk/rev/9aa37a39cf39
Thanks,
Alexandr.
> On Tue, Sep 18, 2012 at 10:34 PM, Alexander Scherbatiy
> <alexandr.scherbatiy at oracle.com> wrote:
>> Hi Sean,
>>
>> The fix and test look good for me.
>>
>> I think that there are areas for improvements like initialization the selRow
>> and selCol variables to the color coordinates defined in the constructor
>> (new JColorChooser(Color.GREEN)). However it is not necessary for this fix.
>>
>> Thanks,
>> Alexandr.
>>
>>
>> On 9/17/2012 10:28 AM, Sean Chou wrote:
>>> Hi Pavel,
>>>
>>> The new webrev is at:
>>> http://cr.openjdk.java.net/~zhouyx/OJDK-61/webrev.09/ .
>>> The missed sync is added.
>>>
>>>
>>> On Sat, Sep 15, 2012 at 12:41 AM, Pavel Porvatov
>>> <pavel.porvatov at oracle.com> wrote:
>>>> Hi Sean,
>>>>
>>>> Hi Pavel,
>>>>
>>>> Thank you very much for testing on MacOS, I have windows and linux only
>>>> for now.
>>>>
>>>> The new patch is at:
>>>> http://cr.openjdk.java.net/~zhouyx/OJDK-61/webrev.08/ .
>>>>
>>>> Your version of the test is wrong. Why did you remove realSync before
>>>> hitting keys? It's possible that invokeLater with
>>>> recentSwatchPanel.requestFocusInWindow will be executed AFTER all keys are
>>>> pressed and the test will fail... Moreover requestFocusInWindow invocation
>>>> doesn't guarantee that recentSwatchPanel has focus because of javadoc:
>>>> * Developers must never
>>>> * assume that this Component is the focus owner until this
>>>> * Component receives a FOCUS_GAINED event.
>>>>
>>>> Regards, Pavel.
>>>>
>>>>
>>>> Please have a look.
>>>>
>>>> On Wed, Sep 12, 2012 at 4:30 PM, Pavel
>>>> Porvatov<pavel.porvatov at oracle.com> wrote:
>>>>> Hi Sean,
>>>>>
>>>>> Unfortunately the test fails on MacOS. That's because JTabbedPane
>>>>> doesn't have focus after window is visible under AquaLAF. You can modify
>>>>> your test like this:
>>>>>
>>>>>
>>>>> SwingUtilities.invokeLater(new Runnable() {
>>>>> @Override
>>>>> public void run() {
>>>>> selectedColor = colorChooser.getColor();
>>>>>
>>>>> Component recentSwatchPanel =
>>>>> Util.findSubComponent(colorChooser, "RecentSwatchPanel");
>>>>>
>>>>> if (recentSwatchPanel == null) {
>>>>> throw new RuntimeException("RecentSwatchPanel not
>>>>> found");
>>>>> }
>>>>>
>>>>> recentSwatchPanel.requestFocusInWindow();
>>>>> }
>>>>> });
>>>>>
>>>>> toolkit.realSync();
>>>>>
>>>>> // Tab to move the focus to MainSwatch
>>>>> Util.hitKeys(robot, KeyEvent.VK_SHIFT, KeyEvent.VK_TAB);
>>>>>
>>>>> // Select the color on right
>>>>> Util.hitKeys(robot, KeyEvent.VK_RIGHT);
>>>>> Util.hitKeys(robot, KeyEvent.VK_RIGHT);
>>>>> Util.hitKeys(robot, KeyEvent.VK_SPACE);
>>>>>
>>>>> I checked, it works on Mac.
>>>>>
>>>>> After the changes
>>>>> 1. There is no need to declare selectedColor as a volatile field
>>>>> 2. Robot and Toolkit are used in one method and can be converted into
>>>>> local variables
>>>>> 3. The click method can be removed
>>>>>
>>>>> Regards, Pavel
>>>>>
>>>>>
>>>>> Hi Pavel,
>>>>>
>>>>> Thanks for the tip, it is modified.
>>>>>
>>>>> The new webrev is at:
>>>>> http://cr.openjdk.java.net/~zhouyx/OJDK-61/webrev.07/ .
>>>>>
>>>>> Please take a look.
>>>>>
>>>>>
>>>>> On Tue, Sep 11, 2012 at 12:57 AM, Pavel
>>>>> Porvatov<pavel.porvatov at oracle.com> wrote:
>>>>>> Hi Sean,
>>>>>>
>>>>>> The fix looks good but you are still using Swing components from
>>>>>> different threads. There is a useful method: regtesthelpres.Util.invokeOnEDT
>>>>>>
>>>>>> Hi Pavel,
>>>>>>
>>>>>> I made a new patch, it is at
>>>>>> http://cr.openjdk.java.net/~zhouyx/OJDK-61/webrev.06/ .
>>>>>> Please take a look.
>>>>>>
>>>>>> About orientation, I also set mainSwatch's top-right corner as (0, 0),
>>>>>> so the top-right color is white in right-to-left orientation.
>>>>>>
>>>>>> That ok.
>>>>>>
>>>>>> Regards, Pavel
>>>>>>
>>>>>>
>>>>>>
>>>>>> On Fri, Sep 7, 2012 at 10:15 PM, Pavel
>>>>>> Porvatov<pavel.porvatov at oracle.com> wrote:
>>>>>>> HI Sean,
>>>>>>>
>>>>>>> Hi Pavel,
>>>>>>>
>>>>>>> Thanks for the comments. I have a question about right-to-left
>>>>>>> orientation.
>>>>>>>
>>>>>>> As the effect of "this instanceof RecentSwatchPanel", with first
>>>>>>> version of my patch is applied, if left key is pressed when choosing color
>>>>>>> on recentSwatch, it move the focus to the color on right side; but when
>>>>>>> choose color on mainSwatch, left is still left. And when the focus is on the
>>>>>>> tab, left key also move the focus to the tab on left side, as well as
>>>>>>> controls on other tabs. I only found this left-right reverse on
>>>>>>> RecentSwatchPanel, I removed the code related. Can you give some
>>>>>>> information about what is the right behavior? Is it "default on top-right,
>>>>>>> grows from right-to-left, but left key still moves left" ?
>>>>>>>
>>>>>>> In right-to-left orientation all components are placed from right to
>>>>>>> left (as you can observe on JColorChooser for example), but the keys LEFT
>>>>>>> and RIGHT work as always.
>>>>>>>
>>>>>>> Regards, Pavel
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> On Wed, Sep 5, 2012 at 9:32 PM, Pavel
>>>>>>> Porvatov<pavel.porvatov at oracle.com> wrote:
>>>>>>>> Hi Sean,
>>>>>>>>
>>>>>>>> Still have some comments
>>>>>>>>
>>>>>>>> 1. Could you please replace requestFocus by requestFocusInWindow?
>>>>>>>> (read javadoc for the details)
>>>>>>>>
>>>>>>>> 2. What's the reason to change
>>>>>>>> - setSelectedColor(color);
>>>>>>>> + getColorSelectionModel().setSelectedColor(color);
>>>>>>>> ?
>>>>>>>> You removed null checking...
>>>>>>>>
>>>>>>>> In new code (e.g. in MainSwatchKeyListener) you could use
>>>>>>>> setSelectedColor(color) as well.
>>>>>>>>
>>>>>>>> 3. This code looks strange:
>>>>>>>> public boolean isFocusTraversable() {
>>>>>>>> - return false;
>>>>>>>> + return true;
>>>>>>>> }
>>>>>>>>
>>>>>>>> I believe we can remove this "hack" at all and just use
>>>>>>>> setFocusable(true) in constructor...
>>>>>>>>
>>>>>>>> 4. You are still using incorrect code formatting, e.g. in
>>>>>>>> "getColorForCell(column,row);"
>>>>>>>>
>>>>>>>> 5. You removed right-to-left orientation from the RecentSwatchPanel.
>>>>>>>> Could you please explain that changes?
>>>>>>>>
>>>>>>>> 6. About right-to-left orientation: I believe the default corner of
>>>>>>>> the marker should be upper-right, but not upper-left. HOME and END keys
>>>>>>>> should work according to orientation as well...
>>>>>>>>
>>>>>>>> The test should be fixed as well:
>>>>>>>>
>>>>>>>> a. What does line 75 mean?
>>>>>>>> 75 if (true) return ;
>>>>>>>> b. Why Swing fields are volatile? Swing components are not thread
>>>>>>>> safe and should be accessed only from the EDT
>>>>>>>> c. What's the reason to have click(int...keys) instead of click(int
>>>>>>>> key)?
>>>>>>>> d. Constants should be in UPPER_CASE
>>>>>>>>
>>>>>>>> Regards, Pavel
>>>>>>>>
>>>>>>>> Hi Pavel,
>>>>>>>>
>>>>>>>> I uploaded http://cr.openjdk.java.net/~zhouyx/OJDK-61/webrev.05/
>>>>>>>> . And reported http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=7194184 .
>>>>>>>>
>>>>>>>> Some details about update:
>>>>>>>> 1. A testcase is added in this webrev, but it checks keyboard
>>>>>>>> accessibility only. I tried to check right-to-left orientation with the
>>>>>>>> testcase attached, but it doesn't work well on windows.
>>>>>>>>
>>>>>>>> 2. " g.setXORMode(Color.WHITE); g.setColor(Color.BLACK); " is not
>>>>>>>> used because the mark is not obvious for light blue-pink area and dark green
>>>>>>>> area, as this image:
>>>>>>>> http://cr.openjdk.java.net/~zhouyx/OJDK-61/screenshot_4.png .
>>>>>>>>
>>>>>>>> Please take a look.
>>>>>>>>
>>>>>>>> On Wed, Aug 22, 2012 at 9:16 PM, Pavel
>>>>>>>> Porvatov<pavel.porvatov at oracle.com> wrote:
>>>>>>>>> Hi Sean,
>>>>>>>>>
>>>>>>>>> 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?
>>>>>>>>>
>>>>>>>>> I attached the test. It will be useful to add some tests to your
>>>>>>>>> change (e.g. show JColorChooser with right-to-left orientation, pressing
>>>>>>>>> some keys by awt Robot and checking the result)
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> 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" .
>>>>>>>>>
>>>>>>>>> You are absolutely right about serialization. But if you take a look
>>>>>>>>> at components serialization/deserialization you will see that BEFORE
>>>>>>>>> serialization javax.swing.plaf.ComponentUI#uninstallUI is invoked and AFTER
>>>>>>>>> deserialization javax.swing.plaf.ComponentUI#installUI is invoked. So
>>>>>>>>> serialization is not broken when new listeners added and removed correctly
>>>>>>>>> with instalUI/uninstallUI methods.
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> Please take a look again, thanks.
>>>>>>>>>
>>>>>>>>> Some additional comments below:
>>>>>>>>> 1. You should assign null to mainSwatchKeyListener and
>>>>>>>>> recentSwatchKeyListener in the uninstallChooserPanel method
>>>>>>>>> 2. Do we have bug in bugs database for your fix? If no, could you
>>>>>>>>> please file a bug with correspondent description. Use that bug number as a
>>>>>>>>> folder name for the webrev, please
>>>>>>>>> 3. The code can be a little bit optimized. There is no need to call
>>>>>>>>> repaint every time in code like this:
>>>>>>>>> + if (selRow> 0) {
>>>>>>>>> + selRow--;
>>>>>>>>> + }
>>>>>>>>> + repaint();
>>>>>>>>>
>>>>>>>>> I'd prefer
>>>>>>>>> + if (selRow> 0) {
>>>>>>>>> + selRow--;
>>>>>>>>> + repaint();
>>>>>>>>> + }
>>>>>>>>> In case KeyEvent.VK_HOME and KeyEvent.VK_END such optimization
>>>>>>>>> doesn't look reasonable for me...
>>>>>>>>> 4. Could you please follow our code guide lines (spaces etc).
>>>>>>>>>
>>>>>>>>> http://www.oracle.com/technetwork/java/javase/documentation/codeconventions-141388.html
>>>>>>>>> 5. I have some concerns about selection mark in paintComponent:
>>>>>>>>> a. On light swatches (e.g. on white) the mark looks like it shifted
>>>>>>>>> on 1 pixel
>>>>>>>>> b. The color selection looks tricky. Does the following code from
>>>>>>>>> DiagramComponent#paintComponent works
>>>>>>>>> g.setXORMode(Color.WHITE);
>>>>>>>>> g.setColor(Color.BLACK);
>>>>>>>>> ?
>>>>>>>>>
>>>>>>>>> Regards, Pavel
>>>>>>>>>
>>>>>>>>> On Thu, Aug 16, 2012 at 10:00 PM, Pavel
>>>>>>>>> Porvatov<pavel.porvatov at oracle.com> wrote:
>>>>>>>>>> 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 at linux.vnet.ibm.com>
>>>>>>>>>> Date: Thu, Aug 9, 2012 at 3:29 PM
>>>>>>>>>> Subject:<Swing Dev> Add keyboard accessibility to JColorChooser
>>>>>>>>>> swatch
>>>>>>>>>> To: swing-dev at 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
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>> --
>>>>>>>>> Best Regards,
>>>>>>>>> Sean Chou
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> import javax.swing.*;
>>>>>>>>> import java.awt.*;
>>>>>>>>>
>>>>>>>>> public class JColorChooserTest {
>>>>>>>>> public static void main(String[] args) {
>>>>>>>>> SwingUtilities.invokeLater(new Runnable() {
>>>>>>>>> @Override
>>>>>>>>> public void run() {
>>>>>>>>> JFrame frame = new JFrame();
>>>>>>>>>
>>>>>>>>> JColorChooser chooser = new JColorChooser();
>>>>>>>>>
>>>>>>>>> chooser.setComponentOrientation(ComponentOrientation.RIGHT_TO_LEFT);
>>>>>>>>> frame.add(chooser);
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> frame.setDefaultCloseOperation(JFrame.EXIT_ON_CLOSE);
>>>>>>>>> frame.setSize(600, 400);
>>>>>>>>> frame.setVisible(true);
>>>>>>>>> }
>>>>>>>>> });
>>>>>>>>> }
>>>>>>>>> }
>>>>>>>>>
>>>>>>>>
>>>>>>>> --
>>>>>>>> Best Regards,
>>>>>>>> Sean Chou
>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>> --
>>>>>>> Best Regards,
>>>>>>> Sean Chou
>>>>>>>
>>>>>>>
>>>>>>
>>>>>> --
>>>>>> Best Regards,
>>>>>> Sean Chou
>>>>>>
>>>>>>
>>>>>
>>>>> --
>>>>> Best Regards,
>>>>> Sean Chou
>>>>>
>>>>>
>>>>
>>>> --
>>>> Best Regards,
>>>> Sean Chou
>>>>
>>>>
>>>
>>> --
>>> Best Regards,
>>> Sean Chou
>>
>
>
More information about the swing-dev
mailing list