The default bug view has changed. See this FAQ.

dexpcom GetUnignoredParent()

RESOLVED FIXED in mozilla14

Status

()

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

People

(Reporter: tbsaunde, Assigned: lavina)

Tracking

(Blocks: 1 bug)

unspecified
mozilla14
All
Mac OS X
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [good first bug][mentor=trev.saunders@gmail.com][lang=c++])

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

5 years ago
change nsAccessibleWrap::GetUnignoredParent() to return an nsAccessible* instead of an already_AddRefed<nsIAccessible> see accessible/src/mac/nsAccessibleWrap.mm line 289
While your here please make it not be recursive too.
then fix all  callers to deal with the new return type.
(Assignee)

Comment 1

5 years ago
I would like to take this. How can I get assigned? This would be my first time doing this. Thanks.
(Reporter)

Comment 2

5 years ago
(In reply to lavina from comment #1)
> I would like to take this. How can I get assigned? This would be my first
> time doing this. Thanks.

if you want to pick this bug it would be best if you have mac, but if not that's probably good too.

its now yours :)
Assignee: nobody → orangedaylily
(Assignee)

Comment 3

5 years ago
Thanks! I'll be working on a mac.
(Assignee)

Comment 4

5 years ago
How should I be testing this? With Universal Access turned on, I was able to break in the code and observed that the execution covers the case where there is no unignored parent all the way to the root. I don't know how to generate a case where there is an unignored parent in the chain. I just manually tested that part by manual manipulation so that GetUnignoredParent() returned something. Please advise. Thanks!
(Reporter)

Comment 5

