densify nsHTMLListAccessible

RESOLVED FIXED in mozilla15

Status

()

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

People

(Reporter: surkov, Assigned: surkov)

Tracking

(Blocks: 1 bug)

unspecified
mozilla15
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Assignee)

Description

5 years ago
Created attachment 620985 [details] [diff] [review]
patch
Attachment #620985 - Flags: review?(trev.saunders)
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
Attachment #620985 - Flags: review?(trev.saunders) → review+
(Assignee)

Comment 2

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

Comment 3

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

Comment 4

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

Comment 5

5 years ago
(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
Target Milestone: mozilla15 → ---

Comment 6

5 years ago
https://hg.mozilla.org/mozilla-central/rev/730cc501c939
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla15
You need to log in before you can comment on or make changes to this bug.