Closed Bug 746358 Opened 8 years ago Closed 8 years ago

New role for definition list

Categories

(Core :: Disability Access APIs, defect)

x86
All
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla15

People

(Reporter: hub, Assigned: hub)

References

Details

Attachments

(1 file, 1 obsolete file)

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)
Hub, if you don't have plans to hack it then could you turn it into good first bug please?
I was thinking of doing it.
Assignee: nobody → 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.
Attachment #616295 - Flags: feedback?(trev.saunders)
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.
Attachment #616295 - Flags: feedback?(trev.saunders)
(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
Attachment #616295 - Attachment is obsolete: true
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 ("
Attachment #617692 - Flags: review?(trev.saunders)
Attachment #617692 - Flags: review?(trev.saunders) → review+
Status: NEW → ASSIGNED
Flags: in-testsuite+
https://hg.mozilla.org/mozilla-central/rev/efa9d8546cc9
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Depends on: 765172
You need to log in before you can comment on or make changes to this bug.