Re: RfR JDK-8076554, [macosx] Custom Swing text components need to allow standard accessibility

[Pete Brunet]

Hi Sergey, I already pushed the fix but ...

On 4/20/16 10:20 AM, Sergey Bylokhov wrote:
> Hi, Pete.
> Just a small notes:
>  - It seems that AXTextChangeNotifier() should be created only if "c
> is Accessible".
That's true.  I added a comment to 8154507/8 and will fix that with one
of those patches.
>  - I am not sure, but when we clear these listeners(which were added
> to AccessibilityEventMonitor), or it is not necessary?
I didn't see a removal mechanism in the current code so I didn't do
that.  If VO is not active I assume the allocations won't happen.  If
someone start/stops VO I agree there would be a unused code in memory,
but I suspect there is no means to unallocate/disable all the a11y
support.  I will check at Apple to see if anyone knows.  Other than for
testers this would be an unusual scenario.
>  - Is it possible that "AccessibleContext.ACCESSIBLE_TEXT_PROPERTY"
> will be fired by JProgressBar or JSlider, if yes then we will get
> double notification(one from AXTextChangeNotifier and one from
> AXProgressChangeNotifier).
I'll need to look into this more when I work on 8154507/8.  I added your
comments to those bugs.

Pete
>
> On 19.04.16 22:16, Phil Race wrote:
>> +1
>>
>> -phil.
>>
>> On 04/19/2016 12:05 PM, Pete Brunet wrote:
>>> New patch: http://cr.openjdk.java.net/~ptbrunet/JDK-8076554/webrev.03/
>>>
>>> Changes:
>>> - removed _AccessibleState
>>> - removed wildcard imports.
>>>
>>> Is this ready to push?
>>>
>>> Pete
>>>
>>> On 4/19/16 12:26 PM, Phil Race wrote:
 oh one nit picky thing - can we get rid of the wild card imports ?

 -phil.

 On 04/19/2016 10:21 AM, Phil Race wrote:
> Which seems to imply the _AccessibleState subclass has been obsolete
> since 1.5
> Although when these tools were unbundled it perhaps could not assume
> 1.5.
>
> The rest of the changes look fine and it is a lot simpler than the
> v0 ..
>
> -phil.
>
> On 04/19/2016 08:54 AM, Pete Brunet wrote:
>> Thanks for noticing that Alexandr.  I see this state was added in
>> 1.5 and apparently the code from which I borrowed this from was
>> never updated.  I will remove that and open a bug to remove that
>> from the original.  -Pete
>>
>> On 4/19/16 7:39 AM, Alexander Scherbatiy wrote:
>>> On 19/04/16 05:50, Pete Brunet wrote:
 Please review the following patch.

 Bug:https://bugs.openjdk.java.net/browse/JDK-8076554
 Patch:http://cr.openjdk.java.net/~ptbrunet/JDK-8076554/webrev.02/

 The problem is that the code is currently hardcoded to only
 support text
 accessibility for JTextComponent using the DocumentListener,
 CaretListener interfaces.  This eliminates the ability to provide
 accessibility for a custom text components.  The resolution is
 to remove
 the JTextComponent gate and use the PropertyChangeListener
 listener
 interface with its ACCESSIBLE_CARET_PROPERTY and
 ACCESSIBLE_TEXT_PROPERTY properties.
>>>   The AccessibleState class already has the MANAGES_DESCENDANTS
>>> constant. What is purpose to add a constant with the same name to
>>> the _AccessibleState?
>>>
>>>   Thanks,
>>>   Alexandr.
 I tested this with the two test cases which can be found at
 http://cr.openjdk.java.net/~ptbrunet/JDK-8076554/test/
 running them side by side with VoiceOver.

 Note that I will fix the similar JProgressBar and JSlider
 restrictions
 in JDK-8154507 and JDK-8154508 respectively.

 Pete


>>>
>>
>

>>>
>>
>
>

Re: RfR JDK-8076554, [macosx] Custom Swing text components need to allow standard accessibility

