629415 - Landing of "deal with cached accessibility tree" broke Thunderbird MozMill tests | TEST-UNEXPECTED-FAIL | test-message-header.js | test_a11y_attrs [PERMANENT ORANGE]

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Mark Banner (:standard8)]

Bug 606924 attachment 504402 [details] [diff] [review] landed a patch that "deals with cached accessibility tree only".

This has broken the Thunderbird MozMill tests:

TEST-UNEXPECTED-FAIL | e:\buildbot\comm-central-trunk-win32-opt-unittest-mozmill\build\mozmill\message-header\test-message-header.js | test_a11y_attrs
  EXCEPTION: headerAccessible is null
    at: test-message-header.js line 278
       verify_header_a11y([object Object],4,[object Array]) test-message-header.js 278
       test_a11y_attrs() test-message-header.js 309
            frame.js 474
            frame.js 530
            frame.js 573
            frame.js 417
            frame.js 579
            server.js 164
            server.js 168

The offending line being here:

http://hg.mozilla.org/comm-central/annotate/4440b9d5b3c2/mail/test/mozmill/message-header/test-message-header.js#l278

There doesn't seem to have been any interface changes with the patch that landed so I'm wondering if this is a regression in the code?

Comment 1

[alexander :surkov (:asurkov)]

Marco, could you look at this? I'd need a11y tree before and after the patch.

Comment 2

[alexander :surkov (:asurkov)]

Could it be because tests are running too early before accessible tree is created? When do they start? Do they wait for any a11y event for that?

Comment 3

[Mark Banner (:standard8)]

(In reply to comment #2)
> Could it be because tests are running too early before accessible tree is
> created? When do they start? Do they wait for any a11y event for that?

The tests currently run when we've finished loading a message. I believe that this is basically once the docshell for the browser element tells us that the message has finished loading.

I'm guessing that the message header updates we fire off before loading the message, though I'm a bit sketchy on exactly what happens there.

Comment 4

[Marco Zehe (:MarcoZ)]

There is definitely no general problem in creating those accessibles. I just downloaded the latest nightly for Thunderbird 3.3 and ran it. Both the preview pane and the full message viewer have the expected info when I load a message and tab through it. NVDA gives me correct results each time. So I suspect that the tests run too early for a11y to be ready, and that because we're keying off of something different now than we used to, these tests are now failing.

Alexander, what exactly did this part of bug 606924 change that might affect this?

And Alex, the trees look identical in both today's and yesterday's nightly builds. So as soon as I get to the tree with either NVDA or AccProbe, it's fully there.

Comment 5

[Mark Banner (:standard8)]

Also, which ally event would we need to wait for before testing this?

Comment 6

[alexander :surkov (:asurkov)]

(In reply to comment #4)

> Alexander, what exactly did this part of bug 606924 change that might affect
> this?

This patch denied accessible creation on accessible request to prevent possible tree creation at wrong time. In general in bug 606924 we started to create the tree after layout what happens async.

> And Alex, the trees look identical in both today's and yesterday's nightly
> builds. So as soon as I get to the tree with either NVDA or AccProbe, it's
> fully there.

Thank you for investigations!

(In reply to comment #5)
> Also, which ally event would we need to wait for before testing this?

If it's a content document then document load complete is fired what is right event to start the test. If this document is chrome but is not main Thunderbird window then it's ok to wait for reorder event on document container (like iframe, browser elements), (I assume chrome document is loaded entirely at once and thus it should work). If this is main Thunderbird window then we might be in trouble since no events may be fired in this case, but timeout should help.

But I guess messages should be a content documents then 
1) if the document doesn't have busy state then start the test
2) otherwise wait for load complete event

Marco, can you please help with JS to make this working?

Comment 7

[Mark Banner (:standard8)]

(In reply to comment #6)
> But I guess messages should be a content documents then 
> 1) if the document doesn't have busy state then start the test
> 2) otherwise wait for load complete event

The message header is chrome and is loaded separately from the message body which is loaded into content.

We are already waiting for the content document not to have a busy state.

Comment 8

[alexander :surkov (:asurkov)]

(In reply to comment #7)
> (In reply to comment #6)
> > But I guess messages should be a content documents then 
> > 1) if the document doesn't have busy state then start the test
> > 2) otherwise wait for load complete event
> 
> The message header is chrome and is loaded separately from the message body
> which is loaded into content.

then reorder event on its container should work.

Comment 9

[Mark Banner (:standard8)]

So I've just tried waiting for the reorder event, but either that doesn't work or my code is wrong (I'm currently testing on an opt build rather than debug, which unfortunately gives me little output).

As that failed, I tried a simple sleep for 10 seconds, however after that I'm still getting the fact that headerAccessible is null. This I wouldn't expect from Marco's tests on trunk.

I'm going to build this in debug mode now and see what I can get out of that.

Comment 10

[alexander :surkov (:asurkov)]

(In reply to comment #9)

> As that failed, I tried a simple sleep for 10 seconds, however after that I'm
> still getting the fact that headerAccessible is null. This I wouldn't expect
> from Marco's tests on trunk.

You have a running a11y before start a sleeping, right? (for example, getting an accessibility service starts it), otherwise you can sleep forever :)

Comment 11

[Mark Banner (:standard8)]

Before hitting that part of the test, we do:

let gAccRetrieval = Cc["@mozilla.org/accessibleRetrieval;1"].
                     getService(Ci.nsIAccessibleRetrieval);

http://mxr.mozilla.org/comm-central/source/mail/test/mozmill/message-header/test-message-header.js#262

From what I can tell that should be starting the a11y service, unless there's something else needed?

Comment 12

[alexander :surkov (:asurkov)]

(In reply to comment #11)

> From what I can tell that should be starting the a11y service, unless there's
> something else needed?

that's correct. I run out of ideas if 10 secs timeout after that doesn't help.

Comment 13

[Mark Banner (:standard8)]

Attachment: Example Header

I've now done some more in-depth debugging. If I alter the test slightly, then getAccessibleFor() works fine and the test passes without any extra code (e.g. sleeps).

I think these would be the steps to reproduce for a failure case:

1) Have two messages in a folder, one of these should contain lots of CC addresses such that the "n more" button is displayed. The other message should have just one CC address (and hence no "n more" displayed).

2) Select the message with lots of CC addresses.

