Description: This component relates to bugs in our support for accessibility APIs on the various platforms. Accessibility APIs allow 3rd party products, such as screen readers used by visually impaired users, to communicate with our content and UI. The APIs we support specifically are MSAA on Windows and
ATK on UNIX/Linux (Apple has not yet published specs for an accessibility API on OS X). This component is not for keyboard, focus or any accessibility bugs other than those relating to the APIs we export.

User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.3a1pre) Gecko/20090821 Minefield/3.7a1pre
Build Identifier:
When designMode is used within an iframe to embed an editable document, the first time the designMode document gets focus, a focus event is correctly fired via accessibility APIs. However, when the designMode document subsequently receives focus, a focus event is never fired to accessibility APIs.
Reproducible: Always
Steps to Reproduce:
1. Open the attached test case.
2. Tab to the editable document. Observe that a focus event is fired via accessibility APIs.
3. Press shift+tab to move to the first button.
4. Press tab to move back to the editable document. Observe that although the focus is visually indicated and typing causes text to appear in the editable document, no focus event is fired to accessibility APIs.
5. Press tab to move to the second button.
6. Repeat step 4.
7. Press alt twice to enter and then exit the menu bar.
8. Repeat step 4.
9. Press alt+tab to switch applications, then press alt+tab to switch back to the browser.
10. Repeat step 4.
Actual Results:
In steps 4, 6, 8 and 10, a focus event was not fired via accessibility APIs for the editable document accessible.
Expected Results:
In steps 4, 6, 8 and 10, a focus event should be fired via accessibility APIs for the editable document accessible, just as it was in step 2.
In steps 4, 6, 8 and 10, the focused state is present on the editable document accessible as it should be. Unfortunately, the focus event is missing.
This bug is really nasty for any rich text editing on the web. Basically, you only get one shot at entering text into the document. Once the focus moves, you can't accessibly edit it anymore.

Created attachment 401498[details][diff][review]
patch
This seems to fix the bug (confirmed via accprobe), but it is pretty blunt
surgery. Some things I'd like confirmation on:
1. Does this bug also affect linux?
2. Does this patch fix this bug?
3. Does my patch break anything (would have broken LSR, does it break Orca)?
Note we have some fairly delicate focus code; makes me nervous. Also Neil has a
note in our code that we shouldn't cache the last focused node... we need to
sort this out.

Comment on attachment 401498[details][diff][review]
patch
This fixes the bug as reported. I think we do not need this code anymore because we have added event coalescing since this was removed code originally written.

I skimmed through our coalesce code and see we don't coalesce focus events from the same subtree (which makes some sense) so I went ahead and confirmed on Ubuntu that this patch doesn't seem to resurrect bug 375747, so I think we are safe.

Created attachment 403537[details][diff][review]
patch 2 (tests bug)
Added a test for this bug (the editor). Not sure how to cover the code removal via automated test. I'm not sure of the steps to reproduce (I can't recreate the issue the code was originally there for).

Comment on attachment 403537[details][diff][review]
patch 2 (tests bug)
r=me with a few nits:
+ <title>Accessible focus event testing</title>
Can this be more specific indicating that we're testing setting focus to a design doc here?
>+ var gA11yEventDumpID = "eventdump";
Should be commented out for "production" I think.

(In reply to comment #12)
> Created an attachment (id=403537) [details]
> patch 2 (tests bug)
>
> Added a test for this bug (the editor). Not sure how to cover the code removal
> via automated test. I'm not sure of the steps to reproduce (I can't recreate
> the issue the code was originally there for).
We have a couple of bugs which might be affected by removed code. These are bug 375747, bug 375746 and bug 375748. It would be really nice to have mochitests for them. I get your point but we should try to come with verisimilar tests even if we can't be sure we are absolutely right when we reproduce them.

Created attachment 404084[details][diff][review]
patch 3 (adds back special cases)
Alexander, after looking at those older bugs I see this area of the code is potentially fragile. Given that we have bug 376803 filed for further related investigation, do you think we can go with this additional check for now so that we no longer regress?

(In reply to comment #17)
> Created an attachment (id=404084) [details]
> patch 3 (adds back special cases)
>
> Alexander, after looking at those older bugs I see this area of the code is
> potentially fragile. Given that we have bug 376803 filed for further related
> investigation, do you think we can go with this additional check for now so
> that we no longer regress?
I just can't see all possible cases. Therefore it's hard to say if we'll regress. The answer doesn't depends on a11y code entirely which makes it harder :) Therefore I suggest to ensure we you're right by tests. Either this should be manual testing or mochitests and I prefer to have mochitests ;) I know this is big work but it's worth to do.

