Milan Zamazal Reporter
	Description • 17 years ago

User-Agent:       Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv:1.9a9pre) Gecko/2007100814 Minefield/3.0a9pre
Build Identifier: Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv:1.9a9pre) Gecko/2007100814 Minefield/3.0a9pre

When using the TAB key to navigate over a XUL application elements and reaching the first <menulist> element, the next TAB key press jumps to the top URL input field instead of the next XUL element.  Please note this is a serious problem as handicapped users (e.g. visually impaired users) can't navigate the XUL window with keyboard and its contents after the first <menulist> element may be completely unreachable for them.

Reproducible: Always

Steps to Reproduce:
1. Go to the URL given above.
2. Press the TAB key several times until you reach the menulist on the page ("item 1").
3. Press the TAB key once more.
Actual Results:  
Focus jumps to the URL input field.

Expected Results:  
Focus should jump to the next XUL element, i.e. the button labeled "Another button".

	Neil Deakin Assignee
	Comment 1 • 17 years ago

This is because of the caret browsing or something which causes the focus to track the caret. Why is that a problem, when there aren't any elements that use a caret in the testcase, you ask? Why, because in addition to the focus tracking the caret, the caret also tracks the focus.

Unfortunately, the caret is set using ranges, and is being set to the menupopup inside the menulist. So, when tab is pressed, it skips out thinking that focus is inside a closed popup.

One possible fix here is to skip elements with popups at http://lxr.mozilla.org/mozilla/source/content/events/src/nsEventStateManager.cpp#5255

	neil@parkwaycc.co.uk
	Comment 2 • 17 years ago

This fixes it for me but I don't know whether it's the right fix...

	Neil Deakin Assignee
	Comment 3 • 17 years ago

Looks good here. I guess Aaron should take a look at this.

	Aaron Leventhal
	Comment 4 • 17 years ago

Really, there's no point in having focus follow caret or caret follow focus from within XUL. Does that only happen when XUL is content or does it also happen when XUL is chrome?

	Milan Zamazal Reporter
	Comment 5 • 17 years ago

The bug doesn't occur when the XUL file is opened as chrome, i.e. with open(..., ..., "chrome").

	Neil Deakin Assignee
	Comment 6 • 17 years ago