[Alexander Scherbatiy]

The fix looks good to me.

Thanks,
Alexandr.

On 20/04/16 01:52, Pete Brunet wrote:

p.s. JPRT ran OK.

Still need one more +1.

On 4/19/16 2:16 PM, Phil Race wrote:

+1

-phil.

On 04/19/2016 12:05 PM, Pete Brunet wrote:

New patch: http://cr.openjdk.java.net/~ptbrunet/JDK-8076554/webrev.03/

Changes:
- removed _AccessibleState
- removed wildcard imports.

Is this ready to push?

Pete

On 4/19/16 12:26 PM, Phil Race wrote:

oh one nit picky thing - can we get rid of the wild card imports ?

-phil.

On 04/19/2016 10:21 AM, Phil Race wrote:
Which seems to imply the _AccessibleState subclass has been 
obsolete since 1.5
Although when these tools were unbundled it perhaps could not 
assume 1.5.


The rest of the changes look fine and it is a lot simpler than the 
v0 ..


-phil.

On 04/19/2016 08:54 AM, Pete Brunet wrote:
Thanks for noticing that Alexandr.  I see this state was added in 
1.5 and apparently the code from which I borrowed this from was 
never updated.  I will remove that and open a bug to remove that 
from the original. -Pete


On 4/19/16 7:39 AM, Alexander Scherbatiy wrote:

On 19/04/16 05:50, Pete Brunet wrote:

Please review the following patch.

Bug:https://bugs.openjdk.java.net/browse/JDK-8076554
Patch:http://cr.openjdk.java.net/~ptbrunet/JDK-8076554/webrev.02/

The problem is that the code is currently hardcoded to only support text
accessibility for JTextComponent using the DocumentListener,
CaretListener interfaces.  This eliminates the ability to provide
accessibility for a custom text components.  The resolution is to remove
the JTextComponent gate and use the PropertyChangeListener listener
interface with its ACCESSIBLE_CARET_PROPERTY and
ACCESSIBLE_TEXT_PROPERTY properties.
  The AccessibleState class already has the MANAGES_DESCENDANTS 
constant. What is purpose to add a constant with the same name 
to the _AccessibleState?


  Thanks,
  Alexandr.

I tested this with the two test cases which can be found at
http://cr.openjdk.java.net/~ptbrunet/JDK-8076554/test/
running them side by side with VoiceOver.

Note that I will fix the similar JProgressBar and JSlider restrictions
in JDK-8154507 and JDK-8154508 respectively.

Pete

Re: [9] Review request for 8154431: Allow source and target based validation for the focus transfer between two JComponents.

[Semyon Sadetsky]

On 4/19/2016 10:41 PM, Phil Race wrote:

On 04/19/2016 11:05 AM, Semyon Sadetsky wrote:

On 4/19/2016 7:47 PM, Phil Race wrote:

Hi,

You are deprecating shouldYieldFocus(JComponent) and yet this class 
directly uses it.

Is this deprecation really the right thing to do ?
Why is this not correct? There are plenty examples in JDK: 
Component#setVisible() & Component#show(), Component#transferFocus() 
& Component#nextFocus(), etc...

This is necessary for backward compatibility.


My question is why deprecate it ?
I can repeat my reply to your anther e-mail in this thread if you did 
not noticed it.


> I deprecated shouldYieldFocus(JCopmponent) and added 
shouldYieldFocus(JCopmponent, JComponent). I cannot simply remove the 
first because
> it's public and can be called from user code. But it's not used in 
JDK code outside this class anymore only internally for compatibility.
> I can remove the @Depricated annotation but still cannot get why do 
we need to keep two overloaded shouldYieldFocus methods if only one is
>  supposed to be used? That may confuse user which one of them to use, 
so @Depricated is a hint.


So far as I can see unless some one wants to over-ride verifyTarget() 
they are

fine to continue over-riding this method and ignore the new one.
Yes, users may continue to use their old code after the fix. No changes 
required on user side. It seems that is what is the backward 
compatibility means.


