Closed Bug 503428 Opened 10 years ago Closed 10 years ago

Caret-moved events missing from Thunderbird Subject field

Categories

(Core :: Disability Access APIs, defect)

x86
Solaris
defect
Not set

Tracking

()

VERIFIED FIXED
Tracking Status
status1.9.2 --- .9-fixed

People

(Reporter: jdiggs, Assigned: ginnchen+exoracle)

References

(Depends on 1 open bug, Blocks 1 open bug)

Details

(Keywords: access, verified1.9.2)

Attachments

(2 files, 1 obsolete file)

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
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.
(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>
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?
(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"
Taking.
Assignee: nobody → ginn.chen
Status: NEW → ASSIGNED
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.
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       }
(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).
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".
(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.
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.
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....
Attached patch patch (obsolete) — Splinter Review
I think it is what you suggested in comment #8.

It passed mochitest-a11y on Solaris.
Attachment #440741 - Flags: review?(surkov.alexander)
(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.
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?
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.
Ginn, do you mean patch from bug 566103 doesn't help here?
Attached patch patch v2Splinter Review
Is it more reasonable?
Attachment #440741 - Attachment is obsolete: true
Attachment #449841 - Flags: review?(surkov.alexander)
Attachment #440741 - Flags: review?(surkov.alexander)
(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.
(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)?
(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.
(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?
(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.
(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 :) ).
Attachment #449841 - Flags: review?(surkov.alexander) → review+
http://hg.mozilla.org/mozilla-central/rev/35f1e03ce928
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Depends on: 573935
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.
Attachment #453335 - Flags: review?(surkov.alexander)
Attachment #453335 - Flags: review?(surkov.alexander) → review+
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.
Attachment #453335 - Flags: approval1.9.2.7?
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
Attachment #453335 - Flags: approval1.9.2.9? → approval1.9.2.9+
Duplicate of this bug: 574303
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
See comment #31.
Status: RESOLVED → VERIFIED
Keywords: verified1.9.2
You need to log in before you can comment on or make changes to this bug.