Created attachment 129010 [details] [review]
possible solution

I cannot reproduce the problem (I even copied messages so that I would have exact duplicates, including the date). However, while trying to I saw something really odd: An equality check on two different accessibles was returning True. This patch ensures that won't happen.

Please let me know if this solves the problem. Thanks!

Michael - can you please try the patch to see if it resolves the problem you were seeing?

Jose I just noticed you added yourself to the CC list. If memory serves me, you were one of the folks on the list who could reproduce this problem. Assuming I'm correct in my recollection, does this patch help solve the problem?

Thanks in advance!

I don't know why I didn't notice the patch when originally submitted. Just tried it and it seems to fix the bug for me (I tested it for two messages with attachments and two messages with the same subject).
(In reply to comment #2)
> Michael - can you please try the patch to see if it resolves the problem you
> were seeing?
> 

Yes, after apply the patch all is working correctly.
However I'd like to point to a peculiar thing that I'm not sure if is related to the bug or not.

Yesterday, as an exercise to understand how orca works, I was exactly trying to identify this bug. I try to debug a function called locusOfFocusChanged present in scripts/apps/Thunderbird/script.py.
I found in the function the following code:

        if self.isDesiredFocusedItem(event.source, rolesList):
        if self.isDesiredFocusedItem(event.source, rolesList):

I inserted a print statement before the first if, and another print statement before the second if.

When I run thunderbird the two prints are executed as expected when navigating in the message list. However after I flat review in the message list, only the first print is executed.
It looks like the functionisDesiredFocusedItem is returning false. 

Thanks.

(In reply to comment #3)
> Jose I just noticed you added yourself to the CC list. If memory serves me, you
> were one of the folks on the list who could reproduce this problem. Assuming
> I'm correct in my recollection, does this patch help solve the problem?
> 
> Thanks in advance!
> 

(In reply to comment #4)
> I don't know why I didn't notice the patch when originally submitted. Just
> tried it and it seems to fix the bug for me (I tested it for two messages with
> attachments and two messages with the same subject).

Yea! Thanks Michael. Patch committed to trunk. As for not noticing, bugzilla email is at best flakey.

Warning, what follows is a long, very long, response to Jose. Feel free to stop reading if you are not interested in Orca internals. :-)

(In reply to comment #5)
> Yes, after apply the patch all is working correctly.

Again I say "Yea!" :-)

> However I'd like to point to a peculiar thing that I'm not sure if is related
> to the bug or not.

I would guess it's not. But I would need to try exactly what you are trying and potentially have a very similar set up in tbird (i.e. are messages being previewed? Are any message windows open? etc.)

> Yesterday, as an exercise to understand how orca works, I was exactly trying to
> identify this bug. I try to debug a function called locusOfFocusChanged present
> in scripts/apps/Thunderbird/script.py.

In a nutshell, the default script's locusOfFocusChanged method (overridden as needed in other scripts) compares two accessible objects (where we were and where we are now) and decides based on that what should be presented to the user. In addition, we have access to the event which triggered the change in location (Did the caret move? Did the focus change? etc.). The event.source is the accessible associated with that event.

> I found in the function the following code:
> 
>         if self.isDesiredFocusedItem(event.source, rolesList):
>         if self.isDesiredFocusedItem(event.source, rolesList):

isDesiredFocusedItem is (in my opinion) a method we need to stop using. *sigh* Anyhoo, what it does is look at a given accessible, and a list of accessible roles, and tries to decide if the given accessible's ancestry matches the list of roles. So here's the first list:

        rolesList = [pyatspi.ROLE_TABLE_CELL, \
                     pyatspi.ROLE_TREE_TABLE, \
                     pyatspi.ROLE_SCROLL_PANE, \
                     pyatspi.ROLE_SCROLL_PANE, \
                     pyatspi.ROLE_FRAME, \
                     pyatspi.ROLE_APPLICATION]

In this case we're examining the source of the event responsible for that change in location. So that accessible is a table cell, and its parent is a tree table, and its grand parent is a scroll pane and its great grand parent is a scroll pane, and its great great grand parent is a frame, and the great great great grand parent is an application, isDesiredFocusedItem will return True. Otherwise False. This method which I hate for a reason I'm about to explain, needs a better name. Like isDesiredObject.

The problem with this method is two-fold:

1. What if there's another accessible with the same hierarchy? (i.e. False positives)

2. What if the hierarchy changes in a later version of the software, like the tree table is in a scroll pane which is in a frame rather than nested inside another scroll pane? (i.e. False negatives and breakage)

My goal, as we see the latter breakage, is to slowly but surely get rid of the use of this method. :-) Anyhoo, getting back to your question....
 
> When I run thunderbird the two prints are executed as expected when navigating
> in the message list. However after I flat review in the message list, only the
> first print is executed.

When arrowing around we get focus: events and object:state-changed:focused events (two of them actually, one for the object which just gave up focus and one for the object which claimed it). In addition, if your preview pane is showing (which I believe it is by default), as you arrow around, the message being displayed changes. It wouldn't do that when you're flat reviewing because you're not actually changing the selected message. Based on what you describe, I'm going to assume that your preview pane is indeed showing. Because the message is appearing (and maybe loading, claiming focus, whatever), it too is spewing out events which we have to consider. (We get lots of events). :-)  In addition, sometimes we do things like set the locusOfFocus to None (which I did in that patch) to trigger the use of locusOfFocusChanged. Regardless, here's that list:

        rolesList = [pyatspi.ROLE_DOCUMENT_FRAME, \
                     pyatspi.ROLE_INTERNAL_FRAME, \
                     pyatspi.ROLE_FRAME, \
                     pyatspi.ROLE_APPLICATION]

