New role for definition list

RESOLVED FIXED in mozilla15

Status

()

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

People

(Reporter: hub, Assigned: hub)

Tracking

Trunk
mozilla15
x86
All
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

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

5 years ago
Hub, if you don't have plans to hack it then could you turn it into good first bug please?
(Assignee)

Comment 2

5 years ago
I was thinking of doing it.
(Assignee)

Updated

5 years ago
Assignee: nobody → hub
(Assignee)

Comment 3

5 years ago
Created attachment 616295 [details] [diff] [review]
Implement new role for definition lists. r=
(Assignee)

Comment 4

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

Comment 6

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

Comment 7

5 years ago
Created attachment 617692 [details] [diff] [review]
Implement new role for definition lists.
(Assignee)

Updated

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

Comment 8

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

Comment 9

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/efa9d8546cc9
Target Milestone: --- → mozilla15
(Assignee)

Updated

5 years ago
Status: NEW → ASSIGNED

Updated

5 years ago
Flags: in-testsuite+
https://hg.mozilla.org/mozilla-central/rev/efa9d8546cc9
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Depends on: 765172
You need to log in before you can comment on or make changes to this bug.