Created attachment 88101 [details] [review]
patch to hopefully solve the problem

Mike please see if this solves the problem.  Thanks!

This seems to be working nicely.  
thanks

Thanks for testing it.  Will, okay to check this in?

Sure - it seems like kind of a nasty hack, but so is the code you're hacking it into.  :-)  I wish we knew what was really going on here.

> Sure - it seems like kind of a nasty hack, but so is the code you're hacking it
> into.  :-)  I wish we knew what was really going on here.

Well, I don't want to be guilty of a nasty hack if I can avoid it.  :-) I looked into it further.  From debugging statements I added to findCommonAncestor(), it seems that we have two different accessibles representing the same frame.

I had positioned the caret within the "Welcome to Orca!" heading on our wiki.  Then I pressed Alt H.

--------------- heading ----------------
section  
    <orca.atspi.Accessible instance at 0x86cf6ec>
section
    <orca.atspi.Accessible instance at 0x86cf66c>
document frame Orca - GNOME Live! 
    <orca.atspi.Accessible instance at 0x868924c>
panel Orca - GNOME Live! 
    <orca.atspi.Accessible instance at 0x86891cc>
scroll pane  
    <orca.atspi.Accessible instance at 0x8691f2c>
panel
    <orca.atspi.Accessible instance at 0x86cf58c>
unknown  
    <orca.atspi.Accessible instance at 0x86cf5cc>
frame Orca - GNOME Live! - Minefield 
    <orca.atspi.Accessible instance at 0x865e06c> <-------
application Minefield 
    <orca.atspi.Accessible instance at 0x861c9ec>
----------------------------------------

--------------- menu ----------------
menu bar Application 
    <orca.atspi.Accessible instance at 0x86cf8cc>
tool bar  
    <orca.atspi.Accessible instance at 0x86cfb8c>
frame Orca - GNOME Live! - Minefield 
    <orca.atspi.Accessible instance at 0x86cf90c> <-------
application Minefield 
    <orca.atspi.Accessible instance at 0x861c9ec>
----------------------------------------

Common Ancestor:  application

Thoughts/suggestions?

> Well, I don't want to be guilty of a nasty hack if I can avoid it.  :-) I
> looked into it further.  From debugging statements I added to
> findCommonAncestor(), it seems that we have two different accessibles
> representing the same frame.

Wonderful.  Do you know what the states are on the frames (i.e., is one of them DEFUNCT?).  Gecko has a design philosophy that they can destroy accessibles for an object whenever they feel like it and create a new one in its place.  An odd philosophy, IMO, but one they are not going to fix.

Seems they are both:
[STATE_ACTIVE, STATE_RESIZABLE, STATE_SHOWING, STATE_VISIBLE]

The other thing that *seems* to be the case is that the frames are not *constantly* becoming new accessibles: There is the accessible of the frame with respect to the heading and another one with respect to the menu.  In other words, going from the heading to the Help menu we see:

Frame from heading's perspective: <orca.atspi.Accessible instance at 0x860e74c>
Frame from the menu's perspective: <orca.atspi.Accessible instance at 0x8849ecc>

When we leave the menu and return to the heading, we're still dealing with those same accessibles; merely in the reverse direction.

Regardless...

Maybe instead of the hack I proposed, we could hack in findCommonAncestor()??  Perhaps when we find the first instance where aParents[i] != bParents[i], we check the role and the name for each.  If the roles are the same and the names are the same, then we get extents.  And if the extents are also the same, then this is still a common ancestor (merely one with a split personality).

Thoughts?

Just to clarify the above:  It's not just from the perspective of the heading, of course; it's from the perspective of the document_frame. 

> Maybe instead of the hack I proposed, we could hack in findCommonAncestor()?? 
> Perhaps when we find the first instance where aParents[i] != bParents[i], we
> check the role and the name for each.  If the roles are the same and the names
> are the same, then we get extents.  And if the extents are also the same, then
> this is still a common ancestor (merely one with a split personality).

This sounds like a fair fix -- perhaps findCommonAncestor should is isSameObject instead of == and isSameObject should be extended to include the check above?

At the same time (I say try the fix out above), we still should figure out if the split personality problem is an Orca bug or a Firefox bug.

Created attachment 88315 [details] [review]
proposed patch

This patch does not use the "nasty hack" approach.  In addition, I removed the similar "nasty hack" that I had done for the Find toolbar support. *hangs head in shame* ;-) This seems much cleaner.

Mike,  I need your feedback on two things please:

1. Going from the menu back to an item in the document frame, even with the fix Will and I discussed, we were speaking the panel that contains the document frame, along with the document frame itself, because these items are not ancestors shared by the menu bar.  Do we want to speak these things prior to speaking the current line in the document frame or not?  In order to be more consistent with what we say in other apps in these circumstances, I guessed that we do not.  In this patch, we do not.  If I guessed wrong, let me know.  

2. When you and I were talking the other day, you observed that Orca was saying "Application menu bar."  That would be because the menu bar that received focus when you pressed Alt H happened to have the name of "Application." (smile)  In this patch, I interpretted your observation to mean that Orca should not be saying "Application menu bar."  Again, if I'm wrong, you know where to find me.

Assuming I'm not wrong, please test this patch.  Works well for me. Thanks!

