751828 - densify nsHTMLListAccessible

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[alexander :surkov (:asurkov)]

Attachment: patch

Comment 1

[Trevor Saunders (:tbsaunde)]

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 (:asurkov)]

(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 (:asurkov)]

(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 4

[alexander :surkov (:asurkov)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/730cc501c939

Comment 5

[alexander :surkov (:asurkov)]

(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]]

https://hg.mozilla.org/mozilla-central/rev/730cc501c939