306058 - tabindex attribute on XUL textbox breaks tabbing backwards

Status: RESOLVED FIXED

(Core :: XUL, defect)

Description

[Mats Palmgren (inactive)]

(Spawned from bug 305840)

tabindex attribute on XUL textbox breaks tabbing backwards.

STEPS TO REPRODUCE
1. load attached testcase
2. TAB and SHIFT-TAB

ACTUAL RESULTS
Typing SHIFT-TAB on a focused textbox we get stuck

EXPECTED RESULTS
Focus moves to previous element in tabbing order

PLATFORMS & BUILDS TESTED
Bug occurs in Firefox trunk on Linux (also after bug 305840 is fixed)
Bug does NOT occur in Mozilla 1.7.8 on Linux (it works reasonably well
anyway, tabbing forward through the document only seems to work
correctly once though)

Comment 1

[Mats Palmgren (inactive)]

Attachment: Testcase

Comment 2

[Alex Vincent [:WeirdAl]]

Something very interesting here.  Firefox 2 (Windows) has this bug, but Minefield trunk (Jan 27, Mac) doesn't.

Comment 3

[Alex Vincent [:WeirdAl]]

Firefox 2 (Mac) isn't affected either.  So this is a Windows bug and a Linux bug, but NOT a Mac bug.

Comment 4

[Alex Vincent [:WeirdAl]]

On Windows, I did a little debugging on the sixth textbox, trying to use shift-tab:

nsEventStateManager.cpp#3685:
+		currentContent	0x05a0ef90 {mPrototype={...} mBindingParent=0x00000000 }	nsIContent *
+		aStartContent	0x04f7b058 {mControllers={...} mType='' mBitField=0x0004 ...}	nsIContent *

javascript:var box5 = document.getElementsByTagName("textbox")[5]; dump(box5 + "\n" + box5.inputField);
[object XULElement @ 0x4f75218 (native @ 0x5a0ef90)]
[object HTMLInputElement @ 0x467c8d8 (native @ 0x4f7b058)]

Note the native values; they match.  In other words, aStartContent is the anonymous XHTML input element of the XUL textbox element, currentContent.

aaronlev: this code has your fingerprints all over it according to cvs blame, for bug 250006.  Can you look into this a bit?

Comment 5

[Alex Vincent [:WeirdAl]]

I now know why MacOS is unaffected.  nsXULElement.cpp#608 checks sTabFocusModelAppliesToXUL, which comes from a Firefox preference accessibility.tabfocus_applies_to_xul.  This preference is FALSE for MacOS (modules/libpref/src/init/all.js#127), and true for the others.

However, the comment in nsXULElement.cpp clearly states this is correct; flipping the preference to FALSE is the wrong solution:

609       // By default, the tab focus model doesn't apply to xul element on any system but OS X.
610       // on OS X we're following it for UI elements (XUL) as sTabFocusModel is based on
611       // "Full Keyboard Access" system setting (see mac/nsILookAndFeel).
612       // both textboxes and list elements (i.e. trees and list) should always be focusable
613       // (textboxes are handled as html:input)

Comment 6

[Alex Vincent [:WeirdAl]]

Attachment: patch, v1

This patch fixes the shift-tab issue.  However, in testing this patch, I found another issue with the testcase.

If you cycle all the way through the tabs forward, including through the URL bar and the search bar, the next tab goes to the first textbox instead of the seventh one (the first has no tabindex attribute, and the seventh has tabindex="1").

This patch does not affect that behavior, but does make it easier to test that particular bustage:

(1) Tab. (end up in box 7)
(2) Shift-tab.
(3) Tab. (end up in box 1)

Comment 7

[Alex Vincent [:WeirdAl]]

filed bug 370031 for the new irregularity in comment 6; it affects HTML as well.

Comment 8

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Do you want to check that it's also not the binding parents binding parent?

Comment 9

[Alex Vincent [:WeirdAl]]

While that certainly could be an issue theoretically (grandparent, great-grandparent, etc.), I think it's irrelevant for tab indexes.  This is actually a bit of a special case, because most anonymous content doesn't have tab indexes (html:input elements do).  I can't think of a case where tab index applies in that fashion.