3) Select the more button. At this stage the addresses will expand.

4) Select the second message, this will effectively cause the more button to collapse again.

5) Re-select the message with the many CCs

This is where getAccessibleFor now starts failing, first of all on the CC addresses.

The attached screenshot shows the header of the test message that is showing the problem.

If I remember correctly, the more header button is effectively turned on and off by detecting overflows and underflows. So there's likely to be some reflows caused.

My suspicion now is that the accessibility code isn't handling this case properly.

Hopefully Marco might be able to reproduce it with the steps given above.

Comment 14

[alexander :surkov (:asurkov)]

(In reply to comment #13)
> Created attachment 508767 [details]
> Example Header
> 
> I've now done some more in-depth debugging. If I alter the test slightly, then
> getAccessibleFor() works fine and the test passes without any extra code (e.g.
> sleeps).

How did you workarounded the problem? Could you think of testcase creation to show the problem please?

> Hopefully Marco might be able to reproduce it with the steps given above.

Marco, please check if you can reproduce this. It sounds like quite serious problem.

Comment 15

[Mark Banner (:standard8)]

(In reply to comment #14)
> (In reply to comment #13)
> > Created attachment 508767 [details]
> > Example Header
> > 
> > I've now done some more in-depth debugging. If I alter the test slightly, then
> > getAccessibleFor() works fine and the test passes without any extra code (e.g.
> > sleeps).
> 
> How did you workarounded the problem? Could you think of testcase creation to
> show the problem please?

Opps, sorry, I missed that bit. During the steps to repeat skip step 3 then it works.

Comment 16

[Marco Zehe (:MarcoZ)]

I am not able to reproduce this with NVDA running. When I expand CCs with the "x more" button/link, then select a message that has fewer CCs, then go back to the previous one, I get the accessibles spoken just fine again. With and without expanding via the "x more" button.
Also if I don't expand the CCs in step 3, and select the other, then reselect the one with many CCs, the accessibles stay.

So NVDA definitely has some influence here that this bug is not exposed to me.

Comment 17

[Mark Banner (:standard8)]

So what's the way forward here? Although Marco can't reproduce, this feels to me like a core issue rather than a test case issue (given that sleep doesn't resolve)?

At the moment I think I'm going to have to disable the broken tests in Thunderbird, so we can get the tree green again, and then work out how we can resolve this.

Comment 18

[alexander :surkov (:asurkov)]

(In reply to comment #17)
> So what's the way forward here?

It would be really helpful to have minimal testcase I can run in Firefox. Otherwise I need to take a time to dig though a11y debugging in Thunderbird what may not so easy.

Comment 19

[Mark Banner (:standard8)]

Ok, I've just been chatting to asuth about this over irc.

Given that the code is still working for MarcoZ with NVDA, we're semi-expecting that this is more likely a test infrastructure issue, possibly to do with issues around XBL.

We have therefore decided to move the broken test to before the one that is triggering the current failure (i.e. before the one doing the more button expansion), this gets the tree green again and maintains most of the test in the non-complex case.

We've decided not to pursue the failure further at this time as we're not quite sure what is going on, and there's no clear benefit to resolving this fully. Whilst we could leave ourselves open to have future failures here, we still have the majority of the test in place and hopefully we'll be covered by other tests and probably some manual testing along the way. Therefore I'm going to resolve this bug as fixed for now.

Comment 20

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

(In reply to comment #13)
> If I remember correctly, the more header button is effectively turned on and
> off by detecting overflows and underflows. So there's likely to be some reflows
> caused.

How exactly does tbird respond to overflows and underflows?

> 
> My suspicion now is that the accessibility code isn't handling this case
> properly.

If so, we should reopen or file a new bug. I was planning to sit with Blake on Monday to deep dive on this.

Comment 21

[Mark Banner (:standard8)]

(In reply to comment #20)
> (In reply to comment #13)
> > If I remember correctly, the more header button is effectively turned on and
> > off by detecting overflows and underflows. So there's likely to be some reflows
> > caused.
> 
> How exactly does tbird respond to overflows and underflows?

I believe that if a list of addresses overflows the bounds, then a "more" button/bit of text appears, at which point the user can click it and the appropriate element expands.

> > My suspicion now is that the accessibility code isn't handling this case
> > properly.
> 
> If so, we should reopen or file a new bug. I was planning to sit with Blake on
> Monday to deep dive on this.

It'd maybe solve the puzzle if you could take a look. I'm sure Blake can help you out.