Last Comment Bug 546068 - Regression: Position is not being updated when atk_text_set_caret_offset is used, effective 11th June 2009 build
: Regression: Position is not being updated when atk_text_set_caret_offset is u...
Status: RESOLVED FIXED
: access, regression
Product: Core
Classification: Components
Component: Disability Access APIs (show other bugs)
: Trunk
: x86 Linux
: -- major with 1 vote (vote)
: mozilla10
Assigned To: alexander :surkov
:
Mentors:
Depends on: 542313 618046 688126
Blocks: orca a11ynext 178324
  Show dependency treegraph
 
Reported: 2010-02-13 19:38 PST by Joanmarie Diggs
Modified: 2011-10-17 03:50 PDT (History)
12 users (show)
surkov.alexander: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
.x+


Attachments
test case (249 bytes, text/html)
2010-02-13 19:38 PST, Joanmarie Diggs
no flags Details
hg diff vs trunk - should work with patch (4.48 KB, patch)
2010-04-02 12:59 PDT, Bill Cox
no flags Details | Diff | Review
Patch to fix Orca navigation (4.30 KB, patch)
2010-04-03 01:36 PDT, Bill Cox
no flags Details | Diff | Review
Patch to fix Orca navigation (5.45 KB, patch)
2010-04-04 03:12 PDT, Bill Cox
no flags Details | Diff | Review
Patch based on using MoveFocus instead of TakeFocus (1.41 KB, patch)
2010-04-23 14:01 PDT, Bill Cox
no flags Details | Diff | Review
More properly formatted patch to fix set_caret_position (2.44 KB, patch)
2010-04-25 23:06 PDT, Bill Cox
enndeakin: review-
Details | Diff | Review
Modified patch based on Neil's feedback (1.83 KB, patch)
2010-05-27 10:44 PDT, Bill Cox
enndeakin: review+
Details | Diff | Review
Patch, including a new mochitest test case (5.05 KB, patch)
2010-05-28 09:35 PDT, Bill Cox
no flags Details | Diff | Review
patch2 (5.65 KB, patch)
2010-12-02 19:32 PST, Fernando Herrera
no flags Details | Diff | Review
Idea (4.95 KB, patch)
2011-01-22 13:12 PST, Fernando Herrera
no flags Details | Diff | Review
patch3 (6.06 KB, patch)
2011-09-21 04:38 PDT, alexander :surkov
no flags Details | Diff | Review
patch4 (6.36 KB, patch)
2011-09-21 04:43 PDT, alexander :surkov
mzehe: review+
enndeakin: review+
Details | Diff | Review

Description Joanmarie Diggs 2010-02-13 19:38:15 PST
Created attachment 426873 [details]
test case

1. Launch Accerciser. 

2. Open the attached test case, tabbing.html, in Firefox 3.6 from 11th June or later. Enable caret navigation by pressing F7 if it is not already enabled.

3. Press Tab to move focus to the 'First' link.

4. In Accerciser, in the tree of accessibles on the left, locate the second heading. With it selected in the tree, type the following in Accerciser's IPython Console to place the caret at the beginning of 'Second Heading':

  acc.queryText().setCaretOffset(0)

5. Alt+Tab to give focus back to the Firefox window and press Tab to move to the next link with respect to your current position.

Expected results: The 'Third' link becomes focused, because your current position is in 'Second Heading'.

Actual results: The 'Second' link becomes focused.

Note: If you use mouse-clicking or the arrows to position the caret instead of setCaretOffset(), things work as expected.

Impact: Users can no longer efficiently use "structural navigation" within content (e.g. Jump to the third section of content and then tab amongst the links there). In addition, due to problems within Gecko's native caret navigation, Orca has had to roll its own caret navigation for Firefox content. This regression means that Orca users cannot even reliably arrow within content and then use Tab to move to the next focusable object with respect to their current position. For these reasons, I'm marking the severity as 'major'.

Regression Window:

Works as expected up through:
Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.2a1pre) Gecko/20090610 Minefield/3.6a1pre

Fails from this build to the present:
Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.2a1pre) Gecko/20090611 Minefield/3.6a1pre
Comment 1 Marco Zehe (:MarcoZ) 2010-02-14 22:55:28 PST
This sounds like another regression from bug 178324, similar to bug 542313.
Comment 2 Steve Holmes 2010-02-14 23:20:14 PST
Adding myself to CC list.
Comment 3 Bill Cox 2010-04-02 01:04:02 PDT
I've verified the behavior Joanmarie describes with her test case.  When I tab to the "First" link it is highlighted with a dotted line appears as a box around the link, and the caret is shown before the F in First.  If I press the down arrow, the highlight on the "First" link goes away, and the caret is positioned before the S in "Second Heading".  However, if I use the setCaretOffset() command to move, rather than the down button, I get a different result.  The "First" link is still highlighted, but the caret is now shown at the start of the "Second Heading" line.

I believe the correct behavior of setCaretOffset() should cause the active link to become non-active.
Comment 4 Bill Cox 2010-04-02 02:44:30 PDT
I have verified that this bug is caused by the cabb8925dcd3 commit, which refactors focus handling.  Unfortunately, it modified over 100 files, so it's still not well narrowed down!
Comment 5 Bill Cox 2010-04-02 12:59:56 PDT
Created attachment 436749 [details] [diff] [review]
hg diff vs trunk - should work with patch
Comment 6 Bill Cox 2010-04-02 13:04:21 PDT
I've located the problem, and think I understand it.  It seems that TakeFocus() fails to work when called from setCaretOffsetCB, because it doesn't want to set to focus on things you can't tab to.  I've added a flag to SetFocus called movedFocus, which basically forces the focus to be set.  This is only done in calls from setCaretOffsetCB.  Several files needed to be patched to deal with the new parameter, but the change really is that simple.  One side effect is that the block of text being read by Orca is now highlighted with a box around it.  I really like this.  Apple does this with their screen reader as well.
Comment 7 Bill Cox 2010-04-03 01:36:15 PDT
Created attachment 436850 [details] [diff] [review]
Patch to fix Orca navigation

This patch can be applied against 40138:fa8b5b822730.  The previous patch had a goober that's fixed in this one.
Comment 8 Bill Cox 2010-04-04 03:12:24 PDT
Created attachment 436913 [details] [diff] [review]
Patch to fix Orca navigation