That said, if you or anyone else can come up with a testcase demonstrating a need for it, I'll happily add support for it.

Comment 10

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Consider something like this:

Original doc:

<div tabindex=1 style="-moz-binding: url('bind.xml#b1')">hello</div>


bind.xml:
...
<binding id="b1">
  <content>
    <html:div tabindex=2 style="-moz-binding: url('bind.xml#b2')">kitty</div>
  </content>
</binding>
<binding id="b2">
  <content>
    <html:div tabindex=3>foobar</div>
  </content>
</binding>


That should result in deeply nested anonymous content with tab indexes.

Comment 11

[Alex Vincent [:WeirdAl]]

Ref comment 10, I'm not able to produce any problems there.  But in that case, the anonymous tabindexes aren't inherited.

Comment 12

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

The important part isn't that they're inherited, but that they're the same, no? Also, you can (at least in theory) inherit through several layers of bindings.

Comment 13

[Alex Vincent [:WeirdAl]]

> The important part isn't that they're inherited, but that they're the same, no?

Well, I *think* it's because it's inherited (and anonymous).  But I can see the "sameness" argument being possible.

> Also, you can (at least in theory) inherit through several layers of bindings.

Yes, that's another plausible possibility.

Comment 14

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Attachment: Testcase XBL file

Comment 15

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Attachment: Testcase XHTML file

This demonstrates multiple levels of nesting. Does the patch fix this one too? I.e. you should be able to click the text and tab backwards.

Comment 16

[Alex Vincent [:WeirdAl]]

(In reply to comment #15)
>Does the patch fix this one too?
> I.e. you should be able to click the text and tab backwards.

Yes, but not quite the way I might have hoped.  I click on the text, shift-tab, focus goes (apparently) nowhere.  I shift-tab again, and focus goes to the overall document.  The third shift-tab takes me to the tabbrowser's tabs, and the fourth takes me to the URL bar.

Still, it's a sizable improvement over an unpatched build (where you can't shift-tab off of it no matter how many times you try).

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a4pre) Gecko/20070404 SeaMonkey/1.5a with patch applied.

Comment 17

[Alex Vincent [:WeirdAl]]

Comment on attachment 254706 [details] [diff] [review]
patch, v1

<sicking> please fix the patch. It's better to have a full fix than a partial one

Also, tabbing forward from the document's body requires two tabs (not one) to get past the targeted node.

Comment 18

[Alex Vincent [:WeirdAl]]

Attachment: patch, v1.1

This patch passes both the original testcase and sicking's new testcase, going both backwards and forwards.

Something in my gut tells me this patch isn't quite right either, but I can't confirm that.  The patch works, it's simple... am I missing something?

Comment 19

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

I think smaug would be a better reviewer here.

Comment 20

[Olli Pettay [:smaug][[e-mail]]]

Attachment: a testcase

Using the patch forward tabbing doesn't work the same way as backward tabbing. Which one is the right, not sure, but at least it should work
the same way in both cases.

Comment 21

[Alex Vincent [:WeirdAl]]

Dang, three good testcases, each testing different things.  Plus the spinoff bug.  

Can we make these into automated testcases somehow, smaug?  I'm not aware of any API that lets us force tab advancing and retreating.

Comment 22

[Olli Pettay [:smaug][[e-mail]]]

Maybe you could use http://lxr.mozilla.org/seamonkey/source/dom/public/idl/base/nsIDOMWindowUtils.idl#115
to dispatch right kinds of key events.

Comment 23

[Trent Mick]

Another data point. This affects Komodo bug http://bugs.activestate.com/show_bug.cgi?id=75931

There is another test case there (the first attachment: http://bugs.activestate.com/attachment.cgi?id=8654) that shows how Shift+Tab doesn't work to get out of the textboxes.

In my testing the Shift+Tab breakage happens in:
- FF2/Win
- FF3/Win
- FF2/Mac OS X

Comment 24

[Alex Vincent [:WeirdAl]]

Does someone else want to take this bug?  After almost a year, I've found insufficient time to progress on this one... :(

Comment 25

[Trent Mick]

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

Comment 26

[Neil Deakin]

Fixed by bug 178324.

Comment 27

[Alex Vincent [:WeirdAl]]

Neil:  All three testcases? :)