503428 - Caret-moved events missing from Thunderbird Subject field

Status: VERIFIED FIXED

(Core :: Disability Access APIs, defect)

Description

[Joanmarie Diggs]

Steps to reproduce:

1. Launch Thunderbird/Shredder

2. Press Ctrl+N to compose a new message

3. Tab to the body of the message and type some text

4. Shift+Tab once to move focus to the Subject field

5. Type some text and then arrow around (while monitoring object:text-caret-moved events using Accerciser)

Expected Results: object:text-caret-moved events would be issued

Actual Results: object:text-caret-moved events are not issued

Comment 1

[Marco Zehe (:MarcoZ)]

Joanie, did this ever work, or was this broken, and if so, can you pinpoint when? We haven't done anything with caret-moved events in ages, so I wonder if this has been around for a while without anyone noticing.

Comment 2

[Joanmarie Diggs]

(In reply to comment #1)
> Joanie, did this ever work, or was this broken, and if so, can you pinpoint
> when? We haven't done anything with caret-moved events in ages, so I wonder if
> this has been around for a while without anyone noticing.

I'm afraid I don't know. We got an Orca bug report about this issue, and when I looked into it last night I discovered the caret-moved events we need were absent. Sorry!

If you'd like, I could add trying to pinpoint when things broke to my to-do list. However, given other obligations on that list, I'm not sure when I'll have the time to do so. And maybe it's always been broken under these conditions. <shrugs>

Comment 3

[Marco Zehe (:MarcoZ)]

OK! Could you do a quick test for me to see if this is very specific to the subject field, or if this is apparent in other single-line text fields as well? 1. Go to Tools/Options and click Advanced.
2. Focus the "After displaying for 5 seconds" textbox.
3. Move the caret onto the 5 and away from it.

Doe shte same problem exist there, too?

Also, do we know if this is a current nightly or a previously released regular beta of Shredder?

Comment 4

[Joanmarie Diggs]

(In reply to comment #3)
> OK! Could you do a quick test for me to see if this is very specific to the
> subject field, or if this is apparent in other single-line text fields as well?
> 1. Go to Tools/Options and click Advanced.
> 2. Focus the "After displaying for 5 seconds" textbox.
> 3. Move the caret onto the 5 and away from it.
> 
> Doe shte same problem exist there, too?

Nope.

> Also, do we know if this is a current nightly or a previously released regular
> beta of Shredder?

The original report indicated "Thunderbird 3.0B2" (Linux is assumed). I am using last night's Shredder: "Mozilla/5.0 (X11; U; SunOS i86pc; en-US; rv:1.9.1.1pre) Gecko/20090709 Lightning/1.0pre Shredder/3.0b3pre"

Comment 5

[Ginn Chen]

Taking.

Comment 6

[Ginn Chen]

It happens sometimes.

I think the problem is here.

425   if (mCaretAccessible) {
426     nsCOMPtr<nsIDOMNSEvent> nsevent(do_QueryInterface(aFocusEvent));
427     if (nsevent) {
428       // Use the originally focused node where the selection lives.
429       // For example, use the anonymous HTML:input instead of the containing
430       // XUL:textbox. In this case, sometimes it is a later focus event
431       // which points to the actual anonymous child with focus, so to be safe 
432       // we need to reset the selection listener every time.
433       // This happens because when some bindings handle focus, they retarget
434       // focus to the appropriate child inside of themselves, but DOM focus
435       // stays outside on that binding parent.
436       nsCOMPtr<nsIDOMEventTarget> domEventTarget;
437       nsevent->GetOriginalTarget(getter_AddRefs(domEventTarget));
438       nsCOMPtr<nsIDOMNode> realFocusedNode(do_QueryInterface(domEventTarget));
439       if (!realFocusedNode) {
440         // When FireCurrentFocusEvent() synthesizes a focus event,
441         // the orignal target does not exist, so use the passed-in node
442         // which is the relevant focused node
443         realFocusedNode = aNode;
444       }
445       if (realFocusedNode) {
446         mCaretAccessible->SetControlSelectionListener(realFocusedNode);
447       }
448     }

From the comments here, we see we need to use HTML:input instead of XUL:textbox for SetControlSelectionListener().
But if we get focus of XUL:textbox from FireCurrentFocusEvent(), we will not be able to get HTML:input from GetOriginalTarget().
Then we call SetControlSelectionListener() for XUL:textbox, we will lose our SelectionListener for the HTML:input.

Comment 7

[Ginn Chen]

surkov, do you have any idea how to fix that?

Is it reasonable to remove this block?
439       if (!realFocusedNode) {
440         // When FireCurrentFocusEvent() synthesizes a focus event,
441         // the orignal target does not exist, so use the passed-in node
442         // which is the relevant focused node
443         realFocusedNode = aNode;
444       }

Comment 8

[alexander :surkov (:asurkov)]

(In reply to comment #7)
> surkov, do you have any idea how to fix that?
> 
> Is it reasonable to remove this block?
> 439       if (!realFocusedNode) {
> 440         // When FireCurrentFocusEvent() synthesizes a focus event,
> 441         // the orignal target does not exist, so use the passed-in node
> 442         // which is the relevant focused node
> 443         realFocusedNode = aNode;
> 444       }

I don't see how it can help.

I think we should fire simulated focus event with initialized original target also. It might work if we change nsRootAccessible::FireCurrentFocusEvent for this. So that original target should be "focusedNode", target will be GetRelevantContentNodeFor() (like it is now).

Comment 9

[Ginn Chen]

If we remove 439-444, we will not call SetControlSelectionListener() for FireCurrentFocusEvent().
It might be reasonable because for current focus target, the selection listener should have already been set.

I'm wondering if the calculation of finalFocusNode in nsRootAccessible::FireAccessibleFocusEvent() should use "realFocusedNode".

Comment 10

[alexander :surkov (:asurkov)]

(In reply to comment #9)
> If we remove 439-444, we will not call SetControlSelectionListener() for
> FireCurrentFocusEvent().
> It might be reasonable because for current focus target, the selection listener
> should have already been set.

I see. That might work if real focus isn't changed but a11y focus only, for example, if user navigates through menu items. However do we really want not to process selection changes for these cases?

> I'm wondering if the calculation of finalFocusNode in
> nsRootAccessible::FireAccessibleFocusEvent() should use "realFocusedNode".

Possibly we could try to however we should ensure we won't fire event from HTML:input inside of XUL:textbox. Possibly we shouldn't since you don't need to have additional calculation of node we should fire focus event on.

Comment 11

[Marco Zehe (:MarcoZ)]

Did you guys talk offline on this further? Any good clues on how to deal with this yet? I'm tempted to request blocking-Thunderbird 3 on this since it is a rather important piece of the UI.

Comment 12

[Joanmarie Diggs]

In addition to this problem being present in Thunderbird, I am seeing what appears to be the very same issue in Instantbird (a Gecko-based chat client). There, if you are in the entry in which you type a message and Shift+Tab up to the chat history (which is in a Document Frame) and then Tab back to the entry, caret-moved events no longer appear for the entry.

Not sure if that gives y'all any further insight into the problem....

Comment 13

[Ginn Chen]

Attachment: patch

I think it is what you suggested in comment #8.

It passed mochitest-a11y on Solaris.

Comment 14

[alexander :surkov (:asurkov)]

(In reply to comment #13)
> Created an attachment (id=440741) [details]
> patch
> 
> I think it is what you suggested in comment #8.

I guess I meant to hack event object so that it provides target and originalTarget both. But I need to refresh my mind on this bug.

Comment 15

[alexander :surkov (:asurkov)]

Ginn, is FireCurrentFocusEvent() called from here http://mxr.mozilla.org/mozilla-central/source/accessible/src/base/nsRootAccessible.cpp#776 in this case? Could you please describe steps to get result from your comment #6?

Comment 16

[Ginn Chen]

Sorry for the late response.

(In reply to comment #14)
> (In reply to comment #13)
> > Created an attachment (id=440741) [details] [details]
> > patch
> > 
> > I think it is what you suggested in comment #8.
> 
> I guess I meant to hack event object so that it provides target and
> originalTarget both. But I need to refresh my mind on this bug.

I need to refresh my mind, too.
I think my patch is not correct, at least it didn't do what the comments say.

(In reply to comment #15)
> Ginn, is FireCurrentFocusEvent() called from here
> http://mxr.mozilla.org/mozilla-central/source/accessible/src/base/nsRootAccessible.cpp#776
> in this case? Could you please describe steps to get result from your comment
> #6?

It is called through nsRootAccessible::nsRunnableMethod_FireCurrentFocusEvent::Run().
Use step 1)-4) in comment #0.

Comment 17

[alexander :surkov (:asurkov)]

Ginn, do you mean patch from bug 566103 doesn't help here?

Comment 18

[Ginn Chen]

Attachment: patch v2

Is it more reasonable?

Comment 19

[alexander :surkov (:asurkov)]

(In reply to comment #17)
> Ginn, do you mean patch from bug 566103 doesn't help here?

ignore this please, I was confused with bug numbers.

Comment 20

[alexander :surkov (:asurkov)]

(In reply to comment #18)
> Created an attachment (id=449841) [details]
> patch v2
> 
> Is it more reasonable?

If I get right then we attach selection listener to right node (HTML input) but will we fire accessible focus event for XUL textbox (or what's used there)?

Comment 21

[Ginn Chen]

(In reply to comment #20)

> If I get right then we attach selection listener to right node (HTML input) but
> will we fire accessible focus event for XUL textbox (or what's used there)?

Yes, focus event is fired for nsXULTextFieldAccessible.
The behavior is not changed by the patch.

Comment 22

[alexander :surkov (:asurkov)]

(In reply to comment #21)

> Yes, focus event is fired for nsXULTextFieldAccessible.
> The behavior is not changed by the patch.

Ok, I see. So when you set event target by setTarget() then it's returned when target and original are requested, right?

Could you think of mochitests?

Comment 23

[Ginn Chen]

(In reply to comment #22)
> (In reply to comment #21)
> 
> > Yes, focus event is fired for nsXULTextFieldAccessible.
> > The behavior is not changed by the patch.
> 
> Ok, I see. So when you set event target by setTarget() then it's returned when
> target and original are requested, right?

I think so.

> Could you think of mochitests?

I think mochitest will be great.
But I don't think I have time to write test for it.
It will be great if you can help me work it out.

Comment 24

[alexander :surkov (:asurkov)]

(In reply to comment #23)
 
> > Could you think of mochitests?
> 
> I think mochitest will be great.
> But I don't think I have time to write test for it.
> It will be great if you can help me work it out.

The best way would be save this for "tomorrow". I think it's ok to take this patch without mochitest since it looks safe but we need to file following up bug to not forget this. Ginn, could you please file a bug and provide steps to reproduce that can be convert into mochitest easy (it shouldn't be about thunderbird of course :) ).

Comment 25

[Ginn Chen]

http://hg.mozilla.org/mozilla-central/rev/35f1e03ce928

Comment 26

[Marco Zehe (:MarcoZ)]

Attachment: Patch2, made for the 1.9.2 branch

Alex, can you quickly look over this to see if I inserted the code snippet at the right spot? The patch had to be merged, and there's quite some more code in the m-c version here.

Comment 27

[Marco Zehe (:MarcoZ)]

Comment on attachment 453335 [details] [diff] [review]
Patch2, made for the 1.9.2 branch

This will imensely help screen reader users particularly on Linux when focus shifts from the message body to the subject field. In certain extensions in Firefox this will also help.

Comment 28

[Daniel Veditz [:dveditz]]

Comment on attachment 453335 [details] [diff] [review]
Patch2, made for the 1.9.2 branch

Approved for 1.9.2.9, a=dveditz for release-drivers

Comment 29

[Marco Zehe (:MarcoZ)]

Landed on 1.9.2: http://hg.mozilla.org/releases/mozilla-1.9.2/rev/25f0e9f9743c

Comment 31

[RaZiel]

What I reported in the dup bug 574303, has now been tested with thunderbird-3.1.3pre.en-US.win32.zip, and the problems have been resolved. gj

Comment 32

[Marco Zehe (:MarcoZ)]

See comment #31.