444644 - When an OBJECT_SHOW event is fired, its MSAA states are not properly set yet.

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect, P1)

Description

[Marco Zehe (:MarcoZ)]

Jamie and I found this while trying to eliminate the speaking of live region text in background tabs of ChatZilla output windows. I tried to test for STATE_SYSTEM_OFFSCREEN (MSAA), which I found present on a hidden tab's table cells properties. However, this state is not present yet when the object is first added (when a new message comes in) and its "show" event is fired.

STR:
1. Start Firefox and ChatZilla.
2. Visit a busy channel and one not so busy. Make sure the busy channel is in the background.
3. Start AccEvent32. Set it to only show OBJECT_SHOW and OBJECT_STATECHANGED events and leave the event info and object data as is.
4. Clear the event output, then focus back to the ChatZilla window and wait a while until a few messages have come in on both channels.
5. Go back to AccEvent32.

Result: A lot of EVENT_SHOW events, one for each table cell that contains either a nickname or an output. All of them have state "normal".
No state change events at all on table cells.

6. Now start AccProbe and navigate to your ChatZilla output, and drill down to the nodes of a foreground and a background tab. The cells on the background tab all have the STATE_SYSTEM_:OFFSCREEN set, while the foreground ones do not, which is correct.

Expected: When the events fire for the cells in the background tabs, the STATE_SYSTEM_OFFSCREEN should already be present.

Interesting side note: All IA2 states were already present when the OBJECT_SHOW event was fired. Checked that with NVDA by asking for MSAA states and IA2 states separately. While the MSAA states were always 0, the IA2 states were non-zero, reflecting what would finally be visible in AccProbe as well.

Comment 1

[Marco Zehe (:MarcoZ)]

This hinders NVDA from properly implementing live region support. Live Regions are an integral part of ARIA. Requesting blocking and setting priority.

Comment 2

[alexander :surkov (:asurkov)]

