Bug 7138 - Implement tabindex for all elements, enabling accessible web apps
: Implement tabindex for all elements, enabling accessible web apps
Status: RESOLVED FIXED
Product: WebKit
Classification: Unclassified
Component: Accessibility
: 420+
: All All
: P2 Normal
Assigned To: Alice Liu
http://developer.mozilla.org/en/docs/...
: InRadar, ReviewedForRadar
Depends on:
Blocks: 10489 13846 4986 12132 12728
  Show dependency treegraph
 
Reported: 2006-02-07 18:45 PST by Aaron Leventhal
Modified: 2009-01-12 15:24 PST (History)
22 users (show)

See Also:


Attachments
patch and regression test (80.36 KB, patch)
2008-04-22 02:37 PDT, Alice Liu
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Aaron Leventhal 2006-02-07 18:45:50 PST
Support tabindex for all elements, thus enabling accessible DHTML/AJAX web applications. Both IE and Gecko-based browsers such as Firefox now support tabIndex on all elements. The Web Applications spec from WHATWG also enables tabIndex for all elements (http://whatwg.org/specs/web-apps/current-work/#the-tabindex)

Basically, any element such as a div or span can be added to the default tab order by setting tabindex="0". In addition, any item with tabindex="-1" can be focused via script or mouse. 

The tabindex system is needed for developing custom widgets which are accessible, from simple widgets like a slider to container widgets like a menubar, tree view or spreadsheet. IBM needs this for web apps it is developing, in order for them to run properly on Safari. Other large organizations are also picking up DHTML accessibility.

* The Gecko documentation on what the behavior should be is here: http://www.mozilla.org/access/keyboard/tabindex
* The MSDN docs for IE's tabindex behavior is here: http://msdn.microsoft.com/workshop/author/dhtml/reference/properties/tabindex.asp
* Test cases and more docs about DHTML accessibility here: http://www.mozilla.org/access/dhtml
* W3C standards work in this area: http://www.w3.org/WAI/PF/roadmap/DHTMLRoadmap110505.html

Work we had to do in Gecko (see https://bugzilla.mozilla.org/show_bug.cgi?id=171366):
- Have each element class support an IsFocusable method which also returns a boolean out to indicate whether it is tabbable.
  Specific implementations of the method need to upcall to the generic element's method, which jas the generic tabindex implementation..
  Then the scripting, mouse click handling and tab key handling  code needs to use the new IsFocusable() method.
- Support the focus() method in nsIDOMNSHTMLElement, which is outside of the official DOM interface but is supported fro all elements.
- Add rules to the default style sheet so that any element with :focus receives a focus outline
- Make cancelling an keydown event cancel the keypress event, for purposes of compatibility with IE. Authors writing cross-browser content should use keydown events to process keystrokes, because IE won't fire keypress events for non-alphanumeric keys.
- Ensure that all elements which are focusable are exposed in the accessibility API hierarchy, so that when they get focused there is an object to fire an accessibility event for.
- Any element that can receive focus must fire onfocus/onblur
Comment 1 Joost de Valk (AlthA) 2006-02-15 14:16:39 PST
Seems a reasonable request, confirming.
Comment 2 Mark Malone 2007-01-05 14:34:50 PST
rdar://4911289
Comment 3 David Kilzer (:ddkilzer) 2007-01-07 05:04:22 PST
See also Bug 9460.

Comment 4 Timothy Hatcher 2007-04-02 11:58:55 PDT
This would help the Web Inspector, since I am converting more of it to HTML. Right now I am working around this with a JavaScript solution.
Comment 5 Nicholas Shanks 2007-05-15 16:30:03 PDT
There is a school of thought that considers tabindex harmful because it changes the users expectations:

e.g.

<input type=text tabindex=1 />
<input type=text tabindex=2 />
<input type=text tabindex=4 />
<input type=text tabindex=3 />

obviously real-world examples are (usually) more complex, but doing something other than what the user expects from a system UI she is used to is a bad thing.
Comment 6 Adam Roben (:aroben) 2007-05-15 17:00:47 PDT
(In reply to comment #5)
> There is a school of thought that considers tabindex harmful because it changes
> the users expectations:
> 
> e.g.
> 
> <input type=text tabindex=1 />
> <input type=text tabindex=2 />
> <input type=text tabindex=4 />
> <input type=text tabindex=3 />
> 
> obviously real-world examples are (usually) more complex, but doing something
> other than what the user expects from a system UI she is used to is a bad
> thing.
> 

   I would hope that web authors would be using tabindex to make the tab order _more_ intuitive and/or closer to the system behavior, not less so.
Comment 7 Aaron Leventhal 2007-05-16 06:34:32 PDT
Adam, check out how tabindex="-1" and tabindex="0" are used to make container widgets that work like their system counterparts. From the sounds of it you're thinking about reordering the tab navigation which isn't what this is really about.

Comment 8 David Bolter 2007-05-23 09:06:14 PDT
I think the severity of this is arguable much greater than "enhancement". We (at dojotoolkit) need this if we are to fully support (i.e. provide keyboard control) WebKit for our DHTML widgets (dijits).
Comment 9 David Kilzer (:ddkilzer) 2007-05-23 10:33:40 PDT
(In reply to comment #8)
> I think the severity of this is arguable much greater than "enhancement". We
> (at dojotoolkit) need this if we are to fully support (i.e. provide keyboard
> control) WebKit for our DHTML widgets (dijits).

Bug 13846 was filed to track issues for the Dojo Toolkit.

Comment 10 Jesse Costello-Good 2007-06-28 10:05:34 PDT
We at TIBCO would very much like this to make it in to Safari ASAP. It is common in Ajax to make custom GUI controls that don't use any of the native browser INPUT tags. In order to make these Ajax controls seems like native controls we need to be able to focus SPAN and DIV.
Comment 11 David Kilzer (:ddkilzer) 2007-07-05 21:58:52 PDT
*** Bug 13464 has been marked as a duplicate of this bug. ***
Comment 12 Victor Tsaran 2007-08-01 14:21:20 PDT
TabIndexability would be an important addition for sophari-based browsers, like the one used by Nokia on S60FP1 devices. Taming this basic issue would hopefully prevent DHTML apps on mobilephone to get out of hand by providing developers with a possibility of making these applications accessible.
Comment 13 Aaron Leventhal 2007-09-25 18:34:07 PDT
Need testcases?

Simple test cases from Opera: 
http://tc.labs.opera.com/html/global-attributes/tabindex/

Advanced test cases from Mozilla: 
http://www.mozilla.org/access/dhtml/checkbox
http://www.mozilla.org/access/dhtml/tree
http://www.mozilla.org/access/dhtml/spreadsheet
http://www.mozilla.org/access/dhtml/pretty-slider

Should also test against Dojo toolkit, which uses this extensively.

Comment 14 Aaron Leventhal 2007-09-25 18:41:17 PDT
Note: Opera, IE and Firefox all have this capability now.

Also, the bug for actual ARIA support is bug 12132.

Comment 15 John A. Bilicki III 2007-10-25 12:06:27 PDT
A live test case, http://www.jabcreations.net/ which provides both a complex layout and accessibility. Firefox, Opera, and IE are all accessible on my site.
Comment 16 Alexey Proskuryakov 2008-01-31 02:55:58 PST
*** Bug 17114 has been marked as a duplicate of this bug. ***
Comment 17 Aaron Leventhal 2008-03-19 17:13:08 PDT
I have some notes on how to implement this here:
http://developer.mozilla.org/en/docs/ARIA_User_Agent_Implementors_Guide#11.3.1.1_HTML_5_Tabindex

If you bug Simon Pieters (zcorpan) from Opera he will give you links to some simple test cases.
Comment 18 Robert Blaut 2008-03-21 11:51:25 PDT
*** Bug 17983 has been marked as a duplicate of this bug. ***
Comment 19 Johan Lund 2008-03-26 03:08:55 PDT
The Bindows AJAX framework will not be able to fully support webkit until this has been fixed.
Our custom focusing system requires it.
Please implement this functionality as soon as possible.
Comment 20 Johan Lund 2008-03-26 08:37:13 PDT
For Bindows support we will be needing at least focusable elements of type div and tr.
Comment 21 Brian Moeskau 2008-04-05 14:56:43 PDT
Add another vote for fixing this from the Ext JS team.  Thanks!
Comment 22 Sebastian Werner 2008-04-11 14:58:01 PDT
We (the qooxdoo team) have worked around the thing, but it would still be great to have native support for this in webkit.
Comment 23 Alice Liu 2008-04-22 02:37:18 PDT
Created attachment 20746 [details]
patch and regression test
Comment 24 Adam Roben (:aroben) 2008-04-22 05:44:20 PDT
Comment on attachment 20746 [details]
patch and regression test

Is it OK that we're changing public OS X headers? Is it OK that we're removing headers that projects using WebKit are likely currently using?

+    bool m_hasChangedTabIndex : 1;

Does Node have room for this extra bit?

There might be a better name for this member, but I'm having trouble thinking of it. Names I've considered include m_hasExplicitTabIndex, m_didSetTabIndex, m_hasTabIndex.

+++ WebCore/html/HTMLElement.idl	(working copy)
@@ -35,6 +35,10 @@ module html {
                  attribute [ConvertNullToNullString] DOMString dir;
                  attribute [ConvertNullToNullString] DOMString className;
 
+                 attribute long            tabIndex;
+        void               blur();
+        void               focus();
+

Indentation looks funny here but it probably is just matching the funny indentation of the rest of the file.

+++ WebCore/html/HTMLLegendElement.cpp	(working copy)
@@ -45,7 +45,7 @@ HTMLLegendElement::~HTMLLegendElement()
 
 bool HTMLLegendElement::isFocusable() const
 {
-    return false;
+    return m_hasChangedTabIndex;
 }

Should this behavior be in HTMLElement instead?

It would be good to test Shift-Tab in the regression test as well.

Your test doesn't seem to test calling focus() on any elements. I think this is a good thing to test, since focus() working on elements that are not normally focusable is the main effect of your patch!

I'm not going to r+ or r- this yet, because I'm interested in the answers to my questions.
Comment 25 Sjoerd Visscher 2008-04-22 06:23:15 PDT
Hmm, there seems to be an incompatibility between IE and Firefox with tabIndex="-1". In IE, elements that are not focusable are also not focusable when tabIndex="-1". In Firefox they are focusable, and this seems what you have implemented here.

But the HTML 5 spec seems to be compatible with IE.
Comment 26 Adam Roben (:aroben) 2008-04-22 06:36:24 PDT
(In reply to comment #25)
> Hmm, there seems to be an incompatibility between IE and Firefox with
> tabIndex="-1". In IE, elements that are not focusable are also not focusable
> when tabIndex="-1". In Firefox they are focusable, and this seems what you have
> implemented here.

When you say "focusable", do you mean that they are included in the tab order (i.e., you can press Tab to reach them), or that you can call element.focus() on them and they will gain keyboard focus, or both, or something different entirely?

> But the HTML 5 spec seems to be compatible with IE.
Comment 27 Sjoerd Visscher 2008-04-22 06:40:29 PDT
With focusable I mean that both .focus() and clicking on the element draws focus outlines on the element. I did not test if that also means that they receive keyboard events, but I would think so.

I have also raised this issue on the whatwg list.
Comment 28 Aaron Leventhal 2008-04-22 06:48:31 PDT
Putting tabindex="-1" on normally unfocusable items should make them focusable via click or script, but not tabbable. However, IE has what I consider a bug in that those items receive focus but don't provide a default focus appearance for them.
Comment 29 Aaron Leventhal 2008-04-22 06:52:49 PDT
The WhatWG spec appears to be wrong, in that it says:
> A negative integer specifies that the element should be removed from the tab 
> order. If the element does normally take focus, it may still be focused using 
> other means (e.g. it could be focused by a click). 
It should also say that a negative integer makes elements focusable but not tabbable, whether they are normally focusable or not.

IE certainly does that -- they just don't show focus, which is really a section 508 mistake on their part. For IE the author has to put in an onfocus handler and set element.style to show focus.
Comment 30 Anne van Kesteren 2008-04-22 07:14:25 PDT
Yes, aaronlev is right. Though theres a difference between IE and Firefox on the handling of invalid values, such as tabindex=blah or tabindex=1s. HTML5 follows IE here ("ignore" the attribute).
Comment 31 Darin Adler 2008-04-22 09:44:40 PDT
Comment on attachment 20746 [details]
patch and regression test

Really great work! I'm happy to see this getting fixed and I love having the new thorough test.

+        * bindings/objc/PublicDOMInterfaces.h:
+        -Remove focus, blur, tabindex from descendants of DOMHTMLElement.

This comment confused me. You're not removing these -- you're moving them to HTMLElement.

I think the name m_hasChangedTabIndex could be improved. I suggest m_tabIndexSetExplicitly, or m_hasTabIndex  or m_tabIndexWasSet or m_tabIndexWasSetExplicitly. WebCore::Document uses m_titleSetExplicitly for a similar situation.

     bool m_focused : 1;
+    bool m_hasChangedTabIndex : 1;
     bool m_active : 1;

You didn't change the comment below that says "1 bit left"; I think you took the last bit, so it has to say "no bits left".

-            setTabIndex(max(static_cast<int>(std::numeric_limits<short>::min()), min(indexstring.toInt(), static_cast<int>(std::numeric_limits<short>::max()))));
+            Node::setTabIndex(max(static_cast<int>(std::numeric_limits<short>::min()), min(indexstring.toInt(), static_cast<int>(std::numeric_limits<short>::max()))));

I'm not happy with the idiom used here. I think the internal Node::setTabIndex function should be renamed to get it out of the way of the public HTMLElement::setTabIndex function. It's usually not good style to hide one non-virtual function with another that does something different. Where else is Node::setTabIndex called? Maybe we can make it be a protected function member?

-    return isContentEditable() && parent() && !parent()->isContentEditable();
+    return (m_hasChangedTabIndex || (isContentEditable() && parent() && !parent()->isContentEditable()));

Another way to write this would be to call Element::isFocusable() instead of checking the m_hasChangedTabIndex flag directly. I'd like to see that flag be private if possible (see below).

No need for that outer set of parentheses around the entire return expression. Please omit them.

 bool HTMLFieldSetElement::isFocusable() const
 {
-    return false;
+    return m_hasChangedTabIndex;
 }

Another way to do this is to call Element::isFocusable, since we're trying to undo what HTMLElement and HTMLGenericFormElement do. I think that's better than having a copy of the logic from Node::isFocusable here, but you might not agree. Same comment about HTMLLabelElement, HTMLLegendElement, HTMLOptGroupElement, and HTMLOptionElement.

You might be able to make the flag private rather than protected if you eliminate these. For future maintenance it would be much better if the tabIndex-related data members were private. We may want to change them around without changing all the code.

+<!--imbedded elements-->

I think these are "embedded" as opposed to "imbedded".

+    printToConsole(elem.id + " blured");

Typo here: "blured" instead of "blurred" with two "r"s.

+        document.write(focusCount+" focus / "+blurCount+" blur events disatched<br>"+consoleOutput.innerHTML);

Typo here: "disatched" instead of "dispatched".

+<frame src="data:text/html,<div id='theConsole'>Console:</div>"/>

I don't think the text "Console:" makes the output easier to read.

I think this test should go in the fast/forms directory rather than fast/events -- even though the test involves the focus and blur events, I think it's really about the form element support for being focused. The test checks that elements can be focused, but I don't see tests to check each element's behavior with no tabIndex, tabIndex of -1, tabIndex of 0, and a positive tabIndex.

The test checks the behavior of the tab key, but tabIndex has other effects as well. For example, it controls whether focus() works at all on some elements. We need tests to cover these changes in behavior.

I'm interested in having tests to check that we have reasonable behavior when a tabIndex is set to values like: -2, -32767, -32768, -32769, 32767, 32768, and also large integers at the 2^31 and 2^32 boundaries. I worry that we truncate integers and do unpredictable things in those cases.

I'd like the test output to be a little clearer about whether it's a success or failure. Currently the output doesn't make it clear that it's success. Also, the test doesn't seem to check the differences in behaviors. For example, where's the test for the difference between tabIndex of 0 and tabIndex of -1?

r=me, as-is, but I'd like to see some refinements too.
Comment 32 Maciej Stachowiak 2008-04-22 11:49:23 PDT
I'm not sure fast/forms makes sense, since tabIndex allows things that are not form elements to be focued too.
Comment 33 Adam Roben (:aroben) 2008-04-23 09:51:03 PDT
(In reply to comment #27)
> With focusable I mean that both .focus() and clicking on the element draws
> focus outlines on the element. I did not test if that also means that they
> receive keyboard events, but I would think so.
> 
> I have also raised this issue on the whatwg list.

Here's the link to the email thread. There's some interesting discussion in there:

http://lists.whatwg.org/pipermail/whatwg-whatwg.org/2008-April/014512.html

Comment 34 Ian 'Hixie' Hickson 2008-04-24 02:53:47 PDT
I've revamped the HTML5 spec. Let me know if I missed anything. It should be much more precise about what should happen now.
Comment 35 Sjoerd Visscher 2008-04-24 03:23:00 PDT
Just one thing: "The tabIndex DOM attribute must reflect the value of the tabIndex content attribute. If the attribute is not present, or parsing its value returns an error, then the DOM attribute must return 0 for elements that are focusable and &#8722;1 for elements that are not focusable."

This does not make sense. A tabIndex="-1" means that the element is focusable, so returning -1 for the DOM attribute for unfocusable elements is wrong. The only value that makes sense is null.
Comment 36 Anne van Kesteren 2008-04-24 06:47:36 PDT
FWIW: IE returns 0 for such elements (attribute absent, attribute error, or not focusable by default) and Firefox and Opera -1.
Comment 37 Anne van Kesteren 2008-04-24 06:48:30 PDT
I should have added, if you want to determine whether an element is focuable having an additional getter would probably be good because contenteditable and maybe other attributes can influence that as well. 
Comment 38 Alice Liu 2008-04-28 18:33:11 PDT
I re-vamped the test to have much better output, and to test .focus instead of tabbing, the former of which this patch is really about.  Also addressed all of Darin's comments, some of which overlapped with Adam's comments.  fast/dom/tabindex-clamp already addresses Darin's desire for a boundary condition on assignment test. 

I also addressed the recently added specification that elements with tabindex unspecified or parsed unsuccessfully shall return 0 for normally focusable elements and -1 for all others.  Previous return value was undefined for the latter. 

committed r32664
Comment 39 Darin Adler 2009-01-12 15:24:19 PST
*** Bug 20596 has been marked as a duplicate of this bug. ***