Last Comment Bug 751828 - densify nsHTMLListAccessible
: densify nsHTMLListAccessible
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Disability Access APIs (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla15
Assigned To: alexander :surkov
:
Mentors:
Depends on:
Blocks: densifya11y
  Show dependency treegraph
 
Reported: 2012-05-04 01:27 PDT by alexander :surkov
Modified: 2012-05-08 11:18 PDT (History)
1 user (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (35.12 KB, patch)
2012-05-04 01:27 PDT, alexander :surkov
tbsaunde+mozbugs: review+
Details | Diff | Review

Description alexander :surkov 2012-05-04 01:27:53 PDT
Created attachment 620985 [details] [diff] [review]
patch
Comment 1 Trevor Saunders (:tbsaunde) 2012-05-06 22:27:39 PDT
Comment on attachment 620985 [details] [diff] [review]
patch

>+  HTMLLIAccessible(nsIContent* aContent, nsDocAccessible* aDoc) :
>   nsHyperTextAccessibleWrap(aContent, aDoc), mBullet(nsnull)
> {
>   mFlags |= eHTMLListItemAccessible;
> 
>   nsBlockFrame* blockFrame = do_QueryFrame(GetFrame());
>   if (blockFrame && blockFrame->HasBullet()) {
>-    mBullet = new nsHTMLListBulletAccessible(mContent, mDoc);
>+    mBullet = new HTMLListBulletAccessible(mContent, mDoc);

btw wouldn't it make more sense to do this in UpdateBullet()?

>     }
>   } else {
>     RemoveChild(mBullet);
>     document->UnbindFromDocument(mBullet);
>     mBullet = nsnull;
>   }
> 
>   // XXXtodo: fire show/hide and reorder events. That's hard to make it
>   // right now because coalescence happens by DOM node.

btw I seem to remember that not being the case last time I looked, so maybe we should fix?

>+class HTMLListAccessible : public nsHyperTextAccessibleWrap
> {
> public:
>-  nsHTMLListAccessible(nsIContent* aContent, nsDocAccessible* aDoc);
>+  HTMLListAccessible(nsIContent* aContent, nsDocAccessible* aDoc) :
>+    nsHyperTextAccessibleWrap(aContent, aDoc) { }
>+  virtual ~HTMLListAccessible() { }
> 
>   // nsISupports
>   NS_DECL_ISUPPORTS_INHERITED
> 
>   // nsAccessible
>   virtual mozilla::a11y::role NativeRole();

nit, I'd really think you should be able to get rid of these mozilla::a11y:: now since you're already in tht namespace

>-  nsRefPtr<nsHTMLListBulletAccessible> mBullet;
>+  nsRefPtr<HTMLListBulletAccessible> mBullet;

btw isn't holding a strong ref here and in the array kind of crazy? also the pretty much completely useless extra word.

>+inline mozilla::a11y::HTMLLIAccessible*
> nsAccessible::AsHTMLListItem()
> {
>   return mFlags & eHTMLListItemAccessible ?

nit, you could change this to use IsHTMLListItem() instead if you like
Comment 2 alexander :surkov 2012-05-07 18:12:46 PDT
(In reply to Trevor Saunders (:tbsaunde) from comment #1)
> >+    mBullet = new HTMLListBulletAccessible(mContent, mDoc);
> 
> btw wouldn't it make more sense to do this in UpdateBullet()?

not obvious, at least not as part of this bug. When accessible is constructed then UpdateBullet() doesn't trrigger so constructor doesn't dupe UpdateBullet(). If constructor calls UpdateBullet() then it means we do children caching but now it happens later when accessible tree is created. Changing that isn't necessary wrong but we should be careful on this.

> >   // XXXtodo: fire show/hide and reorder events. That's hard to make it
> >   // right now because coalescence happens by DOM node.
> 
> btw I seem to remember that not being the case last time I looked, so maybe
> we should fix?

coalescence still happens by DOM node (I have wip in my queue to fix this). Also having usual procedure of show/hide events means we should change here tree creation logic. So one day but not now.

> >   virtual mozilla::a11y::role NativeRole();
> 
> nit, I'd really think you should be able to get rid of these mozilla::a11y::
> now since you're already in tht namespace

good catch

> 
> >-  nsRefPtr<nsHTMLListBulletAccessible> mBullet;
> >+  nsRefPtr<HTMLListBulletAccessible> mBullet;
> 
> btw isn't holding a strong ref here and in the array kind of crazy? also the
> pretty much completely useless extra word.

yep, I'll file bug on this.

> > nsAccessible::AsHTMLListItem()
> > {
> >   return mFlags & eHTMLListItemAccessible ?
> 
> nit, you could change this to use IsHTMLListItem() instead if you like

iirc we do this style, so I'd keep it
Comment 3 alexander :surkov 2012-05-07 18:22:26 PDT
(In reply to alexander :surkov from comment #2)

> > >   virtual mozilla::a11y::role NativeRole();
> > 
> > nit, I'd really think you should be able to get rid of these mozilla::a11y::
> > now since you're already in tht namespace

btw, it seems there's name collision (presumably from MSAA) so I used a11y::role instead.
Comment 5 alexander :surkov 2012-05-08 00:31:01 PDT
(In reply to alexander :surkov from comment #2)

> > >-  nsRefPtr<nsHTMLListBulletAccessible> mBullet;
> > >+  nsRefPtr<HTMLListBulletAccessible> mBullet;
> > 
> > btw isn't holding a strong ref here and in the array kind of crazy? also the
> > pretty much completely useless extra word.

bug 752823 filed
Comment 6 Ed Morley [:emorley] 2012-05-08 11:18:00 PDT
https://hg.mozilla.org/mozilla-central/rev/730cc501c939

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