[Mac] VoiceOver often repeats the document title as if it were a groupbox/fieldset heading/legend

VERIFIED FIXED in mozilla12

Status

()

Core
Disability Access APIs
VERIFIED FIXED
5 years ago
5 years ago

People

(Reporter: MarcoZ, Assigned: hub)

Tracking

(Blocks: 1 bug)

Trunk
mozilla12
x86_64
Mac OS X
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(1 attachment, 5 obsolete attachments)

(Reporter)

Description

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

Comment 1

5 years ago
Forgot to mention that this is with the try-server build that contains patches for bug 705404 and bug 708144.
(Assignee)

Updated

5 years ago
Assignee: nobody → hub
(Reporter)

Comment 2

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

Comment 3

5 years ago
Created attachment 588909 [details] [diff] [review]
Rework the tabs hierarchy in Mac a11y. r=
(Assignee)

Updated

5 years ago
Attachment #588909 - Flags: review?(trev.saunders)
(Assignee)

Comment 4

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

Comment 5

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

Comment 6

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

Comment 8

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

Unicode.
(Assignee)

Comment 9

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

Comment 11

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

Comment 12

5 years ago
Created attachment 589644 [details] [diff] [review]
Rework the tabs hierarchy in Mac a11y.
(Assignee)

Updated

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

Updated

5 years ago
Attachment #589644 - Flags: review?(bolterbugz)
(Assignee)

Comment 13

5 years ago
Created attachment 589979 [details] [diff] [review]
Rework the tabs hierarchy in Mac a11y.
(Assignee)

Updated

5 years ago
Attachment #589644 - Attachment is obsolete: true
Attachment #589644 - Flags: review?(bolterbugz)
(Assignee)

Comment 14

5 years ago
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?
Attachment #589979 - Flags: review+
(Assignee)

Comment 16

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

5 years ago
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?

Comment 19

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

Comment 21

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

Comment 22

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

Comment 23

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

Comment 24

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

Comment 25

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

Comment 26

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

Comment 27

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

Comment 28

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

Comment 29

5 years ago
Created attachment 590964 [details] [diff] [review]
Rework the tabs hierarchy in Mac a11y.
(Assignee)

Updated

5 years ago
Attachment #589979 - Attachment is obsolete: true
(Reporter)

Comment 30

5 years ago
So, are there any comments left to address, or is this ready to land?
(Assignee)

Updated

5 years ago
Attachment #590964 - Flags: review?(trev.saunders)
(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.
Attachment #590964 - Flags: review?(trev.saunders) → review?(surkov.alexander)

Comment 34

5 years ago
(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 35

5 years ago
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?
Attachment #590964 - Flags: review?(surkov.alexander)
(Assignee)

Comment 36

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

Comment 37

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

Comment 38

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

Comment 39

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

Comment 40

5 years ago
Created attachment 591992 [details] [diff] [review]
Rework the tabs hierarchy in Mac a11y.
(Assignee)

Updated

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

Comment 41

5 years ago
Comment on attachment 591992 [details] [diff] [review]
Rework the tabs hierarchy in Mac a11y.

this should address all the remaining issues.
Attachment #591992 - Flags: review?(surkov.alexander)

Comment 42

5 years ago
(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 43

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

Comment 44

5 years ago
Created attachment 592052 [details] [diff] [review]
Rework the tabs hierarchy in Mac a11y.
(Assignee)

Updated

5 years ago
Attachment #591992 - Attachment is obsolete: true
Attachment #591992 - Flags: review?(surkov.alexander)
(Assignee)

Comment 45

5 years ago
Comment on attachment 592052 [details] [diff] [review]
Rework the tabs hierarchy in Mac a11y.

Sorry I had missed these :-/

This should be better.
Attachment #592052 - Flags: review?(surkov.alexander)

Comment 46

5 years ago
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
Attachment #592052 - Flags: review?(surkov.alexander) → review+
(Assignee)

Comment 47

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/31dffd0f03e5
Target Milestone: --- → mozilla12
https://hg.mozilla.org/mozilla-central/rev/31dffd0f03e5
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
(Reporter)

Comment 49

5 years ago
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.
Status: RESOLVED → VERIFIED
(Reporter)

Updated

5 years ago
Duplicate of this bug: 499927
You need to log in before you can comment on or make changes to this bug.