[webkit-reviews] review granted: [Bug 7138] Implement tabindex for all elements, enabling accessible web apps : [Attachment 20746] patch and regression test

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Alice Liu <alice.liu at apple.com>'s
request for review:
Bug 7138: Implement tabindex for all elements, enabling accessible web apps
http://bugs.webkit.org/show_bug.cgi?id=7138

Attachment 20746: patch and regression test
http://bugs.webkit.org/attachment.cgi?id=20746&action=edit

------- Additional Comments from Darin Adler <darin at apple.com>
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.