> 1. Going from the menu back to an item in the document frame, even with the fix
> Will and I discussed, we were speaking the panel that contains the document
> frame, along with the document frame itself, because these items are not
> ancestors shared by the menu bar.  Do we want to speak these things prior to
> speaking the current line in the document frame or not?  In order to be more
> consistent with what we say in other apps in these circumstances, I guessed
> that we do not.  In this patch, we do not.  If I guessed wrong, let me know.  
> 
Your assumtion is correct here this is now behaving exactly as I would like.  
> 2. When you and I were talking the other day, you observed that Orca was saying
> "Application menu bar."  That would be because the menu bar that received focus
> when you pressed Alt H happened to have the name of "Application." (smile)  In
> this patch, I interpretted your observation to mean that Orca should not be
> saying "Application menu bar."  Again, if I'm wrong, you know where to find me.
> 
The way you've handled this is perfect.  When I press alt+h I first just hear help menu which is exactly correct.  
This patch solves the problem I reported really nicely thanks much for doing it.  
> Assuming I'm not wrong, please test this patch.  Works well for me. Thanks!
> 

Great, thanks Mike!  Removing your name from summary field and putting Will's there instead.  :-) 

Will:  Please review this patch when you get a chance.  Thanks!

(Note to Rich:  I'm really liking this new system you suggested!)

Woo woo!  Looks nice.  Thanks!

Thanks Will.  Patch committed.  Changing status to "pending."

(In reply to comment #9)
> At the same time (I say try the fix out above), we still should figure out if
> the split personality problem is an Orca bug or a Firefox bug.

I think it may be us -- at least in some instances.  After being bit *again* by this issue, I tossed some debugging in and think I narrowed it down.  In findNextObject() I put in the following:

    child1 = obj.parent.child(index)
    child2 = obj.parent.child(index)
    print child1, child2

Different accessibles every time.  Then I went into accerciser and tried repeated calls to obj.parent.getChildAtIndex(index).  The children were always the same accessible.

I took a gander at atspi.py and found this in child():

        # [[[TODO: WDW - the AT-SPI appears to give us a different accessible
        # when we repeatedly ask for the same child of a parent that manages
        # its descendants.  So...we probably shouldn't cache those kind of
        # children because we're likely to cause a memory leak.]]]
        #
        # Save away details we now know about this child
        #
        newChild = None
        if index >= 0 and index < self.accessible.childCount:
            accChild = self.accessible.getChildAtIndex(index)
            if accChild:
                newChild = Accessible.makeAccessible(accChild)
                newChild.index = index
                newChild.parent = self
                newChild.app = self.app

So.... I believe we're happily creating new children ourselves.

We call child() in the following methods:

* findNextObj()
* findPreviousObj()
* findNextCaretInOrder()
* findPreviousCaretInOrder()

along with default.onFocus() which Gecko.onFocus() sometimes calls.

Put another way, if I'm correct in my observation that we're creating the new accessibles, we are doing it constantly.

Do we want to check to see if the parent in question does indeed manage its descendants before creating a new child?  Just continue to use isSameObject() rather than do a simple test for equality?  Something else?

> I think it may be us -- at least in some instances.  After being bit *again* by
> this issue, I tossed some debugging in and think I narrowed it down.  In
> findNextObject() I put in the following:
> 
>     child1 = obj.parent.child(index)
>     child2 = obj.parent.child(index)
>     print child1, child2

While the printing of child1 and child2 may make them seem different, what does child1 == child2 give us?

> Different accessibles every time.  Then I went into accerciser and tried
> repeated calls to obj.parent.getChildAtIndex(index).  The children were always
> the same accessible.

Interesting.

>         # [[[TODO: WDW - the AT-SPI appears to give us a different accessible
>         # when we repeatedly ask for the same child of a parent that manages
>         # its descendants.  So...we probably shouldn't cache those kind of
>...
> So.... I believe we're happily creating new children ourselves.

Does parent in this case manage its descendants?

> Put another way, if I'm correct in my observation that we're creating the new
> accessibles, we are doing it constantly.

Accessible.makeAccessible(accChild) only creates a new accessible if one hasn't already been created for the given object.  Otherwise, makeAccessible returns the accessible that has already been created.

> Do we want to check to see if the parent in question does indeed manage its
> descendants before creating a new child?  Just continue to use isSameObject()
> rather than do a simple test for equality?  Something else?

I'm not quite sure.  I think maybe we should hold off until we migrate to pyatspi, unless this is really the problem we're running into.

Thanks!

> While the printing of child1 and child2 may make them seem different, what does
> child1 == child2 give us?

False

> Does parent in this case manage its descendants?

No.  The parent in this case is the document_frame.
 
> Accessible.makeAccessible(accChild) only creates a new accessible if one hasn't
> already been created for the given object.  Otherwise, makeAccessible returns
> the accessible that has already been created.

Okay.  Thanks.  atspi.py's still a bit over my head. :-)

Where I came across the problem this time is with trying to implement the wrapping RFE for structural navigation. For instance, given a page with a single heading, and you're already on that heading, structural navigation should wrap, not find any more headings, and state that fact. So I did a check:

if currentObj == obj:
    yadda, yadda, yadda

currentObj was the heading, obj was the same heading, and the equality test was failing.  Our modified isSameObject() solved it though....

> I'm not quite sure.  I think maybe we should hold off until we migrate to
> pyatspi, unless this is really the problem we're running into.

Sounds good.  Like I said, isSameObject() gets the job done.

> > I'm not quite sure.  I think maybe we should hold off until we migrate to
> > pyatspi, unless this is really the problem we're running into.
> 
> Sounds good.  Like I said, isSameObject() gets the job done.

Thanks for the investigation.  I'm comfortable with using isSameObject.  Is this bug ready for review (i.e., "[pending]" status)?

> Thanks for the investigation.  I'm comfortable with using isSameObject.  Is
> this bug ready for review (i.e., "[pending]" status)?

D'Oh!  Looking at the summary, it is.  :-)

Okay for me to close this one?

Yep this is working great 

Awesome.  Closing as FIXED.