524115 - HyperText accessible should get focus when the caret is positioned inside of it, text is changed or copied into clipboard by ATs

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Joanmarie Diggs]

Steps to reproduce

1. Launch Firefox and Accerciser

2. Enable Firefox's caret browsing if it is not already enabled

3. Navigate to any page with a link (for this example, I'll use Google)

4. Locate a link (e.g. Images) in Accerciser's tree of accessibles. With that link selected in the tree, type the following in Accerciser's iPython console:

    acc.queryText().setCaretOffset(1)
    
5. Alt Tab back to the Firefox window

Expected results: The link in question (e.g. Images) would not only have a visible caret positioned at offset 1, it would also have focus.

Actual results: The caret is positioned as expected, but the link does not have focus.

Impact: Due to various issues with Gecko's native caret navigation, Orca has had to implement its own. We use atk_text_set_caret_offset() to position the caret as the user arrows. When the user arrows into a link, he/she expects to then be able to press Enter to activate that link -- just like one can when arrowing into a link using Gecko's native caret navigation.

Comment 1

[alexander :surkov (:asurkov)]

Attachment: patch

Comment 2

[Marco Zehe (:MarcoZ)]

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

r=me for the test part.

What do the insert/cut/copy/paste methods in the first hunk of the patch have to do with all this?

Comment 3

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

That's cleanup, but it would be good to have test coverage while we're here.

Comment 4

[alexander :surkov (:asurkov)]

(In reply to comment #3)
> That's cleanup, but it would be good to have test coverage while we're here.

Yes, mostly this is cleanup. However I replaced setCaretOffset on SetSelectionRange in some methods.

Comment 5

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

Alexander I'm waiting for the additional tests before I review unless you want a review of the non tests part now, in which case it looks fine but I can't tell if it works ;)

Comment 6

[alexander :surkov (:asurkov)]

Sure, I'll add them today or tomorrow I guess. (If they worked, then they works ;) ).

Comment 7

[alexander :surkov (:asurkov)]

Attachment: patch2

Focus whenever we are going to modify hyper text via insertText and etc. On the one hand it sounds reasonable, one the another hand editor's clipboard operations won't work until element is focused.

Comment 8

[alexander :surkov (:asurkov)]

changing summary

Comment 9

[Marco Zehe (:MarcoZ)]

Comment on attachment 408770 [details] [diff] [review]
patch2

r=me with two nits:

>aClipbaordStr

Please change to aClipboardStr (swap the o and a).

>+    var msg = "pasteText to " + aPos + " postion for element " +

Typo: "position".

Can you start a try-server build and point Joanie to it so she can test if this is what she needs for Orca? Thanks!

Comment 10

[alexander :surkov (:asurkov)]

(In reply to comment #9)
> (From update of attachment 408770 [details] [diff] [review])
> r=me with two nits:

fixed locally
 
> Can you start a try-server build and point Joanie to it so she can test if this
> is what she needs for Orca? Thanks!

sure

Comment 11

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

Comment on attachment 408770 [details] [diff] [review]
patch2

r=me, with Marco's nits fixed and Joanmarie's nod.

I think focusing (TakeFocus) before preforming these operations is ok.

Aside: I wonder why nsIEditor doesn't have an InsertText method.

Comment 12

[Joanmarie Diggs]

(In reply to comment #10)
> (In reply to comment #9)
> > (From update of attachment 408770 [details] [diff] [review] [details])
> > r=me with two nits:
> 
> fixed locally
> 
> > Can you start a try-server build and point Joanie to it so she can test if this
> > is what she needs for Orca? Thanks!
> 
> sure

Where is this try-server build of which you speak? :-)

Comment 13

[alexander :surkov (:asurkov)]

(In reply to comment #12)
> (In reply to comment #10)
> > (In reply to comment #9)

> > > Can you start a try-server build and point Joanie to it so she can test if this
> > > is what she needs for Orca? Thanks!
> > 
> > sure
> 
> Where is this try-server build of which you speak? :-)

Here it is http://build.mozilla.org/tryserver-builds/[e-mail]-try-2f166b0010ff. It makes some time to prepare it :)

Comment 14

[alexander :surkov (:asurkov)]

(In reply to comment #11)

> Aside: I wonder why nsIEditor doesn't have an InsertText method.

Probably because it doesn't make a lot of sense for HTML editor or it's ambiguous.

Comment 15

[alexander :surkov (:asurkov)]

(In reply to comment #14)
> (In reply to comment #11)
> 
> > Aside: I wonder why nsIEditor doesn't have an InsertText method.
> 
> Probably because it doesn't make a lot of sense for HTML editor or it's
> ambiguous.

However I think we should process this case in accessibility for HTML editor to make sure nsIAccessible::InsertText works.

Comment 16

[Joanmarie Diggs]

Just gave it a quick try. It works as expected for links, but I was somewhat surprised when setting the caret position to an offset in the document frame that the document frame seemed to claim focus (as evidenced by the sudden appearance of a focus rectangle around it). Does that mean that anything which can claim focus will claim focus if the user is navigating with Orca controlling the caret?

I think it will be worth running the full suite of Orca regression tests to make sure these changes don't have unforeseen consequences. After I build a patched Firefox for OpenSolaris. :-)

Comment 17

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

(In reply to comment #15)
> (In reply to comment #14)
> > (In reply to comment #11)
> > 
> > > Aside: I wonder why nsIEditor doesn't have an InsertText method.
> > 
> > Probably because it doesn't make a lot of sense for HTML editor or it's
> > ambiguous.
> 
> However I think we should process this case in accessibility for HTML editor to
> make sure nsIAccessible::InsertText works.

Not sure I understand, can you rephrase? :)

Comment 18

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

(In reply to comment #16)
> I think it will be worth running the full suite of Orca regression tests to
> make sure these changes don't have unforeseen consequences. After I build a
> patched Firefox for OpenSolaris. :-)

Yes please!

Comment 19

[alexander :surkov (:asurkov)]

(In reply to comment #16)
> Just gave it a quick try. It works as expected for links, but I was somewhat
> surprised when setting the caret position to an offset in the document frame
> that the document frame seemed to claim focus (as evidenced by the sudden
> appearance of a focus rectangle around it). Does that mean that anything which
> can claim focus will claim focus if the user is navigating with Orca
> controlling the caret?

Isn't it expected? At the first sight it looks reasonable to have unique behaviour.

Comment 20

[alexander :surkov (:asurkov)]

(In reply to comment #17)
> (In reply to comment #15)
> > (In reply to comment #14)
> > > (In reply to comment #11)
> > > 
> > > > Aside: I wonder why nsIEditor doesn't have an InsertText method.
> > > 
> > > Probably because it doesn't make a lot of sense for HTML editor or it's
> > > ambiguous.
> > 
> > However I think we should process this case in accessibility for HTML editor to
> > make sure nsIAccessible::InsertText works.
> 
> Not sure I understand, can you rephrase? :)

I'm not sure we can query nsIPlainTextEditor on HTML editor.

Comment 21

[Joanmarie Diggs]

(In reply to comment #19)

> Isn't it expected? At the first sight it looks reasonable to have unique
> behaviour.

It doesn't seem to happen if I press F7 and arrow from, say, a link into body text. I only see that focus rectangle when using atk_text_set_caret_offset(). Perhaps this is of no consequence....

My only concern, which may be completely unfounded, is that Gecko will be issuing quite a few more events that Orca wasn't anticipating. As a result, we might become too "chatty" and/or decide the user is someplace he/she isn't or something else. Not the end of the world, it just means that testing is needed and possibly modifications to Orca will be needed.

That said, we have a user who wants to arrow to a link and press Enter. Just like he could do if Orca weren't controlling the caret. That's all. As far as I am aware, we weren't suffering from widespread lack of things claiming focus.

<shrugs>

Comment 22

[alexander :surkov (:asurkov)]

(In reply to comment #21)
> (In reply to comment #19)
> 
> > Isn't it expected? At the first sight it looks reasonable to have unique
> > behaviour.
> 
> It doesn't seem to happen if I press F7 and arrow from, say, a link into body
> text. I only see that focus rectangle when using atk_text_set_caret_offset().
> Perhaps this is of no consequence....

Do you mean you don't receive focus event?

> My only concern, which may be completely unfounded, is that Gecko will be
> issuing quite a few more events that Orca wasn't anticipating. As a result, we
> might become too "chatty" and/or decide the user is someplace he/she isn't or
> something else. Not the end of the world, it just means that testing is needed
> and possibly modifications to Orca will be needed.

New event is focus event, nothing else should be expected additionally.

> That said, we have a user who wants to arrow to a link and press Enter. Just
> like he could do if Orca weren't controlling the caret. That's all. As far as I
> am aware, we weren't suffering from widespread lack of things claiming focus.

Any way, if we'll focus link on setCaretOffset then you should get focus event. I just don't completely understand why link handling might be different. Would you expect, for example, focused html:input on setCaretOffset?

Comment 23

[Joanmarie Diggs]

(In reply to comment #22)
> (In reply to comment #21)
> > (In reply to comment #19)
> > 
> > > Isn't it expected? At the first sight it looks reasonable to have unique
> > > behaviour.
> > 
> > It doesn't seem to happen if I press F7 and arrow from, say, a link into body
> > text. I only see that focus rectangle when using atk_text_set_caret_offset().
> > Perhaps this is of no consequence....
> 
> Do you mean you don't receive focus event?

When I said it I meant it figuratively, i.e. "Who cares if there's a difference between native caret navigation and what results from the use of atk_text_set_caret_offset()?"

That having been said, while I don't have time at this particular moment to build and test the patch further on OpenSolaris, I did take another look at it on the VirtualBox install of Ubuntu Karmic I have. And in my (admittedly brief), follow-up testing, Accerciser was NOT registering focus: or object:state-changed:focused events on the document frame even though the focus rectangle appeared around its edges. If that's totally not correct, I'll try to make some time in the next day or so to test it more closely.

> > My only concern, which may be completely unfounded, is that Gecko will be
> > issuing quite a few more events that Orca wasn't anticipating. As a result, we
> > might become too "chatty" and/or decide the user is someplace he/she isn't or
> > something else. Not the end of the world, it just means that testing is needed
> > and possibly modifications to Orca will be needed.
> 
> New event is focus event, nothing else should be expected additionally.
> 
> > That said, we have a user who wants to arrow to a link and press Enter. Just
> > like he could do if Orca weren't controlling the caret. That's all. As far as I
> > am aware, we weren't suffering from widespread lack of things claiming focus.
> 
> Any way, if we'll focus link on setCaretOffset then you should get focus event.
> I just don't completely understand why link handling might be different. Would
> you expect, for example, focused html:input on setCaretOffset?

Okay, that's a good point. I'll concede that one to you. Mind you, it's not been an issue because in that case we do our own grab focus. (We can't solve the link problem by doing our own grab focus because in some instances doing so causes Orca to get stuck/loop on a page.)

Anyhoo...

Links and textual input fields fine. :-) And whatever else I'm sure will be fine (or I'll work around it in Orca, or I'll file another bug). But for the sake of this discussion, I'm wondering about:

* document frame
* headings
* paragraphs
* sections (i.e. <div></div>)
* text objects (which is how acronyms and other things get exposed to us)

You wouldn't think -- or at least I wouldn't have thought -- that those sorts of things might be focusable. But they can be.

Comment 24

[Joanmarie Diggs]

Where are we on this one?

Comment 25

[alexander :surkov (:asurkov)]

Sorry it have been lost from my track. Thank you for bringing the issue back.

1) when you arrows through document in caret navigation mode (F7) you should get focus event when caret goes into focusable element (i.e. link). If it doesn't happen then please file new bug.

2) when you change the caret then everything focusable should be focused - it sounds we agree on this. If you set the caret inside of heading and etc what is not focusable then I would say the focusable parent should be focused, the document frame in this case I think. However the proposed behavior of the patch is a bit different, it tries to focus whenever you change the selection (either you select a range or change the caret position), probably it's not exactly you want.

Comment 26

[Joanmarie Diggs]

(In reply to comment #25)

> 2) when you change the caret then everything focusable should be focused - it
> sounds we agree on this. If you set the caret inside of heading and etc what is
> not focusable then I would say the focusable parent should be focused, the
> document frame in this case I think.

Assuming I'm following you, I think this will be fine.

> However the proposed behavior of the patch
> is a bit different,

So are you planning on creating a new patch to do what you describe? And do you need anything from me?

Thanks!

Comment 27

[alexander :surkov (:asurkov)]

(In reply to comment #26)

> > However the proposed behavior of the patch
> > is a bit different,
> 
> So are you planning on creating a new patch to do what you describe? And do you
> need anything from me?

Do you expect the accessible should be focused if you change the selection?

Comment 28

[Joanmarie Diggs]

(In reply to comment #27)

> Do you expect the accessible should be focused if you change the selection?

Can you give me a specific example? Are we talking about text selection crossing from one accessible object into another, or something else?

Comment 29

[alexander :surkov (:asurkov)]

(In reply to comment #28)
> (In reply to comment #27)
> 
> > Do you expect the accessible should be focused if you change the selection?
> 
> Can you give me a specific example? Are we talking about text selection
> crossing from one accessible object into another, or something else?

I think yes, I meant set_selection(AtkText *aText, gint aSelectionNum, gint aStartOffset, gint aEndOffset). But please ignore the patch doesn't affect on this.

Marco, do you remember whether you tried this patch?

Comment 30

[Joanmarie Diggs]

(In reply to comment #29)

> I think yes, I meant set_selection(AtkText *aText, gint aSelectionNum, gint
> aStartOffset, gint aEndOffset). But please ignore the patch doesn't affect on
> this.

If aText has the caret, and the parent document frame is focused, I would think the object associated with aText would also be focused -- if it were focusable.

Comment 31

[alexander :surkov (:asurkov)]

Yes but if it's focusable and has a caret then it should be already focused. At least if you call set_caret_offset and this patch is applied then the focusable text or its container will be focused.

Comment 32

[Marco Zehe (:MarcoZ)]

I remember reviewing this patch, but not if I actually ran a build with it. I think I mostly looked at the test cases. But it's been a while, and it was a couple of busy months...

Comment 33

[alexander :surkov (:asurkov)]

try server build - http://build.mozilla.org/tryserver-builds/[e-mail]-try-d355c0daa022

Marco, could you try this please?

Comment 34

[alexander :surkov (:asurkov)]

Attachment: patch3

updated to trunk

Comment 35

[Marco Zehe (:MarcoZ)]

(In reply to comment #33)
> try server build -
> http://build.mozilla.org/tryserver-builds/[e-mail]-try-d355c0daa022
> 
> Marco, could you try this please?

Will do. However it would be good if Joanie could test it as well, since I cannot verify the visual aspects of it. Thanks!

Comment 36

[Marco Zehe (:MarcoZ)]

Hmm somehow this build does not show any difference for me. With JAWS, I expected to be able to do something like this:
1. Go to http://www.heise.de/newsticker. This page has a lot of focusable and theoretically non-focusable elements such as links and headings respectively.
2. Using JAWS virtual cursor, go to any of the headings, for example "16.02.2010".
3. Press Ctrl+NumPad0+NumPadDelete. This routes the application's focus to the current position of the virtual cursor.

Before this fix, I expect the virtual cursor not being able to pull the application focus to it.
With this build, I however expected this to work, but it doesn't.

It does seem to work if nothing else is focused (for example no link had previously focus). But if something else was focused before, it remains focused now. This is with and without the try-server build.

Am I getting something wrong?

Comment 37

[alexander :surkov (:asurkov)]

Marco, it's hard to say, JAWS might not use IAccessibleText::caretOffset in this case. On the another hand it's good there is no JAWS regressions which means we could try to land it.

Comment 38

[alexander :surkov (:asurkov)]

landed on 1.9.3 - http://hg.mozilla.org/mozilla-central/rev/78c3802cfad4