Leaving aside the merits of those previous changes, and at least one 
of those I think was dubious,
all you have done is at an @Deprecated annotation. There is no 
@deprecated javadoc tag, and you
have left doc which says when this method is called. In fact that doc 
is now very misleading.

It is not called. You call the new one.
Agreed. I will add @depricated to the method javadoc and ask user do not 
use it for the newly written code.



BTW I see you mis-spell over-ridden as overriden in verifyTarget(..)
Just stats from JDK: 5 usages in comments of "over-riden" and more then 
300 "overriden"... Google Translate was spoiled :)


--Semyon




The new over-loaded shouldYieldFocus() is perhaps not much more than 
a utility.

And the doc says "calls verify(input)" which seems odd since you do not
directly call it. And you are just describing what the default 
implementation

does aren't you ?
This is also necessary for compatibility. There may be 
implementations of the InputVerify where the shouldYieldFocus() is 
overloaded since it is public (that was initial design mistake, I 
suppose). At the same time the shouldYieldFocus() is the entry point 
that plugs InputVerify into the JComponent.
But you are correct "calls verify(input)" is not precisely describes 
what happens in this method. Maybe just make it more indirect, like 
"validate the source and the target components..."?


Now I am rather confused. You make it sound like verify() not 
shouldYieldFocus() is all the public

API should have contained, but you are adding a public over-load of it.
So why does this new method need to be public ? All you need is 
verifyTarget(), dont you ?


-phil.




--Semyon


-phil.

On 04/19/2016 04:40 AM, Semyon Sadetsky wrote:

Hello,

Please review fix for JDK9:
bug: https://bugs.openjdk.java.net/browse/JDK-8154431
webrev: http://cr.openjdk.java.net/~ssadetsky/8154431/webrev.00/

The thing is the Swing validation doesn't allow to validate state 
of the target component of input focus transfer operation.
To support that the fix adds two new methods 
verifyTarget(JComponent target) and shouldYieldFocus(JComponent 
source, JComponent target) to the javax.swing.InputVerifier class, 
and its old shouldYieldFocus(JComponent input) is marked as 
deprecated.

The solution guaranties full compatibility with previous JDK versions.

--Semyon

Re: [9] Review request for 8154431: Allow source and target based validation for the focus transfer between two JComponents.

[Phil Race]

On 04/19/2016 11:05 AM, Semyon Sadetsky wrote:

On 4/19/2016 7:47 PM, Phil Race wrote:

Hi,

You are deprecating shouldYieldFocus(JComponent) and yet this class 
directly uses it.

Is this deprecation really the right thing to do ?
Why is this not correct? There are plenty examples in JDK: 
Component#setVisible() & Component#show(), Component#transferFocus() & 
Component#nextFocus(), etc...

This is necessary for backward compatibility.


My question is why deprecate it ?

So far as I can see unless some one wants to over-ride verifyTarget() 
they are

fine to continue over-riding this method and ignore the new one.

Leaving aside the merits of those previous changes, and at least one of 
those I think was dubious,
all you have done is at an @Deprecated annotation. There is no 
@deprecated javadoc tag, and you
have left doc which says when this method is called. In fact that doc is 
now very misleading.

It is not called. You call the new one.


BTW I see you mis-spell over-ridden as overriden in verifyTarget(..)



The new over-loaded shouldYieldFocus() is perhaps not much more than 
a utility.

And the doc says "calls verify(input)" which seems odd since you do not
directly call it. And you are just describing what the default 
implementation

does aren't you ?
This is also necessary for compatibility. There may be implementations 
of the InputVerify where the shouldYieldFocus() is overloaded since it 
is public (that was initial design mistake, I suppose). At the same 
time the shouldYieldFocus() is the entry point that plugs InputVerify 
into the JComponent.
But you are correct "calls verify(input)" is not precisely describes 
what happens in this method. Maybe just make it more indirect, like 
"validate the source and the target components..."?


