Created attachment 102822 [details]
Javier's test case.

Created attachment 102825 [details] [review]
proposed patch

Try the following:

1. Upon load, tab forward from 1 to 2 to 3.  They should be spoken.
2. Shift tab from 2 to 1.  1 should be spoken.
3. Shift tab from 1 (just once).  This will cause the document frame
   to get focus (as opposed to anything in the document or in a 
   toolbar)
4. Tab once to 1.  It will not be spoken.

In Gecko.onFocus we have this comment:

        # When we get a focus event on the document frame, it's usually
        # because we did a grabFocus on its parent in setCaretPosition.
        # We try to handle this here by seeing if there is already a
        # caret context for the document frame.  If we succeed, then
        # we set the focus on the object that's holding the caret.
        #

And that's what we're doing:  We get the caret context, see that it's the 1 link, and set orca_state.locusOfFocus to that link.  Then when we press tab to move from the document frame back to the "1." link, we see that the new locus of focus is the old locus of focus and hence there's nothing to present so we just sit there.

What this patch does is check to see if the caret context is in something focusable.  If it is, the assumption is that we did not just set the caret context on something (non-focusable) in the document frame which caused the document frame to issue a focus event.  Instead, we presumably shift+tabbed to the document frame.  By setting the locusOfFocus to None in this case, we ensure that the next item we tab to will be spoken because it's not the same object.

Thoughts?

I was just chatting with Will about this.  Mike, Javier, Jon:  This patch solves the issue reported, but when you shift tab to the document frame nothing is spoken.  What would you like displayed and spoken at that point?

When nothing is spoken what has focus?

The entire document frame.  Visually it gets little dots around it (the whole frame).

Joanie just explained to me in chat that in this case nothing actually has focus.  I think we should speak and braille "Document Frame" 

Created attachment 102952 [details] [review]
revision 2

(In reply to comment #6)
> Joanie just explained to me in chat that in this case nothing actually has
> focus.  I think we should speak and braille "Document Frame" 

If only things were that simple. <smile> 

I just tried down arrowing after the document frame had claimed focus and we acted as if we were on the first line already and moved down to the list of links.  So I suppose what we want to announce includes the heading as well??

So then I thought, fine, we'll just let the focus event do it's thing if we had been on something focusable prior to the document frame getting focus.  That will solve the problem of the link not being spoken.  Turns out it also causes us to speak the entire ancestry of the frame up to the application.  :-(

Then I look a look at default.py's findCommonAncestor(). We compare all of a's parents with all of b's parents, but we don't seem to check for the possibility that a itself might be b's parent (or b might be a's).  Will, is this intentional?  If we add this check in, we speak the document frame and then read the heading.  And we still speak the link when it's tabbed to. :-)

Mike what do you think in terms of the user interaction aspects of the attached?

This seems to work on the test document when you shift+tab from the addressbar but when you tab from the "two" link nothing is spoken.  

> Then I look a look at default.py's findCommonAncestor(). We compare all of a's
> parents with all of b's parents, but we don't seem to check for the possibility
> that a itself might be b's parent (or b might be a's).  Will, is this
> intentional?  If we add this check in, we speak the document frame and then
> read the heading.

Oops - definitely not intentional.  The assumption being made in the code was that they were both basically leaf nodes.  Generalizing this to 'a or b might be an ancestor of the other' would be a fine thing to do.  The main purpose of this code, BTW, is just to limit the amount of contextual information that is spoken when moving from a to b: we only speak the context that changed.

