496783 - setParent shouldn't be virtual

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[alexander :surkov (:asurkov)]

We could make setParent non virtual once bug 461921 is fixed. However nsHTMLListBulletAccessible overrides setParent and uses mWeakParent. This was introduced in bug 346721. I have no idea why weak parent is needed. I think we could remove it in the meantime.

Comment 1

[alexander :surkov (:asurkov)]

Attachment: patch

Comment 2

[Marco Zehe (:MarcoZ)]

Comment on attachment 401194 [details] [diff] [review]
patch

>+      // previousSibling
>+      var success = false;
>+      try {
>+        aAcc.parent;

Nit: Comment refers to previous sibling when it should reference parent. r=me

Comment 3

[David Bolter [:davidb] (NeedInfo me for attention)]

Comment on attachment 401194 [details] [diff] [review]
patch

Can you build with --enable-logrefcnt and make sure we don't leak with this patch? (Also check that Zoom text is still happy?)

Also the history of bullets being special worries me.

http://mxr.mozilla.org/mozilla-central/search?find=%2Faccessible%2F&string=bullets

Do bullets still not have a frame?

Comment 4

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Bullets have a frame, but it's an nsBulletFrame rather than an nsTextFrame.

Comment 5

[alexander :surkov (:asurkov)]

(In reply to comment #3)
> (From update of attachment 401194 [details] [diff] [review])
> Can you build with --enable-logrefcnt and make sure we don't leak with this
> patch?

Is this option not in debug build by default? At least I don't do any special steps to prepare build if I like to test leaks. On the another hand garbage collector will care if we don't shutdown accessible properly (here's nsAccessible code works). Also mochitest doesn't show any leak and they would if leaks would be presented (I saw this many times when my new code leaked).

> (Also check that Zoom text is still happy?)

Marco, could you check please since I'm not sure in exact steps of testing. Though everything must be fine because we expose correct accessible tree.

> 
> Also the history of bullets being special worries me.
> 
> http://mxr.mozilla.org/mozilla-central/search?find=%2Faccessible%2F&string=bullets
> 
> Do bullets still not have a frame?

Ok, it's probably another issue. The meantime our nsAccessibleTreeWalker doesn't find it. I'm not sure why. We could file another bug to make bullet implementation correct. Here's I'd like to get rid virtual setParent. It will help to fix bug 342045 which is good to have to fix text attrs implementation.

Comment 6

[David Bolter [:davidb] (NeedInfo me for attention)]

Comment on attachment 401194 [details] [diff] [review]
patch

thanks r=me; with Marco's nit fixed. Given that this is holding up the attributes work, I'm ok with pushing it now and figuring out the zoomtext issue later.

(In reply to comment #4)
> Bullets have a frame, but it's an nsBulletFrame rather than an nsTextFrame.

Thanks David.

(In reply to comment #5)
> (In reply to comment #3)

> nsAccessible code works). Also mochitest doesn't show any leak and they would
> if leaks would be presented (I saw this many times when my new code leaked).

OK.

> > (Also check that Zoom text is still happy?)
> 
> Marco, could you check please since I'm not sure in exact steps of testing.
> Though everything must be fine because we expose correct accessible tree.
> 
> > 
> > Also the history of bullets being special worries me.

I've filed tracker bug 517453 and noted this bug just in case it bites us.

Comment 7

[alexander :surkov (:asurkov)]

landed on 1.9.3 - http://hg.mozilla.org/mozilla-central/rev/333967132e88 with Marco's comment fixed.