Currently nsIDOMNSHTMLInputElement contains readonly attribute boolean tabbable This should either be on nsIFormControl or another non-DOM interface. Johnny, I would like to implement this not just for form controls, but also for anchors, image map areas, frames and iframes. This would: 1. Simplify esm::GetNextTabbableContent() 2. Allow sTabFocusModel to be stored on nsGenericHTMLElement, which would make the various impl's of GetTabbable() cleaner (because they wouldn't have to call back into esm). I think it makes sense for each type of element to know if it's tabbable, rather than have a whole lot of special casing in esm. So, could I go forward with that plan? We'd have to put it on something other than nsIFormControl. Can we put it on nsIContent or nsIXMLContent? BTW, this will affect the implementation of bug 171366, so perhaps I should just do it on nsIFormControl for now.

Johnny suggests we add nsIFrame::IsTabbable() since tabbing doesn't make sense without a presentation.

The question is really what should determine whether something is in the tabbing order. Most (if not all) of the tests are described in terms of content things (e.g., form control things, links). Some of those things (although perhaps not all) are still meaningful and should still be tabbable given multiple display types (e.g., a link that's display:block, display:table-cell, etc.). So I'd probably lean towards wanting the function on nsIContent.

I think it should go on nsIFrame after all, because of 2 things: 1) The tab navigation code traverse the frame tree 2) The frame does need to be checked for some things, and it's easier to go from frame to content via GetContent() then the other way via GetPrimaryFrameFor

*** Bug 250858 has been marked as a duplicate of this bug. ***

Comment on attachment 153053 [details] [diff] [review] Clean up GetNextTabbableContent(). Remove tabbable property from DOM interfaces. Gets rid of some ugly callbacks. Clean up spurious focus outlines (bug 250858.) Looks pretty good to me. I ran it through some basic tests and didn't find any problems.

