Accessibility in main window broken for VoiceOver, VO doesn't see anything but the title bar and its buttons

RESOLVED FIXED in mozilla11

Status

()

Core
Disability Access APIs
--
major
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: MarcoZ, Assigned: hub)

Tracking

(Blocks: 1 bug, {regression})

Trunk
mozilla11
x86_64
Mac OS X
regression
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [bk1])

Attachments

(2 attachments, 3 obsolete attachments)

(Reporter)

Description

6 years ago
STR:
1. Build current trunk with accessibility enabled.
2. Run the build.
3. Run VoiceOver.
4. If you get a "Nightly is not your current browser" dialog, notice that VoiceOver does see all the text in the dialog, plus the checkbox and the two buttons. You can press the No button with CTRL+Option+Space, for example.
5. Once you get to the main window, and Nightly displays its start page, VoiceOver does not see anything but the title bar and the close, minimize, and zoom buttons. Normally, it should also see toolbars, the main web area, and the status bar. But none of that is visible. You can find this out by navigating through the top level hierarchy with CTRL+Option+Arrow right and left.

This is a regression from some time in the Firefox 4 timeframe I believe, where I saw this working better. But finding a regression range is not really an option since no regular nightly build was ever built with accessibility enabled.

Comment 1

6 years ago
Steven, if you get a chance please look into. You might be a best person to detect where we regressed. Thank you for the help.
I'm currently swamped, so it'll be a while before I can get to this.

But until we get someone specifically working on Mac accessibility, it's probably true that I'm the best one to figure this out.

Updated

6 years ago
Assignee: nobody → hfiguiere
(Assignee)

Updated

6 years ago
Whiteboard: [bk1]
(Assignee)

Comment 3

6 years ago
To be honest, I just checked with a custom build of 3.6.24 and it is work than what is in Mozilla central. With Nightly build from mozilla central I can at least partially navigate through the webpage under some circumstances.

As for the dialog it isn't read either way, but we can use the VoiceOver navigation keys to dismiss it (buttons are read).

Definitely broken, not sure if it actually regressed.
Do a mozilla-central build from code just after my patch for bug 560939 landed.
(Assignee)

Comment 5

6 years ago
Using the Accessibility Validator, we actually have a tree that "randomly" miss complete subtrees, consistently with the accessibility issue we observe.
Needs more digging as to why this happens.

There are also some glitches in the implementation that cause some errors with the Validator, the AXMain and AXMinimized property missing from the dialog, and the some extra unexpected properties. I don't think these are fatal but I plan on fixing them too.

Comment 6

