354447 - Create mozTextAccessible class for easier text handling

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Håkan Waara]

In order to support more text-specific attributes (both for editable textfields, and uneditable text nodes), there needs to be a new class. 

We can also cut down on the number of mozAccessibles created, by not creating text leafs (like ATK does).

Comment 1

[Håkan Waara]

Attachment: mozTextAccessible, new files

I keep adding new things, but I think I eventually have to get this in. Sorry for creating so huge patches.

This is a new mozTextAccessible class. These objects are created for all textfields or static texts, and are specialized for text handling.

Overview:

* The attributes the text objects support are according to Apple's spec here http://tinyurl.com/ylx3cr
* It sends out an event when its value changes
* Supports setting text, and getting info such as getting the text length, getting the selected text, etc.

The patch requires other patches, that I will add to other bugs, to make it more coherent and easier to understand.

Comment 2

[alexander :surkov (:asurkov)]

Comment on attachment 243326 [details] [diff] [review]
mozTextAccessible, new files


>+  if ((self = [super initWithAccessible:accessible])) {
>+    CallQueryInterface(accessible, &mGeckoTextAccessible);
>+    CallQueryInterface(accessible, &mGeckoEditableTextAccessible);
>+  }
>+  return self;

Do CallQueryInterface call AddRefs()?

>+
>+- (BOOL)accessibilityIsIgnored
>+{
>+  return NO;
>+}

Again, It' not clear with me why NO.

