If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

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

RESOLVED FIXED

Status

()

Core
Disability Access APIs
P2
major
RESOLVED FIXED
8 years ago
8 years ago

People

(Reporter: Jamie, Assigned: davidb)

Tracking

({access, regression, verified1.9.2})

Trunk
x86
Windows XP
access, regression, verified1.9.2
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.9.2 +
in-testsuite +

Firefox Tracking Flags

(status1.9.2 beta2-fixed)

Details

Attachments

(2 attachments, 7 obsolete attachments)

(Reporter)

Description

8 years ago
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.
(Reporter)

Comment 1

8 years ago
Created attachment 396056 [details]
Test case.
(Reporter)

Updated

8 years ago
Keywords: access

Updated

8 years ago
Blocks: 512196
(Reporter)

Comment 2

8 years ago
This bug is not present in Firefox 3.5.2.

Comment 3

8 years ago
Suspected regression from bug 178324 (the big focus handling refactor).
Blocks: 178324
No longer blocks: 512196
Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: regression

Updated

8 years ago
Severity: normal → major
Flags: blocking1.9.2?
Version: unspecified → Trunk

Updated

8 years ago
Blocks: 512196
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.
Assignee: nobody → bolterbugz
Status: NEW → ASSIGNED
Tryserver build should be here: https://build.mozilla.org/tryserver-builds/dbolter@mozilla.com-fix-designmode-focus/
Flags: blocking1.9.2? → blocking1.9.2+
Priority: -- → P2
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.
Attachment #401498 - Attachment description: exploratory WIP → patch
Attachment #401498 - Flags: review?(surkov.alexander)
Attachment #401498 - Flags: review?(marco.zehe)
oops "this was removed code" should be "this removed code was"
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

8 years ago
Comment on attachment 401498 [details] [diff] [review]
patch

Yeah, this looks right, but want Surkov's eyes on it too. :)
Attachment #401498 - Flags: review?(marco.zehe) → review+

Comment 10

8 years ago
David, I think mochitests are needed 1) for editor 2) for the code you remove.
Alexander, good to have you back :)  Tests coming.
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).
Attachment #401498 - Attachment is obsolete: true
Attachment #403537 - Flags: review?(surkov.alexander)
Attachment #403537 - Flags: review?(marco.zehe)
Attachment #401498 - Flags: review?(surkov.alexander)
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.
Attachment #403537 - Flags: review?(marco.zehe) → review+
Thanks Marco, nits fixed locally.

Comment 15

8 years ago
(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.
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.
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?
Attachment #403537 - Attachment is obsolete: true
Attachment #404084 - Flags: review?(surkov.alexander)
Attachment #403537 - Flags: review?(surkov.alexander)

Comment 18

8 years ago
(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.
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

8 years ago
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.
That's a good test -- I'll add that thanks.
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.
Attachment #404084 - Attachment is obsolete: true
Attachment #404622 - Flags: review?(surkov.alexander)
Attachment #404084 - Flags: review?(surkov.alexander)

Comment 23

8 years ago
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.

Comment 25

8 years ago
(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).
Fired up gdb. Call stacks appear identical.
... inside nsRootAccessible::FireAccessibleFocusEvent.
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.
Created attachment 408082 [details] [diff] [review]
WIP - different approach

Alexander do you prefer this approach? (If so, I'll add the tests from the other patch)
Ugh that won't work... we'll double up focus for other cases. Sigh.
Note to self: I might need to specifically check for focus on "html".

Comment 35

8 years ago
(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

8 years ago
(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?

Comment 39

8 years ago
(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?
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.
Peter, do you have ideas about why the focus is different on the second pass at the editor?
(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?
Created attachment 408434 [details] [diff] [review]
yet another approach (just saying)
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).
Attachment #404622 - Flags: review?(surkov.alexander)
Attachment #404622 - Attachment is obsolete: true
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.
Attachment #408434 - Flags: review?(surkov.alexander)
Note I'll remove the printf and combine with tests from other  patch if you agree on the approach.
Attachment #408434 - Flags: review?(surkov.alexander)
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.
Attachment #408082 - Attachment is obsolete: true
Attachment #408434 - Attachment is obsolete: true
Attachment #410031 - Flags: review?(surkov.alexander)

Comment 48

8 years ago
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.
Attachment #410031 - Flags: review?(surkov.alexander) → review+
Thanks.

Note, I'm sure I don't have full mochitest coverage for all cases :)
Hmmm. I would have preferred to get this into a 1.9.2 beta.
Pushed to central:
http://hg.mozilla.org/mozilla-central/rev/073164ba54ce
Flags: in-testsuite+
Testing locally on a fresh 192, there are problems with this approach. (I might backout from central -- still investigating)
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.
Flags: in-testsuite+
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).
Attachment #410031 - Attachment is obsolete: true
Attachment #411022 - Flags: review?(surkov.alexander)

Comment 55

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

the approach looks ok as well, r=me
Attachment #411022 - Flags: review?(surkov.alexander) → review+
Landed.
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/82c375496e83
http://hg.mozilla.org/mozilla-central/rev/9a82c2bc0687
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
status1.9.2: --- → beta2-fixed
Flags: in-testsuite+
Resolution: --- → FIXED
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)
Keywords: verified1.9.2
You need to log in before you can comment on or make changes to this bug.