712924 - [Mac] li elements inside an ul element are announced as "list box"

Status: RESOLVED FIXED

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

Description

[Marco Zehe (:MarcoZ)]

If a page like the Nightly Start Page or other pages contains li elements within unordered lists, VoiceOver speaks these as "listbox". Expected behavior would be for it to speak the text or link, but not announce the elements as list boxes. The surrounding list is correctly announced as such, and as far as navigation is concerned, VoiceOver sees it the same as in Safari.

This is with the try-server build that contains the patches for bug 705404 and bug 708144.

Comment 1

[Marco Zehe (:MarcoZ)]

Inconvenient, but not critical for basic functionality.

Comment 2

[Hubert Figuiere [:hub]]

bug 738460 might be what needs to be done.

Comment 4

[Hubert Figuiere [:hub]]

Attachment: Fix list implementation: proper roles and subroles. r=

Comment 5

[Marco Zehe (:MarcoZ)]

Comment on attachment 614639 [details] [diff] [review]
Fix list implementation: proper roles and subroles. r=

Just a styling question: Wouldn't it make more sense to get the tag once at the beginning of the method before the switch statement? You need it in all the blocks anyway except "default".

Comment 6

[alexander :surkov (:asurkov)]

Comment on attachment 614639 [details] [diff] [review]
Fix list implementation: proper roles and subroles. r=

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

::: accessible/src/base/nsAccessNode.h
@@ +116,5 @@
>    nsIContent* GetContent() const { return mContent; }
>    virtual nsIDocument* GetDocumentNode() const
>      { return mContent ? mContent->OwnerDoc() : nsnull; }
> +  nsIAtom* GetHTMLTag() const
> +    { return mContent && mContent->IsHTML() ? mContent->Tag() : nsnull; }

do you really want this? I'd go with

nsIContent* content = mGeckoAccessible->GetContent();
if (!content->IsHTML())
  return nil;

if (content->Tag() == nsGkAtoms::ul || content->nsGkAtoms::ol)
  return NSAccessibilityContentListSubrole;

Comment 7

[Hubert Figuiere [:hub]]

Comment on attachment 614639 [details] [diff] [review]
Fix list implementation: proper roles and subroles. r=

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

::: accessible/src/base/nsAccessNode.h
@@ +116,5 @@
>    nsIContent* GetContent() const { return mContent; }
>    virtual nsIDocument* GetDocumentNode() const
>      { return mContent ? mContent->OwnerDoc() : nsnull; }
> +  nsIAtom* GetHTMLTag() const
> +    { return mContent && mContent->IsHTML() ? mContent->Tag() : nsnull; }

I was not sure how you wanted it, and my initial implementation didn't touch nsAccessNode. I'll do that then.

Comment 8

[Hubert Figuiere [:hub]]

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

Comment 9

[Hubert Figuiere [:hub]]

I have one more change to do apparently, related to VoiceOver role description. Not sure why I missed it.

Comment 10

[Hubert Figuiere [:hub]]

Attachment: Part 2: Return the proper role description for definition lists, with localization. r=

Comment 11

[alexander :surkov (:asurkov)]