(In reply to comment #8)
> This seems to work on the test document when you shift+tab from the addressbar
> but when you tab from the "two" link nothing is spoken.  
 
Really?  The "3." link is spoken for me.  I wonder what the difference is.

(In reply to comment #9)
> Oops - definitely not intentional.  The assumption being made in the code was
> that they were both basically leaf nodes.  Generalizing this to 'a or b might
> be an ancestor of the other' would be a fine thing to do.  

Phew! :-)

> The main purpose of
> this code, BTW, is just to limit the amount of contextual information that is
> spoken when moving from a to b: we only speak the context that changed.

Yup.  But, ya know, what occurred to me in the wee hours is that this method might come in handy in our new code for obtaining the line contents (and deciding if something that's on the same line spatially is also on the same line content-wize/conceptually).  Distant cousins, as it were, are probably not on the same line conceptually.  I'll have to play with it a bit and see if it helps.

Mike can you verify that the "3." link is not spoken as you describe in comment #8? And if so, attach a full debug.out to this bug?  Thanks!

All seems alright to me, using rev3485, and patch 102952

After restoring my system I'm not able to reproduce the problem I was having before.

I ran all of the regression tests -- sans the Java ones because I can't seem to get my environment setup. :-( This patch does not seem to introduce any regressions.  Will, given the change to findCommonAncestor(), would you mind running the Java tests before I commit this patch?  Thanks!!

(In reply to comment #14)
> I ran all of the regression tests -- sans the Java ones because I can't seem to
> get my environment setup. :-( This patch does not seem to introduce any
> regressions.  Will, given the change to findCommonAncestor(), would you mind
> running the Java tests before I commit this patch?  Thanks!!

The Java platform support is kind of dicey right now due to some fundamental issues with the Orca<-CORBA->Java connection.  It's going to need more work and I already view it as broken.  So, I say that if you are comfortable with this patch, and it pylints/tests well, don't worry about the Java stuff and go ahead and commit.

When I look at findCommonAncestor in much closer detail, though, I'm not sure why things are broken.  Let's pretend we have a hierarchy that looks like this:

+--app
   +--frame
      +--layered pane
         +--b
            +--a

Here's what I see in the code.  First, we get the ancestry of a and b, using similar blocks of code (bParents block omitted here):

        aParents = [a]
        try:
            parent = a.parent
            while parent and (parent.parent != parent):
                aParents.append(parent)
                parent = parent.parent
            aParents.reverse()
        except:
            debug.printException(debug.LEVEL_FINEST)

This should give us the following for the given hierarchy:

   aParents = [app, frame, layered pane, b, a]
   bParents = [app, frame, layered pane, b]

Then, we go item by item through aParents and bParents list, saving the last identical thing we came across as the commonAncestor:

        maxSearch = min(len(aParents), len(bParents))
        i = 0
        while i < maxSearch:
            if self.isSameObject(aParents[i], bParents[i]):
                commonAncestor = aParents[i]
                i += 1
            else:
                break

So, this would go along 'app', 'frame', 'layered pane', and end up at 'b'.  If 'b' and 'a' were swapped in the hierarchy above, we'd end up at 'a'.  Looking at a more minimal case, I think we'd still end up with the right thing:

   aParents = [b, a]
   bParents = [b]

But, you obviously ran into a case where the algorithm failed.  I wonder if this might be a case of broken hierarchy and your optimization of immediately detecting the parent/child relation of 'a' and 'b' helped avoid that?  Or, maybe the algorithm is flawed and I'm not seeing why?

> When I look at findCommonAncestor in much closer detail, though, I'm not sure
> why things are broken.  

Dang, you're right (of course).... I'm not sure either..... :-(
 
> Here's what I see in the code.  First, we get the ancestry of a and b, using
> similar blocks of code (bParents block omitted here):

Yup.

> Then, we go item by item through aParents and bParents list, saving the last
> identical thing we came across as the commonAncestor:

Yup.

> But, you obviously ran into a case where the algorithm failed.  

Indeed.

> this might be a case of broken hierarchy and your optimization of immediately
> detecting the parent/child relation of 'a' and 'b' helped avoid that?  Or,
> maybe the algorithm is flawed and I'm not seeing why?

It's not the algorithm.  I wasn't reading it right the first time.  Sorry about that!  But here's the kicker:  I added a bunch of debugging statements to findCommonAncestor including:

1. what my patch would return (but it didn't return it)
2. what findCommonAncestor was returning (i.e. the unpatched behavior)
3. whether those objects passed the isSameObject() test 

AND (what I suspected I would not find)

4. whether those objects passed a plain ol' equality test 
  (commonAncestor == foo)

In all cases the objects passed both #3 and #4.  So why the bleeping bleep does returning the object immediately cause us to do the right thing (not speak the hierarchy all the way to the app), whereas taking a leisurely look at the hierarchy cause us to do the wrong/chatty thing even though we're theoretically returning the very same object as the ancestor?

Part of me suspects what you've suggested (I think): destruction/recreation of an accessible and that somewhere else we're doing an equality check instead of isSameObject().  I'll keep looking, but brilliant insights are most certainly welcome. ;-)

I'm not seeing where it is. :-(

So Mike, Will what would you like to do?  Put in the hack which theoretically shouldn't make a difference but does, or leave it out?

If we leave it out the impact will be the following:  If you Tab/Shift+Tab and by doing so give focus to the document frame, there is a healthy chance that we're going to speak the hierarchy from the app down to the frame.  For instance:

Without the hack:

SPEECH OUTPUT: 'Minefield application Testing - Minefield frame Testing panel'
SPEECH OUTPUT: 'Testing html content'

With the hack:

SPEECH OUTPUT: 'Testing html content'

(In reply to comment #17)
> I'm not seeing where it is. :-(

Bummer.

> So Mike, Will what would you like to do?  Put in the hack which theoretically
> shouldn't make a difference but does, or leave it out?

I guess putting it in is fine.  It can be viewed as a 'performance enhancement' since it prevents the need to walk the entire hierarchy when a/b have the parent/child relationship.

(In reply to comment #18)
> (In reply to comment #17)
> > I'm not seeing where it is. :-(
> 
> Bummer.

Of course, the anal retentive thing to do would be to check the "while i < maxSearch:" loop to see where the comparison falls down.  We may indeed be dealing with a broken hierarchy.  If so, we probably should see if it is unexpected and file YAFFB if needed.  It's the due diligence thing to do, even if it is tediously tiresome.  :-(

> Of course, the anal retentive thing to do would be to check the "while i <
> maxSearch:" loop to see where the comparison falls down.  

But I did that.  That's the thing, it doesn't break down.  See comment 16.  I modified my patch so that it would tell me what it would return, but didn't return it.  I put tons of debugging statements in, including in that very while loop, and I compared what was to be returned as the commonAncestor with what my patch would have returned instead.  The two objects are not merely the same object as defined by isSameObject(), they are the same object as defined by ==. 

> dealing with a broken hierarchy.  If so, we probably should see if it is
> unexpected and file YAFFB if needed. 

Agreed, I spent 3 hours or so attempting to do just that.  If I could figure it out, I'd file the bug.

> It's the due diligence thing to do, even if it is tediously tiresome.  :-(

Agreed on both the due diligence and tediously tiresome fronts. ;-)

How about this then:  We check in the Gecko related change so that we speak the link (i.e. original bug).  We leave out the hack.  We open a new bug on this wtf issue so that we can attempt to resolve it properly.

 

Created attachment 103743 [details] [review]
version without the hack

I just committed the fix sans the hack to default.py.  That means this bug is fixed.  The fact that we're chatty when you Tab/Shift+Tab and give focus to the document frame is a different bug.  In fact, it's bug #512103.  :-)

Since the reported issue is fixed, moving this to pending.