Comment on attachment 153053 [details] [diff] [review] Clean up GetNextTabbableContent(). Remove tabbable property from DOM interfaces. Gets rid of some ugly callbacks. Clean up spurious focus outlines (bug 250858.) - In nsEventStateManager::TabIndexFrom(): + nsAutoString tabIndexStr; + aFrom->GetAttr(kNameSpaceID_None, nsHTMLAtoms::tabindex, tabIndexStr); + if (!tabIndexStr.IsEmpty()) { + PRInt32 ec, tabIndexVal = tabIndexStr.ToInteger(&ec); + if (NS_SUCCEEDED(ec)) + *aOutIndex = tabIndexVal; Don't we want to check for the tabindex attribute only for HTML and XUL (and SVG?), doesn't seem like it's right to check for any random namespaced element. Or is this only called if the element is focusable? sr=jst

(In reply to comment #7) > Don't we want to check for the tabindex attribute only for HTML and XUL (and > SVG?) At this point it's only used if the item is focusable.

Checking in dom/public/idl/html/nsIDOMNSHTMLInputElement.idl; /cvsroot/mozilla/dom/public/idl/html/nsIDOMNSHTMLInputElement.idl,v <-- nsIDOMNSHTMLInputElement.idl new revision: 1.8; previous revision: 1.7 done Checking in content/base/public/nsIContent.h; /cvsroot/mozilla/content/base/public/nsIContent.h,v <-- nsIContent.h new revision: 3.86; previous revision: 3.85 done Checking in content/base/src/nsGenericElement.cpp; /cvsroot/mozilla/content/base/src/nsGenericElement.cpp,v <-- nsGenericElement.cpp new revision: 3.342; previous revision: 3.341 done Checking in content/events/public/nsIEventStateManager.h; /cvsroot/mozilla/content/events/public/nsIEventStateManager.h,v <-- nsIEventStateManager.h new revision: 1.54; previous revision: 1.53 done Checking in content/events/src/nsEventStateManager.cpp; /cvsroot/mozilla/content/events/src/nsEventStateManager.cpp,v <-- nsEventStateManager.cpp new revision: 1.513; previous revision: 1.512 done Checking in content/events/src/nsEventStateManager.h; /cvsroot/mozilla/content/events/src/nsEventStateManager.h,v <-- nsEventStateManager.h new revision: 1.114; previous revision: 1.113 done Checking in content/html/content/src/nsGenericHTMLElement.cpp; /cvsroot/mozilla/content/html/content/src/nsGenericHTMLElement.cpp,v <-- nsGenericHTMLElement.cpp new revision: 1.528; previous revision: 1.527 done Checking in content/html/content/src/nsGenericHTMLElement.h; /cvsroot/mozilla/content/html/content/src/nsGenericHTMLElement.h,v <-- nsGenericHTMLElement.h new revision: 1.196; previous revision: 1.195 done Checking in content/html/content/src/nsHTMLAnchorElement.cpp; /cvsroot/mozilla/content/html/content/src/nsHTMLAnchorElement.cpp,v <-- nsHTMLAnchorElement.cpp new revision: 1.131; previous revision: 1.130 done Checking in content/html/content/src/nsHTMLAreaElement.cpp; /cvsroot/mozilla/content/html/content/src/nsHTMLAreaElement.cpp,v <-- nsHTMLAreaElement.cpp new revision: 1.81; previous revision: 1.80 done Checking in content/html/content/src/nsHTMLButtonElement.cpp; /cvsroot/mozilla/content/html/content/src/nsHTMLButtonElement.cpp,v <-- nsHTMLButtonElement.cpp new revision: 1.129; previous revision: 1.128 done Checking in content/html/content/src/nsHTMLImageElement.cpp; /cvsroot/mozilla/content/html/content/src/nsHTMLImageElement.cpp,v <-- nsHTMLImageElement.cpp new revision: 1.180; previous revision: 1.179 done Checking in content/html/content/src/nsHTMLInputElement.cpp; /cvsroot/mozilla/content/html/content/src/nsHTMLInputElement.cpp,v <-- nsHTMLInputElement.cpp new revision: 1.359; previous revision: 1.358 done Checking in content/html/content/src/nsHTMLObjectElement.cpp; /cvsroot/mozilla/content/html/content/src/nsHTMLObjectElement.cpp,v <-- nsHTMLObjectElement.cpp new revision: 1.68; previous revision: 1.67 done Checking in content/html/content/src/nsHTMLSelectElement.cpp; /cvsroot/mozilla/content/html/content/src/nsHTMLSelectElement.cpp,v <-- nsHTMLSelectElement.cpp new revision: 1.212; previous revision: 1.211 done Checking in content/html/content/src/nsHTMLTextAreaElement.cpp; /cvsroot/mozilla/content/html/content/src/nsHTMLTextAreaElement.cpp,v <-- nsHTMLTextAreaElement.cpp new revision: 1.157; previous revision: 1.156 done Checking in content/xul/content/src/nsXULElement.cpp; /cvsroot/mozilla/content/xul/content/src/nsXULElement.cpp,v <-- nsXULElement.cpp new revision: 1.529; previous revision: 1.528 done Checking in content/xul/content/src/nsXULElement.h; /cvsroot/mozilla/content/xul/content/src/nsXULElement.h,v <-- nsXULElement.h new revision: 1.168; previous revision: 1.167 done Checking in layout/base/public/nsIFrame.h; /cvsroot/mozilla/layout/base/public/nsIFrame.h,v <-- nsIFrame.h new revision: 3.248; previous revision: 3.247 done Checking in layout/html/base/src/nsFrame.cpp; /cvsroot/mozilla/layout/html/base/src/nsFrame.cpp,v <-- nsFrame.cpp new revision: 3.493; previous revision: 3.492 done Checking in layout/html/document/src/html.css; /cvsroot/mozilla/layout/html/document/src/html.css,v <-- html.css new revision: 3.177; previous revision: 3.176 done

I've found a minor problem with this: You can't make an nsIDOMXULControlElement unfocusable in CSS. Firt, the ESM asks the frame if it is focusable. The nsIFrame looks at the CSS, and guesses that it probably isn't focusable. The frame then double-checks with the content. The content notices that's it's a xul control element. However it overwrites the focusablility with the disabled property. Perhaps it's return !disabled; should be changed to return tabIndex >= 0;

OK, changing "its" :-[ return !disabled; to return tabIndex >= 0; works for me.

Comment on attachment 154368 [details] [diff] [review] Neil's fix, which I believe makes sense I claim this patch as mine as per comment #11, seeking reviews.

Comment on attachment 154368 [details] [diff] [review] Neil's fix, which I believe makes sense sr=jst

Looks like Neil checked this in.

I think this caused bug 253472.

This also caused bug 261707 (can't focus some anchors that should be focusable) and made XLinks not be focusable...