(In reply to Hub Figuiere [:hub] from comment #8)
> https://hg.mozilla.org/integration/mozilla-inbound/rev/c7145f4ebd47

I'd say you should avoid to have local variables for retvals of inline methods.

Comment 12

[alexander :surkov (:asurkov)]

Comment on attachment 614943 [details] [diff] [review]
Part 2: Return the proper role description for definition lists, with localization. r=

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

::: accessible/src/mac/mozAccessible.mm
@@ +228,5 @@
>    if ([attribute isEqualToString:NSAccessibilityRoleDescriptionAttribute]) {
>      if (mRole == roles::DOCUMENT)
>        return utils::LocalizedString(NS_LITERAL_STRING("htmlContent"));
>  
> +    NSString* subrole = [self subrole];

you don't need to calculate subrole before you hit the role

@@ +233,5 @@
> +
> +    if ((mRole == roles::LISTITEM) && [subrole isEqualToString:@"AXTerm"])
> +      return utils::LocalizedString(NS_LITERAL_STRING("term"));
> +    if ((mRole == roles::PARAGRAPH) && [subrole isEqualToString:@"AXDefinition"])
> +      return utils::LocalizedString(NS_LITERAL_STRING("definition"));

just curious you can think of the case when not LISTITEM and AXTerm or not paragraph and AXDefinition. right?

Would it be better to provide inlines for dt and other stuffs like
bool IsDefinition() const
{
  return mRole == role::PARAGRPAH && mGeckoAcc->GetContent()->NodeInfo()->Equals(HTML_namespace, tagName);
}

and then you can use them for subrole and role description attributes.

@@ +235,5 @@
> +      return utils::LocalizedString(NS_LITERAL_STRING("term"));
> +    if ((mRole == roles::PARAGRAPH) && [subrole isEqualToString:@"AXDefinition"])
> +      return utils::LocalizedString(NS_LITERAL_STRING("definition"));
> +
> +    return NSAccessibilityRoleDescription([self role], subrole);

maybe it's time to have separate method for this attribute?

Comment 13

[alexander :surkov (:asurkov)]

Trevor, would be nice to hear your opinion on comment #12.

Comment 14

[Trevor Saunders (:tbsaunde)]

(In reply to alexander :surkov from comment #12)
> Comment on attachment 614943 [details] [diff] [review]
> Part 2: Return the proper role description for definition lists, with
> localization. r=
> 
> Review of attachment 614943 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: accessible/src/mac/mozAccessible.mm
> @@ +228,5 @@
> >    if ([attribute isEqualToString:NSAccessibilityRoleDescriptionAttribute]) {
> >      if (mRole == roles::DOCUMENT)
> >        return utils::LocalizedString(NS_LITERAL_STRING("htmlContent"));
> >  
> > +    NSString* subrole = [self subrole];
> 
> you don't need to calculate subrole before you hit the role

I don't follow this, he uses the subrole in the next line.

> @@ +233,5 @@
> > +
> > +    if ((mRole == roles::LISTITEM) && [subrole isEqualToString:@"AXTerm"])
> > +      return utils::LocalizedString(NS_LITERAL_STRING("term"));
> > +    if ((mRole == roles::PARAGRAPH) && [subrole isEqualToString:@"AXDefinition"])
> > +      return utils::LocalizedString(NS_LITERAL_STRING("definition"));
> 
> just curious you can think of the case when not LISTITEM and AXTerm or not
> paragraph and AXDefinition. right?

I think I can see the first with five seconds of thought and grep though I may be wrong.  It seems it could happen with xforms or ARIA.

> Would it be better to provide inlines for dt and other stuffs like
> bool IsDefinition() const
> {
>   return mRole == role::PARAGRPAH &&
> mGeckoAcc->GetContent()->NodeInfo()->Equals(HTML_namespace, tagName);
> }
> 
> and then you can use them for subrole and role description attributes.

or maybe we should add  some more roles to our internal set of roles?

> @@ +235,5 @@
> > +      return utils::LocalizedString(NS_LITERAL_STRING("term"));
> > +    if ((mRole == roles::PARAGRAPH) && [subrole isEqualToString:@"AXDefinition"])
> > +      return utils::LocalizedString(NS_LITERAL_STRING("definition"));
> > +
> > +    return NSAccessibilityRoleDescription([self role], subrole);
> 
> maybe it's time to have separate method for this attribute?

seems reasonable.

Comment 15

[alexander :surkov (:asurkov)]

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

> > you don't need to calculate subrole before you hit the role
> 
> I don't follow this, he uses the subrole in the next line.

I meant subrole shouldn't be called when it's not needed. So we could do
if (mRole == roles::LISTITEM) {
  if ([[self subrole] isEqualToString:@"AXTerm"])])
    // bla bla
}

> > just curious you can think of the case when not LISTITEM and AXTerm or not
> > paragraph and AXDefinition. right?
> 
> I think I can see the first with five seconds of thought and grep though I
> may be wrong.  It seems it could happen with xforms or ARIA.

I don't follow this since AXTerm is exposed for example (see prev patch) for the given role and given tag names.

> or maybe we should add  some more roles to our internal set of roles?

I think I'm fine with that.

Comment 16

[Trevor Saunders (:tbsaunde)]

(In reply to alexander :surkov from comment #15)
> (In reply to Trevor Saunders (:tbsaunde) from comment #14)
> 
> > > you don't need to calculate subrole before you hit the role
> > 
> > I don't follow this, he uses the subrole in the next line.
> 
> I meant subrole shouldn't be called when it's not needed. So we could do
> if (mRole == roles::LISTITEM) {
>   if ([[self subrole] isEqualToString:@"AXTerm"])])
>     // bla bla
> }

so, in some cases you delay calling it a litle I don't see what that gains you.  And the price is more ifs.

> > > just curious you can think of the case when not LISTITEM and AXTerm or not
> > > paragraph and AXDefinition. right?
> > 
> > I think I can see the first with five seconds of thought and grep though I
> > may be wrong.  It seems it could happen with xforms or ARIA.
> 
> I don't follow this since AXTerm is exposed for example (see prev patch) for
> the given role and given tag names.

well, consider <div role="listitem">foo</div> ARIA makes the role LISTITEM, but the tag isn't dt, so the subrole is NULL instead of axterm.

> > or maybe we should add  some more roles to our internal set of roles?
> 
> I think I'm fine with that.

I think I prefer to keep platform layers as "dumb" as reasonable.

Comment 17

[alexander :surkov (:asurkov)]

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

> so, in some cases you delay calling it a litle I don't see what that gains
> you.  And the price is more ifs.

we have hundred roles that subrole call is not needed for and have couple roles that subrole makes sense for

> well, consider <div role="listitem">foo</div> ARIA makes the role LISTITEM,
> but the tag isn't dt, so the subrole is NULL instead of axterm.

yep, but the point is we can do subrole call only. If subrole is "axterm" then we can be sure that role is listitem. At least that's true for now. So I wondered if this rule is preserved in the future. That contradicts to idea above though but anyway.

> > > or maybe we should add  some more roles to our internal set of roles?
> > 
> > I think I'm fine with that.
> 
> I think I prefer to keep platform layers as "dumb" as reasonable.

Ok, Hub, are you fine with that?

Comment 18

[Trevor Saunders (:tbsaunde)]

(In reply to alexander :surkov from comment #17)
> (In reply to Trevor Saunders (:tbsaunde) from comment #16)
> 
> > so, in some cases you delay calling it a litle I don't see what that gains
> > you.  And the price is more ifs.
> 
> we have hundred roles that subrole call is not needed for and have couple
> roles that subrole makes sense for

we started calling subrole() for all roles in bug 720001 iirc, do you want to reverse that, that seems tricky to make work.

> > well, consider <div role="listitem">foo</div> ARIA makes the role LISTITEM,
> > but the tag isn't dt, so the subrole is NULL instead of axterm.
> 
> yep, but the point is we can do subrole call only. If subrole is "axterm"
> then we can be sure that role is listitem. At least that's true for now. So
> I wondered if this rule is preserved in the future. That contradicts to idea
> above though but anyway.

ok, I see, I guess the idea atleast makes sense now.  However it seems like comparing an int to a constant before calling a function to compare strings is a good optimization.

Comment 19

[alexander :surkov (:asurkov)]

(In reply to Trevor Saunders (:tbsaunde) from comment #18)
> > > so, in some cases you delay calling it a litle I don't see what that gains
> > > you.  And the price is more ifs.
> > 
> > we have hundred roles that subrole call is not needed for and have couple
> > roles that subrole makes sense for
> 
> we started calling subrole() for all roles in bug 720001 iirc, do you want
> to reverse that, that seems tricky to make work.

I'm not sure what you mean especially since the referred bug doesn't seem to be correct. But compare an int constants and calling subrole when int constant is matched should be fast.

Alternatively we could go with extending internal roles set. So which one do you prefer?

Comment 20

[Trevor Saunders (:tbsaunde)]

(In reply to alexander :surkov from comment #19)
> (In reply to Trevor Saunders (:tbsaunde) from comment #18)
> > > > so, in some cases you delay calling it a litle I don't see what that gains
> > > > you.  And the price is more ifs.
> > > 
> > > we have hundred roles that subrole call is not needed for and have couple
> > > roles that subrole makes sense for
> > 
> > we started calling subrole() for all roles in bug 720001 iirc, do you want
> > to reverse that, that seems tricky to make work.
> 
> I'm not sure what you mean especially since the referred bug doesn't seem to
> be correct. But compare an int constants and calling subrole when int
> constant is matched should be fast.

I meant bug 721001 sorry.

> Alternatively we could go with extending internal roles set. So which one do
> you prefer?

I'd prefer to add to the internal role set atleast in the long term, but I think I oculd call that a follow up.

Comment 21

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

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

Comment 22

[Hubert Figuiere [:hub]]

Comment on attachment 614943 [details] [diff] [review]
Part 2: Return the proper role description for definition lists, with localization. r=

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

::: accessible/src/mac/mozAccessible.mm
@@ +233,5 @@
> +
> +    if ((mRole == roles::LISTITEM) && [subrole isEqualToString:@"AXTerm"])
> +      return utils::LocalizedString(NS_LITERAL_STRING("term"));
> +    if ((mRole == roles::PARAGRAPH) && [subrole isEqualToString:@"AXDefinition"])
> +      return utils::LocalizedString(NS_LITERAL_STRING("definition"));

I can't really think of a case. This is not really documented either on the Mac side.

I'll see with the inlines (need to read the rest of the comments).

@@ +235,5 @@
> +      return utils::LocalizedString(NS_LITERAL_STRING("term"));
> +    if ((mRole == roles::PARAGRAPH) && [subrole isEqualToString:@"AXDefinition"])
> +      return utils::LocalizedString(NS_LITERAL_STRING("definition"));
> +
> +    return NSAccessibilityRoleDescription([self role], subrole);

I guess I can split that out.

Comment 23

[Hubert Figuiere [:hub]]

Comment on attachment 614943 [details] [diff] [review]
Part 2: Return the proper role description for definition lists, with localization. r=

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

::: accessible/src/mac/mozAccessible.mm
@@ +228,5 @@
>    if ([attribute isEqualToString:NSAccessibilityRoleDescriptionAttribute]) {
>      if (mRole == roles::DOCUMENT)
>        return utils::LocalizedString(NS_LITERAL_STRING("htmlContent"));
>  
> +    NSString* subrole = [self subrole];

Well, actually, it is used in all cases: subrole is passed to NSAccessibilityRoleDescription(), line 239, path reached when mRole is neither LISTITEM nor PARAGRAPH (the if short circuit conditions line 234 and 236).

Comment 24

[Hubert Figuiere [:hub]]

> > > > or maybe we should add  some more roles to our internal set of roles?
> > > 
> > > I think I'm fine with that.
> > 
> > I think I prefer to keep platform layers as "dumb" as reasonable.
> 
> Ok, Hub, are you fine with that?

If there was a role for definition lists, I'd use it. This could be a new bug :-)

Comment 25

[Hubert Figuiere [:hub]]

Attachment: Part 2: Return the proper role description for definition lists, with localization.

Comment 26

[Hubert Figuiere [:hub]]

Comment on attachment 615491 [details] [diff] [review]
Part 2: Return the proper role description for definition lists, with localization.

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

::: accessible/src/mac/mozAccessible.mm
@@ +504,5 @@
>  
>    return nil;
>  }
>  
> +-(NSString*)roleDescription

'nit: already added the space after the dash locally. Will be rolled in on checkin

Comment 27

[alexander :surkov (:asurkov)]

Comment on attachment 615491 [details] [diff] [review]
Part 2: Return the proper role description for definition lists, with localization.

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

::: accessible/src/mac/mozAccessible.mm
@@ +514,5 @@
> +  
> +  if ((mRole == roles::LISTITEM) && [subrole isEqualToString:@"AXTerm"])
> +    return utils::LocalizedString(NS_LITERAL_STRING("term"));
> +  if ((mRole == roles::PARAGRAPH) && [subrole isEqualToString:@"AXDefinition"])
> +    return utils::LocalizedString(NS_LITERAL_STRING("definition"));

why don't you want to do
if (mRole == roles::LISTITEM) {
  if ([[self subrole] isEqualToString:@"AXTerm"])
    return bla;
}
to avoid subrole call when you don't need it

Comment 28

[alexander :surkov (:asurkov)]

(In reply to Hub Figuiere [:hub] from comment #24)

> If there was a role for definition lists, I'd use it. This could be a new
> bug :-)

file a bug? perhaps good first bug if yo want

Comment 29

[alexander :surkov (:asurkov)]

(In reply to Hub Figuiere [:hub] from comment #23)

> Well, actually, it is used in all cases: subrole is passed to
> NSAccessibilityRoleDescription(), line 239, path reached when mRole is
> neither LISTITEM nor PARAGRAPH (the if short circuit conditions line 234 and
> 236).

you could pass nil there I think, anyway up to you since your approach looks cleaner

Comment 30

[Hubert Figuiere [:hub]]

(In reply to alexander :surkov from comment #29)
> (In reply to Hub Figuiere [:hub] from comment #23)
> 
> > Well, actually, it is used in all cases: subrole is passed to
> > NSAccessibilityRoleDescription(), line 239, path reached when mRole is
> > neither LISTITEM nor PARAGRAPH (the if short circuit conditions line 234 and
> > 236).
> 
> you could pass nil there I think, anyway up to you since your approach looks
> cleaner

Actually no, I no longer pass nil. I believe it was an error to do so.

Comment 31

[Hubert Figuiere [:hub]]

Comment on attachment 615491 [details] [diff] [review]
Part 2: Return the proper role description for definition lists, with localization.

Need approval. Impact: a11y on MacOS. (still disabled anyway)

Comment 32

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

Note: Should be no risk as this code is not built into Fennec.

Comment 33

[Trevor Saunders (:tbsaunde)]

(In reply to David Bolter [:davidb] from comment #32)
> Note: Should be no risk as this code is not built into Fennec.

The tree rules don't say, but do NPOTB patches actually need review or is a=NPOTB acceptable?

on the other hand while I have no objection to this landing I'm not sure what it helps, but I'm happy to leave that up to hub

Comment 34

[Hubert Figuiere [:hub]]

(In reply to alexander :surkov from comment #28)
> (In reply to Hub Figuiere [:hub] from comment #24)
> 
> > If there was a role for definition lists, I'd use it. This could be a new
> > bug :-)
> 
> file a bug? perhaps good first bug if yo want

https://bugzilla.mozilla.org/show_bug.cgi?id=746358

Comment 35

[Hubert Figuiere [:hub]]

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

Comment 36

[Hubert Figuiere [:hub]]

checked in as it is NPOTB

Comment 37

[(no longer active)]

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

Comment 38

[Drew Willcoxon :adw]

This bug and bug 733513 broke compilation on 10.5 because the first patch here uses a couple of constants introduced in the 10.6 SDK.  I'm not sure which bug if either I should reopen, or if requiring 10.6 to build now is intentional.  If it is intentional, seems like that should be discussed first, and the Mac build docs should be updated.  (But yeah, I should upgrade.)

/usr/bin/g++-4.2 -o mozAccessible.o -c  -fvisibility=hidden -DMOZILLA_INTERNAL_API -D_IMPL_NS_COM -DEXPORT_XPT_API -DEXPORT_XPTC_API -D_IMPL_NS_GFX -D_IMPL_NS_WIDGET -DIMPL_XREAPI -DIMPL_NS_NET -DIMPL_THEBES  -DSTATIC_EXPORTABLE_JS_API -I/Users/adw/m-c/accessible/src/mac -I/Users/adw/m-c/accessible/src/mac/../base -I/Users/adw/m-c/accessible/src/mac/../generic -I/Users/adw/m-c/accessible/src/mac/../html -I/Users/adw/m-c/accessible/src/mac/../xul  -I/Users/adw/m-c/accessible/src/mac -I. -I../../../dist/include -I../../../dist/include/nsprpub  -I/Users/adw/m-c/obj-debug/dist/include/nspr -I/Users/adw/m-c/obj-debug/dist/include/nss      -fPIC  -fno-rtti -Wall -Wpointer-arith -Woverloaded-virtual -Werror=return-type -Wempty-body -Wno-ctor-dtor-privacy -Wno-overlength-strings -Wno-invalid-offsetof -Wno-variadic-macros -Wcast-align -g -gfull -isysroot /Developer/SDKs/MacOSX10.5.sdk -fno-exceptions -fno-strict-aliasing -fshort-wchar -ffunction-sections -fdata-sections -pthread -DNO_X11 -pipe  -DDEBUG -D_DEBUG -DTRACING -g -fno-omit-frame-pointer    -DMOZILLA_CLIENT -include ../../../mozilla-config.h -MD -MF .deps/mozAccessible.pp -fobjc-exceptions /Users/adw/m-c/accessible/src/mac/mozAccessible.mm

/Users/adw/m-c/accessible/src/mac/mozAccessible.mm: In function ‘NSString* -[mozAccessible subrole](mozAccessible*, objc_selector*)’:
/Users/adw/m-c/accessible/src/mac/mozAccessible.mm:479: error: ‘NSAccessibilityContentListSubrole’ was not declared in this scope
/Users/adw/m-c/accessible/src/mac/mozAccessible.mm:482: error: ‘NSAccessibilityDefinitionListSubrole’ was not declared in this scope

Comment 39

[Hubert Figuiere [:hub]]

We should still support building on 10.5, even though a11y support might be for 10.7 and onwards.

Anyway, the other question is why did the try server NOT notice this?

Comment 40

[Hubert Figuiere [:hub]]

Attachment: Part 3: fix build on MacOS 10.5. r=

Comment 41

[Hubert Figuiere [:hub]]

Comment on attachment 622597 [details] [diff] [review]
Part 3: fix build on MacOS 10.5. r=

Can you tell me if that is enough for you to build ? Thanks.

Comment 42

[Drew Willcoxon :adw]

Comment on attachment 622597 [details] [diff] [review]
Part 3: fix build on MacOS 10.5. r=

I can build, thanks Hub.

Comment 43

[Trevor Saunders (:tbsaunde)]

(In reply to Hub Figuiere [:hub] from comment #39)
> We should still support building on 10.5, even though a11y support might be
> for 10.7 and onwards.

I'm not sure I agree, we'll probably drop support for building there and possibly running there fairly soon, at which point we'll want to rip this stuff out.  I atleast am fairly fine with saying if you want to build there build with a11y disabled.

> Anyway, the other question is why did the try server NOT notice this?

because they're 10.7

Comment 44

[alexander :surkov (:asurkov)]

Comment on attachment 622597 [details] [diff] [review]
Part 3: fix build on MacOS 10.5. r=

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

r=me but Trevor raises a good point in comment #43.

Comment 45

[Hubert Figuiere [:hub]]

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

Comment 46

[Joe Drew (not getting mail)]

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