629358 - Baum Retec AG Cobra screen reader popup compatibility issue

Status: VERIFIED FIXED

(Core :: Disability Access APIs, defect)

Description

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

Essentially, iterating the accessibility children of popup windows gives all the accessible children on FF, and correspond to our application's HWND. The popup HWND appears to Cobra to obscure the menu items, and so the visibility calculation is busted.

Comment 1

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

Marco is confirming the cause of the regression with Baum.

Comment 2

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

We'll need to make sure a fix here doesn't regress WE or Jaws (who still use our emulation).

Comment 3

[alexander :surkov (:asurkov)]

Attachment: patch

Comment 4

[alexander :surkov (:asurkov)]

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

Comment 5

[alexander :surkov (:asurkov)]

Attachment: patch

Comment 6

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

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

> HWND
> nsAccessibleWrap::GetHWNDFor(nsAccessible *aAccessible)
> {

I tried to cross reference this with what was removed in http://hg.mozilla.org/mozilla-central/diff/a3607267900e/accessible/src/msaa/nsAccessibleWrap.cpp

I think we need some updated comments.

>-  if (!aAccessible)
>-    return 0;
>+  if (aAccessible) {
>+    nsIFrame* frame = aAccessible->GetFrame();
>+    if (frame) {
>+      nsIWidget* widget = frame->GetNearestWidget();
>+      nsCOMPtr<nsIPresShell> shell(aAccessible->GetPresShell());
>+      nsIViewManager* vm = shell->GetViewManager();
>+      if (vm) {
>+        nsCOMPtr<nsIWidget> rootWidget;
>+        vm->GetRootWidget(getter_AddRefs(rootWidget));
>+        if (rootWidget != widget)
>+          return static_cast<HWND>(widget->GetNativeData(NS_NATIVE_WINDOW));
>+      }
>+    }

Timothy is there a way to do this without views? (When are views going away?)

If it is more risk/work I don't want to suggest worrying about it for FF4.

Comment 7

[alexander :surkov (:asurkov)]

(In reply to comment #6)

> Timothy is there a way to do this without views? (When are views going away?)

we use views in a11y code so it shouldn't bother us actually.

> If it is more risk/work I don't want to suggest worrying about it for FF4.

I can activate this code when Cobra is detected since it sounds nobody else needs it.

Comment 8

[Marco Zehe (:MarcoZ)]

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

This breaks both JAWS 11 and Window-Eyes 7.2 virtual buffers on multiple tabs. Initially switching to a tab yields a virtual buffer, but activating a link, or switching to the next tab or back destroys the virtual buffers, and from that point on, these screen readers think there is no need to invoke virtual buffers alltogether.

With a regular nightly build, this problem doesn't exist. f- from me on this. So if this is needed by Cobra, either only activate this patch when Cobra is detected, or don't put this into FX4 at all.

Comment 9

[alexander :surkov (:asurkov)]

Marco, did you tried the patch or try server build?

Comment 10

[alexander :surkov (:asurkov)]

try server build for the second patch - http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/[e-mail]-459683a4e9ee

Comment 11

[Marco Zehe (:MarcoZ)]

I tried the try-server build.

Comment 12

[Marco Zehe (:MarcoZ)]

(In reply to comment #10)
> try server build for the second patch -
> http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/[e-mail]-459683a4e9ee

gives me an http error 404 "not found".

Comment 13

[alexander :surkov (:asurkov)]

(In reply to comment #11)
> I tried the try-server build.

yeah, your observation is expected, bad patch

(In reply to comment #12)
> (In reply to comment #10)
> > try server build for the second patch -
> > http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/[e-mail]-459683a4e9ee
> 
> gives me an http error 404 "not found".

just pushed it, will be after a while

Comment 14

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

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

r=me with questions answered and with Marco's f/r+; but do get a review for the cobra sniff addition if we need it. Also please be sure this is a risk Marco agrees we can take of course. Moving Timothy's review to optional f?

> HWND
> nsAccessibleWrap::GetHWNDFor(nsAccessible *aAccessible)
> {
>-  if (!aAccessible)
>-    return 0;
>+  if (aAccessible) {
>+    nsIFrame* frame = aAccessible->GetFrame();
>+    if (frame) {
>+      nsIWidget* widget = frame->GetNearestWidget();
>+      nsCOMPtr<nsIPresShell> shell(aAccessible->GetPresShell());
>+      nsIViewManager* vm = shell->GetViewManager();
>+      if (vm) {
>+        nsCOMPtr<nsIWidget> rootWidget;
>+        vm->GetRootWidget(getter_AddRefs(rootWidget));
>+        if (rootWidget != widget)
>+          return static_cast<HWND>(widget->GetNativeData(NS_NATIVE_WINDOW));
>+      }
>+    }

What about the case where we want to return the HWND for the root application accessible? Does that get handled below? I still want comments :)

> 
>-  nsDocAccessible* document = aAccessible->GetDocAccessible();
>-  return document ? static_cast<HWND>(document->GetNativeWindow()) : 0;
>+    nsDocAccessible* document = aAccessible->GetDocAccessible();
>+    if (document)
>+      return static_cast<HWND>(document->GetNativeWindow());
>+  }
>+  return nsnull;

Comment 15

[Timothy Nikkel (:tnikkel)]

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

This is fine. There isn't currently an alternative for a lot of the uses of views, so we have to keep using them.

Comment 16

[Marco Zehe (:MarcoZ)]

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

OK, this one works with JAWS and Window-Eyes in the latest try-server build. I tested single-frame pages, pages with an iFrame and also pages with multiple frames. Can change this to an f+.

Comment 17

[James Teh [:Jamie]]

Should this have any affect on ATs like NVDA that don't use the HWND emulation?

Comment 18

[alexander :surkov (:asurkov)]

(In reply to comment #17)
> Should this have any affect on ATs like NVDA that don't use the HWND emulation?

theoretically, not for NVDA though, that's the change I told to you about: menuitems report popup window and events for menuitems are fired for popup window.

Comment 19

[alexander :surkov (:asurkov)]

(In reply to comment #14)

> What about the case where we want to return the HWND for the root application
> accessible? Does that get handled below? I still want comments :)

I don't think we want HWND for root application accessible.

Comment 20

[alexander :surkov (:asurkov)]

Attachment: patch2 [for landing]

added comments, I checked it with Cobra, it works.

Comment 21

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

Comment on attachment 514155 [details] [diff] [review]
patch2 [for landing]

a=me, I think you've done some good investigations and we can always revert this as a last resort.

Comment 22

[alexander :surkov (:asurkov)]

landed on 2.0 (fx4b12) - http://hg.mozilla.org/mozilla-central/rev/6e85cc1676c5

Comment 23

[Marco Zehe (:MarcoZ)]

Fix confirmed working in B12build1 candidate Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b12) Gecko/20100101 Firefox/4.0b12 by the vendor. Setting Verified.