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.

Inconvenient, but not critical for basic functionality.

bug 738460 might be what needs to be done.

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 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 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.

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

(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 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?

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

(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.

(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.

(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.

(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?

(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.

(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?

(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 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 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).

> > > > 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 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 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

(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

(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

(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 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)

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

(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

(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

checked in as it is NPOTB

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

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 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 on attachment 622597 [details] [diff] [review] Part 3: fix build on MacOS 10.5. r= I can build, thanks Hub.

(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 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.