718625 - [Mac] VoiceOver says "text" after each chunk of text it reads inside paragraphs, does not do that in Safari.

Status: VERIFIED FIXED

(Core :: Disability Access APIs, defect, P2)

Description

[Marco Zehe (:MarcoZ)]

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.

Comment 1

[Marco Zehe (:MarcoZ)]

An inconvenience, but nothing that actually blocks usability.

Comment 2

[Hubert Figuiere [:hub]]

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.

Comment 3

[Marco Zehe (:MarcoZ)]

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

Comment 4

[Hubert Figuiere [:hub]]

It is definitely a serious difference between 10.6 and 10.7.

Comment 5

[Hubert Figuiere [:hub]]

Attachment: Implement TextLeaf accessibles for Mac. r=

Comment 6

[Hubert Figuiere [:hub]]

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 7

[Trevor Saunders (:tbsaunde)]

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

Comment 8

[Hubert Figuiere [:hub]]

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

Comment 9

[Trevor Saunders (:tbsaunde)]

(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

Comment 10

[Hubert Figuiere [:hub]]

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

[Hubert Figuiere [:hub]]

Attachment: Implement TextLeaf accessibles for Mac. Fix Heading title.

Comment 12

[Hubert Figuiere [:hub]]

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 13

[Trevor Saunders (:tbsaunde)]

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.

Comment 14

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

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

Comment 15

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

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

Err bug 768298.

Comment 16

[Hubert Figuiere [:hub]]

Attachment: Implement TextLeaf accessibles for Mac. Fix Heading title.

Comment 18

[Trevor Saunders (:tbsaunde)]

Comment on attachment 636572 [details] [diff] [review]
Implement TextLeaf accessibles for Mac. Fix Heading title.

># HG changeset patch
># User Hub Figuière <[e-mail]>
># 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 }

Comment 19

[Hubert Figuiere [:hub]]

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

Comment 20

[(no longer active)]

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

Comment 21

[Marco Zehe (:MarcoZ)]

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.