801356 - LeafAccessible needs to override Append/InsertChild

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[alexander :surkov (:asurkov)]

spun off bug 741408 comment #18:

> LeafAccessible overrides accessible CacheChildren() and InvalidateChildren()
> but not  AppendChild / RemoveChild so iirc you can technically add children
> and remove them if you like, but admittedly that's kind of hacky

yea, LeafAccessible wants to override those too I guess.

Comment 1

[joseph]

Hi guys, I would like to work on that but I have few questions.

> LeafAccessible overrides accessible CacheChildren() and InvalidateChildren()

That is no LeafAccessible::InvalidateChildren() method at all. And also LeafAccessible::CacheChildren() does nothing, why would you overwrite method in the way it is done now? Isn't it indicate there is something wrong with the design of these classes?

> but not  AppendChild / RemoveChild so iirc you can technically add children
> and remove them if you like, but admittedly that's kind of hacky

Should append/insert/remove child just return false if I understand it correctly?

Cheers,

Comment 2

[alexander :surkov (:asurkov)]

(In reply to joseph from comment #1)
> Hi guys, I would like to work on that but I have few questions.
> 
> > LeafAccessible overrides accessible CacheChildren() and InvalidateChildren()
> 
> That is no LeafAccessible::InvalidateChildren() method at all. And also
> LeafAccessible::CacheChildren() does nothing, why would you overwrite method
> in the way it is done now? Isn't it indicate there is something wrong with
> the design of these classes?

Ignore, InvalidateChildren, I don't see it either.

LeafAccessible::CacheChildren() does nothing to make sure it doesn't have children. Some accessible objects aren't expected to have children but their DOM elements has them (which are accessible in general).

Example:

xul:menuitem must have menuitem accessible which must be a leaf
  xul:label must be accessible until it's in inside xul:menuitem

> > but not  AppendChild / RemoveChild so iirc you can technically add children
> > and remove them if you like, but admittedly that's kind of hacky
> 
> Should append/insert/remove child just return false if I understand it
> correctly?

Yes and don't forget to assert

Comment 3

[joseph]

Attachment: First version of patch.

Please add corresponding documentation.

Comment 4

[alexander :surkov (:asurkov)]

Comment on attachment 680369 [details] [diff] [review]
First version of patch.

Review of attachment 680369 [details] [diff] [review]:
-----------------------------------------------------------------

::: accessible/src/generic/BaseAccessibles.cpp
@@ +42,5 @@
>    // Don't walk into leaf accessibles.
>    return this;
>  }
>  
> +bool LeafAccessible::AppendChild(Accessible* aChild)

please keep return type on own line like
bool
LeafAccessible::

@@ +44,5 @@
>  }
>  
> +bool LeafAccessible::AppendChild(Accessible* aChild)
> +{
> +  NS_ASSERTION(aChild,"Invalid child");

we should assert not depending argument, we should assert if we were called like
NS_NOT_REACHED("AppendChild called on leaf accessible!");

fix other methods too

::: accessible/src/generic/BaseAccessibles.h
@@ +32,5 @@
>    NS_DECL_ISUPPORTS_INHERITED
>  
>    // Accessible
>    virtual Accessible* ChildAtPoint(int32_t aX, int32_t aY,
>                                     EWhichChildAtPoint aWhichChild);

nit: it'd be nice to have an empty line between

Comment 5

[joseph]

Attachment: Making changes suggested by reviewer.

Comment 6

[alexander :surkov (:asurkov)]

Comment on attachment 680606 [details] [diff] [review]
Making changes suggested by reviewer.

Review of attachment 680606 [details] [diff] [review]:
-----------------------------------------------------------------

thanks, do you need help to land it?

::: accessible/src/generic/BaseAccessibles.h
@@ +33,5 @@
>  
>    // Accessible
>    virtual Accessible* ChildAtPoint(int32_t aX, int32_t aY,
>                                     EWhichChildAtPoint aWhichChild);
> +  

nit: white space on empty line

Comment 7

[alexander :surkov (:asurkov)]

Comment on attachment 680606 [details] [diff] [review]
Making changes suggested by reviewer.

f? from Trev as mentor

Comment 8

[joseph]

(In reply to alexander :surkov from comment #6)
> thanks, do you need help to land it?

I have not tried it yet, but I guess there are some straightforward instructions, for instance on the following page? https://developer.mozilla.org/en-US/docs/Developer_Guide/How_to_Submit_a_Patch

> ::: accessible/src/generic/BaseAccessibles.h
> @@ +33,5 @@
> >  
> >    // Accessible
> >    virtual Accessible* ChildAtPoint(int32_t aX, int32_t aY,
> >                                     EWhichChildAtPoint aWhichChild);
> > +  
> 
> nit: white space on empty line

Silly question. What is "nit"? I guess those comments are generated automatically, but I couldn't find any more relevant information about it. Thanks for your answers or comments.

Regards,

Comment 9

[Trevor Saunders (:tbsaunde)]

Comment on attachment 680606 [details] [diff] [review]
Making changes suggested by reviewer.

+  virtual bool AppendChild(Accessible* aChild);

please add MOZ_OVERRIDE MOZ_FINAL for each of these

Comment 10

[joseph]

Attachment: Add MOZ_OVERRIDE and MOZ_FINAL

Comment 11

[alexander :surkov (:asurkov)]

Comment on attachment 680722 [details] [diff] [review]
Add MOZ_OVERRIDE and MOZ_FINAL

you don't need to reqreuest reviews for minor changes you were asked to prepare by reviewers

Comment 12

[joseph]

Quoting from Committing rules and responsibilities page: "Therefore it is unwise to check in if you won't be available for the next 4 hours."

That is almost impossible for me to be available for such a long time? Can you guys take care of it?

Cheers,

Comment 13

[alexander :surkov (:asurkov)]

(In reply to joseph from comment #12)
> Quoting from Committing rules and responsibilities page: "Therefore it is
> unwise to check in if you won't be available for the next 4 hours."

I believe it's about mozilla-central and that's why we all check into mozilla-inbound

> That is almost impossible for me to be available for such a long time? Can
> you guys take care of it?

I'll land it today. Thank you for the work! Please feel free to pick up another bug ;)

Comment 14

[alexander :surkov (:asurkov)]

http://hg.mozilla.org/integration/mozilla-inbound/rev/fdc8ef449613

Comment 15

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/fdc8ef449613