David, I think you should turn those bugs into mochitests. For example, bug
375747 - you should open new window and listen focus event, ensure root accessible for this window doesn't get focus event and the root accessible has't focusable state. Probably I didn't catch your question right, then please correct me.

Created attachment 404622[details][diff][review]
patch 4 (adds another test)
I've added a test to make sure the root accessible of a new window is not focusable/focused. For the other two bugs that were claimed to be fixed a while back by this part of the code, I think we are safe, but I agree we should add tests and I think we could create a bug for creating more robust menu event tests and do the work there?
I'd like to bake this blocker on trunk for a while before getting it into 1.9.2. We can confirm manually that I don't reincarnate those menu event bugs.

That's interesting: you open new window, it has a document which has root accessible, we catch focus for the root accessible however it's not focused. What is focused then? As well, it's worth to add the test where new window containing focusable controls is open.
I guess, it's because of your check if (focusFrame &&). In the meantime we probably won't fire focus event in this case which is probably wrong.
> + if (focusFrame &&
nit: space at the end
Also, probably it doesn't make sense to open new window (which becomes focused), focus owner window and then focus opened window again.
(In reply to comment #22)
> I think we are safe, but I agree we should add
> tests and I think we could create a bug for creating more robust menu event
> tests and do the work there?
I realize tests are routine and takes a lot of time but this is good proof we are in good shape. It shouldn't require huge amount time to add few more tests after you created one :) We have two bugs unaddressed in tests sense
1. bug 375746 - if you'll open window with controls (additionally to empty window) as I asked you above then it should cover this bug
2. bug 375748 - menu bug should be pretty easy, you just need XUL test with menu, open it by click and close it by escape key for example (test_events_caretmove.html has good invokers that could be casted to you needs). Btw, I have mochitest where I make them universal, I'll try to put it today, so you can reuse it.
>
> I'd like to bake this blocker on trunk for a while before getting it into
> 1.9.2. We can confirm manually that I don't reincarnate those menu event bugs.
Could we wait for couple days to finish tests? Working tests make me thing this code is correct :)

(In reply to comment #23)
> That's interesting: you open new window, it has a document which has root
> accessible, we catch focus for the root accessible however it's not focused.
In this patch we are expecting to catch it on the document.
> I guess, it's because of your check if (focusFrame &&). In the meantime we
> probably won't fire focus event in this case which is probably wrong.
Not sure. What wouldn't have a frame?
> Also, probably it doesn't make sense to open new window (which becomes
> focused), focus owner window and then focus opened window again.
This focusing is done as set up to ensure the right state before pushing the real test to the queue.
I'll try to add more tests. It will be tricky.

(In reply to comment #24)
> (In reply to comment #23)
> > That's interesting: you open new window, it has a document which has root
> > accessible, we catch focus for the root accessible however it's not focused.
>
> In this patch we are expecting to catch it on the document.
which document? root?
> > I guess, it's because of your check if (focusFrame &&). In the meantime we
> > probably won't fire focus event in this case which is probably wrong.
>
> Not sure. What wouldn't have a frame?
I'm not sure. Originally I concerned the issue we don't fire focus for root document but probably you will with your patch.
>
> > Also, probably it doesn't make sense to open new window (which becomes
> > focused), focus owner window and then focus opened window again.
>
> This focusing is done as set up to ensure the right state before pushing the
> real test to the queue.
Yeah, but I mean it's probably not necessary.
> I'll try to add more tests. It will be tricky.
Please. It would be easier to understand what happens.

(In reply to comment #25)
> (In reply to comment #24)
> > (In reply to comment #23)
> > > That's interesting: you open new window, it has a document which has root
> > > accessible, we catch focus for the root accessible however it's not focused.
> >
> > In this patch we are expecting to catch it on the document.
>
> which document? root?
>
Well, if I understand correctly, there is only one true root accessible (based on the document that has no parent), otherwise we create a document accessible. The test focused the window that is created, which I'm assuming would attempt to focus the one true root accessible, but I'm looking to see if focus actually goes to the content document accessible - because we want to be sure the one true root never gets focus.
Ideally there would be a way to test "focus didn't go to root accessible" because that is what I care about here.

I took another look at this today with fresh eyes. In nsRootAccessible::FireAccessibleFocusEvent we call nsAccessNode's GetCurrentFocus. The first time the content editable is focused, we get back an nsIDOMWindow from GetCurrentFocus. Subsequent times the content editable is focused we get back an nsIDOMElement (with local name "HTML").
I don't yet know why.
The subsequent times, this nsIDOMelement we get is not the same object as the node passed into FireAccessibleFocusEvent so we bail, assuming that there will be a subsequent focus event for the real focus. The comments in the code say this is about suppressing document focus because real DOM focus will happen next.

I forgot to mention that in GetCurrentFocus, it is the call to the focus manager's GetFocusedElementForWindow that returns something different between the first focus and subsequent focus events (via out params).

(In reply to comment #26)
> Well, if I understand correctly, there is only one true root accessible (based
> on the document that has no parent), otherwise we create a document accessible.
> The test focused the window that is created, which I'm assuming would attempt
> to focus the one true root accessible, but I'm looking to see if focus actually
> goes to the content document accessible - because we want to be sure the one
> true root never gets focus.
Let's define with terms. Document accessible is an accessible for sub document. Root accessible is an accessible for document of the window, root accessible tree can contain document accessibles. Application accessible is top level accessible keeping root accessibles as children.
I just not sure what do you mean by "there is only one true root accessible, otherwise we create a document accessible."

(In reply to comment #28)
> I forgot to mention that in GetCurrentFocus, it is the call to the focus
> manager's GetFocusedElementForWindow that returns something different between
> the first focus and subsequent focus events (via out params).
Probably that is what should be fixed or clarified at least from layout guys if it's expected behaviour?

(In reply to comment #35)
> (In reply to comment #26)
>
> > Well, if I understand correctly, there is only one true root accessible (based
> > on the document that has no parent), otherwise we create a document accessible.
> > The test focused the window that is created, which I'm assuming would attempt
> > to focus the one true root accessible, but I'm looking to see if focus actually
> > goes to the content document accessible - because we want to be sure the one
> > true root never gets focus.
>
> Let's define with terms. Document accessible is an accessible for sub document.
> Root accessible is an accessible for document of the window, root accessible
> tree can contain document accessibles. Application accessible is top level
> accessible keeping root accessibles as children.
>
> I just not sure what do you mean by "there is only one true root accessible,
> otherwise we create a document accessible."
That's just me looking at code nsAccessibilityService::CreateRootAccessible
http://mxr.mozilla.org/mozilla-central/source/accessible/src/base/nsAccessibilityService.cpp#470

(In reply to comment #36)
> (In reply to comment #28)
> > I forgot to mention that in GetCurrentFocus, it is the call to the focus
> > manager's GetFocusedElementForWindow that returns something different between
> > the first focus and subsequent focus events (via out params).
>
> Probably that is what should be fixed or clarified at least from layout guys if
> it's expected behaviour?
Yeah, I'm guessing it is a matter of lazy editor instantiation. Do you know who to ping?

(In reply to comment #40)
> I didn't write the focus manager but when an entire document is in design mode
> then I don't expect any element to be returned as the focused element.
Yeah, I was wondering the same. Neil Deakin, your thoughts?

Comment on attachment 408434[details][diff][review]
yet another approach (just saying)
This is a p2 blocker - I think we should take this. Real bug might lurk in editor. We can spin off a follow up for further investigation.
This bug makes participation on the web suck for our users.

Created attachment 410031[details][diff][review]
patch 4 with extra comments
Workaround. I'm pretty sure that editor or the focus manager is inconsistent here which we can help triage in the spin off bug. nsHTMLHtmlElement should be an ignored element with respect to focus.

Comment on attachment 410031[details][diff][review]
patch 4 with extra comments
the possible minus is we can get excess focus events but you filed following up bug to fix this bug in more nice manner. I still not sure your mochitest covers all possible cases but let save this for follow up bug.

I backed it out. While the tests pass locally on trunk, I encountered a timeout on my new test on central. I'm not happy with this approach and I agree with the concern in comment #48. I'd like to get a fix in for Jamie, but not a funky one.

Created attachment 411022[details][diff][review]
patch (editable state approach)
Alexander, please take a look at your earliest convenience as I'm rejecting my previous approach. This is safer overall; one concern is that we might fire focus for the root accessible on linux for content editable, but actually that is better than no focus event at all. Another concern is the cost of checking the editable state.
Note: I removed the extra (bug 375747) test from this patch since it failed intermittently (with and without my patch). I think this is okay (see above).