Closed Bug 171366 Opened 17 years ago Closed 16 years ago

Implement navindex/tabindex for all elements

Categories

(Core :: User events and focus handling, defect, P1, major)

defect

Tracking

()

RESOLVED FIXED
mozilla1.8beta1

People

(Reporter: erik, Assigned: aaronlev)

References

()

Details

(Keywords: access)

Attachments

(2 files)

It would make sense if all elements that can be focused (-moz-user-focus:
normal) would also support the tabIndex (or the XHTML2.0 navindex)
attribute/property.

http://www.w3.org/TR/html4/interact/forms.html#adef-tabindex
http://www.w3.org/TR/xhtml2/mod-attribute-collections.html#col_Hypertext

IE supports tabIndex on all elements and IE even sets the element to become
focusable if the tabIndex is present and not "-1". If tabIndex is added then
making elements focusable depending on the tabIndex attribbute is simple using css.

[tabIndex] {
   -moz-user-focus: normal;
}

[tabIndex="-1"] {
   -moz-user-focus: none;
}
QA Contact: petersen → amar
Priority: -- → P3
Target Milestone: --- → Future
See also bug 150994, allowing tabindex on <iframe> makes it impossible to tab to
some elements.
I found that setting tabindex = "-1" on many element does not inhibit them from
being focusable. For example: any <a> tag with tabindex set to -1 is still focusable
.
Assignee: attinasi → aaronl
Component: Layout → Keyboard: Navigation
Priority: P3 → --
QA Contact: amar → sairuh
Target Milestone: Future → ---
... tabIndex = -1 _shouldn't_ disable focus. You want the attribute 'disabled',
which will do just that.

In my _opinion_, objects with tabIndex = -1 should be removed from the tabbing
order, but this is not W3C spec.
http://www.w3.org/TR/html401/interact/forms.html#adef-tabindex

(Although it used to be, in the earlier drafts of the HTML 4.0 specification.
If anyone knows why this was changed, let me know -- it would help with bug 217770.)
http://www.w3.org/TR/WD-html40-970917/interact/forms.html#adef-tabindex

However, none of this belongs here anyway. One bug, one issue. You want bug 56809.

As for the original bug: the W3C specification also says precisely which
elements should support tabIndex.
http://www.w3.org/TR/html401/index/attributes.html

Advise VERIFIED INVALID.
We're getting clarification from the W3C now. Apparently the current docs were
meant to represent current, not best, practice.

It's important to be able to make anything focusable/tabbable, since it's
possible to make custom widgets using divs and spans with CSS.

After a lot of thought I think we should follow what IE does, but get the go
ahead from W3C.

Johnny, can you help me get an answer from the DOM group on this stuff? We want
anything to be focusable because of custom widgets developed with JS.
We're getting clarification from the W3C now. Apparently the current docs were
meant to represent current, not best, practice.

It's important to be able to make anything focusable/tabbable, since it's
possible to make custom widgets using divs and spans with CSS.

After a lot of thought I think we should follow what IE does, but get the go
ahead from W3C.

