Marco Zehe (:MarcoZ) Reporter
	Description • 13 years ago

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.

	Marco Zehe (:MarcoZ) Reporter
	Comment 3 • 12 years ago

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?

	Hubert Figuiere [:hub] Assignee
	Comment 4 • 12 years ago

> >+  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.

	Trevor Saunders (:tbsaunde)
	Comment 5 • 12 years ago

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

	Hubert Figuiere [:hub] Assignee
	Comment 6 • 12 years ago

(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)

	Hubert Figuiere [:hub] Assignee
	Comment 7 • 12 years ago

> >+  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.

	Trevor Saunders (:tbsaunde)
	Comment 9 • 12 years ago

> > 
> > >+          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;

	Hubert Figuiere [:hub] Assignee
	Comment 10 • 12 years ago

(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)

	Trevor Saunders (:tbsaunde)
	Comment 12 • 12 years ago

> > 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.

	Hubert Figuiere [:hub] Assignee
	Comment 13 • 12 years ago

(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)

	Marco Zehe (:MarcoZ) Reporter
	Comment 14 • 12 years ago

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.

	Marco Zehe (:MarcoZ) Reporter
	Comment 15 • 12 years ago

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.

	Marco Zehe (:MarcoZ) Reporter
	Comment 16 • 12 years ago

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.

	Marco Zehe (:MarcoZ) Reporter
	Comment 17 • 12 years ago

Note that I filed bug 718633 on the issue described in the last comment.

	Trevor Saunders (:tbsaunde)
	Comment 18 • 12 years ago

(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!

	Hubert Figuiere [:hub] Assignee
	Comment 19 • 12 years ago

> 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.

	Trevor Saunders (:tbsaunde)
	Comment 20 • 12 years ago

(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

	Hubert Figuiere [:hub] Assignee
	Comment 22 • 12 years ago

removed the mozTextAccessible* from the member.

	Trevor Saunders (:tbsaunde)
	Comment 23 • 12 years ago

> 
> >+    CallQueryInterface(geckoParent, &mGeckoTextAccessible);
> 
> use AsHyperText()
> 
> >+  }
> 
> nit, no braces

did you miss these?  I don't see them fixed in the patch.

	Trevor Saunders (:tbsaunde)
	Comment 24 • 12 years ago

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.
>+/*

	David Bolter [:davidb] (NeedInfo me for attention)
	Comment 26 • 12 years ago

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?

	Hubert Figuiere [:hub] Assignee
	Comment 27 • 12 years ago

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.

	alexander :surkov (:asurkov)
	Comment 28 • 12 years ago

Hub, could you give some description of your patch please?

How does tree invalidation work?

	Hubert Figuiere [:hub] Assignee
	Comment 29 • 12 years ago

(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.

	alexander :surkov (:asurkov)
	Comment 30 • 12 years ago

(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 ;) ).

	alexander :surkov (:asurkov)
	Comment 31 • 12 years ago

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.

	Hubert Figuiere [:hub] Assignee
	Comment 32 • 12 years ago

(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

	Hubert Figuiere [:hub] Assignee
	Comment 34 • 12 years ago

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.

	alexander :surkov (:asurkov)
	Comment 35 • 12 years ago

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.

	Hubert Figuiere [:hub] Assignee
	Comment 36 • 12 years ago

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.

	Hubert Figuiere [:hub] Assignee
	Comment 38 • 12 years ago

Comment on attachment 590356 [details] [diff] [review]
Properly handle Heading elements to be recognizable by VoiceOver.

with all the comment addressed.

	alexander :surkov (:asurkov)
	Comment 40 • 12 years ago

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?

	Hubert Figuiere [:hub] Assignee
	Comment 41 • 12 years ago

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.

	Marco Zehe (:MarcoZ) Reporter
	Comment 43 • 12 years ago

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.

	Trevor Saunders (:tbsaunde)
	Comment 44 • 12 years ago

(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()

	Hubert Figuiere [:hub] Assignee
	Comment 45 • 12 years ago

> 
> 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 :-)

	Hubert Figuiere [:hub] Assignee
	Comment 46 • 12 years ago

With the last feedback.
https://hg.mozilla.org/integration/mozilla-inbound/rev/3098655dc8a5

	Marco Bonardo [:mak]
	Comment 47 • 12 years ago

https://hg.mozilla.org/mozilla-central/rev/3098655dc8a5

	Marco Zehe (:MarcoZ) Reporter
	Comment 48 • 12 years ago

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.

	alexander :surkov (:asurkov)
	Comment 50 • 12 years ago

(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.

	Marco Zehe (:MarcoZ) Reporter
	Comment 51 • 12 years ago

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.