575693 - Download Manager and Page Info no longer have an accessible tree

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Marco Zehe (:MarcoZ)]

STR:
1. Open Minefield.
2. Open Download Manager by pressing Ctrl+J.

Expected: Download Manager should have an accessible tree with the list, search field etc in it.
Actual: Has no tree. Nothing is accessible any more except the frame.

Regression range between regular nightly builds:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=68d98f30eda0&tochange=3a9f81291788

Comment 1

[Marco Zehe (:MarcoZ)]

The try-server build from bug 573955 is also still OK, does not show the bug.

Comment 2

[alexander :surkov (:asurkov)]

(In reply to comment #1)
> The try-server build from bug 573955 is also still OK, does not show the bug.

Does it mean it's not our bad?

Comment 3

[Marco Zehe (:MarcoZ)]

No, I believe this might be bug 573910, which was checked in after the try-server build was made.

Comment 4

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

Note to drivers, I think this should block 1.9.3. It is a major regression.

Comment 5

[alexander :surkov (:asurkov)]

(In reply to comment #3)
> No, I believe this might be bug 573910, which was checked in after the
> try-server build was made.

That try sever build was made over bug 573910. Though I did merging and I might do this wrong.

Comment 6

[Mike Beltzner [:beltzner, not reading bugmail]]

It will block Firefox 4, but not beta1.

Comment 7

[Marco Zehe (:MarcoZ)]

Another window that's affected: Tools/Page Info. Same symptom.

Comment 8

[alexander :surkov (:asurkov)]

Btw, native window accessible object is not returned for Firefox window any more, so that accprobe shows application accessible. That doesn't describe why dialogs fails to get their children but taking into account DOMi shows correct tree it makes me think bug 573910, especially because that bug didn't introduce any special handling of dialog accessibles. I think that was window toolbar reorganization, might be bug 513162, at least it looks suspicious for me.

Btw, we always fail to get hwnd here (http://mxr.mozilla.org/mozilla-central/source/accessible/src/msaa/nsAccessibleWrap.cpp#225) and that's not what we expect (therefore we return application accessible).

Comment 9

[alexander :surkov (:asurkov)]

(In reply to comment #8)
> That doesn't describe why
> dialogs fails to get their children but taking into account DOMi shows correct
> tree it makes me think bug 573910, 

I meant it makes me think this bug not guilty.

Comment 10

[:Felipe Gomes (needinfo for replies!)]

It's very likely a fall-out from bug 513162. Xul windows don't use a child widget to hold its content now, they're directly attached to the top-level window.

I don't know the reasons behind that if() and comment, so I won't guess how those should be updated without breaking other parts, but I think that's the reason why that GetParent never returns a hwnd.

Comment 11

[alexander :surkov (:asurkov)]

current logic of get_accParent was introduced in bug 291077. There's a comment there:

1. If the current object is at the root of its own HWND, we return the operating
system's generated accessible for that HWND. Howver, sometimes there are 2 HWNDs
-- 1 containing scrollbars and 1 child of that containing just the client area.
In that case we want to get the accessible from the parent of the two HWNDs,
otherwise get_accParent() will continually loop.

Taking into account this and Felipe's comments I guess we don't need to get HWND for outer window now.

Comment 12

[alexander :surkov (:asurkov)]

confirm regression from bug 513162

Comment 13

[alexander :surkov (:asurkov)]

Attachment: patch

Comment 14

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

(In reply to comment #10)
> It's very likely a fall-out from bug 513162. Xul windows don't use a child
> widget to hold its content now, they're directly attached to the top-level
> window.

Aha! ok... about to r+ then...

Comment 15

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

Comment on attachment 455118 [details] [diff] [review]
patch

r=me but please consider adding a comment like:

// XUL widgets connect content directly to the top level window so we are done (no need to look for parent HWND).

I wish we could get this under test.

Comment 16

[Marco Zehe (:MarcoZ)]

Comment on attachment 455118 [details] [diff] [review]
patch

My build with this patch and NVDA running freezes upon launch. It immediately starts showing as "not responding" in the task bar of Windows 7, and the only thing I can do is kill the process from Task Manager.

This is not the right fix it seems, minusing.

Comment 17

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

Comment on attachment 455118 [details] [diff] [review]
patch

Retracting r+ since I'll need to see another patch. I agree (IRC) it sounds like looping similar to bug 291077 perhaps.

Comment 18

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

Alexander can you debug and see if NVDA is stuck looping acc_getParent?

Comment 19

[alexander :surkov (:asurkov)]

Attachment: make docaccessible a window

It looks NVDA works, though it should loose accessibles in title bar. accprobe shows our hierarchy but UI Spy shows wrong hierarchy, dupe trees are presented.

Comment 20

[alexander :surkov (:asurkov)]

(In reply to comment #19)
> Created an attachment (id=455434) [details]
> make docaccessible a window
> 
> It looks NVDA works, though it should loose accessibles in title bar. accprobe
> shows our hierarchy but UI Spy shows wrong hierarchy, dupe trees are presented.

It doesn't matter whether we respond to OBJID_WINDOW or not, native window accessible is created that contains accessible for menu, tabs and etc toolbars, native title bar accessible and plus document accessible that contains these toolbars as well.

Comment 21

[alexander :surkov (:asurkov)]

Jamie, any ideas? The window responds to OBJID_CLIENT and return document accessible what implements IOleWindow, we don't include native window accessibles into our hierarchy but UISpy shows that accessible tree is duplicated as I described in previous comment.

Comment 22

[alexander :surkov (:asurkov)]

Attachment: exclude native windows accessible from hierarchy

Comment 23

[alexander :surkov (:asurkov)]

(In reply to comment #19)
> Created an attachment (id=455434) [details]
> make docaccessible a window

http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/[e-mail]-b57fbe6b1d83/

Comment 24

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Why are you commenting out this code instead of just removing it?

Comment 25

[alexander :surkov (:asurkov)]

(In reply to comment #24)
> Why are you commenting out this code instead of just removing it?

I don't know what's expected and what we should do, these patches are ideas and tries.

Comment 26

[alexander :surkov (:asurkov)]

(In reply to comment #22)
> Created an attachment (id=455449) [details]
> exclude native windows accessible from hierarchy

http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/[e-mail]-4c8a38420fad/

Comment 27

[James Teh [:Jamie]]

In current nightlies (no patches), the accessibility tree for the Download Manager *is* actually present and correct. The problem is that WM_GETOBJECT for the Download Manager hwnd is not returning a Gecko IAccessible.

As discussed with Alex on IRC, the problem is in widget/src/windows/nsWindow.cpp in nsWindow::GetRootAccessible():
  if (mContentType != eContentTypeInherit) {
    // We're on a MozillaContentWindowClass or MozillaUIWindowClass window.
    // Search for the correct visible child window to get an accessible 
    // document from. Make sure to use an active child window
    HWND accessibleWnd = ::GetTopWindow(mWnd);
Because the MozillaWindowClass window is now gone, mwnd is actually the top level window already and GetTopWindow(mwnd) returns 0. The code continues:
    while (accessibleWnd) {
...
    }
    return nsnull;
Bingo. No Gecko accessible.

Comment 28

[James Teh [:Jamie]]

Two fixes are required here:
* Fix the issue i described in comment #27; and
* Make accParent on top level accessibles return the native window accessible instead of returning the application accessible, as it did before this bug was introduced. I think one of Alex's patches fixes this already.

Comment 29

[alexander :surkov (:asurkov)]

Attachment: patch

It doesn't fix hierarchy but makes return gecko accessible object for OBJID_CLIENT and Firefox dialogs work with NVDA (at least I didn't noticed anything unexpected in NVDA work). When I try to fix hierarchy (the first patch) then NVDA hangs. But we can work on correct hierarchy in bug 441553 and close this one if windows screen readers feels good.

Comment 30

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

What happens for Linux/Orca?

Comment 31

[alexander :surkov (:asurkov)]

(In reply to comment #30)
> What happens for Linux/Orca?

I think linux relies on object hierarchy we provide.

Comment 32

[alexander :surkov (:asurkov)]

(In reply to comment #29)
> Created an attachment (id=455661) [details]
> patch

http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/[e-mail]-ac1cf2f2824f/

Comment 33

[Marco Zehe (:MarcoZ)]

Observations:
1. JAWS no longer loads any content into the virtual buffer.
2. Window-Eyes does, but now treats the menu bar as if it was a context menu instead and easily gets confused.

It would appear that both rely on the window hierarchy which no longer is as it was before.

Comment 34

[alexander :surkov (:asurkov)]

Attachment: patch2

Marco, could you try this one?

Comment 35

[Marco Zehe (:MarcoZ)]

Comment on attachment 455689 [details] [diff] [review]
patch2

No change.

Comment 36

[alexander :surkov (:asurkov)]

(In reply to comment #34)
> Created an attachment (id=455689) [details]
> patch2

http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/[e-mail]-ac1cf2f2824f/

Comment 37

[alexander :surkov (:asurkov)]

(In reply to comment #35)
> (From update of attachment 455689 [details] [diff] [review])
> No change.

How many tabs/windows do you have open?

Comment 38

[Marco Zehe (:MarcoZ)]

(In reply to comment #37)
> How many tabs/windows do you have open?

Happens with either 1 tab and 1 window, 1 tab + another window like the Download Manager, or multiple tabs.

Comment 39

[James Teh [:Jamie]]

(In reply to comment #29)
> It doesn't fix hierarchy
> When I try to fix hierarchy (the first
> patch) then NVDA hangs.
This suggests something is very wrong. We probably need to understand why this is happening and fix it. As I understand it, the code needs to correctly handle cases where there is a MozillaWindowClass outer window as well as cases where there is no MozillaWindowClass window.

> But we can work on correct hierarchy in bug 441553 and
> close this one if windows screen readers feels good.
I don't think we can. As I said on IRC, accParent on the root accessible needs to return the window accessible, not the application accessible, otherwise WindowFromAccessibleObject() will return an incorrect result. NVDA uses IAccessible2::windowHandle, but other ATs might be using WindowFromAccessibleObject(). I think bug 441553 is a separate issue.

Comment 40

[alexander :surkov (:asurkov)]

Firefox window:
HWND #1 - root accessible
  HWND #2 - root accessible for the parent HWND
    HWND #3 - document accessible for loaded web page

1) root accessible is returned for #1 (TopWindow is #2)
2) document accessible is returned for #2 (TopWindow is #3)
3) document accessible is returned for #3 (contentType is inherited)

Perhaps JAWS and WEeys relies on this and therefore they can't load web page with latest patches.

Bookmarks library has one HWND in hierarchy and therefore we return null since there's no child windows and TopWindow returns null as result. This is a reason why firefox dialogs (like bookmarks library and downloads) aren't accessible.

Comment 41

[alexander :surkov (:asurkov)]

Attachment: patch3

Comment 42

[alexander :surkov (:asurkov)]

(In reply to comment #41)
> Created an attachment (id=455845) [details]
> patch3

try server build - http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/[e-mail]-af72c99decd9/

Comment 43

[James Teh [:Jamie]]

(In reply to comment #41)
> Created an attachment (id=455845) [details]
> patch3
This one works very nicely. Thanks!

Comment 44

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

Comment on attachment 455845 [details] [diff] [review]
patch3

I'm fine with the /accessible changes. I want to understand the widget changes... couldn't you just remove the "return nsnull"?

Comment 45

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

Comment on attachment 455845 [details] [diff] [review]
patch3

Anyways r=me for our module :)

I think nsWindow::GetRootAccessible could be tidied up in a follow up bug.

Comment 46

[Marco Zehe (:MarcoZ)]

Comment on attachment 455845 [details] [diff] [review]
patch3

rs feedback based on a talk with Jamie.

Comment 47

[Mike Beltzner [:beltzner, not reading bugmail]]

Does this need sr? Moving to beta3, but we should ping roc out of band to get the review expedited.

Comment 48

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 455845 [details] [diff] [review]
patch3

This doesn't really need sr.

It does, however, need a test.

Comment 49

[alexander :surkov (:asurkov)]

landed on 2.0 (http://hg.mozilla.org/mozilla-central/rev/b6182f82c720).