This patch restricts changes to behavior to just calls to setCaretOffsetCB.  The focus is forced, even though the caret may have moved to a non-tabbable element.  This allows Orca's structural navigation to work with Firefox 3.6 and newer.
Comment 9 David Bolter [:davidb] 2010-04-05 16:05:24 PDT
Bill thanks very much for your patch! I am cc'ing some additional folks who should probably take a look. It isn't clear to me whether setting the focus is the right thing to do.
Comment 10 alexander :surkov 2010-04-05 18:29:56 PDT
(In reply to comment #9)
> I am cc'ing some additional folks who
> should probably take a look.

First of all I would like to get Enn's attention since this is mostly focus manager changes. That was a reason I've asked Enn for feedback on previous patch.
Comment 11 Marco Zehe (:MarcoZ) 2010-04-06 01:44:46 PDT
Comment on attachment 436913 [details] [diff] [review]
Patch to fix Orca navigation

Passing on review request to Alex for the accessibility part and Neil Deakin for the rest, since he wrote the original implementation. We'd also need a Mochitest based on the testcase that exercises this code.
Comment 12 Neil Deakin 2010-04-06 08:15:22 PDT
It seems that setCaretOffset should actually just be calling nsFocusManager::MoveFocus with the nsIFocusManager::MOVEFOCUS_CARET type in order to update the focus when the caret changes. This is what happens when a cursor key is pressed.
Comment 13 Bill Cox 2010-04-06 08:23:37 PDT
I thought of that, and tried it.  Something didn't work though.  While the "First" link is unhighlighted, the tab key still causes a move to the "Second" link, rather than the link after the caret.

Sometime next week I can probably look into why the tab key moves us to the wrong link.  Do you know what function I should put a breakpoint in to see what happens when I press tab?  By the way, Neil, thank you for the feedback, and for the new focus manager.  I'm hoping your work will allow us to get rid of a lot of hacks in Orca that were needed to deal with old Firefox focus/caret issues.  I'm perfectly happy to do the debugging work if you're willing to review my patches and make suggestions.
Comment 14 Neil Deakin 2010-04-06 08:34:27 PDT
The element to tab to is determined by nsFocusManager::DetermineElementToMoveFocus
Comment 15 Bill Cox 2010-04-23 14:01:07 PDT
Created attachment 441149 [details] [diff] [review]
Patch based on using MoveFocus instead of TakeFocus

This patch works much better than the one where I forced the focus.  The change is also localized to one function in nsHyperTextAccessible.cpp, rather than several files.  Basically, rather than TakeFocus, I used MoveFocus with the nsIFocusManager::MOVEFOCUS_CARET flag as Neal suggested.  I watched how MoveFocus gets called when I move with arrow keys, and I called it the same way.  It relies on there being a selection, so rather than calling it before setting the selection where we use to call TakeFocus, I call MoveFocus after selection.

It is not clear to me if this change belongs in the SetSelectionRange function, or in SetCaretOffset, which simply calls SetSelectionRange.  Since the call to TakeFocus in SetSelectionRange doesn't work on non-tabbabe elements, I reasoned that the change belongs there.  However, I also see some versions of Firefox have removed the call to TakeFocus completely, which may make sense.  I made my changes to changeset 41170:020a0670ef30, which is tagged "tip".  If this patch looks like the right approach, I could use a pointer to how to Mozilla-fy it, and resubmit the patch.
Comment 16 Marco Zehe (:MarcoZ) 2010-04-25 22:22:22 PDT
Bill, please ask review on the patch from the appropriate peers/module owners. Also, mark the other patch from april 4 as obsoleted so everybody knows which patch to work on. Thanks!
Comment 17 Bill Cox 2010-04-25 23:06:24 PDT
Created attachment 441434 [details] [diff] [review]
More properly formatted patch to fix set_caret_position

This is the same as the previous patch, but formatted properly.  I've had time to do more user-testing, and I've made a similar patch to the Ubuntu Firefox 3.6.5 package and uploaded it for testing by Vinux users.  This patch does seem to fix this bug.
Comment 18 Marco Zehe (:MarcoZ) 2010-04-26 01:10:47 PDT
Comment on attachment 441434 [details] [diff] [review]
More properly formatted patch to fix set_caret_position

A few comments:
>+  /* Now that selection is done, move the focus to the selection. */
For single-line comments, please use // comment style.

>+    nsIDOMElement* aElement;
>+    fm->MoveFocus(window, nsnull, nsIFocusManager::MOVEFOCUS_CARET, 0, &aElement);

This is a local variable, not a parameter passed into this function. Therefore, it should not be aElement.

Requesting review from Surkov, but leaving review request for Neil Deakin intact for now for correct focusManager usage.
Comment 19 alexander :surkov 2010-04-26 01:54:48 PDT
(In reply to comment #15)

>  Since the call to
> TakeFocus in SetSelectionRange doesn't work on non-tabbabe elements, I reasoned
> that the change belongs there.

I think TakeFocus() should work on focusable element, it shouldn't depend whether element is focusable or not.

> This patch works much better than the one where I forced the focus.  The change
> is also localized to one function in nsHyperTextAccessible.cpp, rather than
> several files.  Basically, rather than TakeFocus, I used MoveFocus with the
> nsIFocusManager::MOVEFOCUS_CARET flag as Neal suggested.  I watched how
> MoveFocus gets called when I move with arrow keys, and I called it the same
> way.  It relies on there being a selection, so rather than calling it before
> setting the selection where we use to call TakeFocus, I call MoveFocus after
> selection.

What's happen when caret navigation mode is off?

Bill, could you please work on mochitests as well? Btw, does a11y mochitests pass?
Comment 20 Bill Cox 2010-04-26 06:33:52 PDT
> For single-line comments, please use // comment style.

Oops!  Sorry... I write a lot of C.

> This is a local variable, not a parameter passed into this function.
> Therefore, it should not be aElement.

I am confused abut the function of this parameter, and was trying to pass a dummy argument.  However, elsewhere I see the parameter declared like this:

nsCOMPtr<nsIDOMElement> result;

And passed with an expression like getter_"AddRefs(result)".  I will change the patch to do this, but I don't know what it does!

> I think TakeFocus() should work on focusable element, it shouldn't depend
> whether element is focusable or not.

I agree that TakeFocus should work on elements that aren't considered focusable.  However, I have traced through the call to TakeFocus many times, and can confidently say that it does not take the focus on non-tabbable elements.

In layout/generic/nsFrame.cpp, in method nsIFrame::IsFocusable, the line:

    isFocusable = mContent->IsFocusable(&tabIndex);

returns false, which causes TakeFocus to do nothing.  My first patch that mostly worked was to add a force-focus flag to TakeFocus, which forced the focus onto unfocusable elements.  The result was that paragraphs were highlighted with a box around them when navigating, which I quite like, but it is different behavior than we expect in Firefox.  It is this inability for TakeFocus to focus on the unfocusable elements that causes this bug.  Neil suggested MoveFocus, as that's what's used when moving the caret normally.  I was able to confirm that Niel was correct, and I added a MoveFocus command that is similar to the one that causes the focus to change due to normal cursor movement.

> What's happen when caret navigation mode is off?

I assume you mean when press F7 to hide the caret, but still using Orca.  It behaves exactly the same way, but I can't see the caret.  The bug is still present.  If you mean Orca Caret navigation off, the bug is also present with the same behavior.  If you mean when Orca isn't running, I'm not able to reproduce the bug because I can't do structural navigation.  However, when the cursor moves from a link into non-focusable text, the link is unhighlighed, and wherever the caret moves, if I press Tab, I go to the link after the caret.  With Orca running, the link stays focused, and unless I highlight another link, Tab takes me to the link after the focused one.  With the change to MoveFocus, the behavior becomes the same as when not running Orca.

> Bill, could you please work on mochitests as well? Btw, does a11y
> mochitests pass?

Sure, but first I'll have to figure out what mochitests are!  I'm sure Google will show me the pages I need to read.  I'll post back here when I find out what happens in mochitests, and if this patch causes no failures, I'll start working on a new mochitest for this bug.
Comment 21 alexander :surkov 2010-04-26 07:05:13 PDT
(In reply to comment #20)

> I agree that TakeFocus should work on elements that aren't considered
> focusable.  

If TakeFocus would work as expected then perpahs you don't need to fix SetSelectionBounds.

> > Bill, could you please work on mochitests as well? Btw, does a11y
> > mochitests pass?
> 
> Sure, but first I'll have to figure out what mochitests are!  I'm sure Google
> will show me the pages I need to read.  I'll post back here when I find out
> what happens in mochitests, and if this patch causes no failures, I'll start
> working on a new mochitest for this bug.

You will find our mochitests in accessible/tests/mochitest. You could change test_text_caret.html file to test nsIAccessibleText::caretOffset for this case. Please catch us on irc to ask futher questions or file them here.
Comment 22 alexander :surkov 2010-04-26 07:06:55 PDT
Enn, how can we focus the focusable element? Currently we do fm->SetFocus(element, 0) and Bill pointed it doesn't work on non-tabbable elements (please see comment #20 for details).
Comment 23 Neil Deakin 2010-04-26 11:17:33 PDT
(In reply to comment #22)
> Enn, how can we focus the focusable element? Currently we do
> fm->SetFocus(element, 0) and Bill pointed it doesn't work on non-tabbable
> elements (please see comment #20 for details).

You don't.

Could you give an example which describes what you need to do?
Comment 24 alexander :surkov 2010-04-26 19:18:12 PDT
(In reply to comment #23)
> (In reply to comment #22)
> > Enn, how can we focus the focusable element? Currently we do
> > fm->SetFocus(element, 0) and Bill pointed it doesn't work on non-tabbable
> > elements (please see comment #20 for details).
> 
> You don't.
> 
> Could you give an example which describes what you need to do?

I think focusable assumes it can be focused I think. AT should expect that. Other reason not-tabbable but focusable element could have keys processing like tabs in addons manager aren't tabbable but if the tab is focused (by click) then you can use right/left arrows to change panels. That's might be bad design. But since focused elements can have own keyboard processing and since it's very confusing focusable element what can't be focused then we need to fix it I think.
Comment 25 Neil Deakin 2010-04-27 04:44:34 PDT
Are you asking about elements that can be focused but not in the tab order? That can be adjusted by setting the element's tabindex attribute.

Note that Mozilla will only ever fire key events at focused elements.
Comment 26 alexander :surkov 2010-04-27 04:58:21 PDT
(In reply to comment #25)
> Are you asking about elements that can be focused but not in the tab order?
> That can be adjusted by setting the element's tabindex attribute.

case by case?

> Note that Mozilla will only ever fire key events at focused elements.

If focusable element that is not in tab order is clicked then will it stay focused?
Comment 27 David Bolter [:davidb] 2010-04-27 06:16:19 PDT
(In reply to comment #26)
> (In reply to comment #25)

> > Note that Mozilla will only ever fire key events at focused elements.
> 
> If focusable element that is not in tab order is clicked then will it stay
> focused?

Example:
<div tabindex="-1">I can be focused if you click on me</div>

Alexander, is this what you mean?
Comment 28 Bill Cox 2010-04-27 08:02:58 PDT
I've run the mochitest-a11y tests, with "make -C objdir-ff-release mochitest-a11y", and I see that at least two tests fail in caret related code.  There were 10 failures before my change, that I assume are just current problems unrelated to my work.  I'm running this on revision 41318:77cb2107b06c.

I will track down why the other tests are failing, and that may give me some insight into what the right fix is.  However, I have to table this work for two or three weeks, to work on the Vinux 3.0 release.  The patch is being tested by Vinux users now, and so far they haven't found problems related to it, so we'll go with this for now, and I'll get back to this bug after the 3.0 release.
Comment 29 alexander :surkov 2010-04-27 17:03:04 PDT
(In reply to comment #27)

> > If focusable element that is not in tab order is clicked then will it stay
> > focused?
> 
> Example:
> <div tabindex="-1">I can be focused if you click on me</div>
> 
> Alexander, is this what you mean?

If it's not in tab order then yes. Or it could be XUL toolbarbutton I guess.
Comment 30 Neil Deakin 2010-04-27 19:37:52 PDT
(In reply to comment #29)
> > Example:
> > <div tabindex="-1">I can be focused if you click on me</div>
> > 
> > Alexander, is this what you mean?
> 
> If it's not in tab order then yes. Or it could be XUL toolbarbutton I guess.

So what is your question? David's example is an element that can be focused but is skipped when pressing Tab to modify the focus. It can be focused via element.focus() or if mouse-clicking on it would do so on the platform.

Is your problem that accessibility wants to use tab navigation? No accessibility code is doing so currently that I can see. It would be calling nsIFocusManager::MoveFocus with the forward/backward type if it was.

I do however see code using nsIFocusManager::SetFocus which will focus an element normally.
Comment 31 alexander :surkov 2010-04-27 20:37:57 PDT
Enn, I must misread the bug. Sorry. The first question is:

Let the caret navigation mode is on, some focusable element is focused. We need to move the caret into heading (HTML h1 for example) and make document focused.

We do
1) nsIFocusManager::SetFocus on the non focusable element
2) Change ranges of related nsISelection object.

I think existing code should work well for HTML input but it appears it doesn't work on rich text elements (like heading) when caret navigation mode is on. What should we do to get desired result?

The second question is: when nsIFocusManager::SetFocus is called on HTML button while focus is in another window then it seems the window containing HTML button is not focused. Is that expected?
Comment 32 Neil Deakin 2010-04-28 05:07:15 PDT
(In reply to comment #31)
> Let the caret navigation mode is on, some focusable element is focused. We need
> to move the caret into heading (HTML h1 for example) and make document focused.

Moving the caret and then using the suggestion I made in comment 12 will update the focus accordingly. The latest patch appears to do that.


> We do
> 1) nsIFocusManager::SetFocus on the non focusable element

That will and should do nothing.


> 2) Change ranges of related nsISelection object.
> 
> I think existing code should work well for HTML input but it appears it doesn't
> work on rich text elements (like heading) when caret navigation mode is on.
> What should we do to get desired result?

How does it not work?

I don't know how the specifics of how the caret is moved. I only know that the caret is moved and the focus updated to match (this is done in nsSelectMoveScrollCommand::DoCommandBrowseWithCaretOn)


> The second question is: when nsIFocusManager::SetFocus is called on HTML button
> while focus is in another window then it seems the window containing HTML
> button is not focused. Is that expected?

The document.activeElement (of the document the button is in) should be set to that button. The window will not be made frontmost unless the raise flag is passed to SetFocus.
Comment 33 Neil Deakin 2010-05-06 08:08:46 PDT
Comment on attachment 441434 [details] [diff] [review]
More properly formatted patch to fix set_caret_position

>+  if (fm) {
>+    nsCOMPtr<nsIPresShell> shell = GetPresShell();
>+    nsCOMPtr<nsIDocument> doc = shell->GetDocument();
>+    nsCOMPtr<nsPIDOMWindow> window = doc->GetWindow();

Do any of these need to be null-checked? If not, I would just get them in one line and not store them in comptrs.


>+    nsIDOMElement* aElement;
>+    fm->MoveFocus(window, nsnull, nsIFocusManager::MOVEFOCUS_CARET, 0, &aElement);

This will leak aElement. You do need to use a comptr here.

The other similar places where the focus is updated to the caret position also use nsIFocusManager::FLAG_NOSCROLL. Is this an intentional difference?
Comment 34 Bill Cox 2010-05-27 10:44:10 PDT
Created attachment 447784 [details] [diff] [review]
Modified patch based on Neil's feedback

Fixed memory leak as Neil suggested.  I wasn't sure if all these objects would be valid, so instead of in-lining the access, I added checks after each.  If someone understands the context for when this function will be called better, I could potentially remove checks and in-line some.  In my light testing, there were no warnings generated by these checks.  I still need to check out why two regressions failed.  I'll work on that next.
Comment 35 Bill Cox 2010-05-27 11:05:19 PDT
With the new patch, applied to 42554:88a6e0534e03 tip, there are no new failures in the a11y tests.  The errors that do occur result in identical log summaries.  I'll get to work on a new mochitest now.
Comment 36 Bill Cox 2010-05-28 09:35:29 PDT
Created attachment 448030 [details] [diff] [review]
Patch, including a new mochitest test case

I've added a mochitest test case in this patch.  Otherwise, it is identical to the previous, which tries to incorporate feedback from Neil's last post.  I've verified that this test case fails without the patch to nsHyperTextAccessible.cpp, and that it passes with the patch.  I've added the new testcase to accessible/tests/mochitest/Makefile.in, and reran the a11y mochitests.  Everything seems to be working.
Comment 37 Attila Hammer 2010-06-01 22:05:28 PDT
Dear developers,

Because if I known right, Firefox 3.6.4 version coming out next week, i have a question only:
Not possible to add Bill already doed caret navigation related patch if not have need another work with need doing this related patch? This modification is very important for visual impaired users.
Bill, in Vinux/vinux-lucid repository, the modifyed 3.6.3 Firefox package what patch use?

If my request is impossible now, I agree, because I am not a Mozilla developer, but lot of visual impaired users very wait this bug fix. In future, the final patch is committed in 3.6.x series, or only 3.7 branch?

Sorry my possible silly questions,

Attila
Comment 38 alexander :surkov 2010-06-02 03:38:14 PDT
The patch doesn't have proper reviews still, wasn't landed on trunk. Usual practice is when patch was landed on trunk and no regressions weren't found during some time then we request an approval to land it on branch. Sometimes approval request consideration takes significant time from mozilla drivers, thus it's nearly impossible to include it into ongoing firefox release I think.
Comment 39 Attila Hammer 2010-06-02 04:15:48 PDT
Hy,

Thank you Alex the answer. I agree.

Attila
Comment 40 alexander :surkov 2010-06-15 00:04:24 PDT
Comment on attachment 448030 [details] [diff] [review]
Patch, including a new mochitest test case


>+  /* Now that selection is done, move the focus to the selection. */
>+  nsCOMPtr<nsIFocusManager> fm = do_GetService(FOCUSMANAGER_CONTRACTID);
>+  if (fm) {
>+    nsCOMPtr<nsIPresShell> shell = GetPresShell();
>+    NS_ENSURE_TRUE(shell, NS_ERROR_FAILURE);
>+    nsCOMPtr<nsIDocument> doc = shell->GetDocument();
>+    NS_ENSURE_TRUE(doc, NS_ERROR_FAILURE);

It's preferable to add GetDocumentNode() method to nsAccessNode and use it here

>+  <script type="application/javascript">
>+    function testFocus(aID, aAcc, aSelectedBefore, aSelectedAfter)
>+    {

nit: you could merge aID and aAcc

>+      is(aAcc.selected, aSelectedBefore,
>+         "Wrong selected state before focus for ID " + aID + "!");
>+      document.getElementById(aID).focus();
>+      is(aAcc.selected, aSelectedAfter,
>+         "Wrong seleccted state after focus for ID " + aID + "!");
>+    }
>+
>+    function doTest()
>+    {
>+      //////////////////////////////////////////////////////////////////////////
>+      // normal hyperlink
>+      var link4 = getAccessible("link4", [nsIAccessibleHyperLink]);
>+      testFocus("link4", link4, false, true);
>+      heading1 = getAccessible("heading1", [nsIAccessibleText]);
>+      heading1.caretOffset = 3;
>+      testFocus("link4", link4, false, true);

I'm not sure what you test here, it looks like you ensure caretOffset removes the focus from link. Can you describe please what you want to check here?
Comment 41 Bill Cox 2010-06-15 00:45:30 PDT
Hi, Alexander.  Yes, I am testing that caretOffset removes the focus from link4.  Without this patch, Firefox leaves the focus on link4 while moving the caret to heading1, so the second time we call testFocus, the test fails.

With this patch in place, we've still found focus related issues in Firefox, so there's more work to do.  For example, when the Firefox window is activated, there is often no focus event generated by Firefox to tell Orca what has the focus.  It would be great to get this patch into the hands of the Orca guys so we could start tracking down the next set of bugs.  In any case, I'll take a look at adding a GetDocumentNode method to nsAccessNode, and I can remove the aAcc parameter.

Thanks,
Bill
Comment 42 alexander :surkov 2010-06-15 01:06:47 PDT
(In reply to comment #41)
> Hi, Alexander.  Yes, I am testing that caretOffset removes the focus from
> link4.  Without this patch, Firefox leaves the focus on link4 while moving the
> caret to heading1, so the second time we call testFocus, the test fails.
> 
> With this patch in place, we've still found focus related issues in Firefox, so
> there's more work to do.  For example, when the Firefox window is activated,
> there is often no focus event generated by Firefox to tell Orca what has the
> focus.  It would be great to get this patch into the hands of the Orca guys so
> we could start tracking down the next set of bugs. 

Is "heading1" getting focus event when you set caret offset on it? If it is then I would like to get focus event testing as well (you could use eventQueue object from events.js).
Comment 43 Bill Cox 2010-06-15 04:34:45 PDT
I'll have to look into this, but my guess is that in FF 3.5, yes, it got focus events, and that after the large focus manager rewrite, no it does not.  I think this behaviour change is also the cause of some of the other issues we're seeing in FF, even with this patch.  However, from the discussion above, it's not clear if it's a bug or not for "heading1" to receive focus events.  That's why I didn't test for it, as the correct behaviour still sounds in dispute.  I'll find time this week to verify all this.
Comment 44 alexander :surkov 2010-06-15 05:20:22 PDT
Comment on attachment 448030 [details] [diff] [review]
Patch, including a new mochitest test case


>+  /* Now that selection is done, move the focus to the selection. */

nit: use //

>+		test_set_caret_offset.html \

it makes sense to reuse test_text_caret.html

>+  <script type="application/javascript">
>+    function testFocus(aID, aAcc, aSelectedBefore, aSelectedAfter)
>+    {
>+      is(aAcc.selected, aSelectedBefore,
>+         "Wrong selected state before focus for ID " + aID + "!");

I find useful to test nsIAccessibleText::selected but I'd like to see a comment we test here whether it's focused or not.

>+      document.getElementById(aID).focus();
>+      is(aAcc.selected, aSelectedAfter,
>+         "Wrong seleccted state after focus for ID " + aID + "!");
>+    }
>+
>+    function doTest()
>+    {
>+      //////////////////////////////////////////////////////////////////////////
>+      // normal hyperlink
>+      var link4 = getAccessible("link4", [nsIAccessibleHyperLink]);
>+      testFocus("link4", link4, false, true);
>+      heading1 = getAccessible("heading1", [nsIAccessibleText]);
>+      heading1.caretOffset = 3;
>+      testFocus("link4", link4, false, true);

technically you could replace it (see test_text_caret.html) when you add new invoker object for this test:

invoke function:

focus link4
set caret offset for heading

event seq:
caret change (expected, this.eventSeq)
focus (not expected, this.unexpectedEventSeq) and please add todo(false, ) so we can get back later to this to decide if we need to fire focus event here

check function:

caret offset for heading
link4 should stay focused

please rerequest review when comments are addressed
btw, thank you for working on this!
Comment 45 Bill Cox 2010-06-15 07:10:01 PDT
Ok, I'll try and update the patch to address these comments.  I'll also investigate the old and new focus behaviour.  One thing I'm fairly confident of however, is that after setting the caret offset in heading1, the test4 link should not (and does not after the patch) have the focus.  Where the focus actually goes, I don't know!  But, I'll find out.
Comment 46 Joanmarie Diggs 2010-07-28 23:15:26 PDT
(In reply to comment #45)
> Ok, I'll try and update the patch to address these comments.

Bill, any progress on this? As you know, this bug keeps biting us (Orca community) in the arse. :-( It would be beyond awesome if we could get it resolved once and for all. Thanks for your work!
Comment 47 Joanmarie Diggs 2010-09-14 04:35:05 PDT
Ping?
Comment 48 Bill Cox 2010-12-01 08:56:41 PST
Just tested this issue in Ubuntu Maverick, and it's still there.  Isn't there someone at Mozilla who recently has been tasked to enhance gecko accessibility? Here would be an excellent place to start.  I've had some health issues, and haven't been involved since September...
Comment 49 alexander :surkov 2010-12-01 09:03:29 PST
Bill, thanks for bringing an attention to this, again. We have technical description of the problem and have a patch. It should be easy enough to finish it. 

Fernando, wanna take a look?
Comment 50 David Bolter [:davidb] 2010-12-02 06:29:20 PST
Approved blocking. Ubuntu bug says "Without this patch, Firefox is basically unusable". Making Fernando the owner at least for now. Bill, thanks again for all your work here.
Comment 51 Fernando Herrera 2010-12-02 19:32:35 PST
Created attachment 494926 [details] [diff] [review]
patch2

I have updated the patch moving the test case to test_text_caret.html and using eventQueue.

I also added code to remove all selections before moving the caret, it makes sense because that is what happens when moving it with the cursor (also is the way to get the caret-moved event fired: updating the selections).

Also, getCaretOffset was failing after a setCaretOffset because of the check "No caret if the focused node is not inside this DOM node and this DOM node is not inside of focused node". For that I'm just doing gLastFocusedNode = GetNode() when setting the caret, but I'm not sure about that solution.

Notice that tests are failing to check that link4 is still focused (MoveFocus will clear the focus).
Comment 52 Fernando Herrera 2011-01-22 13:12:07 PST
Before I lost this patch surkov and I did some time ago, I'm attaching it.

The idea is to clear the focus on every blur event, cleaning our global gLastFocusedNode. And for nsHyperTextAccessible::SetSelectionRange still do the ClearFocus().

However we would need to rework how popup/menu end events are fired.
Comment 53 Fernando Herrera 2011-01-22 13:12:48 PST
Created attachment 506135 [details] [diff] [review]
Idea
Comment 54 David Bolter [:davidb] 2011-03-08 08:28:21 PST
(In reply to comment #52)
> However we would need to rework how popup/menu end events are fired.

Got a plan? :)
Comment 55 alexander :surkov 2011-03-09 00:24:57 PST
make sure to fix regressions in fx5
Comment 56 alexander :surkov 2011-07-01 02:58:51 PDT
(In reply to comment #52)
> Before I lost this patch surkov and I did some time ago, I'm attaching it.
> 
> The idea is to clear the focus on every blur event, cleaning our global
> gLastFocusedNode. And for nsHyperTextAccessible::SetSelectionRange still do
> the ClearFocus().
> 
> However we would need to rework how popup/menu end events are fired.

Honestly I don't recall my complicity in this idea development for this bug and failed to see how it works to make this bug fixed. Maybe it's indented for something else, I'm not sure.
Comment 57 David Bolter [:davidb] 2011-07-14 06:29:47 PDT
Cc'ing Trevor for serendipity.

Fernando, a friendly ping. Can you address comment 56 when you get a chance?
Comment 58 alexander :surkov 2011-07-24 23:34:27 PDT
(In reply to comment #56)
> (In reply to comment #52)
> > Before I lost this patch surkov and I did some time ago, I'm attaching it.
> > 
> > The idea is to clear the focus on every blur event, cleaning our global
> > gLastFocusedNode. And for nsHyperTextAccessible::SetSelectionRange still do
> > the ClearFocus().
> > 
> > However we would need to rework how popup/menu end events are fired.
> 
> Honestly I don't recall my complicity in this idea development for this bug
> and failed to see how it works to make this bug fixed. Maybe it's indented
> for something else, I'm not sure.

Got it, maybe that was *this* bug that we talked about this solution for since this makes sure we fire focus event for document accessible when caret moves to non focusable content. Though we should deal with it separately since this bug can be reproduced easily out of this bug context (for example, shift tab from focused first focusable element).
Comment 59 alexander :surkov 2011-09-21 04:38:03 PDT
Created attachment 561436 [details] [diff] [review]
patch3
Comment 60 alexander :surkov 2011-09-21 04:43:04 PDT
Created attachment 561438 [details] [diff] [review]
patch4

correct patch
Comment 61 Marco Zehe (:MarcoZ) 2011-09-21 04:58:59 PDT
Comment on attachment 561438 [details] [diff] [review]
patch4

>+      //gQueue.push(new setCaretOffsetInvoker("link", 1));

Why is this currently commented out?

And you need caret browsing enabled to see caretmove events, right?
Comment 62 alexander :surkov 2011-09-21 05:05:22 PDT
(In reply to Marco Zehe (:MarcoZ) from comment #61)
> Comment on attachment 561438 [details] [diff] [review]
> patch4
> 
> >+      //gQueue.push(new setCaretOffsetInvoker("link", 1));
> 
> Why is this currently commented out?

testing artifact

> And you need caret browsing enabled to see caretmove events, right?

yes
Comment 63 Marco Zehe (:MarcoZ) 2011-09-21 05:41:23 PDT
Comment on attachment 561438 [details] [diff] [review]
patch4

r=me.
Comment 64 Neil Deakin 2011-09-21 06:37:22 PDT
Comment on attachment 561438 [details] [diff] [review]
patch4

Not sure what you're asking me to review but the call to MoveFocus looks ok.

The FLAG_BYMOVEFOCUS won't flag actually do anything here.
Comment 65 alexander :surkov 2011-09-21 07:01:20 PDT
(In reply to Neil Deakin from comment #64)
> Comment on attachment 561438 [details] [diff] [review]
> patch4
> 
> Not sure what you're asking me to review but the call to MoveFocus looks ok.
> 
> The FLAG_BYMOVEFOCUS won't flag actually do anything here.

Yes, for flags. I removed frameSelection->ScrollSelectionIntoView call. Do I understand right that MoveFocus will do that?
Comment 66 alexander :surkov 2011-10-15 20:40:10 PDT
inbound land https://hg.mozilla.org/try/rev/81d9a36b80b3
Comment 67 :Ehsan Akhgari (busy, don't ask for review please) 2011-10-16 10:52:33 PDT
https://hg.mozilla.org/mozilla-central/rev/0a6b707742dd
Comment 68 Launchpad 2011-10-17 03:43:40 PDT
Attila Hammer added the following comment to Launchpad bug report 602877:

The committed fix will be awailable in Firefox 3.6 for example with 
Lucid release future?

Attila


-- 
http://launchpad.net/bugs/602877
Comment 69 alexander :surkov 2011-10-17 03:50:46 PDT
(In reply to Launchpad from comment #68)

> The committed fix will be awailable in Firefox 3.6 for example with 
> Lucid release future?

No, it'll be available for Firefox 10. It's tricky to port it on earlier releases because it's based on number of other fixes which are likely not going be ported too.

Note You need to log in before you can comment on or make changes to this bug.