Closed Bug 591874 Opened 14 years ago Closed 14 years ago

Windows screen readers are broken due to post-130078 changes in the native widget structure

Categories

(Core :: Widget: Win32, defect, P1)

Product:
Core
Component:
Widget: Win32
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Tracking Flags:
Tracking Status
blocking2.0 --- beta7+

People

(Reporter: MarcoZ, Assigned: surkov)

References

(Blocks 1 open bug)

Details

(Keywords: access, regression, relnote)

Attachments

(2 files, 1 obsolete file)

wip
16.44 KB, patch
Details | Diff | Splinter Review
patch2
27.62 KB, patch
davidb
: review+
MarcoZ
: feedback+
Details | Diff | Splinter Review
This is most likely the result of one or several of the child windows patches that landed on Friday. The regression window is: http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=e1d55bbd1d1d&tochange=6e3f6d18c124 Firefox is no longer useable by any blind user with any assistive technology, be it NVDA or JAWS or any other, breaking over 95% of our blind user base.
blocking2.0: --- → ?
Marco, do you suspect bug 130078?
(In reply to comment #1) > Marco, do you suspect bug 130078? Marco, btw, this bug contains try server build. Could you try it? At least, it may reduce regression range.
Bug 130078 is highly likely the cause here.
(In reply to comment #2) > (In reply to comment #1) > > Marco, do you suspect bug 130078? > > Marco, btw, this bug contains try server build. Could you try it? At least, it > may reduce regression range. though if try server build - http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/tnikkel@gmail.com-cf64eea45e07/ doesn't work then that bug is definitely guilty since it's dated by 23 august.
Yes, the latest try-server build from bug 130078 shows the same problem.
Blocks: 130078
Timothy, could you describe changes?
The content area of the browser used to have its own widget, or native window. Bug 130078 removed this, so there is only one toplevel widget per browser window (on Windows at least). See bug 589529, comment 10 for a good explanation. Sorry for not looping in a11y folks earlier on 130078, I must have had it on a list of stuff todo somewhere, but ultimately it slipped my mind.
(In reply to comment #7) > The content area of the browser used to have its own widget, or native window. > Bug 130078 removed this, so there is only one toplevel widget per browser > window (on Windows at least). See bug 589529, comment 10 for a good > explanation. One problem might be in http://mxr.mozilla.org/mozilla-central/source/accessible/src/msaa/nsAccessibleWrap.cpp#193 if view logic was changed. For example, if nsIFrame::GetViewExternal() started to return nsnull. > Sorry for not looping in a11y folks earlier on 130078, I must have had it on a > list of stuff todo somewhere, but ultimately it slipped my mind. we were cc'ed to this bug (at least I), a long discussion there made our minds to slip as well.
Jamie, if you have a chance to investigate on your side what was broken then it really helps us to fix the problem.
(In reply to comment #8) > One problem might be in > http://mxr.mozilla.org/mozilla-central/source/accessible/src/msaa/nsAccessibleWrap.cpp#193 > if view logic was changed. For example, if nsIFrame::GetViewExternal() started > to return nsnull. We didn't remove any views, so GetViewExternal shouldn't be returning null any more then it used to. The view->GetWidget() call will now only be non-null when it returns the toplevel chrome widget instead of before 130078 when it could also return the toplevel content widget.
To reiterate: Simple MSAA is working: I get speech feedback, focus events etc. when in the awesomebar or when tabbing through a web page. it is specifically the creation of virtual buffers that fails.
So IAccessible::GetParent() logic has been changed, view->GetWidget() returns null for content documents, right? - http://mxr.mozilla.org/mozilla-central/source/accessible/src/msaa/nsAccessibleWrap.cpp#210. Do content document have associated windows with HWND, no?
(In reply to comment #12) > So IAccessible::GetParent() logic has been changed, view->GetWidget() returns > null for content documents, right? - > http://mxr.mozilla.org/mozilla-central/source/accessible/src/msaa/nsAccessibleWrap.cpp#210. Correct. > Do content document have associated windows with HWND, no? Content documents themselves do not have associated windows anymore. Bug 130078 also linked the viewmanager's for the chrome and content documents, so that if you keep looking up the frame/view hierarchy you will still eventually get to a widget.
Are there any changes for nsIView::GetNearestWidget() and nsIViewManager::GetRootWidget()?
(In reply to comment #14) > Are there any changes for nsIView::GetNearestWidget() and > nsIViewManager::GetRootWidget()? Those functions themselves work the same way. But most of the time (except in popups) they'll both be returning the same widget, the top level chrome widget.
Ok, at least one problem is found. IAccessible::GetParent() called on content document accessible should return system accessible for chrome window because nsIPressShell::GetRootFrame() of content document should be scrollable frame and nearest widget should be widget for chrome - http://mxr.mozilla.org/mozilla-central/source/accessible/src/msaa/nsAccessibleWrap.cpp#238. Child to parent hierarchy is broken but I'm not sure if this prevents virtual buffer to work.
(In reply to comment #16) > Ok, at least one problem is found. IAccessible::GetParent() called on content > document accessible should return system accessible for chrome window because > nsIPressShell::GetRootFrame() of content document should be scrollable frame > and nearest widget should be widget for chrome - > http://mxr.mozilla.org/mozilla-central/source/accessible/src/msaa/nsAccessibleWrap.cpp#238. > Child to parent hierarchy is broken but I'm not sure if this prevents virtual > buffer to work. nsIPressShell::GetRootFrame() of any document is never a scrollable frame. Also, I think the line you linked to no longer executes in most cases as scrollable frames do not have views anymore.
(In reply to comment #17) > (In reply to comment #16) > > Ok, at least one problem is found. IAccessible::GetParent() called on content > > document accessible should return system accessible for chrome window because > > nsIPressShell::GetRootFrame() of content document should be scrollable frame > > and nearest widget should be widget for chrome - > > http://mxr.mozilla.org/mozilla-central/source/accessible/src/msaa/nsAccessibleWrap.cpp#238. > > Child to parent hierarchy is broken but I'm not sure if this prevents virtual > > buffer to work. > > nsIPressShell::GetRootFrame() of any document is never a scrollable frame. hm, ok. then hierarchy shouldn't be ok > Also, I think the line you linked to no longer executes in most cases as > scrollable frames do not have views anymore. yes, this code looks like rudiment.
I failed to find anything unusual in hierarchy (except nsXULTabpanelAccessible case which frame implements nsIScrollableFrame and that a11y scrollable frame code triggers what should resutls in wrong hierarchy. I didn't see this code is executed before bug 130078, perhaps GetWidget() changes but I'm not sure). Any way I fixed hierarchy locally but it didn't help: vb doesn't work. Perhaps screen readers expect HWND (system window accessible) for content documents to start the virtual buffer work. I think we need feedback from NVDA guys.
The main problem is that the window class of documents changed from MozillaContentWindowClass to MozillaUIWindowClass. NVDA (and obviously other screen readers as well) uses the window class to restrict the loading of buffers for documents. We didn't originally want to do this, but unfortunately, not restricting based on window class caused us to create documents in cases where we shouldn't such as sub-frames and occasionally the entire application window. This is presumably because Gecko sometimes sets the role to document in cases when it shouldn't, but we were never able to figure out what was going on here, so just restricted it based on window class. I guess we will need to remove this restriction in NVDA and then deal with any regressions as they arise. However, there is also another case of serious brokenness that seems to be new. I'm not sure if it's related to this bug, but here it is anyway; I can file a separate bug if needed. Str: 1. Open http://realgokarts.com/pricing.htm as the only tab in Minefield. 2. Check the name of the top ROLE_WINDOW accessible. Result (correct): The name should be "ARCHERFIELD SPEED KARTS - MINEFIELD". 3. Check the last child of the top ROLE_WINDOW accessible. Result (correct): The client accessible should be the top frame accessible for the browser. 4. Ensure that Minefield will restore this tab next time it starts. 5. Restart Minefield. 6. Check the name of the top ROLE_WINDOW accessible. Expected: The name should be "ARCHERFIELD SPEED KARTS - MINEFIELD" as it was in step 2. Actual: The name is just "ARCHERFIELD SPEED KARTS". 7. Check the last child of the top ROLE_WINDOW accessible. Expected: The client accessible should be the top frame accessible for the browser as it was in step 2. Actual: The client accessible is the document accessible for the web document. Note that once the browser is in this state, the accessibility hierarchy seems to be broken in many other ways until you restart. I haven't debugged this yet, but I see a whole lot of unknown/broken objects.
Marco, not sure if you have an NVDA dev environment set up anymore, but if you do, the relevant check is in source/NVDAObjects/IAccessible/mozilla.py in _get_treeInterceptorClass (around line 75). If you remove the check for windowClassName, that should fix the issue. I'm reluctant to do this in the main NVDA code this close to an NVDA release in case we start seeing the same problems that caused us to add it in the first place. However, maybe we can restrict this change to FF 4.
Jamie, thanks for your input, valuable and very helpful as always! No, I do not have a current NVDA dev environment set up any more. Almost everybody seems to check for these window classes it seems. The symptoms are the same across ATs.
(In reply to comment #20) > The main problem is that the window class of documents changed from > MozillaContentWindowClass to MozillaUIWindowClass. I think it's because content documents shares HWND for chrome window. So we have few options 1) either request screen readers to be changed - very undesirable thing for commercial screen reader users 2) or backout bug 130078 or its part 3) or make some magic on a11y side (create window accessibles instead of system window accessible usage if that helps)
(In reply to comment #22) > Almost everybody seems to check for these window classes it seems. As i noted, we really wanted to avoid this silly check, but it caused problems. In hindsight, we should have tried harder to figure out what was causing them and filed bugs appropriately. However, at the time, we needed to get it working. :)
(In reply to comment #23) > 3) or make some magic on a11y side (create window accessibles instead of system > window accessible usage if that helps) I don't think it will help. You need to make GetClassName() on the window returned from WindowFromAccessibleObject()/IAccessible2::windowHandle on the document accessible return MozillaContentWindowClass. Unless I'm missing something, even if you create fake ROLE_WINDOW accessibles, you'd then have to create a fake HWND with a class name of MozillaContentWindowClass.
Though window class name is not a property of system window accessible object I guess, so we have two options actually or only one - 2nd one.
(In reply to comment #25) > (In reply to comment #23) > > 3) or make some magic on a11y side (create window accessibles instead of system > > window accessible usage if that helps) > I don't think it will help. You need to make GetClassName() on the window > returned from WindowFromAccessibleObject()/IAccessible2::windowHandle on the > document accessible return MozillaContentWindowClass. Unless I'm missing > something, even if you create fake ROLE_WINDOW accessibles, you'd then have to > create a fake HWND with a class name of MozillaContentWindowClass. Yes, unfortunately you're right.
Timothy, can we get back window (or widget in gecko terms if I get right) for content documents?
I think we're going to have to do some sort of emulation here.
(In reply to comment #29) > I think we're going to have to do some sort of emulation here. Can we get bug assignee?
(In reply to comment #30) > (In reply to comment #29) > > I think we're going to have to do some sort of emulation here. > > Can we get bug assignee? I would love to get an assignee from guys who knows this code. I think a11y guys is not good option because 1) nether of us is familiar with this code 2) we have very restricted human resources and so much to do for firefox4.
(In reply to comment #25) > I don't think it will help. You need to make GetClassName() on the window > returned from WindowFromAccessibleObject()/IAccessible2::windowHandle on the > document accessible return MozillaContentWindowClass. Unless I'm missing > something, even if you create fake ROLE_WINDOW accessibles, you'd then have to > create a fake HWND with a class name of MozillaContentWindowClass. Can't you do exactly that?
(In reply to comment #32) > (In reply to comment #25) > > I don't think it will help. You need to make GetClassName() on the window > > returned from WindowFromAccessibleObject()/IAccessible2::windowHandle on the > > document accessible return MozillaContentWindowClass. Unless I'm missing > > something, even if you create fake ROLE_WINDOW accessibles, you'd then have to > > create a fake HWND with a class name of MozillaContentWindowClass. > > Can't you do exactly that? Yes. This is what I mean by "emulation".
Flags: in-testsuite?
Marking as blocking final. We need to fix/workaround this somewhere before ship.
blocking2.0: ? → final+
(In reply to comment #31) > (In reply to comment #30) > > (In reply to comment #29) > > > I think we're going to have to do some sort of emulation here. > > > > Can we get bug assignee? > > I would love to get an assignee from guys who knows this code. I think a11y > guys is not good option because > 1) nether of us is familiar with this code The fix in comment #25 would require changes only in the accessibility code.
I must misunderstand solution from comment #25. How can we override Windows API GetClassName() to return expected class name if we don't get back windows?
Assignee: nobody → khuey
Bug 241993 is where this crap was originally implemented.
blocking2.0: final+ → beta6+
(In reply to comment #36) > I must misunderstand solution from comment #25. How can we override Windows API > GetClassName() to return expected class name if we don't get back windows? This is ugly as all hell, but: 1. Create an HWND with a class name of MozillaContentWindowClass. This HWND is not used for anything except to "trick" a11y clients. It must be a child of the current MozillaUIWindowClass. 2. Make it so that WindowFromAccessibleObject/IAccessible2::windowHandle on a content document accessible returns this "fake" HWND. For WindowFromAccessibleObject, you can make it return the system ROLE_WINDOW object for this HWND or you can make your own. It's probably just easier to return the system generated one. 3. Ensure that all events for the content document accessible specify this "fake" HWND and that Gecko processes events correctly for this HWND when WM_GETOBJECT is sent. Problems: 1. The "fake" HWND will never be "focused" according to Windows; i.e. hwndFocus from GetGUIThreadInfo() will never return this "fake" HWND. Some ATs may depend on this. However, I'm not sure if this was even the case in Firefox 3.6 due to the MozillaWindowClass windows. You should check whether other ATs depend on this. 2. NVDA does some weird stuff to get to the MozillaContentWindowClass window, but from memory, there are MozillaWindowClass windows beneath it. I think Gecko actually fires events on a MozillaWindowClass window. To complicate things further, Gecko often returns different HWNDs for events, WindowFromAccessibleObject() and IAccessible2::windowHandle(). I can't remember precise details on this. Anyway, the point is that it's possible that some ATs look for MozillaWindowClass or some other class. You would need to check with them. 3. The "fake" HWND will probably need to be invisible so it isn't seen by sighted users. The problem is that some ATs may ignore events from invisible windows. NVDA doesn't do this, though it does ignore events from windows that aren't a descendant of the current foreground window. 4. Possibly other complications i haven't even dreamed of yet... give it time... :) All of this is frought with danger and is likely to cause regressions in rather unexpected places. From my point of view, I'll probably just get rid of the window class check in NVDA for Gecko 2 and file bugs for any regressions this causes. It's less likely to cause odd problems than the nasty hack described above. :)
To be clear, the issue here is that screen readers are looking for windows that don't exist, right? If I take the new root window and ask for its IAccessible everything works as expected, correct?
(In reply to comment #39) > To be clear, the issue here is that screen readers are looking for windows that > don't exist, right? If I take the new root window and ask for its IAccessible > everything works as expected, correct? Right. Note the right long term fix here is to get screen readers (AT) to change expectations. We will have problems getting this done quickly with some vendors.
(In reply to comment #38) > 3. The "fake" HWND will probably need to be invisible so it isn't seen by > sighted users. The problem is that some ATs may ignore events from invisible > windows. NVDA doesn't do this, though it does ignore events from windows that > aren't a descendant of the current foreground window. It should just be 0x0 at position 0,0. Hopefully that'll work. (In reply to comment #40) > Right. Note the right long term fix here is to get screen readers (AT) to > change expectations. We will have problems getting this done quickly with some > vendors. The right long term fix should be (and always has been) to have accessibility APIs and the tools that use them not depend on the application's HWND hierarchy --- except for the application window's root HWND, which must always exist anyway.
(In reply to comment #39) > To be clear, the issue here is that screen readers are looking for windows that > don't exist, right? I think it's more correct to say that screen readers are using the wrong window class name to detect whether they should treat a given object as a Gecko content document. (In reply to comment #41) > The right long term fix should be (and always has been) to have accessibility > APIs and the tools that use them not depend on the application's HWND hierarchy This is true, but it requires that we don't need to use the HWND hierarchy as an extra detection/identification mechanism because the information exposed via accessibility APIs is insufficient or incorrect. As noted before, in the past, we had no choice but to use this due to strange, very-difficult-to-reproduce bugs in Gecko.
We should relnote for beta5 that screen readers are broken on Windows especially since 589529 will break them completely.
Keywords: relnote
Updating summary now that 589529 has landed.
Summary: All Windows screen readers' virtual buffers are broken effective August 28, 2010. → All Windows screen readers' are broken effective August 28, 2010.
Can anyone comment on my observations in comment #20? I suspect this isn't just related to a11y, as I'm pretty sure the name of the hwnd is changing.
(In reply to comment #20) > The main problem is that the window class of documents changed from > MozillaContentWindowClass to MozillaUIWindowClass. Not quite. We removed the window with class MozillaContentWindowClass altogether. There is only one window left and it has class MozillaUIWindowClass (bug 589529 changes its name to just MozillaWindowClass), so if you find any window that is the one you'll find. It is the window that contains the whole browser, the titlebar, the location bar, everything. Bug 130078 removed the window that contained the content document.
(In reply to comment #42) > (In reply to comment #41) > > The right long term fix should be (and always has been) to have accessibility > > APIs and the tools that use them not depend on the application's HWND hierarchy > This is true, but it requires that we don't need to use the HWND hierarchy as > an extra detection/identification mechanism because the information exposed via > accessibility APIs is insufficient or incorrect. As noted before, in the past, > we had no choice but to use this due to strange, very-difficult-to-reproduce > bugs in Gecko. Well, we DID do a lot of code restructuring/cleanup in the Firefox 4 timeframe. For example, we got rid of superflous/wrong document accessibles that were sometimes created when opening a new tab. Perhaps the bugs aren't even valid for Firefox 4 any more and using NVDA without the check will work just fine. Jamie, can you make me a build that doesn't check the window class so I can run it for a day or so?
(In reply to comment #48) > > As noted before, in the past, > > we had no choice but to use [the HWND hierarchy] due to strange, very-difficult-to-reproduce > > bugs in Gecko. > Well, we DID do a lot of code restructuring/cleanup in the Firefox 4 timeframe. > For example, we got rid of superflous/wrong document accessibles that were > sometimes created when opening a new tab. Perhaps the bugs aren't even valid > for Firefox 4 any more and using NVDA without the check will work just fine. That is very likely. Unfortunately, we still need to support Firefox 3.6, where this is probably still broken. > Jamie, can you make me a build that doesn't check the window class so I can run > it for a day or so? Sure. I can whip up something which will work for Firefox 4 for testing purposes, but it will most definitely break in Firefox 3.6. I'm out at the moment so probably won't be able to do this until tomorrow morning my time. Doing this fix in a way which will work for both FF 3.6 and 4 is kinda tricky, but should be possible... hopefully.
(In reply to comment #49) > I can whip up something which will work for Firefox 4 for testing > purposes, but it will most definitely break in Firefox 3.6. http://www.jantrid.net/misc/nvda_snapshot_main-3799+noMozillaClassChecks_portable.exe
Here are my results from testing with the NVDA try build from the previous comment: 1. With a nightly build between August 28 and August 30, virtual buffers are restored. 2. With the try-server build from bug 589529 Comment #11, or with a nightly build starting August 31, things are still pretty badly broken. I do get some accessibility events, but basically everything is being treated as if it was a virtual buffer. I also noticed something else: With the latter build, we no longer have an internal frame for the document. This is still the case with a nightly up to yesterday. So whatever that patch did, it had an affect on accessibility tree creation. Basically everything nsOuterDocAccessible is completely gone, and the hierarchy seems very flattened out in other ways, too.
(In reply to comment #51) > Here are my results from testing with the NVDA try build from the previous > comment: > 1. With a nightly build between August 28 and August 30, virtual buffers are > restored. This seems to confirm that all we need to do is fake the class names appropriately. > 2. With the try-server build from bug 589529 Comment #11, or with a nightly > build starting August 31, things are still pretty badly broken. I do get some > accessibility events, but basically everything is being treated as if it was a > virtual buffer. This seems to echo what comment 20 says about creating a document for the entire application window. > I also noticed something else: With the latter build, we no longer have an > internal frame for the document. This is still the case with a nightly up to > yesterday. So whatever that patch did, it had an affect on accessibility tree > creation. Basically everything nsOuterDocAccessible is completely gone, and the > hierarchy seems very flattened out in other ways, too. I don't know what an internal frame is, but all that patch did (or, at least, should have done) is rename anything that was a MozillaUIWindowClass to just MozillaWindowClass.
Summary: All Windows screen readers' are broken effective August 28, 2010. → Windows screen readers are broken due to post-130078 changes in the native widget structure
(In reply to comment #51) > 2. With the try-server build from bug 589529 Comment #11, or with a nightly > build starting August 31, things are still pretty badly broken. I do get some > accessibility events, but basically everything is being treated as if it was a > virtual buffer. Marco, I wasn't seeing this yesterday when I tested. However, I am seeing it intermittently now. I suspect that this would have sometimes happened with the 28 Aug build as well. I believe the problem relates to the fact that we used to test whether an object was inside a document by checking whether it is in the same hwnd or a descendant hwnd, as this is very fast. We can no longer do this. An additional check called IAccessible::accChild() on the document with the unique ID of the object we wanted to check, but this now returns an object even for objects outside the document, so we can't use this any more either. I don't know why this is occurring intermittently, however. It needs further investigation. We really need some fast, reliable way of determining whether an object is a descendant of another object; crawling up the ancestors out-of-process is just way too slow. > I also noticed something else: With the latter build, we no longer have an > internal frame for the document. This is still the case with a nightly up to > yesterday. So whatever that patch did, it had an affect on accessibility tree > creation. Basically everything nsOuterDocAccessible is completely gone, and the > hierarchy seems very flattened out in other ways, too. I'm going to assume you tested this with NVDA object navigation. Remember, the new simple review mode flattens out the hierarchy to make it easier for users. (This wasn't available in the current stable build, but is the default for the upcoming release and thus the test build I made.) I suspect that if you examine the tree with simple review disabled or using AccProbe or similar, it will be as before. :)
Another change to the hwnd structure on Windows happened a few months ago with bug 513162. Before bug 130078 though, bug 513162 probably would not have a big effect on screen readers.
(In reply to comment #54) > Another change to the hwnd structure on Windows happened a few months ago with > bug 513162. Before bug 130078 though, bug 513162 probably would not have a big > effect on screen readers. It had dramatic effect on screen readers as well but we were able to fix this in bug 575693.
Timothy, it looks like the only use of eContentTypeUI that is not top level is in XUL Menu popup frames. Is that correct?
(In reply to comment #57) > Timothy, it looks like the only use of eContentTypeUI that is not top level is > in XUL Menu popup frames. Is that correct? It looks like it.
Obviously, Mozilla must make a decision which suits all affected parties, but allow me to make our position as a screen reader vendor clear: Although they do break accessibility for older versions of NVDA, we are fine with the HWND hierarchy changes. Aside from the fact that this is a major release and progress needs to happen, they ensure that we always get the same hwnd in all cases, where previously it was rather inconsistent. Also, we are fairly certain we can detect them and handle both versions correctly. We think that the fake HWND hack is ugly and could cause more problems than it fixes. We do, however, need a quick way to detect whether an object is a descendant of another object without crawling up the accessibility hierarchy as described above. Note that the fake HWND hack may not actually work as expected. It's possible that other screen readers use the HWND to detect whether the object is within the document like NVDA currently does.
I agree Jamie. FYI, to add to our chats, emails and tweets, I've blogged: http://mindforks.blogspot.com/2010/09/firefox-4-beta-at-vendor-alert.html
Depends on: 592913
(In reply to comment #59) > We > do, however, need a quick way to detect whether an object is a descendant of > another object without crawling up the accessibility hierarchy as described > above. Spun off bug 592913 to address this.
Btw, are separate HWNDs ever created in specific circumstances now or is there always only one per browser window? For example, do popup menus/combo box lists still get a different HWND?
Yes, popups, menus, combobox dropdowns, and plugins still get widgets.
(In reply to comment #59) > Note that the fake HWND hack may not actually work as expected. It's possible > that other screen readers use the HWND to detect whether the object is within > the document like NVDA currently does. Right. It may not working as expected but it should be done. I believe we're not allowed to start from the beginning by making Firefox 4 working with new generation of screen readers only. We don't talk here about users only, we talk about plenty amount of companies that depends on certain software solutions because it prevents them to use Firefox 4 if the problem is not solved on our side. I still think windows structure should be emulated to keep existing screen readers working and the same time we must provide new flexible way so that future versions of screen readers can work successfully when Firefox UI window has only one HWND. It looks like we should keep HWND for Firefox window and emulate HWND for each document in tab. Though I still don't get an idea how it can be done technically.
(In reply to comment #64) > It looks like we should keep HWND for Firefox window and emulate HWND for each > document in tab. Though I still don't get an idea how it can be done > technically. Can each nsDocAccessible create an HWND for its own use? If you can find the parent nsDocAccessible, you can use its HWND to parent the child nsDocAccessible's HWND. The root nsDocAccessible could use the browser window HWND as the parent.
Robert, few implementation details please. How can I create HWND and attach it to windows structure? Also I think it's important to have these HWND focusable, so that AT are able to find focused window. Though I wonder if it regress bug 589529.
If you make the window zero-size then it won't show up and it won't be able to get mouse events, so I hope it won't affect those mouse drivers. I don't think it will be possible to make these HWNDs focusable. That would interfere catastrophically with our focus code. Creating an HWND is fairly easy. See for example nsToolkit::Startup and nsToolkit::CreateInternalWindow.
So I fiddled with this for a good portion of the weekend, but didn't make much progress. The "invisible sub widget" trick from comments 65 and 67 doesn't work with scrolling drivers because they want the MozillaWindowClass window to have focus and size. The "lets create a tree of widgets for eContentTypeUI widgets" breaks the browser pretty horrifically and I think would require a lot of effort to fix (we basically have to keep 3 widgets in sync at all times, and even that won't fix the glass I think). We need to establish whether or not the screenreaders need the widget in question to be focused to work. If they don't, we could create a completely invisible background tree of widgets that matches our old structure solely for a11y purposes. That would be pretty easy and wouldn't require anything else to adapt. If the screenreaders do require focus to determine which widget to ask for a11y information, we're in trouble :-( If screenreaders do require the widget to be focusable, I don't think this can make beta6.
Thanks Kyle. Alexander, can you try the approach in comment #65 and #67, have accessibility create a 0x0 window to return for each document, with the right class?
(In reply to comment #69) > Thanks Kyle. Alexander, can you try the approach in comment #65 and #67, have > accessibility create a 0x0 window to return for each document, with the right > class? I could try I think. Do I get right I should copy/paste code from nsToolkit::Startup/CreateInternalWindow to accessibility code, change "nsToolkitClass" to window name used previously and add window proc to process WM_GETOBJECT event? Where can I get hModule variable to initialize hInstance passed to ::Register/CreateWindow?
FWIW, I don't see how we can ship this after beta6. We need to get to a plan here, pretty quickly.
(In reply to comment #70) > I could try I think. Do I get right I should copy/paste code from > nsToolkit::Startup/CreateInternalWindow to accessibility code, change > "nsToolkitClass" to window name used previously and add window proc to process > WM_GETOBJECT event? Correct > Where can I get hModule variable to initialize hInstance passed to > ::Register/CreateWindow? GetModuleHandle(NULL) should work.
(Yes. Thanks for your weekend efforts Kyle! And thanks for your assessment here Mike.) Here's the current plan: Alexander is trying Roc's suggestion. If that works well enough, we test and land (and immediately deprecate). If it is fragile we should drop it IMO. This is not something we would choose but unfortunately it is an option I think we need to consider. Alexander, Marco and I are working with vendors on "the right fix". We have a decent plan. Depending on the outcome here, we will continue to work with them to make sure there is a plan for the users (re: version compatibilities and upgrade paths).
Priority: -- → P1
Of course I cut out some text that made my point confusing. I mean if the 'resurrection of windows' approach is fragile we should just drop it.
Jamie, how does NVDA use MozillaUIWindowClass? This class was used for Firefox UI window. When it was replaced on MozillaWindowClass, NVDA keep silent when I tab.
(In reply to comment #73) > Alexander is trying Roc's suggestion. > If that works well enough, we test and land (and immediately deprecate). > Alexander, Marco and I are working with vendors on "the right fix". So basically, you want to implement a hacky work around, deprecate it and then go through all of this again in a future release? :) I guess that's fine if the work around requires no change in current ATs. (In reply to comment #75) > Jamie, how does NVDA use MozillaUIWindowClass? This class was used for Firefox > UI window. When it was replaced on MozillaWindowClass, NVDA keep silent when I > tab. See bug 589529 comment #41.
(In reply to comment #73) > Alexander, Marco and I are working with vendors on "the right fix". We have a > decent plan. It'd be good if this plan was kept in the open. Also note that an approach that just works with future versions without specific changes to ATs (relying on version detection) is very difficult. As I noted before, our window class checks were partially instituted because of bugs in past versions of Gecko.
(In reply to comment #77) > (In reply to comment #73) > > Alexander, Marco and I are working with vendors on "the right fix". We have a > > decent plan. > It'd be good if this plan was kept in the open. I think i'll be kept open once it's defined. That's a consequence of the fact Firefox is open source project and as well it should come with Mozilla policy. > Also note that an approach that > just works with future versions without specific changes to ATs (relying on > version detection) is very difficult. That's not just difficult, it's not possible. The "right fix" requires changes on AT client side, like your accChild approach.
(In reply to comment #78) > > Also note that an approach that > > just works with future versions without specific changes to ATs (relying on > > version detection) is very difficult. > That's not just difficult, it's not possible. The "right fix" requires changes > on AT client side Err... sorry, that's not what I meant. I meant that if you go with and then deprecate the HWND faking, you'll still have to get ATs to implement the "right fix" before the next version of Firefox comes out, since they're unlikely to release at the same time as FF. Meanwhile, they'll need to make sure that they're still compatible with older versions because the new version of FF might not be out yet when the new version of the AT is released. > like your accChild approach. The accChild approach actually isn't anything new. We always used accChild in that way to double check that the node was in the document. However, now that the HWNDs have changed, our comparison of HWNDs is no longer useful to determine whether the node is within the tab document, so we need accChild to be more restrictive.
Assignee: khuey → surkov.alexander
Roc, good news (and thanks for your suggestion). From Alexander: "I think we should detect presence of certain screen reader of certain versions and emulate window hierarchy it expects. New versions of screen readers and AT we don't work with will deal with with existing windows hierarchy. It allows us to remove this deprecated approach in future more easy and makes us sure new AT won't depend on this deprecated way. Since we work with most of screen reader vendors then it shouldn't affect on AT users. Does this sound reasonable? Btw, I've got working NVDA. So this approach works. Based on information from other AT vendors I think it this approach works for them as well. I need to get your opinions for the question above and I can polish the patch." Me: Awesome. I agree with the AT detection plan as per our IRC discussion. Let's test your build ASAP and confirm with Marco that this works for other AT.
Alexander, we might also want to add an about:config preference to force the emulation on. Something like: accessibility.deprecated_windows_emulation_hack
The problem might be that different screen readers relies on different parts of old windows hierarchy. I don't like to emulate everything we had and strictly speaking that's not possible.
Jamie, so we don't have a way to detect a presence of NVDA, no for any existing version? Do you feel it's acceptable to ship Firefox 4 completely inaccessible for old NVDA versions? (Just thinking NVDA users might face to a problem to download new NVDA version by inaccessible Firefox 4).
(In reply to comment #82) > The problem might be that different screen readers relies on different parts of > old windows hierarchy. I don't like to emulate everything we had and strictly > speaking that's not possible. Let's wait and see how the other major AT respond to your patch so that we can extrapolate. (re: comment #83) Detecting NVDA is ::GetModuleHandleW(L"VBufBackend_gecko_ia2") If NVDA could add file version info we could make the transition pretty seamless.
(In reply to comment #84) > (re: comment #83) > Detecting NVDA is > ::GetModuleHandleW(L"VBufBackend_gecko_ia2") > > If NVDA could add file version info we could make the transition pretty > seamless. great, thanks
Attached patch wip — — Splinter Review
wip that makes NVDA works for me, it's not for any reviews, just a kind of milestone
(In reply to comment #83) > Jamie, so we don't have a way to detect a presence of NVDA, no for any existing > version? You can detect it, but we don't have any file version info at this stage. > Do you feel it's acceptable to ship Firefox 4 completely inaccessible > for old NVDA versions? (Just thinking NVDA users might face to a problem to > download new NVDA version by inaccessible Firefox 4). This is a fair point. I guess it depends when Firefox 4 final will most likely ship. NVDA 2010.2 (which will contain our fix) should ship in October. (In reply to comment #84) > If NVDA could add file version info we could make the transition pretty > seamless. The problem with this is that I'm not sure how to generate file version info for local and snapshot builds. Quite a lot of our users use these. It really needs to happen without intervention from the builder. The version numbering stuff used by Windows is pretty restrictive. In any case, I'll discuss this again with Mick and perhaps seek input from others off-bug.
Our fix is in NVDA main rev 3826: http://www.nvda-project.org/changeset/main,3826 This code works for both Firefox 3.6 and 4pre.
(In reply to comment #87) > This is a fair point. I guess it depends when Firefox 4 final will most likely > ship. NVDA 2010.2 (which will contain our fix) should ship in October. We will ship after you.
(In reply to comment #87) > > Do you feel it's acceptable to ship Firefox 4 completely inaccessible > > for old NVDA versions? (Just thinking NVDA users might face to a problem to > > download new NVDA version by inaccessible Firefox 4). > This is a fair point. I guess it depends when Firefox 4 final will most likely > ship. NVDA 2010.2 (which will contain our fix) should ship in October. Do NVDA users get automatic update or most of users use latest NVDA version? > (In reply to comment #84) > > If NVDA could add file version info we could make the transition pretty > > seamless. > The problem with this is that I'm not sure how to generate file version info > for local and snapshot builds. Quite a lot of our users use these. It really > needs to happen without intervention from the builder. The version numbering > stuff used by Windows is pretty restrictive. In any case, I'll discuss this > again with Mick and perhaps seek input from others off-bug. If you can add version info to new release then I think we can emulate windows hierarchy for NVDA without version info.
(In reply to comment #90) > Do NVDA users get automatic update No. > or most of users use latest NVDA version? We generally recommend that users update when new stable versions are released. As far as we are aware, most users seem to do this in relatively short order. > If you can add version info to new release then I think we can emulate windows > hierarchy for NVDA without version info. I doubt this will make it into the new release. I think I'd prefer we just don't detect NVDA and make users upgrade. We'll accept responsibility for any fallout from our users. :) The important thing is that FF4 will ship after the required version of NVDA (2010.2).
Ok, fair enough. Let's do not do special handling for old NVDA releases.
Blocks: 594977
Blocks: 593372
Changes have now been made to NVDA main branch, to work with the window hierarchy changes in Gecko 2. NVDA snapshots from revision 3828 onwards contain these changes. NVDA snapshots page: http://www.nvda-project.org/snapshots/ As these changes are in NVDA main, they will be in NVDA's next release (2010.2) when it comes out. Please now use snapshots from revision 3828 to test with Gecko 2, rather than the previous special build of NVDA mentioned in previous comments. These changes now allow the use of Firefox 4 with NVDA. Focus and event tracking work, virtualBuffers are appropriately created, and due to the fixing of bug 592913, NVDA appropriately understands what objects are in or not in a virtualBuffer (thus no longer thinking that all objects in the chrome are part of the document's virtualBuffer). These changes should not affect Gecko 1.9 (i.e. Firefox 3.6). Note though that if a fake window hierarchy is implemented in Gecko 2 for the sake of compatibility with other ATs, NVDA will need to still be carefully tested as its likely that it may revert its logic when finding something that looks like the older window hierarchy. NVDA main 3828 was tested with Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b6pre) Gecko/20100910 Firefox/4.0b6pre
Attached patch patch (obsolete) — — Splinter Review
for JAWS and WE, not tested
Attached patch patch2 — — Splinter Review
JAWS works for me
Attachment #475665 - Attachment is obsolete: true
Attachment #475893 - Flags: review?(bolterbugz)
Status: NEW → ASSIGNED
Comment on attachment 475893 [details] [diff] [review] patch2 I don't think we should add kNVDAModuleHandle until we actually use it for something. >+void >+nsDocAccessibleWrap::Shutdown() >+{ Maybe check mHWND != nsnull as a natural guard here? >+ if (nsWinUtils::IsWindowEmulationEnabled()) { >+ // Destroy window created for root document. >+ if (nsWinUtils::IsTabDocument(mDocument)) { >+ nsAccessibleWrap::sHWNDCache.Remove(mHWND); >+ ::DestroyWindow(static_cast<HWND>(mHWND)); >+ } >+ >+ mHWND = nsnull; >+ } >+ >+ nsDocAccessible::Shutdown(); >+}
Comment on attachment 475893 [details] [diff] [review] patch2 >+bool >+nsWinUtils::IsWindowEmulationEnabled() >+{ >+ return ::GetModuleHandleW(kJAWSModuleHandle) || >+ ::GetModuleHandleW(kWEModuleHandle); >+} I guess this is enough for now. We'll want to check versions in a follow up -- maybe add a XXX comment for this. I wish we had a less fragile way to implement IsTabDocument. Maybe we can add something to toolkit in a follow up.
(In reply to comment #96) > Comment on attachment 475893 [details] [diff] [review] > patch2 > > I don't think we should add kNVDAModuleHandle until we actually use it for > something. I don't mind. > > >+void > >+nsDocAccessibleWrap::Shutdown() > >+{ > > Maybe check mHWND != nsnull as a natural guard here? it shouldn't be called twice, but any way deinitialization stuffs aren't dangerous I think if they repeated. mHWND check doesn't work in the case if emulation isn't on.
Comment on attachment 475893 [details] [diff] [review] patch2 r=me since this is a drastic improvement on what we have; the virtual buffers work again. Let's get this into nightlies and get exposure ASAP.
Attachment #475893 - Flags: review?(bolterbugz) → review+
(In reply to comment #97) > >+nsWinUtils::IsWindowEmulationEnabled() > >+{ > >+ return ::GetModuleHandleW(kJAWSModuleHandle) || > >+ ::GetModuleHandleW(kWEModuleHandle); > >+} > > I guess this is enough for now. We'll want to check versions in a follow up -- > maybe add a XXX comment for this. ok > > I wish we had a less fragile way to implement IsTabDocument. Maybe we can add > something to toolkit in a follow up. it sounds like pure a11y feature
Comment on attachment 475893 [details] [diff] [review] patch2 Agreed, this should go in and get exposure.
Attachment #475893 - Flags: feedback+
(In reply to comment #98) > (In reply to comment #96) > > Comment on attachment 475893 [details] [diff] [review] [details] > > patch2 > > > > I don't think we should add kNVDAModuleHandle until we actually use it for > > something. > > I don't mind. I've forgot to remove this once. I hope that's ok to deal with it later? landed on 2.0 - http://hg.mozilla.org/mozilla-central/rev/a3607267900e
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
(In reply to comment #100) > (In reply to comment #97) > > > >+nsWinUtils::IsWindowEmulationEnabled() > > >+{ > > >+ return ::GetModuleHandleW(kJAWSModuleHandle) || > > >+ ::GetModuleHandleW(kWEModuleHandle); > > >+} David, could you please drive this with AT to polish this? It would be nice to file bug on this.
Depends on: 598578
Depends on: 646442
Flags: in-testsuite?
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: