When navigating http://www.heise.de/newsticker with Ctrl+Option+RightArrow, VoiceOver often repeats the text that is the title of the document, or the name of the nsDocAccessible. It behaves as if it recognizes this document title like it does recognize a group box or fieldset/legend construct within a form when using Safari. This repetition is very irritating since it either covers, or prepends, the actual text that VoiceOver is supposed to speak. STR: 1. Load the page http://www.heise.de/newsticker. 2. Start navigating with Ctrl+Option+RightArrow, and listen to what VoiceOver says. Notice that it often repeats the document title "Heise Online - 7-Tage-News". In Safari, it does not do that.

Forgot to mention that this is with the try-server build that contains patches for bug 705404 and bug 708144.

Further investigation shows that this comes from the tab accessible in the "Browser tabs" toolbar, which is exposed as a grouping. This is wrong and should be a tab page accessible rather than a grouping accessible (in Mac terms, that is). So somewhere we map this tab to something grouping-like. To find this, find the "Browser tabs toolbar" and interact with it. With one tab open, there is the tab itself and an "Open a new tab" button next to it. This is where this superfless speech comes from.

Marco suggested that this patch might have a (bad) influence on iframe. This needs testing.

I checked with an iframe and the behaviour is now identical between Safari and Firefox. The only slight difference is that in Firefox, with my sample, it says "frame frame" while in Safari it says "frame 0". But that would be IMHO a different bug.

I#m testing this try-server build: http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/hfiguiere@mozilla.com-b2c705a32cf8/ I can confirm that the patch from this bug fixes this bug, and possibly more!

Comment on attachment 588909 [details] [diff] [review] Rework the tabs hierarchy in Mac a11y. r= >+ nsMacA11yUtils.mm \ get rid of the ns, and the a11y because its rdundant. having mac in the name is silly but I think we might be best off leaving that for now to namespace it from other utils files we might want to include. >+// Enable to be able to introspect the gecko type with Accessibility inspector >+#define GECKO_DUMP 0 We already have enough #ifdef MY_SPECIAL_DEBUG_FEATURE_MACRO things around gecko I hate to add more is there a reason #ifdef DEBUG doesn't work for you? > #import "mozActionElements.h" >+#import "nsMacA11yUtils.h" nit, blank line after the header for this file. >diff --git a/accessible/src/mac/nsMacA11yUtils.h b/accessible/src/mac/nsMacA11yUtils.h >new file mode 100644 >--- /dev/null >+++ b/accessible/src/mac/nsMacA11yUtils.h >@@ -0,0 +1,60 @@ >+/* -*- Mode: Objective-C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */ nit, vim line. >+#import <Foundation/Foundation.h> nit, it'd be nice if we could only pull in the bits we need not all of the standard lib which I'd expect that does. alternatively can we just not incldue it for now and wait until forward declarations aren't enough? >+namespace mac { namespace utils or util I'm not sure which I like more. >+ * Get a localized string from the string bundle. >+ * Return nil is not found. I'm tempted to say lets return the passed in string in the case there isn't a localization since returning nil seems to lead to the patern of Localize("foo") ? : @"foo"; >+ */ >+NSString* GetLocalizedString(const nsString& aString); name it LocalizeString()? if nothing else its as clear, and shorter. I wonder if we'd be happier if we took a char* and converted to a xpcom string in the function. >+#include "nsCocoaUtils.h" >+#include "nsAccessNode.h" nit, nsAccessNode.h before coco utils and blank line between

> I wonder if we'd be happier if we took a char* and converted to a xpcom > string in the function. > Unicode.

> I'm tempted to say lets return the passed in string in the case there isn't > a localization since returning nil seems to lead to the patern of > Localize("foo") ? : @"foo"; These are string IDs, not human readable strings.

