703770 - Mac Accessibility Verifier errors

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Hubert Figuiere [:hub]]

Attachment: Verifier screenshot.

"Verifying" accessibility in an accessibility enabled build of mozilla-central on MacOS 10.6 give some errors in the "Role Verification" 

See attached screenshot.

This was discovered when working on bug 689105, but I do believe are not the cause of the bug.
They should still be fixed though.

Comment 1

[Hubert Figuiere [:hub]]

Attachment: patch

stashing the patch here for now. it is in my mercurial queue.

Comment 2

[Hubert Figuiere [:hub]]

Attachment: patch

updated patch that had a regression + fixed a comment.

Comment 3

[Marco Zehe (:MarcoZ)]

This may be solving bug 499927 as well.

Comment 4

[alexander :surkov (:asurkov)]

(In reply to Hub Figuiere [:hub] from comment #2)
> Created attachment 575601 [details] [diff] [review] [diff] [details] [review]
> patch
> 
> updated patch that had a regression + fixed a comment.

is it ready for review?

Comment 5

[Hubert Figuiere [:hub]]

(In reply to alexander surkov from comment #4)

> is it ready for review?


Not yet. I want to fix the other problem before. But I wanted to stash it somewhere safe in case I nuke my Mercurial queue by mistake.

Comment 6

[Hubert Figuiere [:hub]]

Attachment: Patch from patch queue

This is the patch.

They might be other similar errors buried deep down.

Comment 7

[alexander :surkov (:asurkov)]

Comment on attachment 579169 [details] [diff] [review]
Patch from patch queue

Review of attachment 579169 [details] [diff] [review]:
-----------------------------------------------------------------

it's nice to see some details why verification fails, i.e. what we do is incorrect rather than generic "Mac Accessibility Verifier errors"

::: accessible/src/mac/mozAccessible.mm
@@ +250,3 @@
>      return [self title];
> +  if ([attribute isEqualToString:NSAccessibilityTitleUIElementAttribute])
> +    return NSAccessibilityUnignoredDescendant(self);

it's rather labelledby relation than descendant

::: accessible/src/mac/mozDocAccessible.mm
@@ +62,5 @@
> +  // if we're expired, we don't support any attributes.
> +  if (mIsExpired)
> +    return [NSArray array];
> +  
> +  static NSArray *generalAttributes = nil;

nit, NSArray*

@@ +64,5 @@
> +    return [NSArray array];
> +  
> +  static NSArray *generalAttributes = nil;
> +  
> +  if (!generalAttributes) {

sort of strange name, why 'attributes' doesn't work?

@@ +70,5 @@
> +    generalAttributes = [[NSArray alloc] initWithObjects:  NSAccessibilityChildrenAttribute, 
> +                         NSAccessibilityParentAttribute,
> +                         NSAccessibilityRoleAttribute,
> +                         NSAccessibilityMainAttribute,  //
> +                         NSAccessibilityMinimizedAttribute,  //

nit: comments?

@@ +80,5 @@
> +                         NSAccessibilityFocusedAttribute,
> +                         NSAccessibilityTitleUIElementAttribute,
> +                         NSAccessibilityDescriptionAttribute,
> +                         NSAccessibilityWindowAttribute,
> +                         nil];

that'd be nice to point which attribute are generic and which are windwo specific (http://developer.apple.com/library/mac/#documentation/Cocoa/Reference/ApplicationKit/Protocols/NSAccessibility_Protocol/Reference/Reference.html). Btw, you don't list them all, any particular reason?

is there a nice way to do attribute inheritance?

@@ +93,5 @@
> +{
> +  NS_OBJC_BEGIN_TRY_ABORT_BLOCK_NIL;
> +  
> +  if ([attribute isEqualToString:NSAccessibilityMainAttribute]) {
> +      return [NSNumber numberWithBool:YES];

nit: wrong indentation, don't parenthesize single if
I wonder if it suites e10s builds

@@ +96,5 @@
> +  if ([attribute isEqualToString:NSAccessibilityMainAttribute]) {
> +      return [NSNumber numberWithBool:YES];
> +  }
> +  if ([attribute isEqualToString:NSAccessibilityMinimizedAttribute]) {
> +    return [NSNumber numberWithBool:NO];

how does it work when window is minimized? btw, do we need to control that?

Comment 8

[Hubert Figuiere [:hub]]

(In reply to alexander :surkov from comment #7)
> 
> @@ +64,5 @@
> > +    return [NSArray array];
> > +  
> > +  static NSArray *generalAttributes = nil;
> > +  
> > +  if (!generalAttributes) {
> 
> sort of strange name, why 'attributes' doesn't work?

Cut and paste from other places. That's how it was written. I can change it.

> 
> @@ +70,5 @@
> > +    generalAttributes = [[NSArray alloc] initWithObjects:  NSAccessibilityChildrenAttribute, 
> > +                         NSAccessibilityParentAttribute,
> > +                         NSAccessibilityRoleAttribute,
> > +                         NSAccessibilityMainAttribute,  //
> > +                         NSAccessibilityMinimizedAttribute,  //
> 
> nit: comments?
> 
> @@ +80,5 @@
> > +                         NSAccessibilityFocusedAttribute,
> > +                         NSAccessibilityTitleUIElementAttribute,
> > +                         NSAccessibilityDescriptionAttribute,
> > +                         NSAccessibilityWindowAttribute,
> > +                         nil];
> 
> that'd be nice to point which attribute are generic and which are windwo
> specific
> (http://developer.apple.com/library/mac/#documentation/Cocoa/Reference/
> ApplicationKit/Protocols/NSAccessibility_Protocol/Reference/Reference.html).
> Btw, you don't list them all, any particular reason?
> 
> is there a nice way to do attribute inheritance?

Not sure there is a nice and efficient way to do this inheritance.


> 
> @@ +93,5 @@
> > +{
> > +  NS_OBJC_BEGIN_TRY_ABORT_BLOCK_NIL;
> > +  
> > +  if ([attribute isEqualToString:NSAccessibilityMainAttribute]) {
> > +      return [NSNumber numberWithBool:YES];
> 
> nit: wrong indentation, don't parenthesize single if

I think we'll have a permanent disagreement on this. Also I asked around and it seems that in most of Gecko the standard is to mandate curly braces. I have seen so many bugs that would have been avoided by that practice.

> I wonder if it suites e10s builds

no idea. What's e10s builds?


> 
> @@ +96,5 @@
> > +  if ([attribute isEqualToString:NSAccessibilityMainAttribute]) {
> > +      return [NSNumber numberWithBool:YES];
> > +  }
> > +  if ([attribute isEqualToString:NSAccessibilityMinimizedAttribute]) {
> > +    return [NSNumber numberWithBool:NO];
> 
> how does it work when window is minimized? btw, do we need to control that?

Good question, will check and fix.


I seems that submitting that patch was a bit too hasty.

Comment 9

[Hubert Figuiere [:hub]]

Re: bracing.

The coding style document is pretty clear.

https://developer.mozilla.org/En/Mozilla_Coding_Style_Guide#Control_structures

Comment 10

[Trevor Saunders (:tbsaunde)]

(In reply to Hub Figuiere [:hub] from comment #9)
> Re: bracing.
> 
> The coding style document is pretty clear.
> 
> https://developer.mozilla.org/En/
> Mozilla_Coding_Style_Guide#Control_structures

That's not really true,  its pretty inconsistant, see the errors section.  It suggests everything from

if (NS_FAILED(x)) return y;

to

if (NS_FAILED(x))
  return y;

and a couple others, not to mention the people who prefer NS_ENSURE_*.

In any case there are a couple other  things where accessible/ doesn't follow the style guide since it makes more sense, for example Parent() can return NULL, so the style guide claims it should be GetParent().

(In reply to Hub Figuiere [:hub] from comment #8)
> (In reply to alexander :surkov from comment #7)
> > 
> > @@ +64,5 @@
> > > +    return [NSArray array];
> > > +  
> > > +  static NSArray *generalAttributes = nil;
> > > +  
> > > +  if (!generalAttributes) {
> > 
> > sort of strange name, why 'attributes' doesn't work?
> 
> Cut and paste from other places. That's how it was written. I can change it.
> 
> > 
> > @@ +70,5 @@
> > > +    generalAttributes = [[NSArray alloc] initWithObjects:  NSAccessibilityChildrenAttribute, 
> > > +                         NSAccessibilityParentAttribute,
> > > +                         NSAccessibilityRoleAttribute,
> > > +                         NSAccessibilityMainAttribute,  //
> > > +                         NSAccessibilityMinimizedAttribute,  //
> > 
> > nit: comments?
> > 
> > @@ +80,5 @@
> > > +                         NSAccessibilityFocusedAttribute,
> > > +                         NSAccessibilityTitleUIElementAttribute,
> > > +                         NSAccessibilityDescriptionAttribute,
> > > +                         NSAccessibilityWindowAttribute,
> > > +                         nil];
> > 
> > that'd be nice to point which attribute are generic and which are windwo
> > specific
> > (http://developer.apple.com/library/mac/#documentation/Cocoa/Reference/
> > ApplicationKit/Protocols/NSAccessibility_Protocol/Reference/Reference.html).
> > Btw, you don't list them all, any particular reason?
> > 
> > is there a nice way to do attribute inheritance?
> 
> Not sure there is a nice and efficient way to do this inheritance.
> 
> 
> > 
> > @@ +93,5 @@
> > > +{
> > > +  NS_OBJC_BEGIN_TRY_ABORT_BLOCK_NIL;
> > > +  
> > > +  if ([attribute isEqualToString:NSAccessibilityMainAttribute]) {
> > > +      return [NSNumber numberWithBool:YES];
> > 
> > nit: wrong indentation, don't parenthesize single if
> 
> I think we'll have a permanent disagreement on this. Also I asked around and
> it seems that in most of Gecko the standard is to mandate curly braces. I
> have seen so many bugs that would have been avoided by that practice.

*shrug* I don't mind you disagreeing with the style (I disagree with 2 space tabs and *s), but I'd prefer people followed the style for accessible/ if only to save me having to point it out in reviews.

Comment 11

[alexander :surkov (:asurkov)]

(In reply to Hub Figuiere [:hub] from comment #8)
> (In reply to alexander :surkov from comment #7)
> > 
> > @@ +64,5 @@
> > > +    return [NSArray array];
> > > +  
> > > +  static NSArray *generalAttributes = nil;
> > > +  
> > > +  if (!generalAttributes) {
> > 
> > sort of strange name, why 'attributes' doesn't work?
> 
> Cut and paste from other places. That's how it was written. I can change it.

yeah, please. Do cleaning-ups as yo go.

> > is there a nice way to do attribute inheritance?
> 
> Not sure there is a nice and efficient way to do this inheritance.

maybe macros, too ugly? I don't like current approach because it has weak error proof. If new attribute is added or old one is removed then we need to fix all entries. That's bad.

Trevor, ideas?

> > > +  if ([attribute isEqualToString:NSAccessibilityMainAttribute]) {
> > > +      return [NSNumber numberWithBool:YES];
> > 
> > nit: wrong indentation, don't parenthesize single if
> 
> I think we'll have a permanent disagreement on this. Also I asked around and
> it seems that in most of Gecko the standard is to mandate curly braces. I
> have seen so many bugs that would have been avoided by that practice.

Unfortunately Gecko code style is inconsistent from module to module. But everybody tries to make sure the style is consistent within the module. So this style is used in accessibility module. If you hack on layout module then you will be likely asked to use curly braces.

> > I wonder if it suites e10s builds
> 
> no idea. What's e10s builds?

rootAccessible is created for tab document, so we expose main and other attributes applicable for window. That's something we should keep in mind. If there's no easy way to address it then let's introduce comment like "XXXe10s: bla bla" so we can find these places easy in the future.

Comment 12

[Trevor Saunders (:tbsaunde)]

> > > is there a nice way to do attribute inheritance?
> > 
> > Not sure there is a nice and efficient way to do this inheritance.
> 
> maybe macros, too ugly? I don't like current approach because it has weak
> error proof. If new attribute is added or old one is removed then we need to
> fix all entries. That's bad.
> 
> Trevor, ideas?

macro's like

#define MOZ_ACCESSIBLE_ATTRIBUTES nsParent, nsChildren, nsRole ...

#define MOZ_TEXT_ACCESSIBLE_ATTRIBUTES nsCarretPos, nsText, nsFont ...

seems fairly reasonable to me.

Comment 13

[Hubert Figuiere [:hub]]

(In reply to alexander :surkov from comment #11)

> > > is there a nice way to do attribute inheritance?
> > 
> > Not sure there is a nice and efficient way to do this inheritance.
> 
> maybe macros, too ugly? I don't like current approach because it has weak
> error proof. If new attribute is added or old one is removed then we need to
> fix all entries. That's bad.

Actually I did it the Objective-C way. See in the new patch.

Comment 14

[Hubert Figuiere [:hub]]

Attachment: Patch from patch queue v2

Comment 15

[Hubert Figuiere [:hub]]

Comment on attachment 579821 [details] [diff] [review]
Patch from patch queue v2

Void patch

Comment 16

[Hubert Figuiere [:hub]]

Attachment: Patch from patch queue v2.1

Fixed patch. There was a stupid error I should have caught.

Comment 17

[Trevor Saunders (:tbsaunde)]

Comment on attachment 579920 [details] [diff] [review]
Patch from patch queue v2.1


>+  if ([attribute isEqualToString:NSAccessibilityTitleUIElementAttribute]) {
>+    mozAccessible* acc = nil;
does objective C++ require declarations at the beginning of a block? :(

>+    Relation rel(mGeckoAccessible->RelationByType(nsIAccessibleRelation::RELATION_LABELLED_BY));

I think I prefer rel = mGeckoAcc->RelationByType()

>+    nsAccessible* tempAcc = rel.Next();
>+    if(tempAcc) {
>+      acc = GetNativeFromGeckoAccessible(tempAcc);
>+    }
>+    return acc;

return tempAcc ? GetNativeFromGeckoAccessible(tempAcc) : nil;

>+  if (!attributes) {
>+    attributes = [[super accessibilityAttributeNames] mutableCopy];
>+    // standard attributes that are shared and supported by root accessible (AXMain) elements.
>+    [attributes addObject:NSAccessibilityMainAttribute];
>+    [attributes addObject:NSAccessibilityMinimizedAttribute];
>+  }

do we have an idea how often this is called / how fast object copies are in objective C++?

>+  if ([attribute isEqualToString:NSAccessibilityMainAttribute]) {
>+      return [NSNumber numberWithBool:[[self window] isMainWindow]];
>+  }
>+  if ([attribute isEqualToString:NSAccessibilityMinimizedAttribute]) {
>+    return [NSNumber numberWithBool:[[self window] isMiniaturized]];
>+  }

no curlies, and blank line after if.

Comment 18

[alexander :surkov (:asurkov)]

(In reply to Trevor Saunders (:tbsaunde) from comment #17)

> do we have an idea how often this is called / how fast object copies are in
> objective C++?

it's static so it shouldn't be an issue

Comment 19

[alexander :surkov (:asurkov)]

Comment on attachment 579920 [details] [diff] [review]
Patch from patch queue v2.1

Review of attachment 579920 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with my and Trevor's comments addressed

::: accessible/src/mac/mozDocAccessible.mm
@@ +68,5 @@
> +  if (!attributes) {
> +    attributes = [[super accessibilityAttributeNames] mutableCopy];
> +    // standard attributes that are shared and supported by root accessible (AXMain) elements.
> +    [attributes addObject:NSAccessibilityMainAttribute];
> +    [attributes addObject:NSAccessibilityMinimizedAttribute];

nit: it seems the comment in a wrong place

btw, what about e10s stuffs (comment #11)

@@ +81,5 @@
> +{
> +  NS_OBJC_BEGIN_TRY_ABORT_BLOCK_NIL;
> +  
> +  if ([attribute isEqualToString:NSAccessibilityMainAttribute]) {
> +      return [NSNumber numberWithBool:[[self window] isMainWindow]];

nit: wrong indentation

Comment 20

[Hubert Figuiere [:hub]]

> >+  if ([attribute isEqualToString:NSAccessibilityTitleUIElementAttribute]) {
> >+    mozAccessible* acc = nil;
> does objective C++ require declarations at the beginning of a block? :(

No. it is like C++.

> 
> >+    Relation rel(mGeckoAccessible->RelationByType(nsIAccessibleRelation::RELATION_LABELLED_BY));
> 
> I think I prefer rel = mGeckoAcc->RelationByType()

OK. I just took it the way it was done elsewhere in our code.
 
> >+    nsAccessible* tempAcc = rel.Next();
> >+    if(tempAcc) {
> >+      acc = GetNativeFromGeckoAccessible(tempAcc);
> >+    }
> >+    return acc;
> 
> return tempAcc ? GetNativeFromGeckoAccessible(tempAcc) : nil;

I guess you don't like my less compact style (that is strictly equivalent)

> >+  if (!attributes) {
> >+    attributes = [[super accessibilityAttributeNames] mutableCopy];
> >+    // standard attributes that are shared and supported by root accessible (AXMain) elements.
> >+    [attributes addObject:NSAccessibilityMainAttribute];
> >+    [attributes addObject:NSAccessibilityMinimizedAttribute];
> >+  }
> 
> do we have an idea how often this is called / how fast object copies are in
> objective C++?

It is called exactly once - this is a static variable. The copy just copy the array, not the objects inside that are refcounted (like anything in FoundationKit) and that are, in that case, constants.

Will fix as well as Alex.

Comment 21

[Trevor Saunders (:tbsaunde)]

> > >+    Relation rel(mGeckoAccessible->RelationByType(nsIAccessibleRelation::RELATION_LABELLED_BY));
> > 
> > I think I prefer rel = mGeckoAcc->RelationByType()
> 
> OK. I just took it the way it was done elsewhere in our code.

we have a lot of both styles depending on where you look :/

> > >+    nsAccessible* tempAcc = rel.Next();
> > >+    if(tempAcc) {
> > >+      acc = GetNativeFromGeckoAccessible(tempAcc);
> > >+    }
> > >+    return acc;
> > 
> > return tempAcc ? GetNativeFromGeckoAccessible(tempAcc) : nil;
> 
> I guess you don't like my less compact style (that is strictly equivalent)

not as much as my equivelent and shorter version :)

> > >+  if (!attributes) {
> > >+    attributes = [[super accessibilityAttributeNames] mutableCopy];
> > >+    // standard attributes that are shared and supported by root accessible (AXMain) elements.
> > >+    [attributes addObject:NSAccessibilityMainAttribute];
> > >+    [attributes addObject:NSAccessibilityMinimizedAttribute];
> > >+  }
> > 
> > do we have an idea how often this is called / how fast object copies are in
> > objective C++?
> 
> It is called exactly once - this is a static variable. The copy just copy
> the array, not the objects inside that are refcounted (like anything in
> FoundationKit) and that are, in that case, constants.

yeah, I'm a clown sorry.

Comment 22

[Hubert Figuiere [:hub]]

Attachment: Patch from patch queue v2.2

Comment 23

[David Bolter [:davidb] (NeedInfo me for attention)]

Comment on attachment 580121 [details] [diff] [review]
Patch from patch queue v2.2

Inbound land: https://hg.mozilla.org/integration/mozilla-inbound/rev/c7d9b329a45d

Comment 24

[Trevor Saunders (:tbsaunde)]

Comment on attachment 580121 [details] [diff] [review]
Patch from patch queue v2.2


>+  if ([attribute isEqualToString:NSAccessibilityMainAttribute]) {
>+    return [NSNumber numberWithBool:[[self window] isMainWindow]];
>+  }
>+  if ([attribute isEqualToString:NSAccessibilityMinimizedAttribute]) {
>+    return [NSNumber numberWithBool:[[self window] isMiniaturized]];
>+  }

you didn't get rid of these braces, which I'm pretty sure was a review comment.

Comment 25

[alexander :surkov (:asurkov)]

(In reply to Trevor Saunders (:tbsaunde) from comment #24)

> >+  if ([attribute isEqualToString:NSAccessibilityMinimizedAttribute]) {
> >+    return [NSNumber numberWithBool:[[self window] isMiniaturized]];
> >+  }
> 
> you didn't get rid of these braces, which I'm pretty sure was a review
> comment.

The same issue like in bug 455443, sounds like a trend :) Hub, your call.

Comment 26

[Trevor Saunders (:tbsaunde)]

(In reply to alexander :surkov from comment #25)
> (In reply to Trevor Saunders (:tbsaunde) from comment #24)
> 
> > >+  if ([attribute isEqualToString:NSAccessibilityMinimizedAttribute]) {
> > >+    return [NSNumber numberWithBool:[[self window] isMiniaturized]];
> > >+  }
> > 
> > you didn't get rid of these braces, which I'm pretty sure was a review
> > comment.
> 
> The same issue like in bug 455443, sounds like a trend :) Hub, your call.


since I'm about to push some stuff anyway I wouldn't have any problem just pushing the fix.

Comment 27

[alexander :surkov (:asurkov)]

(In reply to Trevor Saunders (:tbsaunde) from comment #26)

> since I'm about to push some stuff anyway I wouldn't have any problem just
> pushing the fix.

that'd be nice

Comment 28

[Trevor Saunders (:tbsaunde)]

(In reply to alexander :surkov from comment #27)
> (In reply to Trevor Saunders (:tbsaunde) from comment #26)
> 
> > since I'm about to push some stuff anyway I wouldn't have any problem just
> > pushing the fix.
> 
> that'd be nice

https://hg.mozilla.org/integration/mozilla-inbound/rev/c2370a1dba1d

Comment 29

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/c7d9b329a45d

Comment 30

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/c2370a1dba1d