Last Comment Bug 746358 - New role for definition list
: New role for definition list
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Disability Access APIs (show other bugs)
: Trunk
: x86 All
: -- normal (vote)
: mozilla15
Assigned To: Hubert Figuiere [:hub]
:
Mentors:
Depends on: 765172
Blocks:
  Show dependency treegraph
 
Reported: 2012-04-17 15:15 PDT by Hubert Figuiere [:hub]
Modified: 2012-06-19 11:03 PDT (History)
3 users (show)
surkov.alexander: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Implement new role for definition lists. r= (20.54 KB, patch)
2012-04-18 14:45 PDT, Hubert Figuiere [:hub]
no flags Details | Diff | Review
Implement new role for definition lists. (14.97 KB, patch)
2012-04-23 16:18 PDT, Hubert Figuiere [:hub]
tbsaunde+mozbugs: review+
Details | Diff | Review

Description Hubert Figuiere [:hub] 2012-04-17 15:15:22 PDT
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 2012-04-17 19:45:38 PDT
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] 2012-04-17 21:13:40 PDT
I was thinking of doing it.
Comment 3 Hubert Figuiere [:hub] 2012-04-18 14:45:38 PDT
Created attachment 616295 [details] [diff] [review]
Implement new role for definition lists. r=
Comment 4 Hubert Figuiere [:hub] 2012-04-18 14:49:46 PDT
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) 2012-04-18 15:38:47 PDT
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] 2012-04-23 15:46:56 PDT
(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] 2012-04-23 16:18:44 PDT
Created attachment 617692 [details] [diff] [review]
Implement new role for definition lists.
Comment 8 Hubert Figuiere [:hub] 2012-04-23 16:57:11 PDT
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 10 :Ehsan Akhgari (busy, don't ask for review please) 2012-04-25 07:17:56 PDT
https://hg.mozilla.org/mozilla-central/rev/efa9d8546cc9

Note You need to log in before you can comment on or make changes to this bug.