<Swing Dev> Fwd: Add keyboard accessibility to JColorChooser swatch
Pavel Porvatov
pavel.porvatov at oracle.com
Fri Sep 14 16:41:54 UTC 2012
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/
> <http://cr.openjdk.java.net/%7Ezhouyx/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 <mailto: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/
>> <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
>>
>
>
>
>
> --
> Best Regards,
> Sean Chou
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/swing-dev/attachments/20120914/37451731/attachment.html>
More information about the swing-dev
mailing list