Now I am rather confused. You make it sound like verify() not 
shouldYieldFocus() is all the public

API should have contained, but you are adding a public over-load of it.
So why does this new method need to be public ? All you need is 
verifyTarget(), dont you ?


-phil.




--Semyon


-phil.

On 04/19/2016 04:40 AM, Semyon Sadetsky wrote:

Hello,

Please review fix for JDK9:
bug: https://bugs.openjdk.java.net/browse/JDK-8154431
webrev: http://cr.openjdk.java.net/~ssadetsky/8154431/webrev.00/

The thing is the Swing validation doesn't allow to validate state of 
the target component of input focus transfer operation.
To support that the fix adds two new methods verifyTarget(JComponent 
target) and shouldYieldFocus(JComponent source, JComponent target) 
to the javax.swing.InputVerifier class, and its old 
shouldYieldFocus(JComponent input) is marked as deprecated.

The solution guaranties full compatibility with previous JDK versions.

--Semyon

Re: Review Request of 8152492: [macosx swing] double key event actions when using Mac menubar

[Avik Niyogi]

Hi All,
I have added some comments and made some minor tweaks as per the inputs 
received.
Please review my code changes as available at this link below:

http://cr.openjdk.java.net/~aniyogi/8152492/webrev.02/ 


With Regards,
Avik Niyogi

> On 19-Apr-2016, at 6:08 pm, Sergey Bylokhov  
> wrote:
> 
> On 19.04.16 11:46, Avik Niyogi wrote:
>> Hi All,
>> Please review my code changes with inputs received:
> 
> Can you please add some description of your changes, it seems you removed all 
> code which was added by you previously.
> 
>> http://cr.openjdk.java.net/~aniyogi/8152492/webrev.01/ 
>> 
>> 
>> With Regards,
>> Avik Niyogi
>>> On 19-Apr-2016, at 11:47 am, Avik Niyogi >> 
>>> >> wrote:
>>> 
>>> Hi Sergey,
>>> I have tried all these Input sources:
>>> 
>>> I am unable to reproduce the test failure as mentioned by you with
>>> these inputs. Please provide some screenshots and/or details of the
>>> procedure used for this test failure.
>>> Thank you in advance.
>>> 
>>> With Regards,
>>> Avik Niyogi
>>> 
 On 08-Apr-2016, at 8:15 pm, Sergey Bylokhov
  
 >> 
 wrote:
 
 Hi, Avik.
 It seems there is one more bug in this "if".
 Change the input source to "Hindi Dev.."/"Russian.." then this test
 fails anyway(it will produce the double action).
 Moreover if I change the source input to Hindi Transliteration, the
 test sometimes crashed.
 Please double check.
 
 On 06.04.16 13:42, Avik Niyogi wrote:
> Hi All,
> 
> Kindly review the bug fix for JDK 9.
> 
> *Bug:*
> https://bugs.openjdk.java.net/browse/JDK-8152492
> 
> *Webrev:*
> http://cr.openjdk.java.net/~aniyogi/8152492/webrev.00/
> 
> *Issue:*
> For OSX, having there is provision of non-integrated menubar. for this
> case, triggering the menu action by key press triggered double action in
> some cases
> 
> *Cause:*
> Due to the use of Shift modifier being a soft modifier in case of mac.
> For example “Shift + m” is actually “M” and “Shift + ,” is “<“ .
> So such cases can not be used as shortcuts. This needs to be
> accounted for.
> Also, cases where “Cmd + Shift + c” is used as a shortcut, it should not
> morph that to a “Cmd + C” command and should detect c itself.
> Also, “Shift + ↑” has no equivalent soft modification and can be taken
> as a shortcut. This has to be accounted for.
> 
> *Fix:*
> All required edge case scenarios for the soft modification of Shift and
> cases of valid modifiers were taken into account.
> 
> With Regards,
> Avik Niyogi
 
 
 --
 Best regards, Sergey.
>>> 
>> 
> 
> 
> -- 
> Best regards, Sergey.