Johnny, can you help me get an answer from the DOM group on this stuff? We want
anything to be focusable because of custom widgets developed with JS.
Severity: enhancement → major
Priority: -- → P1
Target Milestone: --- → mozilla1.8beta
(In reply to comment #6)
> After a lot of thought I think we should follow what IE does, but get the go
> ahead from W3C.
> 
> Johnny, can you help me get an answer from the DOM group on this stuff? We want
> anything to be focusable because of custom widgets developed with JS.

Aaron, there is no more DOM working group at the W3C, and I see little hope of
getting any direction or go ahead from what used to be the DOM WG. I'd propose
that we do what we think is the right thing to do (and be compatible with IE as
far as possible), and once we're there, we can announce what we've done to the
DOM-IG mailing list and request that our changes be incorporated into possible
future erratas etc.
Johnny, IE actually handles focusability of custom html widgets very well. As
you said, it would be best if we simply follow their lead as closely as possible
and then publish a note to the DOM WG for errata.

IE supports the use of the tabindex attribute on nearly all elements to indicate
that something is focusable.

The details of how it works are available on the tabindex page on MSDN:
http://msdn.microsoft.com/library/default.asp?url=/workshop/author/dhtml/reference/properties/tabindex.asp

Based on these datails, I suggest we follow IE in a way that is efficient and
sensible for Mozilla:

1. Any element with a tabindex >= "0" is in the tab order, any element with a
negative tabindex is removed from the tab order.
   - Mozilla impl: patch nsEventStateManager::GetNextTabbableContent()

2. Any element with a tabindex >= "0" can receive a .focus() via script, any
element with a negative tabindex cannot receive .focus()
   - Mozilla impl: add nsIDOMNSHTMLElement::focus()  -- see
http://lxr.mozilla.org/seamonkey/source/dom/public/idl/html/nsIDOMNSHTMLElement.idl

3. Any element that receives focus will have a default :focus rule for showing
the focus outline.
   - Mozilla impl: Need a rule for html.css, something like div:focus,
span:focus, ... { -moz-outline: 1px dotted invert; }

4. Any element that can receive focus will fire onfocus/onblur 
   - Mozilla impl: we already do this

5. Any element with a tabindex attribute will be in the accessibility hierarchy.
   - Mozilla impl: this is something we can handle in the accessibility module
How about
*[tabindex] { -moz-user-focus: normal; }
*[tabindex^="-"] { -moz-user-focus: none; }
Or is that too simplistic?
That would work, but I think there are 2 potential problems:
1. It makes us check for a tabindex on every element which will affect page load
perf.
2. Will it even work if style sheets are turned off?

(In reply to comment #9)
> How about
> *[tabindex] { -moz-user-focus: normal; }
> *[tabindex^="-"] { -moz-user-focus: none; }
> Or is that too simplistic?

Keywords: access, sec508
Attachment #150957 - Flags: review?(bryner)
Comment on attachment 150957 [details] [diff] [review]
Fix the items mentioned in comment 8, plus use the default cursor when hovering over a focusable label

>--- dom/public/idl/html/nsIDOMNSHTMLElement.idl	17 Apr 2004 21:50:10 -0000	1.5
>+++ dom/public/idl/html/nsIDOMNSHTMLElement.idl	16 Jun 2004 18:09:38 -0000
>@@ -54,9 +54,12 @@
>   readonly attribute long             scrollWidth;
> 
>   readonly attribute long             clientHeight;
>   readonly attribute long             clientWidth;
> 
>+  void blur();
>+  void focus();
>+

Would it make sense to add tabIndex as an attribute here also?	Does IE allow
it to be scripted as a property?


>--- content/html/content/src/nsGenericHTMLElement.cpp	10 Jun 2004 00:11:38 -0000	1.512
>+++ content/html/content/src/nsGenericHTMLElement.cpp	16 Jun 2004 18:09:46 -0000
>+nsresult
>+nsGenericHTMLElement::Focus()
>+{
>+  // Generic HTML elements are focusable only if 
>+  // tabindex explicitly set and tabindex >= 0
>+  nsAutoString tabIndexStr;
>+  GetAttr(kNameSpaceID_None, nsHTMLAtoms::tabindex, tabIndexStr);
>+  if (!tabIndexStr.IsEmpty()) {
>+    PRInt32 rv, tabIndexVal = tabIndexStr.ToInteger(&rv);
>+    if (NS_SUCCEEDED(rv) && tabIndexVal >= 0) {
>+      SetElementFocus(PR_TRUE);
>+      return NS_OK;
>+    }
>+  }
>+
>+  return NS_ERROR_FAILURE;

This will make calling .focus() on an element that doesn't have tabindex set
throw an exception, is that intended?

r=me with those questions addressed (and if this is all as intended, no
problem, I'm just not familiar with how this works from script in IE).
Attachment #150957 - Flags: review?(bryner) → review+
(In reply to comment #13)
> (From update of attachment 150957 [details] [diff] [review])
> >--- dom/public/idl/html/nsIDOMNSHTMLElement.idl	17 Apr 2004 21:50:10 -0000	1.5
> >+++ dom/public/idl/html/nsIDOMNSHTMLElement.idl	16 Jun 2004 18:09:38 -0000
> >@@ -54,9 +54,12 @@
> >   readonly attribute long             scrollWidth;
> > 
> >   readonly attribute long             clientHeight;
> >   readonly attribute long             clientWidth;
> > 
> >+  void blur();
> >+  void focus();
> >+
> 
> Would it make sense to add tabIndex as an attribute here also?	Does IE allow
> it to be scripted as a property?
It probably makes sense, on the other hand IE doesn't allow it. Let's follow IE
for now.
 
> 
> >--- content/html/content/src/nsGenericHTMLElement.cpp	10 Jun 2004 00:11:38
-0000	1.512
> >+++ content/html/content/src/nsGenericHTMLElement.cpp	16 Jun 2004 18:09:46 -0000
> >+nsresult
> >+nsGenericHTMLElement::Focus()
> >+{
> >+  // Generic HTML elements are focusable only if 
> >+  // tabindex explicitly set and tabindex >= 0
> >+  nsAutoString tabIndexStr;
> >+  GetAttr(kNameSpaceID_None, nsHTMLAtoms::tabindex, tabIndexStr);
> >+  if (!tabIndexStr.IsEmpty()) {
> >+    PRInt32 rv, tabIndexVal = tabIndexStr.ToInteger(&rv);
> >+    if (NS_SUCCEEDED(rv) && tabIndexVal >= 0) {
> >+      SetElementFocus(PR_TRUE);
> >+      return NS_OK;
> >+    }
> >+  }
> >+
> >+  return NS_ERROR_FAILURE;
> 
> This will make calling .focus() on an element that doesn't have tabindex set
> throw an exception, is that intended?
The default tabindex is 0 for normally focusable elements, so it will only throw
an exception of something is definitely not focusable, which I think is good.

> 
> r=me with those questions addressed (and if this is all as intended, no
> problem, I'm just not familiar with how this works from script in IE).
> 

Attachment #150957 - Flags: superreview?(jst)
(In reply to comment #14)

> > Would it make sense to add tabIndex as an attribute here also?	Does IE allow
> > it to be scripted as a property?
> It probably makes sense, on the other hand IE doesn't allow it. Let's follow IE
> for now.

IE allows this to be scripted

element.tabIndex = 1
var n = element.tabIndex;

...and of course using getAttribute/setAttribute even though IE does not
correctly implement these methods.
> IE allows this to be scripted
> 
> element.tabIndex = 1
> var n = element.tabIndex;

Thanks, I accidentally was not capitalizing the letter I in tabIndex.

I'll put that in the .idl and test it. Will post new patch for jst if requested
to do so.
Blocks: 249998
(In reply to comment #14)
> (In reply to comment #13)
[...]
> > >+
> > >+  return NS_ERROR_FAILURE;
> > 
> > This will make calling .focus() on an element that doesn't have tabindex set
> > throw an exception, is that intended?
> The default tabindex is 0 for normally focusable elements, so it will only throw
> an exception of something is definitely not focusable, which I think is good.

Does IE also throw an exception? If not, I don't think we should. Throwing an
exception where people don't expect it causes their scripts to stop executing,
and that can lead to sites breaking for no good reason.
Comment on attachment 150957 [details] [diff] [review]
Fix the items mentioned in comment 8, plus use the default cursor when hovering over a focusable label

Nit of the day:

+nsGenericHTMLElement::HandleDOMEvent(nsIPresContext* aPresContext,
+				  nsEvent* aEvent,
+				  nsIDOMEvent** aDOMEvent,
+				  PRUint32 aFlags,
+				  nsEventStatus* aEventStatus)

Fix next-line indentation

sr=jst with the above comments addressed.
Attachment #150957 - Flags: superreview?(jst) → superreview+
Attached patch Final patchSplinter Review
Checking in dom/public/idl/html/nsIDOMNSHTMLElement.idl;
/cvsroot/mozilla/dom/public/idl/html/nsIDOMNSHTMLElement.idl,v  <-- 
nsIDOMNSHTMLElement.idl
new revision: 1.6; previous revision: 1.5
done
Checking in layout/html/base/src/nsTextFrame.cpp;
/cvsroot/mozilla/layout/html/base/src/nsTextFrame.cpp,v  <--  nsTextFrame.cpp
new revision: 1.466; previous revision: 1.465
done
Checking in layout/html/document/src/html.css;
/cvsroot/mozilla/layout/html/document/src/html.css,v  <--  html.css
new revision: 3.175; previous revision: 3.174
done
Checking in accessible/src/base/nsAccessibilityService.cpp;
/cvsroot/mozilla/accessible/src/base/nsAccessibilityService.cpp,v  <-- 
nsAccessibilityService.cpp
new revision: 1.113; previous revision: 1.112
done
Checking in accessible/src/base/nsBaseWidgetAccessible.h;
/cvsroot/mozilla/accessible/src/base/nsBaseWidgetAccessible.h,v  <-- 
nsBaseWidgetAccessible.h
new revision: 1.12; previous revision: 1.11
done
Checking in accessible/src/base/nsBaseWidgetAccessible.cpp;
/cvsroot/mozilla/accessible/src/base/nsBaseWidgetAccessible.cpp,v  <-- 
nsBaseWidgetAccessible.cpp
new revision: 1.27; previous revision: 1.26
done
Checking in content/base/src/nsGenericElement.cpp;
/cvsroot/mozilla/content/base/src/nsGenericElement.cpp,v  <--  nsGenericElement.cpp
new revision: 3.339; previous revision: 3.338
done
Checking in content/events/src/nsEventStateManager.cpp;
/cvsroot/mozilla/content/events/src/nsEventStateManager.cpp,v  <-- 
nsEventStateManager.cpp
new revision: 1.509; previous revision: 1.508
done
Checking in content/html/content/src/nsGenericHTMLElement.cpp;
/cvsroot/mozilla/content/html/content/src/nsGenericHTMLElement.cpp,v  <-- 
nsGenericHTMLElement.cpp
new revision: 1.520; previous revision: 1.519
done
Checking in content/html/content/src/nsGenericHTMLElement.h;
/cvsroot/mozilla/content/html/content/src/nsGenericHTMLElement.h,v  <-- 
nsGenericHTMLElement.h
new revision: 1.193; previous revision: 1.192
done
Checking in content/html/content/src/nsHTMLAnchorElement.cpp;
/cvsroot/mozilla/content/html/content/src/nsHTMLAnchorElement.cpp,v  <-- 
nsHTMLAnchorElement.cpp
new revision: 1.130; previous revision: 1.129
done
Checking in content/html/content/src/nsHTMLAreaElement.cpp;
/cvsroot/mozilla/content/html/content/src/nsHTMLAreaElement.cpp,v  <-- 
nsHTMLAreaElement.cpp
new revision: 1.80; previous revision: 1.79
done
Checking in content/html/content/src/nsHTMLButtonElement.cpp;
/cvsroot/mozilla/content/html/content/src/nsHTMLButtonElement.cpp,v  <-- 
nsHTMLButtonElement.cpp
new revision: 1.128; previous revision: 1.127
done
Checking in content/html/content/src/nsHTMLInputElement.cpp;
/cvsroot/mozilla/content/html/content/src/nsHTMLInputElement.cpp,v  <-- 
nsHTMLInputElement.cpp
new revision: 1.357; previous revision: 1.356
done
Checking in content/html/content/src/nsHTMLObjectElement.cpp;
/cvsroot/mozilla/content/html/content/src/nsHTMLObjectElement.cpp,v  <-- 
nsHTMLObjectElement.cpp
new revision: 1.67; previous revision: 1.66
done
Checking in content/html/content/src/nsHTMLSelectElement.cpp;
/cvsroot/mozilla/content/html/content/src/nsHTMLSelectElement.cpp,v  <-- 
nsHTMLSelectElement.cpp
new revision: 1.210; previous revision: 1.209
done
Checking in content/html/content/src/nsHTMLSharedObjectElement.cpp;
/cvsroot/mozilla/content/html/content/src/nsHTMLSharedObjectElement.cpp,v  <-- 
nsHTMLSharedObjectElement.cpp
new revision: 1.67; previous revision: 1.66
done
Checking in content/html/content/src/nsHTMLTextAreaElement.cpp;
/cvsroot/mozilla/content/html/content/src/nsHTMLTextAreaElement.cpp,v  <-- 
nsHTMLTextAreaElement.cpp
new revision: 1.156; previous revision: 1.155
done
Status: NEW → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
I'm tempted to do a partial backout until problems can be resolved.
i have to click twice or three times on select dropdowns to make them open now -
also a regression from here? Similar: bug 250191
That's bug 250208
indeed, bug 250208 was caused by this bug (backing this one out fixes it)
Could this have caused bug 250352?
Another bug that seems to stem from this one: bug 250858. Please see if you can
reproduce it :)
This change made it impossible to focus textfields with the mouse if the form
they're in has a high tabindex (see bug 252984 on John Kerry's "contact" page).
If a form control has a negative tab index and is thus taken out of the tab 
order, should it still be focusable by the mouse? The Web Apps spec says yes at 
the moment.
Yes it should be.

An example of something with a negative tabindex would be a grid cell. The grid
itself would have a nonnegative tabindex so it can get tabbed to. Onfocus the
grid would set focus() to the last focused cell. Arrow keys would be captured
and the appropriate cell would get cell(). You could also click on a cell
directly to focus it.


(In reply to comment #30)
> If a form control has a negative tab index and is thus taken out of the tab 
> order, should it still be focusable by the mouse? The Web Apps spec says yes at 
> the moment.
*** Bug 295973 has been marked as a duplicate of this bug. ***
Component: Keyboard: Navigation → User events and focus handling
You need to log in before you can comment on or make changes to this bug.