Last Comment Bug 712923 - [Mac] Heading elements h1 through h6 are not recognizable by VoiceOver
: [Mac] Heading elements h1 through h6 are not recognizable by VoiceOver
Status: VERIFIED FIXED
:
Product: Core
Classification: Components
Component: Disability Access APIs (show other bugs)
: Trunk
: x86_64 Mac OS X
: -- normal (vote)
: mozilla12
Assigned To: Hubert Figuiere [:hub]
:
Mentors:
: 499931 718633 (view as bug list)
Depends on:
Blocks: osxa11y 718633
  Show dependency treegraph
 
Reported: 2011-12-22 06:06 PST by Marco Zehe (:MarcoZ)
Modified: 2012-01-28 09:03 PST (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
simple test case (487 bytes, text/html)
2012-01-04 17:27 PST, Hubert Figuiere [:hub]
no flags Details
Properly handle Heading elements to be recognizable by VoiceOver. r= (16.74 KB, patch)
2012-01-12 17:11 PST, Hubert Figuiere [:hub]
tbsaunde+mozbugs: review-
Details | Diff | Splinter Review
Properly handle Heading elements to be recognizable by VoiceOver. (12.13 KB, patch)
2012-01-13 14:23 PST, Hubert Figuiere [:hub]
no flags Details | Diff | Splinter Review
Properly handle Heading elements to be recognizable by VoiceOver. (12.12 KB, patch)
2012-01-16 13:49 PST, Hubert Figuiere [:hub]
no flags Details | Diff | Splinter Review
Properly handle Heading elements to be recognizable by VoiceOver. (11.99 KB, patch)
2012-01-17 18:31 PST, Hubert Figuiere [:hub]
no flags Details | Diff | Splinter Review
Properly handle Heading elements to be recognizable by VoiceOver. (11.64 KB, patch)
2012-01-17 22:19 PST, Hubert Figuiere [:hub]
tbsaunde+mozbugs: review+
surkov.alexander: checkin-
Details | Diff | Splinter Review
Properly handle Heading elements to be recognizable by VoiceOver. (11.23 KB, patch)
2012-01-19 20:26 PST, Hubert Figuiere [:hub]
no flags Details | Diff | Splinter Review
Properly handle Heading elements to be recognizable by VoiceOver. (8.07 KB, patch)
2012-01-20 14:38 PST, Hubert Figuiere [:hub]
surkov.alexander: review+
Details | Diff | Splinter Review
Properly handle Heading elements to be recognizable by VoiceOver. (8.15 KB, patch)
2012-01-23 09:49 PST, Hubert Figuiere [:hub]
mzehe: review+
Details | Diff | Splinter Review

Description Marco Zehe (:MarcoZ) 2011-12-22 06:06:29 PST
This is based on the try-server build that contains the patches for bug 705404 and bug 708144.

If a page contains headings 1 through 6 (h1, h2 ..., h6 elements), VoiceOver does not recognize the text being of that heading type. In addition, navigating by heading, Ctrl+Option+Cmd+H, only produces an error sound indicating that the page does not contain any headings. This is true for the Nightly start page as well as any other page.

STR:
1. WithVoiceOver running, load Nightly, and let it bring up the start page.
2. Press Ctrl+Option+Cmd+H to navigate to headings.

Result: After a while, a beep is heard indicating that no headings were found.
Expected: VoiceOver should speak the next h1 through h6 element (any of them) it finds.
Comment 1 Hubert Figuiere [:hub] 2012-01-04 17:27:12 PST
Created attachment 585957 [details]
simple test case
Comment 2 Hubert Figuiere [:hub] 2012-01-12 17:11:18 PST
Created attachment 588246 [details] [diff] [review]
Properly handle Heading elements to be recognizable by VoiceOver. r=
Comment 3 Marco Zehe (:MarcoZ) 2012-01-12 23:02:15 PST
Comment on attachment 588246 [details] [diff] [review]
Properly handle Heading elements to be recognizable by VoiceOver. r=

This looks great, can't wait to test it!

One question:
>+  PRUint32 count = mGeckoTextAccessible->CharacterCount();
>+  
>+  nsAutoString text;
>+  nsresult rv = mGeckoTextAccessible->GetText(0, count, text);

This is count, not count-1, because of the null terminator, right?
Comment 4 Hubert Figuiere [:hub] 2012-01-12 23:04:30 PST
> >+  nsresult rv = mGeckoTextAccessible->GetText(0, count, text);
> 
> This is count, not count-1, because of the null terminator, right?

It is count to say "after the last one". To get the first character it would be 0, 1, for example.
Comment 5 Trevor Saunders (:tbsaunde) 2012-01-13 09:49:52 PST
Comment on attachment 588246 [details] [diff] [review]
Properly handle Heading elements to be recognizable by VoiceOver. r=


>+          mozHTMLAccessible.mm \

name it mozHeadingAccessible.mm?

>           $(NULL)
>           
> 
> EXPORTS = \
>   nsAccessNodeWrap.h \
>   nsTextAccessibleWrap.h \
>   nsAccessibleWrap.h \
>   nsARIAGridAccessibleWrap.h \
>@@ -73,16 +74,17 @@ EXPORTS = \
>   nsHTMLImageAccessibleWrap.h \
>   nsHTMLTableAccessibleWrap.h \
>   nsApplicationAccessibleWrap.h \
>   mozDocAccessible.h \
>   mozAccessible.h \
>   mozAccessibleProtocol.h \
>   mozActionElements.h \
>   mozTextAccessible.h \
>+  mozHTMLAccessible.h \

do something that gets exported include this?

btw please file a bug to clean up all these exports, we should only export what we actually need to.

>diff --git a/accessible/src/mac/mozHTMLAccessible.h b/accessible/src/mac/mozHTMLAccessible.h
>new file mode 100644
>--- /dev/null
>+++ b/accessible/src/mac/mozHTMLAccessible.h
>@@ -0,0 +1,52 @@
>+/* -*- Mode: Objective-C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */

please add the appropriate vim line too :)

