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

RESOLVED FIXED in mozilla12

Status

()

Core
Disability Access APIs
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: hub, Assigned: hub)

Tracking

Trunk
mozilla12
x86
Mac OS X
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 4 obsolete attachments)

(Assignee)

Description

6 years ago
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.
(Assignee)

Updated

6 years ago
Assignee: nobody → hub
(Assignee)

Comment 1

6 years ago
Created attachment 585591 [details] [diff] [review]
proposed patch

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

Updated

6 years ago
Attachment #585591 - Flags: review?
(Assignee)

Comment 2

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

Comment 3

6 years ago
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 (.
(Assignee)

Comment 4

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

Comment 5

6 years ago
Created attachment 585932 [details] [diff] [review]
The role description should be HTML Content. r=
(Assignee)

Updated

6 years ago
Attachment #585591 - Attachment is obsolete: true
Attachment #585591 - Flags: review?
(Assignee)

Updated

6 years ago
Attachment #585932 - Flags: review?(trev.saunders)
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
Attachment #585932 - Flags: review?(trev.saunders) → feedback?(l10n)
(Assignee)

Comment 7

6 years ago
(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.
(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.
(Assignee)

Comment 9

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

Comment 10

6 years ago
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?
(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

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

Looks OK to me.
Attachment #585932 - Flags: feedback?(l10n) → feedback+
(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

6 years ago
Yes, you should support changing the language at runtime. Sorry, didn't find that question.
(Assignee)

Comment 15

6 years ago
(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.
(Assignee)

Comment 16

6 years ago
(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

6 years ago
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

6 years ago
PS: of course, you need to have a langpack installed, too.
(Assignee)

Comment 19

6 years ago
Created attachment 586307 [details] [diff] [review]
The role description should be HTML Content.
(Assignee)

Updated

6 years ago
Attachment #585932 - Attachment is obsolete: true
(Assignee)

Comment 20

6 years ago
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.
Attachment #586307 - Flags: review?(trev.saunders)
(Assignee)

Comment 21

5 years ago
Created attachment 586454 [details] [diff] [review]
The role description should be HTML Content.
(Assignee)

Updated

5 years ago
Attachment #586307 - Attachment is obsolete: true
Attachment #586307 - Flags: review?(trev.saunders)
(Assignee)

Updated

5 years ago
Attachment #586454 - Flags: review?(trev.saunders)
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
Attachment #586454 - Flags: review?(trev.saunders) → review+
(Assignee)

Comment 23

5 years ago
(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.
(Assignee)

Comment 24

5 years ago
Created attachment 586590 [details] [diff] [review]
The role description should be HTML Content.
(Assignee)

Updated

5 years ago
Attachment #586454 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Attachment #586590 - Flags: checkin?(trev.saunders)
(Assignee)

Updated

5 years ago
Keywords: checkin-needed
Add checkin-needed once this is reviewed.
Keywords: checkin-needed
(Assignee)

Comment 26

5 years ago
(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].
Keywords: checkin-needed
Landed on the accessibility project branch:
http://hg.mozilla.org/projects/accessibility/rev/8f723d91fb82
Keywords: checkin-needed
Target Milestone: --- → mozilla12
Landed on m-c: http://hg.mozilla.org/mozilla-central/rev/8f723d91fb82
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Attachment #586590 - Flags: checkin?(trev.saunders)
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).
(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.
Thanks a lot Marco :-)

Comment 32

5 years ago
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.
> 
> ::: 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

5 years ago
(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.
(Assignee)

Comment 35

5 years ago
Lets fix the localization stuff with the patch for bug 712927
(Assignee)

Comment 36

5 years ago
Also with bug 712927, the StringBundle is no longer exposed outside the MacUtils.mm
(Assignee)

Comment 37

5 years ago
> > 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...
(Assignee)

Comment 38

5 years ago
See attachment 589979 [details] [diff] [review] for bug 712927
You need to log in before you can comment on or make changes to this bug.