6 years ago
(In reply to Hub Figuiere [:hub] from comment #5)

> There are also some glitches in the implementation that cause some errors
> with the Validator, the AXMain and AXMinimized property missing from the
> dialog, and the some extra unexpected properties. I don't think these are
> fatal but I plan on fixing them too.

please file separate bugs for that.
(Assignee)

Comment 7

6 years ago
Created attachment 576392 [details] [diff] [review]
proposed patch

The proposed patch. It needs more testing, but I do believe this is the actual problem. It seems to solve all the issues I had with the hierarchy.

I need to test on a non debug build.

Comment 8

6 years ago
Comment on attachment 576392 [details] [diff] [review]
proposed patch

>diff --git a/accessible/src/mac/nsAccessibleWrap.mm b/accessible/src/mac/nsAccessibleWrap.mm
>--- a/accessible/src/mac/nsAccessibleWrap.mm
>+++ b/accessible/src/mac/nsAccessibleWrap.mm
>@@ -256,16 +256,19 @@ nsAccessibleWrap::IsIgnored()
> 
> void
> nsAccessibleWrap::GetUnignoredChildren(nsTArray<nsRefPtr<nsAccessibleWrap> > &aChildrenArray)
> {
>   // we're flat; there are no children.
>   if (nsAccUtils::MustPrune(this))
>     return;
> 
>+  // make sure the children are here.
>+  EnsureChildren();

You never should call it explicitly. Children are cached automatically when accessibility starts but that happens async. That could be a problem. Maybe we should deliver then events that tree was changed instead? Can you describe in details what happens here?

Comment 9

6 years ago
Btw, it'll be great to get regression bug. I think you could bisect extension (http://mercurial.selenic.com/wiki/BisectExtension) for that.
(Assignee)

Comment 10

6 years ago
What happens is that in some situation we don't have the children created. As you said it is done asynchronously and that's exactly what happens. Adding the "EnsureChildren()" make sure they are here when we query the accessible tree.

I guess we will have to deliver the events that tree was changed instead.
(Assignee)

Comment 11

6 years ago
To be honest, I'm not sure how it was a regression per see. Comment 3.

Comment 12

6 years ago
(In reply to Hub Figuiere [:hub] from comment #10)

> I guess we will have to deliver the events that tree was changed instead.

If this is a cause then nsaccessibility prootocol provides events AXUIElementCreated and AXUIElementDestroyed. But that shouldn't be issue since we never delivered them. Did you checked if mac accessible children cache doesn't get updated by any chance?

Comment 13

6 years ago
(In reply to alexander surkov from comment #12)
> Did you checked if mac accessible children
> cache doesn't get updated by any chance?

For example, nsAccessibleWrap::InvalidateChildren doesn't trigger on nsOuterDocAccessible, what could lead for example tab document accessible is not presented in mac cached children.

Comment 14

6 years ago
(In reply to alexander surkov from comment #13)
> (In reply to alexander surkov from comment #12)
> > Did you checked if mac accessible children
> > cache doesn't get updated by any chance?
> 
> For example, nsAccessibleWrap::InvalidateChildren doesn't trigger on
> nsOuterDocAccessible, what could lead for example tab document accessible is
> not presented in mac cached children.

yes, the problem mac cached children (mChildren) on root accessible weren't invalidated.

Comment 15

6 years ago
(In reply to alexander surkov from comment #14)

> yes, the problem mac cached children (mChildren) on root accessible weren't
> invalidated.

and those debug observations are right because initial tree creation doesn't trigger invalidateChildren. That'll be a fix.

Comment 16

6 years ago
btw, invalidateChildren approach doesn't work in either case because children invalidation on ignored accessible that has unignored children doesn't update mac children cache of the parent.

Updated

6 years ago
Depends on: 705404

Comment 17

6 years ago
regression from bug 646368 which is sort of expected (see bug 705404 comment #5)
Blocks: 646368
(Assignee)

Comment 18

6 years ago
Created attachment 578444 [details] [diff] [review]
proposed patch

This the new patch. Almost the same as the old one except that this time EnsureChildren() is called in -[mozAccessible children]

nsAccessibleWrap::GetUnignoredChildren() is only called from that location anyway.

-[mozAccessible children] is only called from the MacOS Universal Access API and caches the result until invalidated by a tree update.
Attachment #576392 - Attachment is obsolete: true
(Assignee)

Updated

6 years ago
Attachment #578444 - Flags: review?(surkov.alexander)

Comment 19

6 years ago
Comment on attachment 578444 [details] [diff] [review]
proposed patch

Layout is responsible for time of accessible tree creation. The patch introduces extra code path for that and put the creation time out of our control. It's harder to debug any broken tree issues and it can lead to broken accessible tree because we operate on unfinished layout. So I wouldn't try this approach as long as we have alternative.
Attachment #578444 - Flags: review?(surkov.alexander) → review-
(Assignee)

Comment 20

6 years ago
Created attachment 578684 [details] [diff] [review]
proposed patch

I had to expose "IsInitialized" so that I don't cache an empty children list.
Attachment #578444 - Attachment is obsolete: true
(Assignee)

Updated

6 years ago
Attachment #578684 - Flags: review?(surkov.alexander)
(Assignee)

Comment 21

6 years ago
Comment on attachment 578684 [details] [diff] [review]
proposed patch

cancelling review. There is a typo....
Attachment #578684 - Flags: review?(surkov.alexander)
(Assignee)

Comment 22

6 years ago
Created attachment 578750 [details] [diff] [review]
proposed patch v4.1
Attachment #578684 - Attachment is obsolete: true
Attachment #578750 - Flags: review?
(Assignee)

Updated

6 years ago
Attachment #578750 - Flags: review? → review?(surkov.alexander)
(Assignee)

Updated

6 years ago
Attachment #578750 - Flags: review?(bolterbugz)
(Assignee)

Comment 23

6 years ago
This fix the VoiceOver at startup. But reloading the page, or going to a new one make it empty again. This is also the case before the regression.

Comment 24

6 years ago
Comment on attachment 578750 [details] [diff] [review]
proposed patch v4.1


> - (NSArray*)children
> {
>   NS_OBJC_BEGIN_TRY_ABORT_BLOCK_NIL;
> 
>   if (mChildren)
>     return mChildren;
>-    
>+  
>+  if(!mGeckoAccessible->IsInitialized())
>+    return nil;

use AreChildrenCached

if (mChildren || !mGeckoAccessible-AreChildrenCached())
  return mChildren;
Attachment #578750 - Flags: review?(surkov.alexander) → review+

Updated

6 years ago
Attachment #578750 - Flags: review?(bolterbugz) → review+
(Assignee)

Comment 25

6 years ago
Created attachment 579167 [details] [diff] [review]
Final patch from patch queue

Can't commit with Level 1.

Has been run on a try server.
Attachment #579167 - Flags: checkin?

Comment 26

6 years ago
inbound land https://hg.mozilla.org/integration/mozilla-inbound/rev/95e631d313a3

Updated

6 years ago
Attachment #579167 - Flags: checkin?
https://hg.mozilla.org/mozilla-central/rev/95e631d313a3
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla11
(Assignee)

Updated

6 years ago
No longer depends on: 705404
You need to log in before you can comment on or make changes to this bug.