714976 - [mac] the Role description for the web content is AXWebArea instead of "HTML Content"

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Hubert Figuiere [:hub]]

The Role description for the web content is AXWebArea instead of "HTML Content". This is not user friendly and Voice Over reads it. "You are inside a AXWebArea".

Safari use the word "HTML Content". Let's use that and ensure it is localized.

Comment 1

[Hubert Figuiere [:hub]]

Attachment: proposed patch

Proposed patch, in principle. It is missing localization support has this string is "human readable" (actually read out loud by VoiceOver)

Comment 2

[Hubert Figuiere [:hub]]

bug 673689 will require changes to this patch (or vice versa).

Comment 3

[Marco Zehe (:MarcoZ)]

Comment on attachment 585591 [details] [diff] [review]
proposed patch

>+    if ((mRole == nsIAccessibleRole::ROLE_INTERNAL_FRAME) || (mRole == nsIAccessibleRole::ROLE_DOCUMENT_FRAME))

Just a nit: Put the part after the || onto a new line and make sure it is indented such that it startss at the same position as the second opening (.

Comment 4

[Hubert Figuiere [:hub]]

I have a new patch that deal with localization. Still missing a few bits for the strings, I'll update when it is solved.

Comment 5

[Hubert Figuiere [:hub]]

Attachment: The role description should be HTML Content. r=

Comment 6

[Trevor Saunders (:tbsaunde)]

Comment on attachment 585932 [details] [diff] [review]
The role description should be HTML Content. r=


>+#include "nsAccessNode.h"

what for?

>+static NSString* 
>+GetMacLocalizedString(const nsString& aString)

drop the Mac part since its in mac/ of course its for mac stuff only

>+{
>+  nsresult rv;

declare at first use not here

>+  nsCOMPtr<nsIStringBundleService> bundleService =
>+      mozilla::services::GetStringBundleService();

use namespace mozilla and just services::GetBlah()

>+  
>+  if (!bundleService)

no blank line

>+    return nil;
>+  
>+  nsCOMPtr<nsIStringBundle> bundle;
>+  rv = bundleService->CreateBundle(ACCESSIBLE_BUNDLE_URL,
>+                                   getter_AddRefs(bundle));
>+  
>+  if (!NS_SUCCEEDED(rv))
>+    return nil;

no blank line, and I'd probably do NS_ENSURE_SUCCESS(rv, nil)

>+  
>+  nsString text;
>+  rv = bundle->GetStringFromName(aString.get(),
>+                                 getter_Copies(text));
>+  if (NS_SUCCEEDED(rv))
>+    return [NSString stringWithCharacters:text.BeginReading()
>+                                  length:text.Length()];

NS_ENSURE_SUCCESS again, then make the last return be the success one

>+static NSString* sAXWebAreaLocalizedRoleDescription = nil;

do you have numbers showing a need to cache this? I'm not sure if we need to support changing the language at run time, Axel?

>+  if ([attribute isEqualToString:NSAccessibilityRoleDescriptionAttribute]) {
>+    if ((mRole == nsIAccessibleRole::ROLE_INTERNAL_FRAME) 
>+        || (mRole == nsIAccessibleRole::ROLE_DOCUMENT_FRAME))
>+      return GetAXWebAreaLocalizedRoleDescription();
>     return NSAccessibilityRoleDescription([self role], nil);

blank line between returns

Comment 7

[Hubert Figuiere [:hub]]

(In reply to Trevor Saunders (:tbsaunde) from comment #6)
> Comment on attachment 585932 [details] [diff] [review]
> The role description should be HTML Content. r=
> 
> 
> >+#include "nsAccessNode.h"
> 
> what for?

for ACCESSIBLE_BUNDLE_URL

> >+  nsCOMPtr<nsIStringBundleService> bundleService =
> >+      mozilla::services::GetStringBundleService();
> 
> use namespace mozilla and just services::GetBlah()

I did it that way because it is done like that everywhere else.

Will address the rest.

Comment 8

[Trevor Saunders (:tbsaunde)]

(In reply to Hub Figuiere [:hub] from comment #7)
> (In reply to Trevor Saunders (:tbsaunde) from comment #6)
> > Comment on attachment 585932 [details] [diff] [review]
> > The role description should be HTML Content. r=
> > 
> > 
> > >+#include "nsAccessNode.h"
> > 
> > what for?
> 
> for ACCESSIBLE_BUNDLE_URL

ok, thanks

> > >+  nsCOMPtr<nsIStringBundleService> bundleService =
> > >+      mozilla::services::GetStringBundleService();
> > 
> > use namespace mozilla and just services::GetBlah()
> 
> I did it that way because it is done like that everywhere else.
> 
> Will address the rest.

so far as the ones in accessible/ we really should do it the way I suggested, but don't because of some weird historical accident involving timing or something.  Note some of the files where it occurs also have using namespace mozilla for other things anyway, but if you'd really prefer I gues we can fix them all together.

Comment 9

[Hubert Figuiere [:hub]]

I'll wait for the l10n feedback before submitting a new patch.

Comment 10

[Axel Hecht [:Pike]]

Comment on attachment 585932 [details] [diff] [review]
The role description should be HTML Content. r=

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

Is there any concrete question you have?

::: dom/locales/en-US/chrome/accessibility/mac/accessible.properties
@@ +12,5 @@
>  activate=       Activate
>  cycle   =       Cycle
> +
> +# API
> +htmlContent = HTML Content

Not sure what the comment here means?

Comment 11

[Marco Zehe (:MarcoZ)]

(In reply to Axel Hecht [:Pike] from comment #10)
> Is there any concrete question you have?

I think the main reason we're looping you in here is because we want to make sure we're using the right approach for this kind of string. It is something that will be spoken by VoiceOver (the Mac screen reader for the blind) whenever a user navigates the Firefox UI and encounters the area where the actual web page is being rendered.

> 
> ::: dom/locales/en-US/chrome/accessibility/mac/accessible.properties
> @@ +12,5 @@
> >  activate=       Activate
> >  cycle   =       Cycle
> > +
> > +# API
> > +htmlContent = HTML Content
> 
> Not sure what the comment here means?

Yes the comment should be clearer, stating that this is spoken by VoiceOver when navigation lands on the web rendering area. This mirrors what VoiceOver says in this instance for Safari.

Comment 12

[Axel Hecht [:Pike]]

Comment on attachment 585932 [details] [diff] [review]
The role description should be HTML Content. r=

Looks OK to me.

Comment 13

[Trevor Saunders (:tbsaunde)]

(In reply to Axel Hecht [:Pike] from comment #10)
> Comment on attachment 585932 [details] [diff] [review]
> The role description should be HTML Content. r=
> 
> Review of attachment 585932 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Is there any concrete question you have?

wel, see

> NS_ENSURE_SUCCESS again, then make the last return be the success one
> 
> >+static NSString* sAXWebAreaLocalizedRoleDescription = nil;
> 
> do you have numbers showing a need to cache this? I'm not sure if we need to
> support changing the language at run time, Axel?

Comment 14

[Axel Hecht [:Pike]]

Yes, you should support changing the language at runtime. Sorry, didn't find that question.

Comment 15

[Hubert Figuiere [:hub]]

(In reply to Axel Hecht [:Pike] from comment #10)

> ::: dom/locales/en-US/chrome/accessibility/mac/accessible.properties
> @@ +12,5 @@
> >  activate=       Activate
> >  cycle   =       Cycle
> > +
> > +# API
> > +htmlContent = HTML Content
> 
> Not sure what the comment here means?

Sorry. I have put a much nicer explanation in the next version of the patch.

Comment 16

[Hubert Figuiere [:hub]]

(In reply to Axel Hecht [:Pike] from comment #14)
> Yes, you should support changing the language at runtime. Sorry, didn't find
> that question.

How do you change the UI language on Mac while Firefox is running?

Comment 17

[Axel Hecht [:Pike]]

locale switcher addon or go into about:config and edit general.useragent.locale, and then open a new window. There's no restart required.

Comment 18

[Axel Hecht [:Pike]]

PS: of course, you need to have a langpack installed, too.

Comment 19

[Hubert Figuiere [:hub]]

Attachment: The role description should be HTML Content.

Comment 20

[Hubert Figuiere [:hub]]

Comment on attachment 586307 [details] [diff] [review]
The role description should be HTML Content.

Updated with all the comments.

Including the caching of the string.

Comment 21

[Hubert Figuiere [:hub]]

Attachment: The role description should be HTML Content.

Comment 22

[Trevor Saunders (:tbsaunde)]

Comment on attachment 586454 [details] [diff] [review]
The role description should be HTML Content.

>+  static nsIStringBundle *GetStringBundle()

obligatory nit for Alex type* not type *

>+    { return gStringBundle; }

I think it probably makes sense to stop caching this altogether, but lets leave that for its own bug.

>+  nsString text;

nsAutoString

>+  if ([attribute isEqualToString:NSAccessibilityRoleDescriptionAttribute]) {
>+    if ((mRole == nsIAccessibleRole::ROLE_INTERNAL_FRAME) 
>+        || (mRole == nsIAccessibleRole::ROLE_DOCUMENT_FRAME))
>+      return GetLocalizedString(NS_LITERAL_STRING("htmlContent")) ? : @"HTML Content";
>+
>     return NSAccessibilityRoleDescription([self role], nil);
>+  }
>   if ([attribute isEqualToString:NSAccessibilityDescriptionAttribute])

blank line after }

r=me with that fixed

Comment 23

[Hubert Figuiere [:hub]]

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

> >+  nsString text;
> 
> nsAutoString

Not since I'm using it as an out argument for an XPCOM method call. Actually the proper type is nsXPIDLString.

Comment 24

[Hubert Figuiere [:hub]]

Attachment: The role description should be HTML Content.

Comment 25

[Dão Gottwald [:dao]]

Add checkin-needed once this is reviewed.

Comment 26

[Hubert Figuiere [:hub]]

(In reply to Dão Gottwald [:dao] from comment #25)
> Add checkin-needed once this is reviewed.

It has been reviewed. See attachment 586454 [details] [diff] [review].

Comment 27

[Marco Zehe (:MarcoZ)]

Landed on the accessibility project branch:
http://hg.mozilla.org/projects/accessibility/rev/8f723d91fb82

Comment 28

[Marco Zehe (:MarcoZ)]

Landed on m-c: http://hg.mozilla.org/mozilla-central/rev/8f723d91fb82

Comment 29

[Francesco Lodolo [:flod]]

Any help finding this feature in Safari? I'd like to use the same localization used in Safari for consistency (if it's not awful).

Comment 30

[Marco Zehe (:MarcoZ)]

(In reply to flod (Francesco Lodolo) from comment #29)
> Any help finding this feature in Safari? I'd like to use the same
> localization used in Safari for consistency (if it's not awful).

Open Safari, turn on VoiceOver with Cmd+F5, and with Option+Ctrl+Left and RightArrow, navigate through the main window until you hear "HTML Content". At the bottom of the screen, you also get a visual representation of what VoiceOver speaks.

Comment 31

[Francesco Lodolo [:flod]]

Thanks a lot Marco :-)

Comment 32

[alexander :surkov (:asurkov)]

Comment on attachment 586590 [details] [diff] [review]
The role description should be HTML Content.

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

::: accessible/src/mac/mozAccessible.mm
@@ +138,5 @@
> +
> +  nsXPIDLString text;
> +  nsresult rv = nsAccessNode::GetStringBundle()->GetStringFromName(aString.get(),
> +                                 getter_Copies(text));
> +  NS_ENSURE_SUCCESS(rv, nil);

nsAccessible provides GetTranslatedString. Why don't you use it or change it the way you need it. Exposing stringBundle object doesn't look like a good idea.

Comment 33

[Trevor Saunders (:tbsaunde)]

> 
> ::: accessible/src/mac/mozAccessible.mm
> @@ +138,5 @@
> > +
> > +  nsXPIDLString text;
> > +  nsresult rv = nsAccessNode::GetStringBundle()->GetStringFromName(aString.get(),
> > +                                 getter_Copies(text));
> > +  NS_ENSURE_SUCCESS(rv, nil);
> 
> nsAccessible provides GetTranslatedString. Why don't you use it or change it

I think a util function that returns a mac string is fine, though perhaps wrapping nsAccessible::GetTranslatedString() makes more sense.

btw, objections to moving it into some util file?

> the way you need it. Exposing stringBundle object doesn't look like a good
> idea.

Well, I believe that we shouldn't be caching the string bundle like that in the first place, but I don't see one more user of it making that fix any harder.

Comment 34

[alexander :surkov (:asurkov)]

(In reply to Trevor Saunders (:tbsaunde) from comment #33)
> > 
> > ::: accessible/src/mac/mozAccessible.mm
> > @@ +138,5 @@
> > > +
> > > +  nsXPIDLString text;
> > > +  nsresult rv = nsAccessNode::GetStringBundle()->GetStringFromName(aString.get(),
> > > +                                 getter_Copies(text));
> > > +  NS_ENSURE_SUCCESS(rv, nil);
> > 
> > nsAccessible provides GetTranslatedString. Why don't you use it or change it
> 
> I think a util function that returns a mac string is fine, 

that's ok if you plan to use it often, because otherwise I don't see a huge win GetLocalizedString over GetTranslatedString(text); nsCocoaUtils::ToNSString(text);.

> though perhaps
> wrapping nsAccessible::GetTranslatedString() makes more sense.
>
> btw, objections to moving it into some util file?

yep, if you like to expose some inline analogue. Technically I don't think it's good idea to keep this as a part of accessnode object, it belongs to accessibility service I'd say or utility as you suggest.
 
> > the way you need it. Exposing stringBundle object doesn't look like a good
> > idea.
> 
> Well, I believe that we shouldn't be caching the string bundle like that in
> the first place, but I don't see one more user of it making that fix any
> harder.

maybe we shouldn't.

Comment 35

[Hubert Figuiere [:hub]]

Lets fix the localization stuff with the patch for bug 712927

Comment 36

[Hubert Figuiere [:hub]]

Also with bug 712927, the StringBundle is no longer exposed outside the MacUtils.mm

Comment 37

[Hubert Figuiere [:hub]]

> > nsAccessible provides GetTranslatedString. Why don't you use it or change it
> 
> I think a util function that returns a mac string is fine, though perhaps
> wrapping nsAccessible::GetTranslatedString() makes more sense.

I'm fine with that but nsAccessible::GetTranslatedString() is protected...

Comment 38

[Hubert Figuiere [:hub]]

See attachment 589979 [details] [diff] [review] for bug 712927