Closed Bug 399427 Opened 14 years ago Closed 13 years ago

TAB always jumps from XUL menulists to the URL input field

Categories

(Core :: XUL, defect, P2)

x86
Linux
defect

Tracking

()

RESOLVED FIXED
mozilla1.9.1

People

(Reporter: pdm, Assigned: enndeakin)

References

()

Details

(Keywords: access, fixed1.9.1, regression)

Attachments

(1 file, 1 obsolete file)

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".
Component: Disability Access → Keyboard: Navigation
Product: Firefox → Core
QA Contact: disability.access → keyboard.navigation
Component: Keyboard: Navigation → XP Toolkit/Widgets: XUL
QA Contact: keyboard.navigation → xptoolkit.xul
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


Status: UNCONFIRMED → NEW
Ever confirmed: true
Attached patch Possible patch (obsolete) — Splinter Review
This fixes it for me but I don't know whether it's the right fix...
Looks good here. I guess Aaron should take a look at this.
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?
The bug doesn't occur when the XUL file is opened as chrome, i.e. with open(..., ..., "chrome").
(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?

Neil D, I don't completely follow.
(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.
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.
I don't know which Neil you're asking but I've got no objection to disabling caret browsing in content XUL documents.
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).

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.
Do we want to use this patch here, or create another one to disable caret browsing for xul documents?
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.
Duplicate of this bug: 426599
427838 was blocking so I'm moving its blocking status here, given that we have more debugging and a patch here.
Flags: blocking1.9+
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.
So disabling caret browsing for XUL docs is not going to help.
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.
Who's going to assess the risk?  Neil Deakin?  Aaron? Let's get an answer ASAP.  Time has run out.
Attachment #286334 - Flags: review?(enndeakin)
Whiteboard: [needs review enndeakin]
Comment on attachment 286334 [details] [diff] [review]
Possible patch

This patch will cause a crash, see bug 414018.
Attachment #286334 - Flags: review?(enndeakin) → review-
Whiteboard: [needs review enndeakin]
Taking this bug off the blocker list per roc's suggestion in comment 20.

Wanted1.9.0.x+.
Flags: wanted1.9.0.x+
Flags: blocking1.9-
Flags: blocking1.9+
Comment on attachment 286334 [details] [diff] [review]
Possible patch

The cause of that crash was fixed in bug 414338.
Attachment #286334 - Flags: review- → review?(enndeakin)
Component: XP Toolkit/Widgets: XUL → XUL
QA Contact: xptoolkit.xul → xptoolkit.widgets
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.
Duplicate of this bug: 459647
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.
Flags: blocking1.9.1?
Target Milestone: --- → mozilla1.9.1b2
Blocking or not, this won't block beta 2 ...
Target Milestone: mozilla1.9.1b2 → mozilla1.9.1
(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?
Flags: blocking1.9.1? → blocking1.9.1+
Priority: -- → P2
Whiteboard: [has wip patch]
Whiteboard: [has wip patch] → [needs review]
Blocks: 461981
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.
So do you think we should not block FF3.1 on this?
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?
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.
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.
Attached patch as suggestedSplinter Review
Attachment #361629 - Flags: superreview?(roc)
Attachment #361629 - Flags: review?(neil)
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.
Attachment #361629 - Flags: superreview?(roc) → superreview+
Attachment #361629 - Flags: review?(neil) → review+
Whiteboard: [needs review] → [needs landing]
Attachment #286334 - Attachment is obsolete: true
Attachment #286334 - Flags: review?(enndeakin)
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Whiteboard: [needs landing] → [needs 1.9.1 landing]
Keywords: fixed1.9.1
Whiteboard: [needs 1.9.1 landing]
I backed part of this out on 1.9.1 due to focus-related issues.
Keywords: fixed1.9.1
Whiteboard: [needs 1.9.1 landing]
Is there a bug for those issues? What were the issues?
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.
Assignee: neil → enndeakin
Checked this back in to 1.9.1 again
Keywords: fixed1.9.1
Whiteboard: [needs 1.9.1 landing]
Still not work in:

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1b3) Gecko/20090305 Firefox/3.1b3
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
You need to log in before you can comment on or make changes to this bug.