5 years ago
(In reply to lavina from comment #4)
> How should I be testing this? With Universal Access turned on, I was able to
> break in the code and observed that the execution covers the case where
> there is no unignored parent all the way to the root. I don't know how to
> generate a case where there is an unignored parent in the chain. I just
> manually tested that part by manual manipulation so that
> GetUnignoredParent() returned something. Please advise. Thanks!

I'm a little suprised that you couldn't find a case where there was a un ignored parent, but that's about all the testing I think you can do.
(Assignee)

Comment 6

5 years ago
Created attachment 611248 [details] [diff] [review]
patch for bug 723424
(Reporter)

Comment 7

5 years ago
Comment on attachment 611248 [details] [diff] [review]
patch for bug 723424

>-  nsCOMPtr<nsIAccessible> accessibleParent(mGeckoAccessible->GetUnignoredParent());
>+  
>+  nsAccessible* accessibleParent = mGeckoAccessible->GetUnignoredParent();

don't add another blank line please.

>-  virtual already_AddRefed<nsIAccessible> GetUnignoredParent();
>+  virtual nsAccessible* GetUnignoredParent();

I can't think of a reason it should stay virtual.

>+    nsAccessibleWrap* parentWrap = static_cast<nsAccessibleWrap*>(Parent());
>+    while (parentWrap && parentWrap->IsIgnored()) 
>+    {
>+        parentWrap = static_cast<nsAccessibleWrap*>(parentWrap->Parent());
>+    }

no braces please.

other wise looks good, thanks!

btw in future set review or feedback flags to ? with a email of  someone who can review the change.
(Assignee)

Comment 8

5 years ago
Created attachment 611312 [details] [diff] [review]
revised patch for bug 723424
Attachment #611248 - Attachment is obsolete: true
Attachment #611312 - Flags: review?(trev.saunders)
(Reporter)

Comment 9

5 years ago
Comment on attachment 611312 [details] [diff] [review]
revised patch for bug 723424

>-  nsCOMPtr<nsIAccessible> accessibleParent(mGeckoAccessible->GetUnignoredParent());
>+  nsAccessible* accessibleParent = mGeckoAccessible->GetUnignoredParent();

it'd be nice if someone changed the name of that variable to just parent, but you don't need to do that here if you don't want, we should probably just inline GetUnIgnoredParent() all together so doing it later is no big deal.

>   if (accessibleParent) {
>     id nativeParent = GetNativeFromGeckoAccessible(accessibleParent);
>     if (nativeParent)
>       return mParent = GetClosestInterestingAccessible(nativeParent);

btw this seems kind of crazy GetUnIgnoredParent() only returns things that aren't ignored then GetNativeFromGeckoAccessiblee() checks the accessible in question isn't ignored, and then GetClosesInterestingAccessible() checks again.

modulo existing madness this seems good.
Attachment #611312 - Flags: review?(trev.saunders)
Attachment #611312 - Flags: review+
Attachment #611312 - Flags: feedback?(surkov.alexander)

Comment 10

5 years ago
Comment on attachment 611312 [details] [diff] [review]
revised patch for bug 723424

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

nice

::: accessible/src/mac/nsAccessibleWrap.h
@@ +99,5 @@
>     * Returns this accessible's all children, adhering to "flat" accessibles by 
>     * not returning their children.
>     */
>    void GetUnignoredChildren(nsTArray<nsRefPtr<nsAccessibleWrap> >& aChildrenArray);
> +  nsAccessible* GetUnignoredParent();

add 'const' please

::: accessible/src/mac/nsAccessibleWrap.mm
@@ +285,3 @@
>  nsAccessibleWrap::GetUnignoredParent()
>  {
> +    // go up the chain to find a parent that is not ignored

go -> Go. '.' in the end

@@ +291,2 @@
>      
> +    return parentWrap;

wrong indentation of whole block, 2 spaces indent
Attachment #611312 - Flags: feedback?(surkov.alexander) → feedback+
(Assignee)

Comment 11

5 years ago
(In reply to alexander :surkov from comment #10)
> >    void GetUnignoredChildren(nsTArray<nsRefPtr<nsAccessibleWrap> >& aChildrenArray);
> > +  nsAccessible* GetUnignoredParent();
> 
> add 'const' please
> 

This would mean I'll have to do a const_cast for the argument to GetNativeFromGeckoAccessible, and not be changing the prototype of the function to take a const argument which would then affect other function calls within it. Right?
   id nativeParent = GetNativeFromGeckoAccessible(const_cast<nsAccessible*>(accessibleParent));

Comment 12

5 years ago
(In reply to lavina from comment #11)

> > add 'const' please
> > 
> 
> This would mean I'll have to do a const_cast for the argument to
> GetNativeFromGeckoAccessible, and not be changing the prototype of the
> function to take a const argument which would then affect other function
> calls within it. Right?

I meant
nsAccessible* GetUnignoredParent() const;

GetUnignoredParent() calls const Parent() method only, so you shouldn't get any problems here
(Assignee)

Comment 13

5 years ago
Created attachment 611352 [details] [diff] [review]
Revised patch for bug 723424

Updated

5 years ago
Attachment #611352 - Attachment is patch: true

Updated

5 years ago
Attachment #611312 - Attachment is obsolete: true
(Assignee)

Comment 14

5 years ago
Created attachment 611354 [details] [diff] [review]
revised again
Attachment #611352 - Attachment is obsolete: true
Attachment #611354 - Flags: review?(trev.saunders)
Attachment #611354 - Flags: feedback?(surkov.alexander)

Comment 15

5 years ago
Comment on attachment 611354 [details] [diff] [review]
revised again

you don't need to rerequest review or feedback for simple changes that you and reviewers were agree upon
Attachment #611354 - Flags: review?(trev.saunders)
Attachment #611354 - Flags: feedback?(surkov.alexander)
Attachment #611354 - Flags: feedback+
(Assignee)

Comment 16

5 years ago
So, am I done for this bug? Is there another step after this?

Thanks for your help, Alexander and Trevor.

Comment 17

5 years ago
(In reply to lavina from comment #16)
> So, am I done for this bug? Is there another step after this?

yes, I'll land it. If you like then please try another one ;)

> Thanks for your help, Alexander and Trevor.

Thanks to you actually. It was nice work.

Comment 18

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/075a2f69249c
This patch has landed in mozilla-central for Firefox 14, and will be included in tomorrow's nightly build.  Thank you!
https://hg.mozilla.org/mozilla-central/rev/075a2f69249c
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla14
You need to log in before you can comment on or make changes to this bug.