When VoiceOver reads text on web pages that is not a link and not part of a form field, VoiceOver says "text" at the end of each chunk of text. It does not do that in Safari. This is irritating, since when you use the "Read all" command (Ctrl+Option+A), the flow of text gets interrupted by the word "text" whenever a new paragraph etc. is encountered.

An inconvenience, but nothing that actually blocks usability.

I no longer get the thing to say "text" after each paragraph. There are a few case where it is incorrect, like for list, though.

(In reply to Hub Figuiere [:hub] from comment #2) > I no longer get the thing to say "text" after each paragraph. I do. 1. Go to http://www.heise.de/mac-and-i/meldung/Twitter-App-integriert-Aktivitaetsstream-1562578.html 2. Navigate to the Heading Level 1 by pressing CTRL+Option+Cmd+H. 3. Press Ctrl+Option+Right Arrow. Result: VoiceOver will read the first chunk of text before the first link, and say "text" at the end of the text. if you do the same in Safari, VoiceOver won't say "text" at the end.

It is definitely a serious difference between 10.6 and 10.7.

Comment on attachment 634556 [details] [diff] [review] Implement TextLeaf accessibles for Mac. r= I have a try build going on for this patch https://tbpl.mozilla.org/?tree=Try&rev=c84f7585a10b

Comment on attachment 634556 [details] [diff] [review] Implement TextLeaf accessibles for Mac. r= > @implementation mozHeadingAccessible > >+- (NSString*)title >+{ >+ nsAutoString title; >+ Accessible* child = mGeckoAccessible->FirstChild(); >+ if (child) >+ child->GetName(title); so, I'm not sure exactly what your trying to achieve here, but that seems pretty suspect > #import "mozAccessible.h" > > #import "HyperTextAccessible.h" >+#import "TextLeafAccessible.h" use a forward decl when possible >+@interface mozTextLeafAccessible : mozAccessible >+{ >+ mozilla::a11y::TextLeafAccessible* mGeckoTextLeafAccessible; // strong first a owning ref shouldn't be needed, second if you need one use a refPtr, and third we almost certainly don't want to cache this anyway. >+ if ([attribute isEqualToString:NSAccessibilityTitleAttribute]) >+ return @""; kind of weird things expect this, and it isn't immediately obvious from Apples docs we should do this, but ok >- nsAutoString text; >- nsresult rv = mGeckoTextAccessible-> >- GetText(0, nsIAccessibleText::TEXT_OFFSET_END_OF_TEXT, text); >- NS_ENSURE_SUCCESS(rv, @""); >+ if (mGeckoTextAccessible) { I don't see the point of this check since there was a return if !mGeckoTextAccessible earlier. >+- (id)initWithAccessible:(AccessibleWrap*)accessible >+{ >+ if ((self = [super initWithAccessible:accessible])) { >+ CallQueryInterface(accessible, &mGeckoTextLeafAccessible); per irc use AsTextLeaf() that really shouldn't compile. However it really doesn't make sense to cache this. >+- (NSString*)text >+{ >+ if (!mGeckoAccessible || !mGeckoTextLeafAccessible) >+ return nil; so, if the only place this can be called from externally is ValueForAttribute() I'd tend to think it should do the null check, and things it calls should just assume a non-null internal accessible

(In reply to Trevor Saunders (:tbsaunde) from comment #7) > Comment on attachment 634556 [details] [diff] [review] > Implement TextLeaf accessibles for Mac. r= > > > @implementation mozHeadingAccessible > > > >+- (NSString*)title > >+{ > >+ nsAutoString title; > >+ Accessible* child = mGeckoAccessible->FirstChild(); > >+ if (child) > >+ child->GetName(title); > > so, I'm not sure exactly what your trying to achieve here, but that seems > pretty suspect It is the only way I found to get the actual content of the Heading to get the title attribute as needed by Mac a11y. > >+@interface mozTextLeafAccessible : mozAccessible > >+{ > >+ mozilla::a11y::TextLeafAccessible* mGeckoTextLeafAccessible; // strong > > first a owning ref shouldn't be needed, second if you need one use a refPtr, > and third we almost certainly don't want to cache this anyway. This is not long strong. Just "AsTextLeaf()". The Gecko accessible object will be the same throughout the lifetime of the object. As for caching it, we have seen it is immutable, but AsTextLeaf() can return nsnull in case the object isn't the right kind. Saves on checking all the time. > >+ if ([attribute isEqualToString:NSAccessibilityTitleAttribute]) > >+ return @""; > > kind of weird things expect this, and it isn't immediately obvious from > Apples docs we should do this, but ok Empty string. That's it. > > >+- (id)initWithAccessible:(AccessibleWrap*)accessible > >+{ > >+ if ((self = [super initWithAccessible:accessible])) { > >+ CallQueryInterface(accessible, &mGeckoTextLeafAccessible); > > per irc use AsTextLeaf() that really shouldn't compile. However it really > doesn't make sense to cache this. Yes I switched to use AsTextLeaf(). But this did compile. Really. > > >+- (NSString*)text > >+{ > >+ if (!mGeckoAccessible || !mGeckoTextLeafAccessible) > >+ return nil; > > so, if the only place this can be called from externally is > ValueForAttribute() I'd tend to think it should do the null check, and > things it calls should just assume a non-null internal accessible There is probably a way to minimize these checks, but it is so much simplier to do it here.

(In reply to Hub Figuiere [:hub] from comment #8) > (In reply to Trevor Saunders (:tbsaunde) from comment #7) > > Comment on attachment 634556 [details] [diff] [review] > > Implement TextLeaf accessibles for Mac. r= > > > > > @implementation mozHeadingAccessible > > > > > >+- (NSString*)title > > >+{ > > >+ nsAutoString title; > > >+ Accessible* child = mGeckoAccessible->FirstChild(); > > >+ if (child) > > >+ child->GetName(title); > > > > so, I'm not sure exactly what your trying to achieve here, but that seems > > pretty suspect > > It is the only way I found to get the actual content of the Heading to get > the title attribute as needed by Mac a11y. sure, but does it work for something like <h1>some heading<a>with a link</a> and more text</h1> ? > > >+@interface mozTextLeafAccessible : mozAccessible > > >+{ > > >+ mozilla::a11y::TextLeafAccessible* mGeckoTextLeafAccessible; // strong > > > > first a owning ref shouldn't be needed, second if you need one use a refPtr, > > and third we almost certainly don't want to cache this anyway. > > This is not long strong. Just "AsTextLeaf()". > The Gecko accessible object will be the same throughout the lifetime of the > object. > As for caching it, we have seen it is immutable, but AsTextLeaf() can return > nsnull in case the object isn't the right kind. Saves on checking all the > time. sure, but saving a and, compare and jump doesn't seem like its worth a word in each object. Especially when you consider the optimization we can do of some of the HypertextAccessible methods. If we ever have profiling data showing that test is really hot we can remove the check and require callers to be sure what they want to convert is actually of the right type. > > > > >+- (id)initWithAccessible:(AccessibleWrap*)accessible > > >+{ > > >+ if ((self = [super initWithAccessible:accessible])) { > > >+ CallQueryInterface(accessible, &mGeckoTextLeafAccessible); > > > > per irc use AsTextLeaf() that really shouldn't compile. However it really > > doesn't make sense to cache this. > > Yes I switched to use AsTextLeaf(). But this did compile. Really. I believe you it just confuses and saddens me > > >+- (NSString*)text > > >+{ > > >+ if (!mGeckoAccessible || !mGeckoTextLeafAccessible) > > >+ return nil; > > > > so, if the only place this can be called from externally is > > ValueForAttribute() I'd tend to think it should do the null check, and > > things it calls should just assume a non-null internal accessible > > There is probably a way to minimize these checks, but it is so much simplier > to do it here. I'm not sure I'm convinced, but ok

(In reply to Trevor Saunders (:tbsaunde) from comment #9) > (In reply to Hub Figuiere [:hub] from comment #8) > > (In reply to Trevor Saunders (:tbsaunde) from comment #7) > > > Comment on attachment 634556 [details] [diff] [review] > > > Implement TextLeaf accessibles for Mac. r= > > > > > > > @implementation mozHeadingAccessible > > > > > > > >+- (NSString*)title > > > >+{ > > > >+ nsAutoString title; > > > >+ Accessible* child = mGeckoAccessible->FirstChild(); > > > >+ if (child) > > > >+ child->GetName(title); > > > > > > so, I'm not sure exactly what your trying to achieve here, but that seems > > > pretty suspect > > > > It is the only way I found to get the actual content of the Heading to get > > the title attribute as needed by Mac a11y. > > sure, but does it work for something like <h1>some heading<a>with a link</a> > and more text</h1> ? Doh... Excellent point. Now I need to figure out how to get the whole text :-/

Comment on attachment 635043 [details] [diff] [review] Implement TextLeaf accessibles for Mac. Fix Heading title. Address the comments. Fix the title issue for Headings (when there is more than one child).

Comment on attachment 635043 [details] [diff] [review] Implement TextLeaf accessibles for Mac. Fix Heading title. > @implementation mozHeadingAccessible > >+- (NSString*)title >+{ >+ nsAutoString title; >+ nsIContent* content = mGeckoAccessible->GetContent(); >+ if (!content) >+ return nil; GetContent() can only return null for defunct accessibles, or for some crazy cases like nsHTMLWin32ObjectAccessible and nsRootAccessibleWrap, I'm not sure there are any of these on mac, but either a accessible for a heading will never have null mContent, so you can remove this check. >+ >+ nsresult rv = content->GetTextContent(title); I can see a couple problems with this 1. I don't think it gets generated content, I tend to think people who use that with a heading are crazy, but we should probably support it. 2. the text the DOM gives you can be out of sync with the text methods on HypertextAccessible will give you if the DOM has changed but layout hasn't yet reflowed. I think what I'd suggest is calling GetText() on the hypertext accessible and then crwling through its kids with GetText() to fill in for EOCs if mac cann't do that itself. That doesn't seem fun, but I believe its the right solution. David does that seem right to you? >+@interface mozTextLeafAccessible : mozAccessible >+{ >+ mozilla::a11y::TextLeafAccessible* mGeckoTextLeafAccessible; I still think this is a pretty bad trade of memory for performance.

As per IRC, I'd vote to preserve what is left of Hub's sanity (growing hg patch queue) and allow a follow up (presumably bug 68298) for the flattened text. (I think we may want to reconsider the eRule for GetName with headings). There is something about walking the subtree in the mac wrapper that doesn't feel right anyways...

(In reply to David Bolter [:davidb] from comment #14) > and allow a follow up (presumably bug 68298) Err bug 768298.

Comment on attachment 636572 [details] [diff] [review] Implement TextLeaf accessibles for Mac. Fix Heading title. ># HG changeset patch ># User Hub Figuière <hfiguiere@mozilla.com> ># Date 1340675830 25200 ># Node ID 631fb1cfd503129200830bcaa3fb1385ab00d61c ># Parent 35abd49dc6dd9440a28cb9bd3b571135fd132220 >Bug 718625 - Implement TextLeaf accessibles for Mac. Fix Heading title. r=tbsaunde > >diff --git a/accessible/src/mac/AccessibleWrap.mm b/accessible/src/mac/AccessibleWrap.mm >--- a/accessible/src/mac/AccessibleWrap.mm >+++ b/accessible/src/mac/AccessibleWrap.mm >@@ -84,20 +84,22 @@ AccessibleWrap::GetNativeType () > > case roles::PAGETABLIST: > return [mozTabsAccessible class]; > > case roles::ENTRY: > case roles::STATICTEXT: > case roles::CAPTION: > case roles::ACCEL_LABEL: >- case roles::TEXT_LEAF: > case roles::PASSWORD_TEXT: > // normal textfield (static or editable) >- return [mozTextAccessible class]; >+ return [mozTextAccessible class]; >+ >+ case roles::TEXT_LEAF: >+ return [mozTextLeafAccessible class]; > > case roles::LINK: > return [mozLinkAccessible class]; > > case roles::COMBOBOX: > return [mozPopupButtonAccessible class]; > > default: >diff --git a/accessible/src/mac/mozAccessible.mm b/accessible/src/mac/mozAccessible.mm >--- a/accessible/src/mac/mozAccessible.mm >+++ b/accessible/src/mac/mozAccessible.mm >@@ -467,17 +467,17 @@ GetClosestInterestingAccessible(id anObj > } > > - (NSString*)title > { > NS_OBJC_BEGIN_TRY_ABORT_BLOCK_NIL; > > nsAutoString title; > mGeckoAccessible->Name(title); >- return title.IsEmpty() ? nil : [NSString stringWithCharacters:title.BeginReading() length:title.Length()]; >+ return nsCocoaUtils::ToNSString(title); > > NS_OBJC_END_TRY_ABORT_BLOCK_NIL; > } > > - (id)value > { > NS_OBJC_BEGIN_TRY_ABORT_BLOCK_NIL; > >@@ -510,17 +510,18 @@ GetClosestInterestingAccessible(id anObj > { > NS_OBJC_BEGIN_TRY_ABORT_BLOCK_NIL; > > if (mGeckoAccessible->IsDefunct()) > return nil; > > nsAutoString desc; > mGeckoAccessible->Description(desc); >- return desc.IsEmpty() ? nil : [NSString stringWithCharacters:desc.BeginReading() length:desc.Length()]; >+ >+ return nsCocoaUtils::ToNSString(desc); > > NS_OBJC_END_TRY_ABORT_BLOCK_NIL; > } > > - (NSString*)help > { > NS_OBJC_BEGIN_TRY_ABORT_BLOCK_NIL; > >diff --git a/accessible/src/mac/mozHTMLAccessible.mm b/accessible/src/mac/mozHTMLAccessible.mm >--- a/accessible/src/mac/mozHTMLAccessible.mm >+++ b/accessible/src/mac/mozHTMLAccessible.mm >@@ -8,16 +8,28 @@ > #import "mozHTMLAccessible.h" > > #import "HyperTextAccessible.h" > > #import "nsCocoaUtils.h" > > @implementation mozHeadingAccessible > >+- (NSString*)title >+{ >+ nsAutoString title; >+ // XXX use the flattening API when there are available >+ // see https://bugzilla.mozilla.org/show_bug.cgi?id=768298 nit, just bug xxxxxx is fine >+ // This content could be slightly out of sync currently nit, slightly doesn't seem to be either accuret or really helpful here. >+ if (!supportedAttributes) { >+ supportedAttributes = [[super accessibilityAttributeNames] mutableCopy]; >+ [supportedAttributes removeObject:NSAccessibilityChildrenAttribute]; >+ } >+ return supportedAttributes; nit, blank line after }

Verified fixed in Mozilla/5.0 (Macintosh; Intel Mac OS X 10.7; rv:16.0) Gecko/16.0 Firefox/16.0 from the 2012-07-09. Strange that it doesn't show me the actual build date.