399427 - TAB always jumps from XUL menulists to the URL input field

Status: RESOLVED FIXED

(Core :: XUL, defect, P2)

Description

[Milan Zamazal]

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

Comment 1

[Neil Deakin]

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

Comment 2

[[e-mail]]

Attachment: Possible patch

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

Comment 3

[Neil Deakin]

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

Comment 4

[Aaron Leventhal]

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?

Comment 5

[Milan Zamazal]

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

Comment 6

[Neil Deakin]

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

Comment 7

[Aaron Leventhal]

Neil D, I don't completely follow.

Comment 8

[Neil Deakin]

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

Comment 9

[Aaron Leventhal]

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.

Comment 10

[[e-mail]]

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

Comment 11

[Aaron Leventhal]

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

Comment 12

[[e-mail]]

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.

Comment 13

[Neil Deakin]

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

Comment 14

[Aaron Leventhal]

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.

Comment 17

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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

Comment 18

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 19

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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

Comment 20

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 21

[Damon Sicore (:damons)]

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

Comment 22

[Neil Deakin]

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

This patch will cause a crash, see bug 414018.

Comment 23

[Damon Sicore (:damons)]

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

Wanted1.9.0.x+.

Comment 24

[[e-mail]]

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

The cause of that crash was fixed in bug 414338.

Comment 25

[Christophe Charron]

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.

Comment 27

[Scott Turner]

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.

Comment 28

[Mike Beltzner [:beltzner, not reading bugmail]]

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

Comment 29

[Neil Deakin]

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

Comment 30

[Neil Deakin]

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.

Comment 31

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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

Comment 32

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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?

Comment 33

[Neil Deakin]

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.

Comment 34

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 35

[Neil Deakin]

Attachment: as suggested

Comment 36

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 37

[Neil Deakin]

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

Comment 38

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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

Comment 39

[Neil Deakin]

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.

Comment 40

[Neil Deakin]

Checked this back in to 1.9.1 again

Comment 41

[Dusan Halicky]

Still not work in:

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

Comment 42

[Dusan Halicky]

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

Comment 43

[Firefox Bug Husbandry Bot]

https://bugzilla.mozilla.org/show_bug.cgi?id=1527381