<Swing Dev> Fwd: Add keyboard accessibility to JColorChooser swatch
Sean Chou
zhouyx at linux.vnet.ibm.com
Fri Sep 14 06:12:44 UTC 2012
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/ .
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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/swing-dev/attachments/20120914/4f0c4fa4/attachment.html>
More information about the swing-dev
mailing list