512059 - Accessibility focus event never fired for designMode document after the first focus

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect, P2)

Description

[James Teh [:Jamie]]

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.

Comment 1

[James Teh [:Jamie]]

Attachment: Test case.

Comment 2

[James Teh [:Jamie]]

This bug is not present in Firefox 3.5.2.

Comment 3

[Marco Zehe (:MarcoZ)]

Suspected regression from bug 178324 (the big focus handling refactor).

Comment 4

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

Attachment: 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 5

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

Tryserver build should be here: https://build.mozilla.org/tryserver-builds/[e-mail]-fix-designmode-focus/

Comment 6

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

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.

Comment 7

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

oops "this was removed code" should be "this removed code was"

Comment 8

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

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.

Comment 9

[Marco Zehe (:MarcoZ)]

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

Yeah, this looks right, but want Surkov's eyes on it too. :)

Comment 10

[alexander :surkov (:asurkov)]

David, I think mochitests are needed 1) for editor 2) for the code you remove.

Comment 11

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

Alexander, good to have you back :)  Tests coming.

Comment 12

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

Attachment: 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 13

[Marco Zehe (:MarcoZ)]

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.

Comment 14

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

Thanks Marco, nits fixed locally.

Comment 15

[alexander :surkov (:asurkov)]

(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.

Comment 16

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

OK yes I looked at bug 375747 and wasn't able to recreate it manually with this patch applied. I'll look at the other two thanks; I don't want to break anything.

Comment 17

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

Attachment: 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?

Comment 18

[alexander :surkov (:asurkov)]

(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.

Comment 19

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

I prefer mochitests too... but hmmm where to start? Marco, Alexander if you guys have ideas let me know, in the meantime I'll try to figure out the right cases.

Comment 20

[alexander :surkov (:asurkov)]

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.

Comment 21

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

That's a good test -- I'll add that thanks.

Comment 22

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

Attachment: 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.

Comment 23

[alexander :surkov (:asurkov)]

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 :)

Comment 24

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

(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.

Comment 25

[alexander :surkov (:asurkov)]

(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.

Comment 26

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

(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.

Comment 27

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

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.

Comment 28

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

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).

Comment 29

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

Fired up gdb. Call stacks appear identical.

Comment 30

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

... inside nsRootAccessible::FireAccessibleFocusEvent.

Comment 31

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

OK what's happening is that GetFocusedElementForWindow (in nsAccessNode::GetCurrentFocus) is returning the html node for the editable document. There will be no focus event fired later for this node.

Comment 32

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

Attachment: WIP - different approach

Alexander do you prefer this approach? (If so, I'll add the tests from the other patch)

Comment 33

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

Ugh that won't work... we'll double up focus for other cases. Sigh.

Comment 34

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

Note to self: I might need to specifically check for focus on "html".

Comment 35

[alexander :surkov (:asurkov)]

(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."

Comment 36

[alexander :surkov (:asurkov)]

(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?

Comment 37

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

(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

Comment 38

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

(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?

Comment 39

[alexander :surkov (:asurkov)]

(In reply to comment #38)

> Yeah, I'm guessing it is a matter of lazy editor instantiation. Do you know who
> to ping?

Daniel, Neil, Robert?

Comment 40

[[e-mail]]

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.

Comment 41

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

Peter, do you have ideas about why the focus is different on the second pass at the editor?

Comment 42

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

(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 43

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

Attachment: yet another approach (just saying)

Comment 44

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

Comment on attachment 404622 [details] [diff] [review]
patch 4 (adds another test)

Note: reuse tests from this patch, but relying on the absence of a frame is a bogus approach (thanks Enn).

Comment 45

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

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.

Comment 46

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

Note I'll remove the printf and combine with tests from other  patch if you agree on the approach.

Comment 47

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

Attachment: 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 48

[alexander :surkov (:asurkov)]

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.

Comment 49

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

Thanks.

Note, I'm sure I don't have full mochitest coverage for all cases :)

Comment 50

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

Hmmm. I would have preferred to get this into a 1.9.2 beta.

Comment 51

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

Pushed to central:
http://hg.mozilla.org/mozilla-central/rev/073164ba54ce

Comment 52

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

Testing locally on a fresh 192, there are problems with this approach. (I might backout from central -- still investigating)

Comment 53

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

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.

Comment 54

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

Attachment: 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).

Comment 55

[alexander :surkov (:asurkov)]

Comment on attachment 411022 [details] [diff] [review]
patch (editable state approach)

the approach looks ok as well, r=me

Comment 56

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

Landed.
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/82c375496e83
http://hg.mozilla.org/mozilla-central/rev/9a82c2bc0687

Comment 57

[Marco Zehe (:MarcoZ)]

Verified fixed on Mozilla/5.0 (Windows; U; Windows NT 5.1; de; rv:1.9.2b2) Gecko/20091108 Firefox/3.6b2 (.NET CLR 3.5.30729)