Closed
Bug 746358
Opened 13 years ago
Closed 13 years ago
New role for definition list
Categories
(Core :: Disability Access APIs, defect)
Tracking
()
RESOLVED
FIXED
mozilla15
People
(Reporter: hub, Assigned: hub)
References
Details
Attachments
(1 file, 1 obsolete file)
14.97 KB,
patch
|
tbsaunde
:
review+
|
Details | Diff | Splinter Review |
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•13 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•13 years ago
|
||
I was thinking of doing it.
Assignee | ||
Updated•13 years ago
|
Assignee: nobody → hub
Assignee | ||
Comment 3•13 years ago
|
||
Assignee | ||
Comment 4•13 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 5•13 years ago
|
||
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•13 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•13 years ago
|
||
Assignee | ||
Updated•13 years ago
|
Attachment #616295 -
Attachment is obsolete: true
Assignee | ||
Comment 8•13 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)
Updated•13 years ago
|
Attachment #617692 -
Flags: review?(trev.saunders) → review+
Assignee | ||
Comment 9•13 years ago
|
||
Target Milestone: --- → mozilla15
Assignee | ||
Updated•13 years ago
|
Status: NEW → ASSIGNED
Updated•13 years ago
|
Flags: in-testsuite+
Comment 10•13 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•