(In reply to Hub Figuiere [:hub] from comment #8) > > I wonder if we'd be happier if we took a char* and converted to a xpcom > > string in the function. > > > > Unicode. that's only a problem if your unicode isn't utf8. (In reply to Hub Figuiere [:hub] from comment #9) > > I'm tempted to say lets return the passed in string in the case there isn't > > a localization since returning nil seems to lead to the patern of > > Localize("foo") ? : @"foo"; > > These are string IDs, not human readable strings. In the cases we have so far we still return them in the case that we don't have a localization, and that seems about as reasonable as anything else. I don't terrible mind either of these as they are, but util functions are to make our lives easier and don't have an API requirements on them, so I tend to think we should factor out whatever common pattern there is, but if you have plans for usage that doesn't fit the pattern that's fair enough.

(In reply to Trevor Saunders (:tbsaunde) from comment #10) > (In reply to Hub Figuiere [:hub] from comment #8) > > > I wonder if we'd be happier if we took a char* and converted to a xpcom > > > string in the function. > > > > Unicode. > > that's only a problem if your unicode isn't utf8. We are not happier because of all the machinery involved to provide the Unicode UTF-16 string to the StringBundle from a string literal. NS_LITERAL_STRING is definitely the right thing, and no it does not work with a non literal.

Update patch to take into consideration comments at https://bugzilla.mozilla.org/show_bug.cgi?id=714976#c32

Comment on attachment 589979 [details] [diff] [review] Rework the tabs hierarchy in Mac a11y. Review of attachment 589979 [details] [diff] [review]: ----------------------------------------------------------------- r=me (but I'm still learning objective C) Glad you incorporated the translated string change from the other bug comment. I'm just trusting you on the mac/nsRoleMap.h changes. Aside: Regarding the DEBUG def. We should probably start using something like A11Y_DEBUG in my opinion, because DEBUG is just too noisy in the console. Also DEBUG actually changes code in other modules which isn't necessarily always what we want. I don't require changes on this patch though. ::: accessible/src/mac/MacUtils.h @@ +44,5 @@ > +class nsString; > + > +namespace mozilla { > +namespace a11y { > +namespace mac { I think Trevor wanted ::utils here instead of mac. I actually don't mind, but would be good to hear rationale. ::: accessible/src/mac/mozAccessible.mm @@ +244,5 @@ > if ([attribute isEqualToString:NSAccessibilityValueAttribute]) > return [self value]; > if ([attribute isEqualToString:NSAccessibilityRoleDescriptionAttribute]) { > + if (mRole == roles::DOCUMENT) > + return mac::LocalizedString(NS_LITERAL_STRING("htmlContent")) ? : @"HTML Content"; Good. ::: accessible/src/mac/mozActionElements.mm @@ +122,5 @@ > NS_OBJC_BEGIN_TRY_ABORT_BLOCK_NIL; > > + if ([action isEqualToString:NSAccessibilityPressAction]) { > + if (mGeckoAccessible->Role() == roles::PAGETAB) > + return mac::LocalizedString(NS_LITERAL_STRING("switch")) ? : @"Switch"; Was the capital 'S' intended? @@ +153,5 @@ > +- (BOOL)isTab > +{ > + return (mGeckoAccessible > + && (mGeckoAccessible->Role() == roles::PAGETAB)); > +} Nit: can this get inlined?

> ::: accessible/src/mac/MacUtils.h > @@ +44,5 @@ > > +class nsString; > > + > > +namespace mozilla { > > +namespace a11y { > > +namespace mac { > > I think Trevor wanted ::utils here instead of mac. I actually don't mind, > but would be good to hear rationale. Because it is Mac specific. I would think a "util" namespace would be platform agnostic. > ::: accessible/src/mac/mozActionElements.mm > @@ +122,5 @@ > > NS_OBJC_BEGIN_TRY_ABORT_BLOCK_NIL; > > > > + if ([action isEqualToString:NSAccessibilityPressAction]) { > > + if (mGeckoAccessible->Role() == roles::PAGETAB) > > + return mac::LocalizedString(NS_LITERAL_STRING("switch")) ? : @"Switch"; > > Was the capital 'S' intended? The current value in the properties file is with an uppercase "S". > @@ +153,5 @@ > > +- (BOOL)isTab > > +{ > > + return (mGeckoAccessible > > + && (mGeckoAccessible->Role() == roles::PAGETAB)); > > +} > > Nit: can this get inlined? Unfortunately we can't. This is Objective-C.

(In reply to Hub Figuiere [:hub] from comment #16) > > ::: accessible/src/mac/MacUtils.h > > @@ +44,5 @@ > > > +class nsString; > > > + > > > +namespace mozilla { > > > +namespace a11y { > > > +namespace mac { > > > > I think Trevor wanted ::utils here instead of mac. I actually don't mind, > > but would be good to hear rationale. > > Because it is Mac specific. I would think a "util" namespace would be > platform agnostic. I'd think it would be what it says on the tin that is to say util stuff, not neccessarily for all platforms or for only one. On the other hand mac adds no meaning on tom of the fact that its for mac which is implied by it being in mac/.

Comment on attachment 589979 [details] [diff] [review] Rework the tabs hierarchy in Mac a11y. Review of attachment 589979 [details] [diff] [review]: ----------------------------------------------------------------- ::: accessible/src/base/nsAccessible.h @@ +602,5 @@ > * Return container widget this accessible belongs to. > */ > virtual nsAccessible* ContainerWidget() const; > > + static nsresult GetTranslatedString(const nsAString& aKey, nsAString& aStringOut); while you are here, can you change it to something like static void TranslateString(...) btw, what about nsAccessNode::GetStringBundle? ::: accessible/src/mac/MacUtils.mm @@ +47,5 @@ > +namespace a11y { > +namespace mac { > + > +/** > + * Get a localized string from the string bundle. maybe "from a11y string bundle" @@ +48,5 @@ > +namespace mac { > + > +/** > + * Get a localized string from the string bundle. > + * Return nil is not found. is -> if? @@ +58,5 @@ > + > + nsresult rv = nsAccessible::GetTranslatedString(aString, text); > + NS_ENSURE_SUCCESS(rv, nil); > + > + return !text.IsEmpty() ? nsCocoaUtils::ToNSString(text) : nil; return text.IsEmpty() ? nil : nsCocoaUtils:: ::: accessible/src/mac/mozAccessible.mm @@ +244,5 @@ > if ([attribute isEqualToString:NSAccessibilityValueAttribute]) > return [self value]; > if ([attribute isEqualToString:NSAccessibilityRoleDescriptionAttribute]) { > + if (mRole == roles::DOCUMENT) > + return mac::LocalizedString(NS_LITERAL_STRING("htmlContent")) ? : @"HTML Content"; wrong indentation when LocalizedString returns nil? Should we care about that actually? ::: accessible/src/mac/mozActionElements.h @@ +57,5 @@ > + > +/* Class for tabs - not individual tabs */ > +@interface mozTabsAccessible : mozAccessible > +{ > + NSMutableArray* mTabs; do you really need to keep tabs separately, dynamic calculation is not good option? ::: accessible/src/mac/mozActionElements.mm @@ +152,5 @@ > > +- (BOOL)isTab > +{ > + return (mGeckoAccessible > + && (mGeckoAccessible->Role() == roles::PAGETAB)); wrong indentation, do not put operator at line start ::: accessible/src/mac/nsAccessibleWrap.mm @@ +102,5 @@ > { > // if this button may show a popup, let's make it of the popupbutton type. > return HasPopup() ? [mozPopupButtonAccessible class] : > [mozButtonAccessible class]; > } do you expect some tabs to have popups?

(In reply to Hub Figuiere [:hub] from comment #16) > > > +- (BOOL)isTab > > > +{ > > > + return (mGeckoAccessible > > > + && (mGeckoAccessible->Role() == roles::PAGETAB)); > > > +} > > > > Nit: can this get inlined? > > Unfortunately we can't. This is Objective-C. I think David meant to keep this on one line.

(In reply to alexander :surkov from comment #18) > ::: accessible/src/mac/nsAccessibleWrap.mm > @@ +102,5 @@ > > { > > // if this button may show a popup, let's make it of the popupbutton type. > > return HasPopup() ? [mozPopupButtonAccessible class] : > > [mozButtonAccessible class]; > > } > > do you expect some tabs to have popups? I wondered this too but not enough to ask. Is this something Safari special cases?

(In reply to David Bolter [:davidb] from comment #20) > > > return HasPopup() ? [mozPopupButtonAccessible class] : > > > [mozButtonAccessible class]; > > > } > > > > do you expect some tabs to have popups? > > I wondered this too but not enough to ask. Is this something Safari special > cases? I think Hub just added one more 'case' statement into existing code

Comment on attachment 589979 [details] [diff] [review] Rework the tabs hierarchy in Mac a11y. Review of attachment 589979 [details] [diff] [review]: ----------------------------------------------------------------- ::: accessible/src/base/nsAccessible.h @@ +602,5 @@ > * Return container widget this accessible belongs to. > */ > virtual nsAccessible* ContainerWidget() const; > > + static nsresult GetTranslatedString(const nsAString& aKey, nsAString& aStringOut); I'd like to avoid, here, making changes that clearly might impact all platforms. ::: accessible/src/mac/mozAccessible.mm @@ +244,5 @@ > if ([attribute isEqualToString:NSAccessibilityValueAttribute]) > return [self value]; > if ([attribute isEqualToString:NSAccessibilityRoleDescriptionAttribute]) { > + if (mRole == roles::DOCUMENT) > + return mac::LocalizedString(NS_LITERAL_STRING("htmlContent")) ? : @"HTML Content"; Yes we do. That mean we return a default untranslated string. This is what one call "degrade nicely". ::: accessible/src/mac/mozActionElements.h @@ +57,5 @@ > + > +/* Class for tabs - not individual tabs */ > +@interface mozTabsAccessible : mozAccessible > +{ > + NSMutableArray* mTabs; The universal access API a called more often than we change the number of tabs. So yes. ::: accessible/src/mac/mozActionElements.mm @@ +152,5 @@ > > +- (BOOL)isTab > +{ > + return (mGeckoAccessible > + && (mGeckoAccessible->Role() == roles::PAGETAB)); I put on one line as it end at column 76. ::: accessible/src/mac/nsAccessibleWrap.mm @@ +102,5 @@ > { > // if this button may show a popup, let's make it of the popupbutton type. > return HasPopup() ? [mozPopupButtonAccessible class] : > [mozButtonAccessible class]; > } I didn't touch that. And yes they might have (the actually do). I only added the case to deal with the tab button being a button and treated as such. That's how safari does it.

> > > Nit: can this get inlined? > > > > Unfortunately we can't. This is Objective-C. > > I think David meant to keep this on one line. Nope. He meant "inline". We clarified it on IRC.

(In reply to Hub Figuiere [:hub] from comment #22) > > + static nsresult GetTranslatedString(const nsAString& aKey, nsAString& aStringOut); > > I'd like to avoid, here, making changes that clearly might impact all > platforms. it shouldn't ever fail, so basically that's method renaming. I wouldn't expect it affects on other platforms. > ::: accessible/src/mac/mozAccessible.mm > @@ +244,5 @@ > > if ([attribute isEqualToString:NSAccessibilityValueAttribute]) > > return [self value]; > > if ([attribute isEqualToString:NSAccessibilityRoleDescriptionAttribute]) { > > + if (mRole == roles::DOCUMENT) > > + return mac::LocalizedString(NS_LITERAL_STRING("htmlContent")) ? : @"HTML Content"; > > Yes we do. That mean we return a default untranslated string. This is what > one call "degrade nicely". as far as I remember that's not really localized bundle so it always returns "HTML Content". On the another side if we would have localized versions of the file then it would contain untranslated string which is "HTMLContent" again. > > + NSMutableArray* mTabs; > > The universal access API a called more often than we change the number of > tabs. So yes. ok, that's something weird because mTabs are quite close to mChildren. maybe one day we need to make sure mTabs and mChildren is the same thing. > > + return (mGeckoAccessible > > + && (mGeckoAccessible->Role() == roles::PAGETAB)); > > I put on one line as it end at column 76. thanks > > // if this button may show a popup, let's make it of the popupbutton type. > > return HasPopup() ? [mozPopupButtonAccessible class] : > > [mozButtonAccessible class]; > > } > > I didn't touch that. And yes they might have (the actually do). example please?

(In reply to Hub Figuiere [:hub] from comment #23) > > I think David meant to keep this on one line. > > Nope. He meant "inline". We clarified it on IRC. ok, then does it make sense to keep a method for that since it's used once only?

> > ::: accessible/src/mac/mozAccessible.mm > > @@ +244,5 @@ > > > if ([attribute isEqualToString:NSAccessibilityValueAttribute]) > > > return [self value]; > > > if ([attribute isEqualToString:NSAccessibilityRoleDescriptionAttribute]) { > > > + if (mRole == roles::DOCUMENT) > > > + return mac::LocalizedString(NS_LITERAL_STRING("htmlContent")) ? : @"HTML Content"; > > > > Yes we do. That mean we return a default untranslated string. This is what > > one call "degrade nicely". > > as far as I remember that's not really localized bundle so it always returns > "HTML Content". It is. I added it in a previous patch. > > > > + NSMutableArray* mTabs; > > > > The universal access API a called more often than we change the number of > > tabs. So yes. > > ok, that's something weird because mTabs are quite close to mChildren. maybe > one day we need to make sure mTabs and mChildren is the same thing. "quite close" is the keyword. We have the [+] button, or the two [<] and [>] buttons depending on the number of tabs. Safari layout is similar with AXChildren having more than AXTabs. > > > // if this button may show a popup, let's make it of the popupbutton type. > > > return HasPopup() ? [mozPopupButtonAccessible class] : > > > [mozButtonAccessible class]; > > > } > > > > I didn't touch that. And yes they might have (the actually do). > > example please? They actually don't (I'm confusing popup and contextual menus). Then that code works anyway. The problem is that before my patch, tabs were considered as group instead of a button we can activate to switch. Which is what I'm doing here.

(In reply to Hub Figuiere [:hub] from comment #26) > > > ::: accessible/src/mac/mozAccessible.mm > > > @@ +244,5 @@ > > > > if ([attribute isEqualToString:NSAccessibilityValueAttribute]) > > > > return [self value]; > > > > if ([attribute isEqualToString:NSAccessibilityRoleDescriptionAttribute]) { > > > > + if (mRole == roles::DOCUMENT) > > > > + return mac::LocalizedString(NS_LITERAL_STRING("htmlContent")) ? : @"HTML Content"; > > > > > > Yes we do. That mean we return a default untranslated string. This is what > > > one call "degrade nicely". > > > > as far as I remember that's not really localized bundle so it always returns > > "HTML Content". > > It is. I added it in a previous patch. I meant english locale is used not depending on your locale. > > > > + NSMutableArray* mTabs; > > > > > > The universal access API a called more often than we change the number of > > > tabs. So yes. > > > > ok, that's something weird because mTabs are quite close to mChildren. maybe > > one day we need to make sure mTabs and mChildren is the same thing. > > "quite close" is the keyword. We have the [+] button, or the two [<] and [>] > buttons depending on the number of tabs. Safari layout is similar with > AXChildren having more than AXTabs. yes, right, that's quite close is keyword and that's I don't like, we keep two arrays that are mostly the same. Does webkit keep two arrays as well? > > > > > // if this button may show a popup, let's make it of the popupbutton type. > > > > return HasPopup() ? [mozPopupButtonAccessible class] : > > > > [mozButtonAccessible class]; > > > > } > > > > > > I didn't touch that. And yes they might have (the actually do). > > > > example please? > > They actually don't (I'm confusing popup and contextual menus). Then that > code works anyway. The problem is that before my patch, tabs were considered > as group instead of a button we can activate to switch. Which is what I'm > doing here. I see the code is correct, just useless. I think I'd prefer to keep it separately.

> yes, right, that's quite close is keyword and that's I don't like, we keep > two arrays that are mostly the same. Does webkit keep two arrays as well? It is not WebKit. It is Safari, and we don't have Safari source code. But the Accessibility Inspector show two different arrays from Safari anyway as AXChildren also contains he "new tab" button in Safari (and eventually the "more tab"). Also this Tabs array is what makes VoiceOver decide to say "Tab 1 of 2".

So, are there any comments left to address, or is this ready to land?

(In reply to Hub Figuiere [:hub] from comment #28) > > yes, right, that's quite close is keyword and that's I don't like, we keep > > two arrays that are mostly the same. Does webkit keep two arrays as well? > > It is not WebKit. It is Safari, and we don't have Safari source code. But Safari uses webkit though, and I seem to see stuff that looks like an impl of the mac a11y protocol in it.

(In reply to alexander :surkov from comment #24) > (In reply to Hub Figuiere [:hub] from comment #22) > > > > + static nsresult GetTranslatedString(const nsAString& aKey, nsAString& aStringOut); > > > > I'd like to avoid, here, making changes that clearly might impact all > > platforms. > > it shouldn't ever fail, so basically that's method renaming. I wouldn't > expect it affects on other platforms. I'm going to agree that I'd really like to see this get fixed while your here.

Comment on attachment 590964 [details] [diff] [review] Rework the tabs hierarchy in Mac a11y. I'm not nearly with it enough to review tonight, so -> Alex in the hope he is.

(In reply to Trevor Saunders (:tbsaunde) from comment #31) > (In reply to Hub Figuiere [:hub] from comment #28) > > > yes, right, that's quite close is keyword and that's I don't like, we keep > > > two arrays that are mostly the same. Does webkit keep two arrays as well? > > > > It is not WebKit. It is Safari, and we don't have Safari source code. But > > Safari uses webkit though, and I seem to see stuff that looks like an impl > of the mac a11y protocol in it. I think Webkit doesn't have accessibility implementation for UI used in Safari. I guess that's why Hub said that.

Comment on attachment 590964 [details] [diff] [review] Rework the tabs hierarchy in Mac a11y. Review of attachment 590964 [details] [diff] [review]: ----------------------------------------------------------------- btw, I don't where you answered the comment "btw, what about nsAccessNode::GetStringBundle?" cancelling review until comments are answered ::: accessible/src/base/nsAccessible.cpp @@ +584,5 @@ > *aIndexInParent = IndexInParent(); > return *aIndexInParent != -1 ? NS_OK : NS_ERROR_FAILURE; > } > > +nsresult nsAccessible::TranslateString(const nsAString& aKey, nsAString& aStringOut) nit: nsresult on own line btw, iirc I said that before, I don't see why we need nsresult, void should work perfectly ::: accessible/src/base/nsAccessible.h @@ +610,5 @@ > * Return container widget this accessible belongs to. > */ > virtual nsAccessible* ContainerWidget() const; > > + static nsresult TranslateString(const nsAString& aKey, nsAString& aStringOut); comment it? ::: accessible/src/mac/MacUtils.mm @@ +44,5 @@ > +#include "nsCocoaUtils.h" > + > +namespace mozilla { > +namespace a11y { > +namespace mac { I don't recall if you and Trevor agreed on it namespace name. What's the status? ::: accessible/src/mac/mozAccessible.mm @@ +244,5 @@ > if ([attribute isEqualToString:NSAccessibilityValueAttribute]) > return [self value]; > if ([attribute isEqualToString:NSAccessibilityRoleDescriptionAttribute]) { > + if (mRole == roles::DOCUMENT) > + return mac::LocalizedString(NS_LITERAL_STRING("htmlContent")) ? : @"HTML Content"; Did you answered when LocalizedString fails and "HTML Content" is used? ::: accessible/src/mac/mozActionElements.mm @@ +151,5 @@ > } > > +- (BOOL)isTab > +{ > + return (mGeckoAccessible && (mGeckoAccessible->Role() == roles::PAGETAB)); Did you answered why you need a function for that?

Comment on attachment 590964 [details] [diff] [review] Rework the tabs hierarchy in Mac a11y. Review of attachment 590964 [details] [diff] [review]: ----------------------------------------------------------------- ::: accessible/src/mac/MacUtils.mm @@ +44,5 @@ > +#include "nsCocoaUtils.h" > + > +namespace mozilla { > +namespace a11y { > +namespace mac { My take is that this is mac specific. His take is that it is a "utility". What's your take. ::: accessible/src/mac/mozAccessible.mm @@ +244,5 @@ > if ([attribute isEqualToString:NSAccessibilityValueAttribute]) > return [self value]; > if ([attribute isEqualToString:NSAccessibilityRoleDescriptionAttribute]) { > + if (mRole == roles::DOCUMENT) > + return mac::LocalizedString(NS_LITERAL_STRING("htmlContent")) ? : @"HTML Content"; If the localization is found, it use the localization, otherwise "HTML Content" (which happen to be the default English localization). ::: accessible/src/mac/mozActionElements.mm @@ +151,5 @@ > } > > +- (BOOL)isTab > +{ > + return (mGeckoAccessible && (mGeckoAccessible->Role() == roles::PAGETAB)); Simply because I know I will need it for some other case. Call it a hunch if you want, but this is way cleaner IMHO.

(In reply to Hub Figuiere [:hub] from comment #36) > > +namespace mozilla { > > +namespace a11y { > > +namespace mac { > > My take is that this is mac specific. His take is that it is a "utility". > What's your take. I don't have strong opinion on this. I think I could live with either case. Right now we have #nsCoreUtils for platform stuffs #nsAccUtils for a11y stuffs #nsWinUtils for windows stuffs I think we could have common utils namespace keeping methods in different files or we could have coreutils, a11yutils, winutils and macutils namespaces. > > + return mac::LocalizedString(NS_LITERAL_STRING("htmlContent")) ? : @"HTML Content"; > > If the localization is found, it use the localization, otherwise "HTML > Content" (which happen to be the default English localization). right, the question is when the localization is not found. at least that's not possible in DOMInspector extension for example which gives you an error during building. Maybe Firefox localized version allows to do that, I don't know. Anyway, it's weird that you need to maintain human readable strings through whole code. For example, if you need to use one key twice then you need to make sure you keep all instances of "HTML Content" string in sync. > > +- (BOOL)isTab > > +{ > > + return (mGeckoAccessible && (mGeckoAccessible->Role() == roles::PAGETAB)); > > Simply because I know I will need it for some other case. ok. But if that's supposed to be used inside of mozButtonAccessible then maybe it makes sense to create a separate class for tab. Up to you though.

Removed the literal if the localization fail. isTab is now used in two other locations. I don't know why it was not. I didn't make it a separate class because it is still an AXButton. Just that its action is "Switch". Using namespace utils instead of mac. New patch to come soon.

> btw, I don't where you answered the comment "btw, what about > nsAccessNode::GetStringBundle?" I answered by no longer using it in the new code since I use nsAccessible::TranslateString() instead.

Comment on attachment 591992 [details] [diff] [review] Rework the tabs hierarchy in Mac a11y. this should address all the remaining issues.

(In reply to Hub Figuiere [:hub] from comment #39) > > btw, I don't where you answered the comment "btw, what about > > nsAccessNode::GetStringBundle?" > > I answered by no longer using it in the new code since I use > nsAccessible::TranslateString() instead. right, but it's still presented in code. It should be removed here if you didn't removed it in other patches.

Comment on attachment 591992 [details] [diff] [review] Rework the tabs hierarchy in Mac a11y. Review of attachment 591992 [details] [diff] [review]: ----------------------------------------------------------------- I'd r+ if you were addressed all previous comments ::: accessible/src/base/nsAccessible.cpp @@ +584,5 @@ > *aIndexInParent = IndexInParent(); > return *aIndexInParent != -1 ? NS_OK : NS_ERROR_FAILURE; > } > > +nsresult nsAccessible::TranslateString(const nsAString& aKey, nsAString& aStringOut) still not addressed ::: accessible/src/base/nsAccessible.h @@ +610,5 @@ > * Return container widget this accessible belongs to. > */ > virtual nsAccessible* ContainerWidget() const; > > + static nsresult TranslateString(const nsAString& aKey, nsAString& aStringOut); not addressed

Comment on attachment 592052 [details] [diff] [review] Rework the tabs hierarchy in Mac a11y. Sorry I had missed these :-/ This should be better.

Comment on attachment 592052 [details] [diff] [review] Rework the tabs hierarchy in Mac a11y. Review of attachment 592052 [details] [diff] [review]: ----------------------------------------------------------------- ::: accessible/src/base/nsAccessible.h @@ +611,5 @@ > */ > virtual nsAccessible* ContainerWidget() const; > > + /** > + * Obtain in aStringOut the string for key aKey in the current locale. maybe: return the localized string for the given key in general you don't need to mention variable names in method description. If you need then you should use @param for that like /** * Return the localized string by the given key * * @param aKey [in] the key * @param */ but that's not necessary in this case

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.