>+- (NSArray*)accessibilityAttributeNames
>+{
>+  static NSArray *supportedAttributes = nil;
>+  if (!supportedAttributes) {
>+    // standard attributes that are shared and supported by all generic elements.
>+    supportedAttributes = [[NSArray alloc] initWithObjects:NSAccessibilityParentAttribute, // required
>+                                                           NSAccessibilityRoleAttribute,   // required
>+                                                           NSAccessibilityTitleAttribute,
>+                                                           NSAccessibilityValueAttribute, // required
>+                                                           NSAccessibilitySubroleAttribute,
>+                                                           NSAccessibilityRoleDescriptionAttribute,
>+                                                           NSAccessibilityPositionAttribute, // required
>+                                                           NSAccessibilitySizeAttribute, // required
>+                                                           NSAccessibilityWindowAttribute, // required
>+                                                           NSAccessibilityFocusedAttribute, // required
>+                                                           NSAccessibilityEnabledAttribute, // required
>+                                                           kTopLevelUIElementAttribute, // required (on OS X 10.4+)
>+                                                           kInstanceDescriptionAttribute, // required (on OS X 10.4+)
>+                                                           /* text-specific attributes */
>+                                                           NSAccessibilitySelectedTextAttribute, // required
>+                                                           NSAccessibilitySelectedTextRangeAttribute, // required
>+                                                           NSAccessibilityNumberOfCharactersAttribute, // required
>+                                                           // TODO: NSAccessibilityVisibleCharacterRangeAttribute, // required
>+                                                           nil];

Again, can you use this construction:
  attributes = [[[super accessibilityAttributeNames] arrayByAddingObject:NSAccessibilitySelectedTextAttribute] retain];
to reuse mozAccessible::accessibilityAttributeNames?

Also, it looks like you forgot about "Insertion Point Line Number" attribute.

>+- (BOOL)accessibilityIsAttributeSettable:(NSString*)attribute
>+{
>+  if ([attribute isEqualToString:NSAccessibilityValueAttribute])
>+    return [self isReadOnly];
>+  
>+  return [super accessibilityIsAttributeSettable:attribute];
>+}

Please give me a link where I can see what attributes are setable. It's not clear for me why NSAccessibilityValueAttribute is setable only.

>+- (void)accessibilitySetValue:(id)value forAttribute:(NSString *)attribute
>+{
>+  if ([attribute isEqualToString:NSAccessibilityValueAttribute]) {
>+    if ([value isKindOfClass:[NSString class]])
>+      [self setText:(NSString*)value];
>+  } else
>+    [super accessibilitySetValue:value forAttribute:attribute];
>+}

I guess it's better to test here if attribute is setable by accessibilityIsAttributeSettable method calling instead additional checking in setText() method.

>+- (NSString*)subrole
>+{
>+  // text accessibles have two different subroles in Cocoa: secure textfield (passwords) and search field

nit: use XXX.

>+#pragma mark -
>+
>+- (void)valueDidChange
>+{
>+  NSAccessibilityPostNotification([self accessibleSelf],
>+                                  NSAccessibilityValueChangedNotification);
>+}

I guess valueDidChange and setText is property of editable text. I'd suggest to have additional accessible object for this.

Comment 3

[Håkan Waara]

(In reply to comment #2)
> (From update of attachment 243326 [details] [diff] [review] [edit])
> 
> >+  if ((self = [super initWithAccessible:accessible])) {
> >+    CallQueryInterface(accessible, &mGeckoTextAccessible);
> >+    CallQueryInterface(accessible, &mGeckoEditableTextAccessible);
> >+  }
> >+  return self;
> 
> Do CallQueryInterface call AddRefs()?

If it QIs it always addrefs, see the impl of any QueryInterface, and you'll see NS_ADDREF :)

> >+
> >+- (BOOL)accessibilityIsIgnored
> >+{
> >+  return NO;
> >+}
> 
> Again, It' not clear with me why NO.
> 
> >+- (NSArray*)accessibilityAttributeNames
> >+{
> >+  static NSArray *supportedAttributes = nil;
> >+  if (!supportedAttributes) {
> >+    // standard attributes that are shared and supported by all generic elements.
> >+    supportedAttributes = [[NSArray alloc] initWithObjects:NSAccessibilityParentAttribute, // required
> >+                                                           NSAccessibilityRoleAttribute,   // required
> >+                                                           NSAccessibilityTitleAttribute,
> >+                                                           NSAccessibilityValueAttribute, // required
> >+                                                           NSAccessibilitySubroleAttribute,
> >+                                                           NSAccessibilityRoleDescriptionAttribute,
> >+                                                           NSAccessibilityPositionAttribute, // required
> >+                                                           NSAccessibilitySizeAttribute, // required
> >+                                                           NSAccessibilityWindowAttribute, // required
> >+                                                           NSAccessibilityFocusedAttribute, // required
> >+                                                           NSAccessibilityEnabledAttribute, // required
> >+                                                           kTopLevelUIElementAttribute, // required (on OS X 10.4+)
> >+                                                           kInstanceDescriptionAttribute, // required (on OS X 10.4+)
> >+                                                           /* text-specific attributes */
> >+                                                           NSAccessibilitySelectedTextAttribute, // required
> >+                                                           NSAccessibilitySelectedTextRangeAttribute, // required
> >+                                                           NSAccessibilityNumberOfCharactersAttribute, // required
> >+                                                           // TODO: NSAccessibilityVisibleCharacterRangeAttribute, // required
> >+                                                           nil];
> 
> Again, can you use this construction:
>   attributes = [[[super accessibilityAttributeNames]
> arrayByAddingObject:NSAccessibilitySelectedTextAttribute] retain];
> to reuse mozAccessible::accessibilityAttributeNames?

Like in the other bug, that method only works if you add objects, you can't append a whole array unfortunately.  I've even filed a RFE to apple about this.

> Also, it looks like you forgot about "Insertion Point Line Number" attribute.

Good catch.

> >+- (BOOL)accessibilityIsAttributeSettable:(NSString*)attribute
> >+{
> >+  if ([attribute isEqualToString:NSAccessibilityValueAttribute])
> >+    return [self isReadOnly];
> >+  
> >+  return [super accessibilityIsAttributeSettable:attribute];
> >+}
> 
> Please give me a link where I can see what attributes are setable. It's not
> clear for me why NSAccessibilityValueAttribute is setable only.

file:///dev/null/ ;)

I don't there is docs for this. Right now, we support setting the value.

> 
> >+- (void)accessibilitySetValue:(id)value forAttribute:(NSString *)attribute
> >+{
> >+  if ([attribute isEqualToString:NSAccessibilityValueAttribute]) {
> >+    if ([value isKindOfClass:[NSString class]])
> >+      [self setText:(NSString*)value];
> >+  } else
> >+    [super accessibilitySetValue:value forAttribute:attribute];
> >+}
> 
> I guess it's better to test here if attribute is setable by
> accessibilityIsAttributeSettable method calling instead additional checking in
> setText() method.

A user is required to use isAttributeSettable before trying to set a value, so I can skip the test in setText, good catch.

> 
> >+- (NSString*)subrole
> >+{
> >+  // text accessibles have two different subroles in Cocoa: secure textfield (passwords) and search field
> 
> nit: use XXX.

OK

> 
> >+#pragma mark -
> >+
> >+- (void)valueDidChange
> >+{
> >+  NSAccessibilityPostNotification([self accessibleSelf],
> >+                                  NSAccessibilityValueChangedNotification);
> >+}
> 
> I guess valueDidChange and setText is property of editable text. I'd suggest to
> have additional accessible object for this.
> 

Ok, good point. I think it might help me in the future to have separate static and editable text.

Comment 4

[alexander :surkov (:asurkov)]

Comment on attachment 243326 [details] [diff] [review]
mozTextAccessible, new files

canceling review request because I assume there will new patch.

Comment 5

[Håkan Waara]

Attachment: new mozTextAccessible, v2

I've added some TODO:s, localization decision is pending and I will split mozTextAccessible into mozTextAccessible and mozTextfieldAccessible (editable) in a follow-up bug.

(I have such a huge amount of changes accumulated already that I want to land this, and keep working on a clean tree.)

Comment 6

[Håkan Waara]

Finally checked in to trunk