746358 - New role for definition list

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Hubert Figuiere [:hub]]

We need new roles for definition list <dl>: DEFINITION list. 

Also <dt> and <dd> are currently LISTITEM and PARAGRAPH respectively. They should instead be something like TERM and DEFINITION.

See bug 712924 for part of the rationale (MacOS universal access API has subroles for that)

Comment 1

[alexander :surkov (:asurkov)]

Hub, if you don't have plans to hack it then could you turn it into good first bug please?

Comment 2

[Hubert Figuiere [:hub]]

I was thinking of doing it.

Comment 3

[Hubert Figuiere [:hub]]

Attachment: Implement new role for definition lists. r=

Comment 4

[Hubert Figuiere [:hub]]

Comment on attachment 616295 [details] [diff] [review]
Implement new role for definition lists. r=

Here is the patch, seeking feedback to make sure I do it properly.

mochitest passed on Linux. Try build in progress for Windows (actually all platform)

Mac hasn't been tested yet, but the code changes are in there.

Comment 5

[Trevor Saunders (:tbsaunde)]

Comment on attachment 616295 [details] [diff] [review]
Implement new role for definition lists. r=

>+
>+nsHTMLTermAccessible::
>+nsHTMLTermAccessible(nsIContent* aContent, nsDocAccessible* aDoc) :
>+  nsHTMLLIAccessible(aContent, aDoc)
>+{
>+  UpdateBullet(false);
>+}  
>+
>+role
>+nsHTMLTermAccessible::NativeRole()
>+{
>+  return roles::TERM;
>+}

I think just checking mContent->Tag() in nsHTMLLiAccessible::NativeRole() would probably make more sense than adding a class.

>+NS_IMPL_ISUPPORTS_INHERITED0(nsHTMLDefListAccessible, nsHyperTextAccessible)

is this needed, or can we get away with just inheriting nsHypertextAccessibleWrap's impls?

>+class nsHTMLDefListAccessible : public nsHyperTextAccessibleWrap

making these elements have there own class may make sense, but if we decide to do  that I think it should be its own bug.

otherwise the role stuff seems fine.

Comment 6

[Hubert Figuiere [:hub]]

(In reply to Trevor Saunders (:tbsaunde) from comment #5)
> Comment on attachment 616295 [details] [diff] [review]
> Implement new role for definition lists. r=
> 
> >+
> >+nsHTMLTermAccessible::
> >+nsHTMLTermAccessible(nsIContent* aContent, nsDocAccessible* aDoc) :
> >+  nsHTMLLIAccessible(aContent, aDoc)
> >+{
> >+  UpdateBullet(false);
> >+}  
> >+
> >+role
> >+nsHTMLTermAccessible::NativeRole()
> >+{
> >+  return roles::TERM;
> >+}
> 
> I think just checking mContent->Tag() in nsHTMLLiAccessible::NativeRole()
> would probably make more sense than adding a class.

I was not sure of the proper course of action. Not subclassing seems less heavy handed. I'm good with that.

> 
> >+NS_IMPL_ISUPPORTS_INHERITED0(nsHTMLDefListAccessible, nsHyperTextAccessible)
> 
> is this needed, or can we get away with just inheriting
> nsHypertextAccessibleWrap's impls?

No idea. I just copied over from similar classes.

> 
> >+class nsHTMLDefListAccessible : public nsHyperTextAccessibleWrap
> 
> making these elements have there own class may make sense, but if we decide
> to do  that I think it should be its own bug.
> 
> otherwise the role stuff seems fine.

ok

Comment 7

[Hubert Figuiere [:hub]]

Attachment: Implement new role for definition lists.

Comment 8

[Hubert Figuiere [:hub]]

Comment on attachment 617692 [details] [diff] [review]
Implement new role for definition lists.

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

::: accessible/src/html/nsHTMLTextAccessible.cpp
@@ +258,5 @@
>  
>  role
>  nsHTMLLIAccessible::NativeRole()
>  {
> +  if(mContent->Tag() == nsGkAtoms::dt)

nit: already fixed the spacing missing for the "if ("

@@ +412,5 @@
>  
>  role
>  nsHTMLListAccessible::NativeRole()
>  {
> +  if(mContent->Tag() == nsGkAtoms::dl)

nit: already fixed the spacing missing for the "if ("

Comment 9

[Hubert Figuiere [:hub]]

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

Comment 10

[(no longer active)]

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