>+ * ***** END LICENSE BLOCK ***** */
>+
>+
>+
>+#import "mozAccessible.h"

only one blank line please

>+
>+@class mozTextAccessible;
>+class nsHyperTextAccessible;
>+
>+@interface mozHeadingAccessible : mozAccessible
>+{
>+  mozTextAccessible* mTextAccessible;

why can't we just store this in mChildren[0]?

>+  nsHyperTextAccessible*  mGeckoTextAccessible;         // strong

Currently this leaks since you don't drop the refrence when the object is destroyed.  You should use nsRefPtr if you are holding a reference.

My prefered solution here would be that you don't hold a reference, but instead make the pointer null when it is shutdown by overriding expire()

alternatively   don't store the aditional pointer, and just down cast it with AsHyperText() when you need it.

>diff --git a/accessible/src/mac/mozHTMLAccessible.mm b/accessible/src/mac/mozHTMLAccessible.mm
>new file mode 100644
>--- /dev/null
>+++ b/accessible/src/mac/mozHTMLAccessible.mm
>@@ -0,0 +1,113 @@
>+/* -*- Mode: Objective-C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */

same

>+ * ***** END LICENSE BLOCK ***** */
>+
>+

nit, one line only

>+#include "nsCocoaUtils.h"
>+
>+#import "mozHTMLAccessible.h"
>+#import "mozTextAccessible.h"

is there a reason for this ordering? its weird to put generic headers before a11y ones

>+- (id)initWithAccessible:(nsAccessibleWrap*)geckoParent

aGeckoParent

>+{
>+  if ((self = [super initWithAccessible:geckoParent])) {

is that actually failable?

>+    CallQueryInterface(geckoParent, &mGeckoTextAccessible);

use AsHyperText()

>+  }

nit, no braces

>+- (NSString*)role
>+{
>+  return @"AXHeading";

why can't you change the value in the role map, and let the mozAccessible impl deal with this?

>+}
>+
>+- (NSString*)title
>+{
>+  if (!mGeckoTextAccessible)
>+    return nil;
>+  
>+  PRUint32 count = mGeckoTextAccessible->CharacterCount();

use nsIAccessibleText::END_OF_TEXT or whatever it is instead

>+  
>+  nsAutoString text;
>+  nsresult rv = mGeckoTextAccessible->GetText(0, count, text);
>+  
>+  if (!NS_SUCCEEDED(rv))

nit, no blank line

>+    return nil;
>+  
>+  return text.IsEmpty() ? nil : nsCocoaUtils::ToNSString(text);

alternatively NS_SUCCEDED(rv) &&!text.IsEmpty() ? blah : nil;

>+- (id)value
>+{
>+  PRUint32 level = mGeckoTextAccessible->GetLevelInternal();

Have I missed something which means it must not be null? or do you need to check here?

>+  mChildren = [[NSMutableArray alloc] init];
>+

nit, no blank line

>+  mTextAccessible = [[mozTextAccessible alloc] initWithAccessible:mGeckoAccessible];
>+  [mChildren addObject:mTextAccessible];
>+  [mTextAccessible setParent:self];

doesn't initWithAccessible() handle setting the parent for you?

>+- (void)appendChild:(nsAccessible*)aAccessible
>+{
>+  [mTextAccessible appendChild:aAccessible];

so, mChildren can never be anything other than an alias to mTextAccessible right? doesn't this make it kind of useless?

>diff --git a/accessible/src/mac/mozTextAccessible.h b/accessible/src/mac/mozTextAccessible.h
>--- a/accessible/src/mac/mozTextAccessible.h
>+++ b/accessible/src/mac/mozTextAccessible.h
>@@ -1,8 +1,46 @@

I'd prefer a seperate bug be filed for adding licenses to existing files.

> #include "nsAccessibleWrap.h"
>+#include "nsCocoaUtils.h"

nit, blank line after a11y headers before generic ones please

>   if ((self = [super initWithAccessible:accessible])) {
>+    mRole = accessible->Role();

doesn't initWithAccessible do that for you?

>   if ([attribute isEqualToString:NSAccessibilityValueAttribute])
>-    return [self selectedText];
>+    return [self selectedText] ? : [self text];

I'd be more comfortable with this if you found the part of the spec that says  this does what you want

>+- (NSString*)text
>+{
>+  if (mGeckoTextAccessible) {

early return please

>+    
>+    nsXPIDLString text;

that method doesn't do a copy to caller thing, it assumes it has a normal nsAString& so nsXPIDLString doesn't win you anything here.

>+    nsresult rv = mGeckoTextAccessible->GetText(0, mGeckoTextAccessible->CharacterCount(), text);

use nsIAccessibleText::END_OF_TEXT

note that checking the role of the accessible  might not be  quiet enough since select opt groups also get role heading  as well as what ever can have the  html5 role attribute set to heading
note that
Comment 6 Hubert Figuiere [:hub] 2012-01-13 13:11:28 PST
(In reply to Trevor Saunders (:tbsaunde) from comment #5)
> Comment on attachment 588246 [details] [diff] [review]
> Properly handle Heading elements to be recognizable by VoiceOver. r=
> 
> 
> >+          mozHTMLAccessible.mm \
> 
> name it mozHeadingAccessible.mm?

Because I reserve the right add more of these classes related to HTML "semantics".

> >+
> >+@class mozTextAccessible;
> >+class nsHyperTextAccessible;
> >+
> >+@interface mozHeadingAccessible : mozAccessible
> >+{
> >+  mozTextAccessible* mTextAccessible;
> 
> why can't we just store this in mChildren[0]?

That wouldn't be mChildren[0] anyway.


> >+#include "nsCocoaUtils.h"
> >+
> >+#import "mozHTMLAccessible.h"
> >+#import "mozTextAccessible.h"
> 
> is there a reason for this ordering? its weird to put generic headers before
> a11y ones

I find it weird to put the local header before those from the platform.

> >+{
> >+  if ((self = [super initWithAccessible:geckoParent])) {
> 
> is that actually failable?

yes it is.

> >+- (NSString*)role
> >+{
> >+  return @"AXHeading";
> 
> why can't you change the value in the role map, and let the mozAccessible
> impl deal with this?

No because this object create automatically a child object that will be matching what is in the roleMap.
The other solution was to change the roleMap and then in the mozTextAccessible special case a few things to deal with it. Simplicity wins with that one.

> >+    return nil;
> >+  
> >+  return text.IsEmpty() ? nil : nsCocoaUtils::ToNSString(text);
> 
> alternatively NS_SUCCEDED(rv) &&!text.IsEmpty() ? blah : nil;

There is a line between concise syntax and readability. This crosses it.

> >+  mTextAccessible = [[mozTextAccessible alloc] initWithAccessible:mGeckoAccessible];
> >+  [mChildren addObject:mTextAccessible];
> >+  [mTextAccessible setParent:self];
> 
> doesn't initWithAccessible() handle setting the parent for you?

Nope. This is not a bug.

> >+- (void)appendChild:(nsAccessible*)aAccessible
> >+{
> >+  [mTextAccessible appendChild:aAccessible];
> 
> so, mChildren can never be anything other than an alias to mTextAccessible
> right? doesn't this make it kind of useless?

We can make an expensive call to [mChildren objectAt:0] each time we want it (ie somewhat often).

> >   if ((self = [super initWithAccessible:accessible])) {
> >+    mRole = accessible->Role();
> 
> doesn't initWithAccessible do that for you?

Good catch. The was leftover from the more complicated solution of not overriding -[mozHeadingAccessible role]
 
> >   if ([attribute isEqualToString:NSAccessibilityValueAttribute])
> >-    return [self selectedText];
> >+    return [self selectedText] ? : [self text];
> 
> I'd be more comfortable with this if you found the part of the spec that
> says  this does what you want

Which spec? (yes this is the fun part, this is NOT specified)
Comment 7 Hubert Figuiere [:hub] 2012-01-13 13:28:15 PST
> >+  nsHyperTextAccessible*  mGeckoTextAccessible;         // strong
> 
> Currently this leaks since you don't drop the refrence when the object is
> destroyed.  You should use nsRefPtr if you are holding a reference.
> 
> My prefered solution here would be that you don't hold a reference, but
> instead make the pointer null when it is shutdown by overriding expire()

Indeed I forgot to implement -[mozHeadingAccessible expire] like it is done for mozTextAccessible.
Comment 8 Hubert Figuiere [:hub] 2012-01-13 14:23:29 PST
Created attachment 588526 [details] [diff] [review]
Properly handle Heading elements to be recognizable by VoiceOver.
Comment 9 Trevor Saunders (:tbsaunde) 2012-01-16 12:27:38 PST
> > 
> > >+          mozHTMLAccessible.mm \
> > 
> > name it mozHeadingAccessible.mm?
> 
> Because I reserve the right add more of these classes related to HTML
> "semantics".

ok, I  gues that's fine by me.

> > >+
> > >+@class mozTextAccessible;
> > >+class nsHyperTextAccessible;
> > >+
> > >+@interface mozHeadingAccessible : mozAccessible
> > >+{
> > >+  mozTextAccessible* mTextAccessible;
> > 
> > why can't we just store this in mChildren[0]?
> 
> That wouldn't be mChildren[0] anyway.

sure, it would be some objective C csyntax, but it was easier to write and I thought it would be clear I meant the first element of the children cache.

> > >+#include "nsCocoaUtils.h"
> > >+
> > >+#import "mozHTMLAccessible.h"
> > >+#import "mozTextAccessible.h"
> > 
> > is there a reason for this ordering? its weird to put generic headers before
> > a11y ones
> 
> I find it weird to put the local header before those from the platform.

that may be, but I'm pretty sure I know of other projects where its done too.

> > >+{
> > >+  if ((self = [super initWithAccessible:geckoParent])) {
> > 
> > is that actually failable?
> 
> yes it is.

if its just because malloc can fail that's worth checking since we almost always use a infalable allocator by default.

> > >+- (NSString*)role
> > >+{
> > >+  return @"AXHeading";
> > 
> > why can't you change the value in the role map, and let the mozAccessible
> > impl deal with this?
> 
> No because this object create automatically a child object that will be
> matching what is in the roleMap.
> The other solution was to change the roleMap and then in the
> mozTextAccessible special case a few things to deal with it. Simplicity wins
> with that one.

yeah, ok, I see what's going on here, this two object thing sucks a bit :\

> > >+    return nil;
> > >+  
> > >+  return text.IsEmpty() ? nil : nsCocoaUtils::ToNSString(text);
> > 
> > alternatively NS_SUCCEDED(rv) &&!text.IsEmpty() ? blah : nil;
> 
> There is a line between concise syntax and readability. This crosses it.

needless to say I'd disagree, and would claim that two paths to  return the same thing right next to each other is a little weird.

> > >+  mTextAccessible = [[mozTextAccessible alloc] initWithAccessible:mGeckoAccessible];
> > >+  [mChildren addObject:mTextAccessible];
> > >+  [mTextAccessible setParent:self];
> > 
> > doesn't initWithAccessible() handle setting the parent for you?
> 
> Nope. This is not a bug.

yeah, not sure why I thought it did.

> > >+- (void)appendChild:(nsAccessible*)aAccessible
> > >+{
> > >+  [mTextAccessible appendChild:aAccessible];
> > 
> > so, mChildren can never be anything other than an alias to mTextAccessible
> > right? doesn't this make it kind of useless?
> 
> We can make an expensive call to [mChildren objectAt:0] each time we want it
> (ie somewhat often).

is that really expensive??? :-( :-(
if objective C and its standard library really are that bad I gues we do have to deal with it though.  I wonder if it would be better to cache the children in a nsTArray and then make a objective C array when someone tries to get the attribute. 

Since caching like this has a cost I'd prefer not to make this optimization without knowing we need it.

> > >   if ([attribute isEqualToString:NSAccessibilityValueAttribute])
> > >-    return [self selectedText];
> > >+    return [self selectedText] ? : [self text];
> > 
> > I'd be more comfortable with this if you found the part of the spec that
> > says  this does what you want
> 
> Which spec? (yes this is the fun part, this is NOT specified)

the C++ or objective C one I am refering to x ? : y;
Comment 10 Hubert Figuiere [:hub] 2012-01-16 12:52:04 PST
(In reply to Trevor Saunders (:tbsaunde) from comment #9)
 
> > > >+{
> > > >+  if ((self = [super initWithAccessible:geckoParent])) {
> > > 
> > > is that actually failable?
> > 
> > yes it is.
> 
> if its just because malloc can fail that's worth checking since we almost
> always use a infalable allocator by default.

This is how the Objective-C init sequence work. Often misunderstood by newbies, but init is allowed to return nil. In that case it should make sure to release the allocated object. This is the standard form to write it, any other may cause clang static analyzer to complain.

> yeah, ok, I see what's going on here, this two object thing sucks a bit :\

This is part of having a different object hierarchy for Mac.
 
> > > >+- (void)appendChild:(nsAccessible*)aAccessible
> > > >+{
> > > >+  [mTextAccessible appendChild:aAccessible];
> > > 
> > > so, mChildren can never be anything other than an alias to mTextAccessible
> > > right? doesn't this make it kind of useless?
> > 
> > We can make an expensive call to [mChildren objectAt:0] each time we want it
> > (ie somewhat often).
> 
> is that really expensive??? :-( :-(

Yep

> if objective C and its standard library really are that bad I gues we do
> have to deal with it though.  I wonder if it would be better to cache the
> children in a nsTArray and then make a objective C array when someone tries
> to get the attribute. 

No, because [mozAccessible children] is supposed to return a NSArray* (it is called by the OS)
 
> > > >   if ([attribute isEqualToString:NSAccessibilityValueAttribute])
> > > >-    return [self selectedText];
> > > >+    return [self selectedText] ? : [self text];
> > > 
> > > I'd be more comfortable with this if you found the part of the spec that
> > > says  this does what you want
> > 
> > Which spec? (yes this is the fun part, this is NOT specified)
> 
> the C++ or objective C one I am refering to x ? : y;

I misunderstood what you were talking about. 

For the ternary conditional operator this is apparently a GNU C extension, but clang supports it as well. The only compiler we have to care about on Mac.
(the MSVC compatibility isn't an issue here)
Comment 11 Hubert Figuiere [:hub] 2012-01-16 13:49:45 PST
Created attachment 588991 [details] [diff] [review]
Properly handle Heading elements to be recognizable by VoiceOver.
Comment 12 Trevor Saunders (:tbsaunde) 2012-01-16 14:33:25 PST
> > yeah, ok, I see what's going on here, this two object thing sucks a bit :\
> 
> This is part of having a different object hierarchy for Mac.

yes, but more particularly what I dislike is this the mac tree needs two objects for a heading thing.

> > > > >+- (void)appendChild:(nsAccessible*)aAccessible
> > > > >+{
> > > > >+  [mTextAccessible appendChild:aAccessible];
> > > > 
> > > > so, mChildren can never be anything other than an alias to mTextAccessible
> > > > right? doesn't this make it kind of useless?
> > > 
> > > We can make an expensive call to [mChildren objectAt:0] each time we want it
> > > (ie somewhat often).
> > 
> > is that really expensive??? :-( :-(
> 
> Yep

needless to say UGH

> > if objective C and its standard library really are that bad I gues we do
> > have to deal with it though.  I wonder if it would be better to cache the
> > children in a nsTArray and then make a objective C array when someone tries
> > to get the attribute. 
> 
> No, because [mozAccessible children] is supposed to return a NSArray* (it is
> called by the OS)

I mean create the objective C array when we need to pass it to the system.

> > > > >   if ([attribute isEqualToString:NSAccessibilityValueAttribute])
> > > > >-    return [self selectedText];
> > > > >+    return [self selectedText] ? : [self text];
> > > > 
> > > > I'd be more comfortable with this if you found the part of the spec that
> > > > says  this does what you want
> > > 
> > > Which spec? (yes this is the fun part, this is NOT specified)
> > 
> > the C++ or objective C one I am refering to x ? : y;
> 
> I misunderstood what you were talking about. 
> 
> For the ternary conditional operator this is apparently a GNU C extension,

interesting, happen to have a link? I'm curious

> but clang supports it as well. The only compiler we have to care about on
> Mac.

we care about gcc there at least for now.
Comment 13 Hubert Figuiere [:hub] 2012-01-16 14:40:21 PST
(In reply to Trevor Saunders (:tbsaunde) from comment #12)

> > > if objective C and its standard library really are that bad I gues we do
> > > have to deal with it though.  I wonder if it would be better to cache the
> > > children in a nsTArray and then make a objective C array when someone tries
> > > to get the attribute. 
> > 
> > No, because [mozAccessible children] is supposed to return a NSArray* (it is
> > called by the OS)
> 
> I mean create the objective C array when we need to pass it to the system.

And this is done that way. See the children method.

> > For the ternary conditional operator this is apparently a GNU C extension,
> 
> interesting, happen to have a link? I'm curious

This is the gcc 4.2.x section about C extensions.

http://gcc.gnu.org/onlinedocs/gcc-4.2.4/gcc/Conditionals.html#Conditionals
 
> > but clang supports it as well. The only compiler we have to care about on
> > Mac.
> 
> we care about gcc there at least for now.

I meant gcc AND clang are all we care about in that case. (I still build with gcc here)
Comment 14 Marco Zehe (:MarcoZ) 2012-01-17 05:46:56 PST
I am testing with the try-server build from this location:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/hfiguiere@mozilla.com-b2c705a32cf8/

I can confirm that headings are now being recognized when navigating with CTRL+Option+RightArrow and CTRL+Option+LeftArrow. There are still some problems:
1. Text is not always complete. For example on http://www.mozilla.org/en-US/firefox/fx/, there is a Heading level 3 that only has the text "Let's be " spoken. Also, the initial Heading level 1 does not have anything but "Object replacement character" spoken.

Also, navigating by headings with either the object chooser (Ctrl+Option+U) or by headings (ctrl+option+cmd+h forward, ctrl+option+cmd+shift+h backwards) does not work yet.
Comment 15 Marco Zehe (:MarcoZ) 2012-01-17 05:50:27 PST
Additonla info: Headings on this page: http://www.heise.de/newsticker/ CAN be navigated to using the above heading navigation commands. Also, the object chooser (ctrl+option+u) gets populated on this page. So there's something on mozilla.org that's preventing these functions from working, whereas on other pages, they DO work.
Comment 16 Marco Zehe (:MarcoZ) 2012-01-17 06:14:23 PST
More info: "Object replacement character" is the embedded character that represents a link in the (for example) IAccessibleText interface. On http://www.marcozehe.de, for example, all the headings that are also links to the actual blog posts have only this "object replacement character" spoken.
Comment 17 Marco Zehe (:MarcoZ) 2012-01-17 06:35:37 PST
Note that I filed bug 718633 on the issue described in the last comment.
Comment 18 Trevor Saunders (:tbsaunde) 2012-01-17 15:50:08 PST
(In reply to Hub Figuiere [:hub] from comment #13)
> (In reply to Trevor Saunders (:tbsaunde) from comment #12)
> 
> > > > if objective C and its standard library really are that bad I gues we do
> > > > have to deal with it though.  I wonder if it would be better to cache the
> > > > children in a nsTArray and then make a objective C array when someone tries
> > > > to get the attribute. 
> > > 
> > > No, because [mozAccessible children] is supposed to return a NSArray* (it is
> > > called by the OS)
> > 
> > I mean create the objective C array when we need to pass it to the system.
> And this is done that way. See the children method.

I think your mis understanding what I'm suggesting.  yes, children () creates the array when the os wants it, but it also uses the objective C array as the objects internal cache of its children.  I'm suggesting changing the internal cache so we can use a more reasonable array internally, and then create temporary objective C arrays when the os needs one.  It seems like   this would help in bug 712927 too.

> > > For the ternary conditional operator this is apparently a GNU C extension,
> > 
> > interesting, happen to have a link? I'm curious
> 
> This is the gcc 4.2.x section about C extensions.
> 
> http://gcc.gnu.org/onlinedocs/gcc-4.2.4/gcc/Conditionals.html#Conditionals

thanks!
Comment 19 Hubert Figuiere [:hub] 2012-01-17 17:47:38 PST
> I think your mis understanding what I'm suggesting.  yes, children ()
> creates the array when the os wants it, but it also uses the objective C
> array as the objects internal cache of its children.  I'm suggesting
> changing the internal cache so we can use a more reasonable array
> internally, and then create temporary objective C arrays when the os needs
> one.  It seems like   this would help in bug 712927 too.

The cache is only used to return the array to the OS. I don't see anything more reasonable in what you are trying to suggest, both in term of performance and simplicity.
Comment 20 Trevor Saunders (:tbsaunde) 2012-01-17 18:16:42 PST
(In reply to Hub Figuiere [:hub] from comment #19)
> > I think your mis understanding what I'm suggesting.  yes, children ()
> > creates the array when the os wants it, but it also uses the objective C
> > array as the objects internal cache of its children.  I'm suggesting
> > changing the internal cache so we can use a more reasonable array
> > internally, and then create temporary objective C arrays when the os needs
> > one.  It seems like   this would help in bug 712927 too.
> 
> The cache is only used to return the array to the OS. I don't see anything
> more reasonable in what you are trying to suggest, both in term of
> performance and simplicity.

sure *now* all we use it for is that, but doing this would let you get rid of the child pointer you cache in this patch, and I'd expect it would improve your iteration over the children in bug 712927
Comment 21 Hubert Figuiere [:hub] 2012-01-17 18:31:38 PST
Created attachment 589379 [details] [diff] [review]
Properly handle Heading elements to be recognizable by VoiceOver.
Comment 22 Hubert Figuiere [:hub] 2012-01-17 18:32:59 PST
removed the mozTextAccessible* from the member.
Comment 23 Trevor Saunders (:tbsaunde) 2012-01-17 18:58:52 PST
> 
> >+    CallQueryInterface(geckoParent, &mGeckoTextAccessible);
> 
> use AsHyperText()
> 
> >+  }
> 
> nit, no braces

did you miss these?  I don't see them fixed in the patch.
Comment 24 Trevor Saunders (:tbsaunde) 2012-01-17 21:43:57 PST
Comment on attachment 589379 [details] [diff] [review]
Properly handle Heading elements to be recognizable by VoiceOver.

0,0 +1,110 @@
>+/* -*- Mode: Objective-C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
>+/* vim:expandtab:shiftwidth=2:tabstop=2:
>+ */

nit, see atk/ for the nicer way of doing this, you don't actually need this second line.

with my previous comment fixed this looks good.
>+/*
Comment 25 Hubert Figuiere [:hub] 2012-01-17 22:19:46 PST
Created attachment 589406 [details] [diff] [review]
Properly handle Heading elements to be recognizable by VoiceOver.
Comment 26 David Bolter [:davidb] 2012-01-18 05:37:18 PST
Comment on attachment 588991 [details] [diff] [review]
Properly handle Heading elements to be recognizable by VoiceOver.

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

::: accessible/src/mac/mozTextAccessible.mm
@@ +82,5 @@
>      return [self selectedText];
>    // Apple's SpeechSynthesisServer expects AXValue to return an AXStaticText
>    // object's AXSelectedText attribute.  See bug 674612.
>    if ([attribute isEqualToString:NSAccessibilityValueAttribute])
> +    return [self selectedText] ? : [self text];

Is this right?
Comment 27 Hubert Figuiere [:hub] 2012-01-18 08:27:23 PST
Yes it is. It [self selectedText] is not nil, it will return it, otherwise, it will return [self text]

see comment 10 and comment 13.
Comment 28 alexander :surkov 2012-01-18 08:59:40 PST
Hub, could you give some description of your patch please?

How does tree invalidation work?
Comment 29 Hubert Figuiere [:hub] 2012-01-18 11:43:00 PST
(In reply to alexander :surkov from comment #28)
> Hub, could you give some description of your patch please?

The Mac accessibe tree need an AXHeading with an AXStaticText to represent a heading, but we only have on Gecko object. That's what I'm doing.

> How does tree invalidation work?

It will work well. mozHeadingAccessible when invalidating children will get rid of its mozTextAccessible that is only known to it.
Comment 30 alexander :surkov 2012-01-19 01:31:43 PST
(In reply to Hub Figuiere [:hub] from comment #29)
> (In reply to alexander :surkov from comment #28)
> > Hub, could you give some description of your patch please?
> 
> The Mac accessibe tree need an AXHeading with an AXStaticText to represent a
> heading, but we only have on Gecko object. That's what I'm doing.

we have two Gecko objects, maybe they are cut off on mac. So I'd rather to expose it than wild dances with mac tree creation.

> > How does tree invalidation work?
> 
> It will work well. mozHeadingAccessible when invalidating children will get
> rid of its mozTextAccessible that is only known to it.

if you mean invalidateChildren then it doesn't cover all cases (if you didn't fixed that while I was away ;) ).
Comment 31 alexander :surkov 2012-01-19 01:32:32 PST
Comment on attachment 589406 [details] [diff] [review]
Properly handle Heading elements to be recognizable by VoiceOver.

please don't land it until we discuss all concerns.
Comment 32 Hubert Figuiere [:hub] 2012-01-19 16:44:20 PST
(In reply to alexander :surkov from comment #30)
> (In reply to Hub Figuiere [:hub] from comment #29)
> > (In reply to alexander :surkov from comment #28)
> > > Hub, could you give some description of your patch please?
> > 
> > The Mac accessibe tree need an AXHeading with an AXStaticText to represent a
> > heading, but we only have on Gecko object. That's what I'm doing.
> 
> we have two Gecko objects, maybe they are cut off on mac. So I'd rather to
> expose it than wild dances with mac tree creation.

This effectively change a lot of things. Why did I miss that, and why do we hide it (I might have an idea for the later.)

I'll submit a new patch, hopefully this might also fix bug 718633
Comment 33 Hubert Figuiere [:hub] 2012-01-19 20:26:30 PST
Created attachment 590095 [details] [diff] [review]
Properly handle Heading elements to be recognizable by VoiceOver.
Comment 34 Hubert Figuiere [:hub] 2012-01-19 20:28:29 PST
Comment on attachment 590095 [details] [diff] [review]
Properly handle Heading elements to be recognizable by VoiceOver.

I have no idea why I went with that other complicated route. This much more simple.
Comment 35 alexander :surkov 2012-01-20 04:18:20 PST
Comment on attachment 590095 [details] [diff] [review]
Properly handle Heading elements to be recognizable by VoiceOver.

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

yes, that's much better than it was, thanks for doing this, I still need to get some question addressed before r+

::: accessible/src/mac/mozHTMLAccessible.mm
@@ +52,5 @@
> +
> +  nsAutoString text;
> +  nsresult rv = 
> +    mGeckoAccessible->AsHyperText()->GetText(0, nsIAccessibleText::TEXT_OFFSET_END_OF_TEXT, 
> +                                             text);

I assume that VoiceOver expects to get a text from title rather than accessible name then I think you should get a name from accessible tree rather than text because in case when header accessible contains something else than plain text then voiceover is going to announce embedded characters instead of real text, that's not what we want I think.

::: accessible/src/mac/mozTextAccessible.mm
@@ +82,5 @@
>      return [self selectedText];
>    // Apple's SpeechSynthesisServer expects AXValue to return an AXStaticText
>    // object's AXSelectedText attribute.  See bug 674612.
>    if ([attribute isEqualToString:NSAccessibilityValueAttribute])
> +    return [self selectedText] ? : [self text];

that's strange that NSAccessibilityValueAttribute is different from value method. Is it on demand?

btw, please give explanation why you need mozTextAccessible changes for this bug. That's not evident for me.
Comment 36 Hubert Figuiere [:hub] 2012-01-20 13:44:57 PST
Comment on attachment 590095 [details] [diff] [review]
Properly handle Heading elements to be recognizable by VoiceOver.

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

::: accessible/src/mac/mozHTMLAccessible.mm
@@ +52,5 @@
> +
> +  nsAutoString text;
> +  nsresult rv = 
> +    mGeckoAccessible->AsHyperText()->GetText(0, nsIAccessibleText::TEXT_OFFSET_END_OF_TEXT, 
> +                                             text);

It is gone to. it no longer seems to be needed.

::: accessible/src/mac/mozTextAccessible.mm
@@ +82,5 @@
>      return [self selectedText];
>    // Apple's SpeechSynthesisServer expects AXValue to return an AXStaticText
>    // object's AXSelectedText attribute.  See bug 674612.
>    if ([attribute isEqualToString:NSAccessibilityValueAttribute])
> +    return [self selectedText] ? : [self text];

It is gone now. I guess with all the changes I ended up make I should have removed that too.
Comment 37 Hubert Figuiere [:hub] 2012-01-20 14:38:11 PST
Created attachment 590356 [details] [diff] [review]
Properly handle Heading elements to be recognizable by VoiceOver.
Comment 38 Hubert Figuiere [:hub] 2012-01-20 14:39:25 PST
Comment on attachment 590356 [details] [diff] [review]
Properly handle Heading elements to be recognizable by VoiceOver.

with all the comment addressed.
Comment 39 Hubert Figuiere [:hub] 2012-01-20 15:27:05 PST
*** Bug 718633 has been marked as a duplicate of this bug. ***
Comment 40 alexander :surkov 2012-01-21 03:00:48 PST
Comment on attachment 590356 [details] [diff] [review]
Properly handle Heading elements to be recognizable by VoiceOver.

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

r=me with comments addressed

::: accessible/src/mac/mozHTMLAccessible.mm
@@ +48,5 @@
> +{
> +  if (!mGeckoAccessible)
> +    return nil;
> +
> +  PRUint32 level = mGeckoAccessible->AsHyperText()->GetLevelInternal();

I think that must be a crash for something like <img role="heading"/>

::: accessible/src/mac/nsAccessibleWrap.mm
@@ +116,2 @@
>      case roles::ENTRY:
>      case roles::STATICTEXT:

btw, is optimizer supposed to do well here since case statements aren't ordered the same way with enum Role?
Comment 41 Hubert Figuiere [:hub] 2012-01-22 13:38:21 PST
Comment on attachment 590356 [details] [diff] [review]
Properly handle Heading elements to be recognizable by VoiceOver.

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

::: accessible/src/mac/mozHTMLAccessible.mm
@@ +48,5 @@
> +{
> +  if (!mGeckoAccessible)
> +    return nil;
> +
> +  PRUint32 level = mGeckoAccessible->AsHyperText()->GetLevelInternal();

Oops. I'll add that to my testcase and fix the code.

::: accessible/src/mac/nsAccessibleWrap.mm
@@ +116,2 @@
>      case roles::ENTRY:
>      case roles::STATICTEXT:

I would expect it to be.
Comment 42 Hubert Figuiere [:hub] 2012-01-23 09:49:37 PST
Created attachment 590758 [details] [diff] [review]
Properly handle Heading elements to be recognizable by VoiceOver.
Comment 43 Marco Zehe (:MarcoZ) 2012-01-23 10:29:14 PST
Comment on attachment 590758 [details] [diff] [review]
Properly handle Heading elements to be recognizable by VoiceOver.

>+  nsHyperTextAccessible* hypertext = mGeckoAccessible->AsHyperText();
>+  if (!hypertext)
>+    return nil;

Nit: name it hypertextAcc, so it is clear this is an accessible. I know this sounds nitpicky, but we do it elsewhere, too, and should be consistent. r=me for this new part with that fixed.
Comment 44 Trevor Saunders (:tbsaunde) 2012-01-23 13:47:02 PST
(In reply to Marco Zehe (:MarcoZ) from comment #43)
> Comment on attachment 590758 [details] [diff] [review]
> Properly handle Heading elements to be recognizable by VoiceOver.
> 
> >+  nsHyperTextAccessible* hypertext = mGeckoAccessible->AsHyperText();
> >+  if (!hypertext)
> >+    return nil;
> 
> Nit: name it hypertextAcc, so it is clear this is an accessible. I know this
> sounds nitpicky, but we do it elsewhere, too, and should be consistent. r=me
> for this new part with that fixed.

huh, we aren't very consistant about that, and i think I'd generally prefer we didn't append Acc when we don't need to deconflict, but whatever.

btw for future note a cleaner way of doing this would be to use IsHyperText()
Comment 45 Hubert Figuiere [:hub] 2012-01-23 13:55:34 PST
> 
> huh, we aren't very consistant about that, and i think I'd generally prefer
> we didn't append Acc when we don't need to deconflict, but whatever.
> 
> btw for future note a cleaner way of doing this would be to use IsHyperText()

The patch still hasn't been committed, so I can still change it :-)
Comment 46 Hubert Figuiere [:hub] 2012-01-23 14:43:54 PST
With the last feedback.
https://hg.mozilla.org/integration/mozilla-inbound/rev/3098655dc8a5
Comment 47 Marco Bonardo [::mak] 2012-01-24 04:59:32 PST
https://hg.mozilla.org/mozilla-central/rev/3098655dc8a5
Comment 48 Marco Zehe (:MarcoZ) 2012-01-24 07:35:12 PST
Verified fixed in a local build off of mozilla-central and by code inspection. No regular builds with accessibility enabled exist yet, thus this way of verification.
Comment 49 Hubert Figuiere [:hub] 2012-01-24 18:25:37 PST
*** Bug 499931 has been marked as a duplicate of this bug. ***
Comment 50 alexander :surkov 2012-01-25 20:41:54 PST
(In reply to Trevor Saunders (:tbsaunde) from comment #44)
> (In reply to Marco Zehe (:MarcoZ) from comment #43)
> > Comment on attachment 590758 [details] [diff] [review]
> > Properly handle Heading elements to be recognizable by VoiceOver.
> > 
> > >+  nsHyperTextAccessible* hypertext = mGeckoAccessible->AsHyperText();
> > >+  if (!hypertext)
> > >+    return nil;
> > 
> > Nit: name it hypertextAcc, so it is clear this is an accessible. I know this
> > sounds nitpicky, but we do it elsewhere, too, and should be consistent. r=me
> > for this new part with that fixed.
> 
> huh, we aren't very consistant about that, and i think I'd generally prefer
> we didn't append Acc when we don't need to deconflict, but whatever.

yep, I prefer to not append 'acc' to the end of variable name of accessible object. So if we have 'row' variable name then usually it means row accessible. In case of DOM node then it'll be a rowNode, rowContent or rowElm. In case of indices then it's rowIdx and so on.
Comment 51 Marco Zehe (:MarcoZ) 2012-01-28 09:03:51 PST
Verified in Mozilla/5.0 (Macintosh; Intel Mac OS X 10.7; rv:12.0a1) Gecko/20120128 Firefox/12.0a1 built from the projects/accessibility branch.

Note You need to log in before you can comment on or make changes to this bug.