Gecko content (web pages, email messages, etc.) lives in a document frame which, apparently, at least for now, lives in an internal frame, which lives in a frame which lives in an application.

> It looks like the functionisDesiredFocusedItem is returning false. 

Because arrowing around can cause the displayed message to change (and emit events for our consideration as a result) it would make sense that isDesiredFocusedItem returns True when arrowing, but False when flat reviewing.

I sure hope this makes sense and helps in your understanding!

(In reply to comment #5)
> identify this bug. I try to debug a function called locusOfFocusChanged present
> in scripts/apps/Thunderbird/script.py.
> I found in the function the following code:
> 
>         if self.isDesiredFocusedItem(event.source, rolesList):
>         if self.isDesiredFocusedItem(event.source, rolesList):
> 
> I inserted a print statement before the first if, and another print statement
> before the second if.
> 
> When I run thunderbird the two prints are executed as expected when navigating
> in the message list. However after I flat review in the message list, only the
> first print is executed.
> It looks like the functionisDesiredFocusedItem is returning false. 

I'd have to see the exact code that Jose wrote, but I'm concerned about the above.  When I look at Thunderbird/script.py:locusOfFocusChanged, there are no return statements.  So, if Jose put the print lines at the same indent level as the if's, then both lines should always get executed.  If that's the case, then we might be running into a stack trace that's causing the method to abort early.

Jose - can you post your code along with a sequence of steps you used to reproduce the problem?  We might need to open a new bug.

> isDesiredFocusedItem is (in my opinion) a method we need to stop using. *sigh*

Yep.  It's a last resort method that I've never liked.

(In reply to comment #7)
OK, here is what I did.
Firstly I changed the function locusOfFocusChanged as follow:

        rolesList = [pyatspi.ROLE_TABLE_CELL, \
                     pyatspi.ROLE_TREE_TABLE, \
                     pyatspi.ROLE_SCROLL_PANE, \
                     pyatspi.ROLE_SCROLL_PANE, \
                     pyatspi.ROLE_FRAME, \
                     pyatspi.ROLE_APPLICATION]
        print(event.source)
        if self.isDesiredFocusedItem(event.source, rolesList):
            print("ok 1")
            if isinstance(orca_state.lastInputEvent, input_event.KeyboardEvent):
                string = orca_state.lastNonModifierKeyEvent.event_string
                print(string)
                if string == "Delete":
                    oldLocusOfFocus = None

compile orca:
./autogen.sh &&make &&sudo make install

Start orca in a console.
Start thunderbird

Make sure that message pane is disabled.
Find the inbox message list.
Press the down key.

Observe in the console where orca was started that the three prints included in the code are shown.

Experiment with home, end, up and delete key, and note the result are the same.

Now flat in the message list, pressing for example caps lock + i key (laptop layout).

Press down or up key again and note that the second and the third print are not executed, only the first print.

> (In reply to comment #5)
> > identify this bug. I try to debug a function called locusOfFocusChanged present
> > in scripts/apps/Thunderbird/script.py.
> > I found in the function the following code:
> > 
> >         if self.isDesiredFocusedItem(event.source, rolesList):
> >         if self.isDesiredFocusedItem(event.source, rolesList):
> > 
> > I inserted a print statement before the first if, and another print statement
> > before the second if.
> > 
> > When I run thunderbird the two prints are executed as expected when navigating
> > in the message list. However after I flat review in the message list, only the
> > first print is executed.
> > It looks like the functionisDesiredFocusedItem is returning false. 
> 
> I'd have to see the exact code that Jose wrote, but I'm concerned about the
> above.  When I look at Thunderbird/script.py:locusOfFocusChanged, there are no
> return statements.  So, if Jose put the print lines at the same indent level as
> the if's, then both lines should always get executed.  If that's the case, then
> we might be running into a stack trace that's causing the method to abort
> early.
> 
> Jose - can you post your code along with a sequence of steps you used to
> reproduce the problem?  We might need to open a new bug.
> 
> > isDesiredFocusedItem is (in my opinion) a method we need to stop using. *sigh*
> 
> Yep.  It's a last resort method that I've never liked.
> 

(In reply to comment #8)
>         print(event.source)
>         if self.isDesiredFocusedItem(event.source, rolesList):
>             print("ok 1")

Thanks!  There are two self.isDesiredFocusedItem(event.source, rolesList) lines in the method.  What I thought you had done was put a print statement before the first if, and another print statement before the second if.  What you really did was put a print outside and inside the first if.  Many thanks for clarifying this.  My concern is gone.  :-)

(In reply to comment #9)
> (In reply to comment #8)
> >         print(event.source)
> >         if self.isDesiredFocusedItem(event.source, rolesList):
> >             print("ok 1")
> 
> Thanks!  There are two self.isDesiredFocusedItem(event.source, rolesList) lines
> in the method.  What I thought you had done was put a print statement before
> the first if, and another print statement before the second if.  What you
> really did was put a print outside and inside the first if.  Many thanks for
> clarifying this.  My concern is gone.  :-)
> 
Heh. I misread it as well. Oh well, we suck. That's what I get for trying to play mind reader.

In debugging some other stuff, I came across the patch for this bug, which is forcing Orca's locus of focus to None by setting orca_state.locusOfFocus field directly.  This is something we should avoid -- we generally only expect this field to be set inside orca.py:setLocusOfFocus.  The impact of setting this outside setLocusOfFocus is that it bypasses the debug messages that let us determine when the locus changes, and it can also lead us to believe the new locus of focus is in a much different place than the old locus of focus.

Joanie - do you think there is some other way we can handle the use case for this bug other than setting locusOfFocus outside the setLocusOfFocus method?

(In reply to comment #11)
> Joanie - do you think there is some other way we can handle the use case for
> this bug other than setting locusOfFocus outside the setLocusOfFocus method?

Hmmmm... Perhaps we could use setLocusOfFocus to set locusOfFocus to None and then immediately call setLocusOfFocus again. Failing that, perhaps we could modify setLocusOfFocus to take an optional argument, force, which prevents setLocusOfFocus from returning even if it concludes the locusOfFocus hasn't changed. Do you have other ideas?

I'm still working on the whereAmI refactor, but after I finish that I can download and install the latest t-bird and see what I can work out.

Created attachment 135732 [details] [review]
Patch to add force parameter

(In reply to comment #12)
> Hmmmm... Perhaps we could use setLocusOfFocus to set locusOfFocus to None and
> then immediately call setLocusOfFocus again. 

This ends up mucking with the context -- instead of going from old to new, it goes from old to None to new, which will create more context than it should.

> Failing that, perhaps we could
> modify setLocusOfFocus to take an optional argument, force, which prevents
> setLocusOfFocus from returning even if it concludes the locusOfFocus hasn't
> changed. 

This is what I was thinking as well.  I went ahead and made the attached patch and checked it in.  Seems to work nicely from the testing I did.  Hope I didn't miss some detail, though.