Closed Bug 629358 Opened 9 years ago Closed 9 years ago

Baum Retec AG Cobra screen reader popup compatibility issue

Categories

(Core :: Disability Access APIs, defect)

x86
Windows 7
defect
Not set

Tracking

()

VERIFIED FIXED
mozilla2.0b12
Tracking Status
blocking2.0 --- final+

People

(Reporter: davidb, Assigned: surkov)

References

Details

(Keywords: regression, Whiteboard: [softblocker])

Attachments

(2 files, 1 obsolete file)

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.
Marco is confirming the cause of the regression with Baum.
OS: Mac OS X → Windows 7
blocking2.0: --- → ?
Whiteboard: regression
blocking2.0: ? → final+
Keywords: regression
Whiteboard: regression → [softblocker]
Assignee: nobody → bolterbugz
We'll need to make sure a fix here doesn't regress WE or Jaws (who still use our emulation).
Attached patch patch (obsolete) — Splinter Review
Assignee: bolterbugz → surkov.alexander
Status: NEW → ASSIGNED
Attached patch patchSplinter Review
Attachment #513052 - Attachment is obsolete: true
Attachment #513478 - Flags: review?(bolterbugz)
Attachment #513478 - Flags: feedback?(marco.zehe)
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.
Attachment #513478 - Flags: review?(tnikkel)
(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 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.
Attachment #513478 - Flags: feedback?(marco.zehe) → feedback-
Marco, did you tried the patch or try server build?
I tried the try-server build.
(In reply to comment #10)
> try server build for the second patch -
> http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/surkov.alexander@gmail.com-459683a4e9ee

gives me an http error 404 "not found".
(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/surkov.alexander@gmail.com-459683a4e9ee
> 
> gives me an http error 404 "not found".

just pushed it, will be after a while
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;
Attachment #513478 - Flags: review?(tnikkel)
Attachment #513478 - Flags: review?(bolterbugz)
Attachment #513478 - Flags: review+
Attachment #513478 - Flags: feedback?(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.
Attachment #513478 - Flags: feedback?(tnikkel) → feedback+
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+.
Attachment #513478 - Flags: feedback- → feedback+
Should this have any affect on ATs like NVDA that don't use the HWND emulation?
(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.
(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.
added comments, I checked it with Cobra, it works.
Attachment #514155 - Flags: approval2.0?
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.
Attachment #514155 - Flags: approval2.0? → approval2.0+
landed on 2.0 (fx4b12) - http://hg.mozilla.org/mozilla-central/rev/6e85cc1676c5
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.0b12
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.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.