<Swing Dev> Fwd: Add keyboard accessibility to JColorChooser swatch
Pavel Porvatov
pavel.porvatov at oracle.com
Wed Sep 12 08:30:02 UTC 2012
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/
> <http://cr.openjdk.java.net/%7Ezhouyx/OJDK-61/webrev.07/> .
>
> Please take a look.
>
>
> On Tue, Sep 11, 2012 at 12:57 AM, Pavel Porvatov
> <pavel.porvatov at oracle.com <mailto: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/
>> <http://cr.openjdk.java.net/%7Ezhouyx/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 <mailto: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
>>> <mailto: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/
>>>> <http://cr.openjdk.java.net/%7Ezhouyx/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
>>>> <http://cr.openjdk.java.net/%7Ezhouyx/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
>>>> <mailto: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/
>>>>> <http://cr.openjdk.java.net/%7Ezhouyx/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
>>>>> <http://cr.openjdk.java.net/%7Ezhouyx/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
>>>>> <mailto: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/
>>>>>> <http://cr.openjdk.java.net/%7Ezhouyx/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
>>>>>> <mailto: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
>>>>>> <mailto: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
>>>>>> <http://cr.openjdk.java.net/%7Ezhouyx/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
>>>>>> <http://cr.openjdk.java.net/%7Ezhouyx/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/
>>>>>> <http://cr.openjdk.java.net/%7Ezhouyx/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
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/swing-dev/attachments/20120912/4a5ce266/attachment.html>
More information about the swing-dev
mailing list