(In reply to comment #0)

is it possible to create separate testcase?

> Interesting side note: All IA2 states were already present when the OBJECT_SHOW
> event was fired. Checked that with NVDA by asking for MSAA states and IA2
> states separately. While the MSAA states were always 0, the IA2 states were
> non-zero, reflecting what would finally be visible in AccProbe as well.
> 

that's quite strange

Comment 3

[Damon Sicore (:damons)]

Marco, is this a regression from Fx 3?

Comment 4

[Marco Zehe (:MarcoZ)]

(In reply to comment #3)
> Marco, is this a regression from Fx 3?

No, this problem is also present in FX 3.0.x.

Comment 5

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

There has been some work that may have impacted this. Does it still present?

Comment 6

[Marco Zehe (:MarcoZ)]

This is still present in the combination  (Quit: ChatZilla 0.9.84 [Firefox 3.2a1pre/20090228034747].

Comment 7

[Marco Zehe (:MarcoZ)]

I just tested this with ChatZilla and Orca on Linux, and it's reproducible there, too. Text from background tabs leaks through and is spoken via live region updates, like on Windows.

Comment 8

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

I've reproduced on Mac via trace output.

Do we want to send these events at all for background tabs?

Comment 9

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

Please disregard comment #8.

Debugging in our core at nsAccessible::FireAccessibleEvent
When a table row (chat message) is inserted, we have three events:
1. EVENT_TEXT_INSERTED
2. EVENT_DOM_CREATE
3. EVENT_REORDER

In all three cases the related accessible has nsIAccessibleStates::STATE_OFFSCREEN set (aside: these accessibles have no accessible name).

For windows the follow mappings should happen:
1. EVENT_TEXT_INSERTED (should be mapped to IA2_EVENT_TEXT_INSERTED)
2. EVENT_DOM_CREATE (should be mapped to MSAA EVENT_OBJECT_SHOW)
3. EVENT_REORDER (should be mapped to MSAA EVENT_OBJECT_REORDER)

I wonder if someone could please go back to AccEvent32 and attach a log of the event information when a chat message appears in a background tab?

Comment 10

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

Uhm.

I'm looking at nsAccessibleWrap::get_states (in accessible/src/msaa). We call:
GetState(&states, &extraStates)

But don't seem to use "states" sufficiently to properly fill the outparam "aStates".

I must be missing something?

Comment 11

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

Attachment: exploratory fix

Marco, can you try this (untested, uncompiled) patch against NVDA with your modified event_show which includes a check for controlTypes.STATE_OFFSCREEN?

If this works you can keep the change in NVDA, but I'd like to make the FF fix more general/powerful.

Comment 12

[alexander :surkov (:asurkov)]

Iirc get_states of IA2 exposes IA2 states only, AT use get_accstates of IA to get MSAA states.

Comment 13

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

Attachment: exploratory

(In reply to comment #12)
> Iirc get_states of IA2 exposes IA2 states only, AT use get_accstates of IA to
> get MSAA states.

Thanks Alexander. This would be the right exploratory patch then.

I really need to get my windows dev vm set up.

Comment 14

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

Attachment: WIP 1

Marco, can you try this patch out?

Comment 15

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

Attachment: WIP 2

Actually this should be more robust. Calling Flush_Layout should update the rect visibility which we need for state offscreen. (thx roc and bz)

Comment 16

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

Attachment: WIP 3

removed bailing when no dom node, since we later want to check and expose state defunct in the extra state outparam.

Comment 17

[Marco Zehe (:MarcoZ)]

The results with this latest patch are inconsistent. First of all, I needed to apply it to mozilla-1.9.1, not mozilla-central, because mozilla-central currently has a broken accessible tree for role="log", at least with NVDA.

I have a feeling the situation is a bit better, in that not everything leaks through from all ChatZilla tabs. Some things, however, do still leak through, and I haven't yet found out what exactly triggers it. One thing that always comes through is an assertive message, for example something with my nick in it will be spoken in both the current channel and the server info tab. Other stuff such as Firebot messages don't get leaked from other channels most of the time, but again, not always consistently suppressed.

So this general direction seems to be right, we're njust not completely there yet.

Comment 18

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

Attachment: WIP 4

Marco, using Accessibility Probe, these events are appearing invisible and offscreen for each background chat message:
EVENT_OBJECT_REORDER
IA2_EVENT_TEXT_INSERTED
EVENT_OBJECT_SHOW

Can you give it a whirl with NVDA on 1.9.1?

Comment 19

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

(Note I still need to confirm the MSAA state problem)

Comment 20

[Marco Zehe (:MarcoZ)]

Here are my findings with the latest patch:
If I let NVDA create a particular channel's buffer, that channel will leak through. If I just open a channel and let it chatter away without switching to it with F6, as soon as I switch to a different channel, the previous channel won't leak through.

This is a difference from behaviour without the patch, where channels would *always* leak through whether I had NVDA create a buffer for it or not.

So the following sequence of steps can repro the bug:
1. Open Shiretoko compiled with this patch.
2. Open ChatZilla.
3. Without switching to the start pane, with the focus simply in the edit field, type /sslserver irc.mozilla.org 6697 and hit enter.
4. Once that server finishes loading, type /join #accessibility.
5. Listen to stuff as it comes in.
6. Type /join #maildev (or any other channel of your preference) where you know that has regular updates.
7. Let it chatter away. Notice that nothing from the accessibility channel is spoken.
8. Press Ctrl+Shift+Tab to switch back to #accessibility. Notice that now the #accessibility stuff gets spoken, but the other channel is silent.
9. Press F6. This will put focus in the output window, which is HTML content, and will make NVDA generate the virtual buffer.
10. Press Ctrl+Tab. This will switch to the other channel and put focus back to the edit field. Notice that now, chatter comes from both the #maildev and #accessibility channels.
11. Switch back to #accessibility. Notice that now, only #accessibility updates will be spoken. Reason: #maildev still has no virtual buffer created.

These steps repro stuff consistently for me, and again, this is different than without the patch.

Comment 21

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

That is some awesome research. Jamie, Mick, can you dig into this on your side? I'm wondering why the virtual buffer would get in the way here.

Comment 22

[Marco Zehe (:MarcoZ)]

Mick or Jamie, do you have any idea why NVDA might get state bits differently once it creates a virtual buffer for a tab? See my previous comment with the steps to repro. Let me know if you need a build of Firefox with this patch included.

Comment 23

[James Teh [:Jamie]]

(In reply to comment #22)
> Mick or Jamie, do you have any idea why NVDA might get state bits differently
> once it creates a virtual buffer for a tab?
This is very odd. The only thing I can think of is that we hold a reference to the root document object once a buffer is created and we don't release that reference until the buffer is destroyed. I don't understand why this would affect the states on new objects, though. If I'm correct, you should see this problem in AccProbe as well if you first let NVDA create a buffer.

This theory should be relatively easy to test:
1. Do *not* let NVDA create a buffer, but instead, find the output window with the object navigator.
2. Turn off "Caret moves review cursor" (NVDA+6) and "Focus moves navigator object" (NVDA+7) to stop the navigator from moving.
3. Now ctrl+tab to the other channel and check for leakage.

Comment 24

[Marco Zehe (:MarcoZ)]

OK, I did the test, and here's what I found:
1. The rule of "virtual buffer causing the tabs to start leaking" is more general. Basically, it is enough if I let NVDA create a virtual buffer for *any* channel, as soon as I do that, the background tabs start leaking through. I confirmed this twice.
2. As long as I don't create a virtual buffer anywhere within my ChatZilla session, I can use the Object Navigator to walk into the current output window, leave it there, and shift focus to a different tab. Only the live region updates from that tab will then be spoken, but no background tabs.
These, again, start leaking as soon as I create a v buffer for the current tab.

Comment 25

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

Is it possible NVDA is not updating the offscreen state if/when storing objects in the virtual buffer? Jamie? This is a pickle.

Comment 26

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

Attachment: WIP 4.1 (check IsDefunct)

Comment 27

[James Teh [:Jamie]]

The generation of the virtual buffer and NVDA's access to the objects for other purposes are completely separate calls using different instances of the object. To make the separation even clearer, the virtual buffer is generated in-process, while the rest of NVDA is out-of-process. The state bits cannot be shared between the two for the purposes of live regions, as live regions are not handled by the virtual buffer.

It is worth noting that both NVDA and the virtual buffer will handle events for objects within the buffer, the former in-process, the latter out-of-process. My theory is that once the object is instantiated once (or perhaps the first time it is queried for states), subsequent instantiation/queries are incorrect. One way to test this would be to run two instances of accProbe. To really duplicate the test, you probably want to run one in-process and one out-of-process.

Failing this, Marco, as offered earlier, can you please provide a build which includes this patch?

Comment 28

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

(In reply to comment #27)
> Failing this, Marco, as offered earlier, can you please provide a build which
> includes this patch?

I went ahead and started a build. Should be here soon: https://build.mozilla.org/tryserver-builds/2009-05-07_18:[e-mail]-bug444644/

Comment 29

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

(In reply to comment #27)
> Failing this, Marco, as offered earlier, can you please provide a build which
> includes this patch?

Hi James, did you get a chance to try the patched build?

Comment 30

[James Teh [:Jamie]]

My testing reveals that this is not related to NVDA's virtual buffers. I performed the following steps with the buffers disabled. This should also be demonstrable using AccProbe or the like.

Marco, you can disable the buffers by changing this line in virtualBufferHandler.py:
classString="virtualBuffers.gecko_ia2.Gecko_ia2"
to:
return

Steps:
1. Open Chatzilla and join a channel (channel1).
2. Join a second channel (channel2).
3. Press f6 to move to the output window.
4. Press shift+ctrl+tab to move to channel1.
5. Observe that messages from the background tab (channel2) do not have the offscreen or invisible states.

6. Press ctrl+tab to move to channel2.
7. Press f6 to move to the output window.
8. Press shift+f6 to move back to the input window.
9. Press shift+ctrl+tab to move to channel1.
10. Observe that messages from the background tab (channel2) *do* have the offscreen and invisible states.

Overall observation: If focus is in the output window when moving to a different channel, the offscreen and invisible states are not set correctly. If focus is not in the output window, all is well.

Comment 31

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

(In reply to comment #30)
> Overall observation: If focus is in the output window when moving to a
> different channel, the offscreen and invisible states are not set correctly. If
> focus is not in the output window, all is well.

Thanks for documenting your findings Jamie, and thanks for working on channel tonight. Perhaps we are doing something funky in gecko based on a document's gLastFocusedNode. I'll need to get familiar with that code.

Comment 32

[:Gijs (he/him)]

So basically, we have an inputbox key press handler: http://mxr.mozilla.org/seamonkey/source/extensions/irc/xul/content/handlers.js#336
which seems to just throw such an event away.

Then we have a window one: http://mxr.mozilla.org/seamonkey/source/extensions/irc/xul/content/handlers.js#539 which seems to do the right thing.

It would be good to check with a debugger (probably not breaking but counting the number of hits on a breakpoint) if the latter of those fires for both cases.

I mean, in theory if you do this in the input box, it'll hit the event handler for the box, not do anything, then hit the one on the window, and then Just Work. The one on the chat window should just hit the window one.

I'm not sure where that logic fails, then, or how internal state gets subtly altered by what happens so that it ends up with a11y breaking.

Based on the fact that it fails for the output window, I would be tempted to say it might be the tab handlers itself taking care of the tab switch, which might not change internal state in ChatZilla as to what the current content window is.

The tabpanel/tabs stuff we have is kind of... funky... because of old issues with the "normal" implementation. It uses a deck, and manually decides which thing is visible for a given selected tab. I would test some of the above myself if I wasn't on mac - is there a way I could still test for the accessibility states you mention on that platform?

Comment 33

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

Will be interesting to re-test this bug after bug 178324 is fixed.

Comment 34

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

Marco, I pushed the log role fix so testing this should be easier. I think chatzilla tapanel/tabs stuff "is kind of... funky..." might indeed be hitting us here after all (comment #32). There are a few more tests I want to try before punting this over.

Gijs, can you tell us what aria live stuff chatzilla is currently using? Including details on when polite and assertive would be great, and also note that aria-channel is not supported.

Comment 35

[Marco Zehe (:MarcoZ)]

I'm testing this with a custom built ChatZilla that 's already updated with the aria-channel removed. ChatZilla uses role="log" on the table that holds the output, and uses aria-live="assertive" whenever it needs to ping a user (same as when the icon in the status bar starts flashing).

Comment 36

[:Gijs (he/him)]

In addition to what Marco said (which is accurate), https://wiki.mozilla.org/User:GijsKruitbosch/ChatA11Y_AppAuthors#Example:_ChatZilla might help.

Comment 37

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

Attachment: patch 1

This patch is reported by Marco to improve behavior. There is still work to be done investigating the chatzilla focus weirdness (see comment #30).

The consensus is to land this patch, and file a separate bug for the chatzilla focus investigation, pointing back to this bug.

Comment 38

[alexander :surkov (:asurkov)]

Comment on attachment 383491 [details] [diff] [review]
patch 1


>+  if (!IsDefunct()) {

Why you don't use early return?

>+    // update view (and styles etc)
>+    nsCOMPtr<nsIPresShell> presShell = nsCoreUtils::GetPresShellFor(mDOMNode);

it's more robust to use nsAccessNode::GetPresShell() I think

>+    presShell->FlushPendingNotifications(Flush_Layout);

every call should be very good documented, i.e. why it is necessary here.

Comment 39

[alexander :surkov (:asurkov)]

btw, could your approach help to the bug 498015?

Comment 40

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

(In reply to comment #38)
> (From update of attachment 383491 [details] [diff] [review])
> 
> >+  if (!IsDefunct()) {
> 
> Why you don't use early return?

I can't bail here because we want to expose the EXT_STATE_DEFUNCT (in GetStateInternal) :)


> 
> >+    // update view (and styles etc)
> >+    nsCOMPtr<nsIPresShell> presShell = nsCoreUtils::GetPresShellFor(mDOMNode);
> 
> it's more robust to use nsAccessNode::GetPresShell() I think

Done.

> 
> >+    presShell->FlushPendingNotifications(Flush_Layout);
> 
> every call should be very good documented, i.e. why it is necessary here.

Will do.

Comment 41

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

(In reply to comment #39)
> btw, could your approach help to the bug 498015?

I've asked Rob to take a look.

Comment 42

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

Attachment: patch 2

Addresses comments.

Comment 43

[alexander :surkov (:asurkov)]

Comment on attachment 383762 [details] [diff] [review]
patch 2


>+    // Flush layout so that all the frame construction, reflow, and styles are
>+    // up-to-date. We don't flush the display because we don't care about
>+    // painting.

It's much better :) But I'd like to see comment why we need to flush layout in THIS place exactly.

I understand we should flush layout when calculating the states because we rely on layout to grab the states (but we need to document this). What about events?

waiting for comments and new patch :)

Comment 44

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

Attachment: patch 3 - improved comments

Comment 45

[alexander :surkov (:asurkov)]

Comment on attachment 383854 [details] [diff] [review]
patch 3 - improved comments

r=me

Comment 46

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

Thanks. I filed bug 499068 for follow up.

Now what remains for this bug is filing a Chatzilla bug with whatever information we have about how its custom algorithms for deciding what tabs are visible might be failing. (see comment #32).

Gijs, did you get your windows env set up?

Comment 47

[:Gijs (he/him)]

(In reply to comment #46)
> Thanks. I filed bug 499068 for follow up.
> 
> Now what remains for this bug is filing a Chatzilla bug with whatever
> information we have about how its custom algorithms for deciding what tabs are
> visible might be failing. (see comment #32).
> 
> Gijs, did you get your windows env set up?

No. I realised that my CD/DVD drive actually doesn't work - something which I've been meaning to go to an Apple Store about, but haven't so far, and right now I can't seem to get a Genius bar reservation for the next 3 days... Blegh. Anyway, I can try at uni, but I won't be able to build there (no disk space).


Actually, now that I think about it... why is this a ChatZilla problem? From comment #30, wouldn't it be Gecko's job to take focus away from an element that's disappearing, and pick the Next Best Thing or the main window or something? I'd guess that's all that's really wrong...

Comment 48

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

(In reply to comment #47)
> Actually, now that I think about it... why is this a ChatZilla problem? From
> comment #30, wouldn't it be Gecko's job to take focus away from an element
> that's disappearing, and pick the Next Best Thing or the main window or
> something? I'd guess that's all that's really wrong...

As per IRC:
We can't recreate this problem in FF proper and we have code to ensure something is always focused (which presumably removes focus from the previously focused element automatically). I might be able to give more detailed diagnosis but I was having general almost-hang issues with the latest CZilla and tools like Accprobe on Windows.

Comment 49

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

Attachment: Test: mimic chat messages in FF tabs

Attaching a test (originally from Gijs) used to confirm this bug is not reproducible in firefox+gecko. To use the test, unzip the files and load accessibility.html. Open a new tab and confirm that new messages in the background tab don't leak through (have state OFFSCREEN).

Comment 50

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

Pushed as changeset: http://hg.mozilla.org/mozilla-central/rev/456d6d0bd8a4

Opened bug 499731 for pixie hunting.

Comment 51

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

Follow up: that changeset combined with our frame tree walker and the frame poisoning work are thought to have caused bug 525579. We might back this out (at least) for 1.9.2.