Position is not being updated when atk_text_set_caret_offset is used

This sounds like another regression from bug 178324, similar to bug 542313.

Adding myself to CC list.

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.

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!

Created an attachment (id=436749)
hg diff vs trunk - should work with patch

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.

Created an attachment (id=436850)
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.

Created an attachment (id=436913)
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.

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.

(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.

(From update of attachment 436913)
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.

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.

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.

The element to tab to is determined by nsFocusManager::DetermineElementToMoveFocus

Created an attachment (id=441149)
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.

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!

Created an attachment (id=441434)
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.

(From update of attachment 441434)
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.

(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?

> 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.

(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.

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).

(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?

(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.

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.

(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?

(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?

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.

(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.

(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.

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?

(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.

(From update of attachment 441434)
>+ 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?

Created an attachment (id=447784)
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.

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.

Created an attachment (id=448030)
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.

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

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.

Hy,

Thank you Alex the answer. I agree.

Attila

(From update of attachment 448030)

>+ /* 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?

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

(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).

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.

(From update of attachment 448030)

>+ /* 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!

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.

(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!

Ping?

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...

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?

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.

Created attachment 494926
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).

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.

Created attachment 506135
Idea

(In reply to comment #52)
> However we would need to rework how popup/menu end events are fired.

Got a plan? :)

make sure to fix regressions in fx5

(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.

Cc'ing Trevor for serendipity.

Fernando, a friendly ping. Can you address comment 56 when you get a chance?

(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).

Created attachment 561436
patch3

Created attachment 561438
patch4

correct patch

Comment on attachment 561438
patch4

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

Why is this currently commented out?

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

(In reply to Marco Zehe (:MarcoZ) from comment #61)
> Comment on attachment 561438
> 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 on attachment 561438
patch4

r=me.

Comment on attachment 561438
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.

(In reply to Neil Deakin from comment #64)
> Comment on attachment 561438
> 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?

inbound land https://hg.mozilla.org/try/rev/81d9a36b80b3

https://hg.mozilla.org/mozilla-central/rev/0a6b707742dd

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

Attila

(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.