(In reply to comment #4)
> Really, there's no point in having focus follow caret or caret follow focus
> from within XUL. Does that only happen when XUL is content or does it also
> happen when XUL is chrome?

It only applies to content shells, not chrome shells. There's a check for that earlier in the MoveCaretToFocus method. Maybe it should also only apply to those where the selection is visible and/or valid?

	Aaron Leventhal
	Comment 7 • 17 years ago

Neil D, I don't completely follow.

	Neil Deakin Assignee
	Comment 8 • 17 years ago

(In reply to comment #7)
> Neil D, I don't completely follow.

Don't follow what?

The caret tracking stuff only applies to content docshells, so XUL loaded in a browser will have this issue. XUL loaded in a chrome window will not.

	Aaron Leventhal
	Comment 9 • 17 years ago

Neil, do you think caret browsing should apply to XUL in a content doc shell? I think caret browsing should be turned off for XUL docs no matter where they are.

	neil@parkwaycc.co.uk
	Comment 10 • 17 years ago

I don't know which Neil you're asking but I've got no objection to disabling caret browsing in content XUL documents.

	Aaron Leventhal
	Comment 11 • 17 years ago

Thanks. Even if we do that, do you think we should still use your patch. It's smaller code, but I don't really understand how it works (sorry to be dense).

	neil@parkwaycc.co.uk
	Comment 12 • 17 years ago

The previous code normally moves the caret to just inside the element, unless it deems the element unsuitable in which case it moves the caret to just outside the element. My patch tries to move the caret to just outside the element, but if it can't it tries to move the caret to just inside the element.

	Neil Deakin Assignee
	Comment 13 • 17 years ago

Do we want to use this patch here, or create another one to disable caret browsing for xul documents?

	Aaron Leventhal
	Comment 14 • 17 years ago

For Firefox 3 we should just disable caret browsing for XUL docs. That's not risky and it does what we need.

As far as the original patch, we might want it later just for the fact that it reduces and simplifies code. If it is a better way to do what we do now, I think we should consider for trunk after we branch.

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 17 • 16 years ago

427838 was blocking so I'm moving its blocking status here, given that we have more debugging and a patch here.

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 18 • 16 years ago

I don't think this related to caret browsing mode. I see the bug on Windows when I'm not in caret browsing mode. When caret browsing is disabled, there's still a caret, you just can't see it.

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 19 • 16 years ago

So disabling caret browsing for XUL docs is not going to help.

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 20 • 16 years ago

I think we should take Neil's patch in comment #2, or if its risk outweighs the pain of this bug, we should just take this bug off the blocking list.

	Damon Sicore (:damons)
	Comment 21 • 16 years ago

Who's going to assess the risk?  Neil Deakin?  Aaron? Let's get an answer ASAP.  Time has run out.

	Neil Deakin Assignee
	Comment 22 • 16 years ago

Comment on attachment 286334 [details] [diff] [review]
Possible patch

This patch will cause a crash, see bug 414018.

	Damon Sicore (:damons)
	Comment 23 • 16 years ago

Taking this bug off the blocker list per roc's suggestion in comment 20.

Wanted1.9.0.x+.

	neil@parkwaycc.co.uk
	Comment 24 • 16 years ago

Comment on attachment 286334 [details] [diff] [review]
Possible patch

The cause of that crash was fixed in bug 414338.

	Christophe Charron
	Comment 25 • 16 years ago

So, if the crash cause is fixed, why don't apply the patch ?
Because of this bug, a remote app can't be seriously shown and can't be used at all.

	Scott Turner
	Comment 27 • 16 years ago

In support of Christophe Charron, we've been running FF 3.0 built with the patch as our internal remote XUL app browser and haven't seen any issues. The patch seems stable.

	Mike Beltzner [:beltzner, not reading bugmail]
	Comment 28 • 16 years ago

Blocking or not, this won't block beta 2 ...

	Neil Deakin Assignee
	Comment 29 • 16 years ago

(In reply to comment #19)
> So disabling caret browsing for XUL docs is not going to help.

I think what Aaron meant here is to not update the caret position for xul documents, which currently still occurs. (having caret browsing disabled doesn't affect it).

Neil, do you think the patch here is the right fix?

	Neil Deakin Assignee
	Comment 30 • 16 years ago

I'm generally concerned about potential regressions from this patch. I really don't know what the range code does.

This bug only applies to xul with menulists loaded into a web browser (It doesn't occur to chrome applications). The workaround for specific cases is in bug 461981. Also, this is fixed by the focus work in bug 178324.

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 31 • 16 years ago

So do you think we should not block FF3.1 on this?

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 32 • 16 years ago

It seems to me that a simple and safe fix would be to check for XUL documents instead of checking for chrome shells in MoveCaretToFocus. Then your focus work will fix this "for real". Neil, what do you think?

	Neil Deakin Assignee
	Comment 33 • 16 years ago

I think that's probably what we should do. In fact, I notice that the comment at the top of MoveCaretToFocus implies that this is supposed to be the case.

That change won't fix 461981 though, as the feed preview is xhtml with a xul menulist in it.

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 34 • 16 years ago

what if we test currentFocusNode->IsNodeOfType(eXUL)?

Also, what does this do?
    nsCOMPtr<nsIContent> selectionContent, endSelectionContent;
    nsIFrame *selectionFrame;
    PRUint32 selectionOffset;
    GetDocSelectionLocation(getter_AddRefs(selectionContent),
                            getter_AddRefs(endSelectionContent),
                            &selectionFrame, &selectionOffset);
We don't seem to use any of those values.

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 36 • 16 years ago

Comment on attachment 361629 [details] [diff] [review]
as suggested

I think we could probably remove the docshell-is-not-chrome check, but I'm happy to do that after your big focus rework is done.

	Neil Deakin Assignee
	Comment 37 • 16 years ago

I backed part of this out on 1.9.1 due to focus-related issues.

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 38 • 16 years ago

Is there a bug for those issues? What were the issues?

	Neil Deakin Assignee
	Comment 39 • 16 years ago

Sorry, I wasn't clear. It was backed due to the problems with the event suppression changes, so that it was clearer that that patch caused the problems. I plan to check this back in again for the next beta.

	Neil Deakin Assignee
	Comment 40 • 16 years ago

Checked this back in to 1.9.1 again

	Dusan Halicky
	Comment 41 • 16 years ago

Still not work in:

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1b3) Gecko/20090305 Firefox/3.1b3

	Dusan Halicky
	Comment 42 • 15 years ago

Fixed in Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.2a1pre) Gecko/20090425 Minefield/3.6a1pre

Thanks, it took you only 2 years