Accessibility inner classes in AccessibleJTable do not stick to AccessibleComponent.getLocationOnScreen api (original) (raw)

Pavel Porvatov pavel.porvatov at oracle.com
Fri Aug 17 11:13:27 UTC 2012

Hi Frank,

Hi Pavel, Thanks for your review again. Another review package incorporating your comments is @ http://cr.openjdk.java.net/~dingxmin/7188612/webrev.01 Could you please take a look at it ? The fix looks good for me.

Regards, Pavel

Thanks, Frank On 8/16/2012 11:02 PM, Pavel Porvatov wrote: Hi Frank,

Hi Pavel, Thanks for your comments from which I learned a lot. A new review patch is created by incorporating your comments @

http://cr.openjdk.java.net/~dingxmin/7188612/webrev.00/ Could you please review it again? I have few comments about the last webrev: 1. There is no need to re-throw exceptions via exThrownInEDT. You can just throw RuntimeException if the test fails. It's ok to throw RuntimeException on the EDT, jtreg will fail in that case 2. Can we join two SwingUtilities.invokeAndWait into one? 3. Comments 96 // cast to interface AccessibleComponent to call getLocationOnScreen() and 109 // cast to interface AccessibleComponent to call getLocationOnScreen() look unnecessary Other code looks good for me Regards, Pavel Best regards, Frank On 8/6/2012 9:54 PM, Pavel Porvatov wrote: Hi Frank,

The fix looks good but I have several comments for the test: 1. The Swing components should be created and accessed only from EDT thread. Therefore the assertGetLocation method should be on EDT as well 2. I'd remove lines 59-61 3. The lines 79 // Display the window. 80 frame.setVisible(true); 81 // make the frame invisible to test getLocationOnScreen() of 82 // JTable$AccessibleJTable$AccessibleJTableHeaderCell 83 // and JTable$AccessibleJTable$AccessibleJTableCell 84 frame.setVisible(false); don't guaranty that the frame becomes visible for awhile. You should use SunToolkit.realsync for that (take a look at other tests). Is it really necessary for the test to show and hide the frame? 4. Why don't you cast getAccessibleContext into AccessibleTable in one line, but wrote six lines for that: 88 AccessibleContext context = (AccessibleContext) table 89 .getAccessibleContext(); 90 // To get JTable$AccessibleJTable$AccessibleJTableHeaderCell instance, 91 // we need to first 92 // convert the context var to type AccessibleTable 93 AccessibleTable accessibleTable = (AccessibleTable) context; 5. Could you please remove useless comments like: 79 // Display the window. 80 frame.setVisible(true); 90 // To get JTable$AccessibleJTable$AccessibleJTableHeaderCell instance, 91 // we need to first 92 // convert the context var to type AccessibleTable 94 // and then get JTable$AccessibleJTable$AccessibleTableHeader etc. Only comments like 81 // make the frame invisible to test getLocationOnScreen() of 82 // JTable$AccessibleJTable$AccessibleJTableHeaderCell 83 // and JTable$AccessibleJTable$AccessibleJTableCell 100 // getLocation() must be null according to its javadoc and no exception 101 // is thrown looks reasonable for me in the provided test Regards, Pavel

Hi guys According to javadoc of AccessibleComponent.getLocationOnScreen, it returns null instead of throwing IllegalComponentStateException when the corresponding GUI object is not visible. However, two accessibility inner classes of JTable, JTable$AccessibleJTable$AccessibleJTableCell and Table$AccessibleJTable$AccessibleJTableHeaderCell implementing AccessibleComponent throw IllegalComponentStateException when the corresponding JTable is not visible. This behavior conflicts with the spec. I have submitted a bug in sunbug system reporting the issue, and received two acknowledgement emails (bug id 7188613 and 7188612, don't know why there are two?). I created a patch to fix the issue with a jtreg test case. Both the patch and the test are uploaded into the following space. http://cr.openjdk.java.net/~youdwei/ojdk-491/webrev.02/ Would anyone help to review the patch? Best regards, Frank

More information about the swing-dev mailing list