Closed Bug 386996 Opened 13 years ago Closed 13 years ago

Can't tab past disabled inputs or textareas

Categories

(Core :: DOM: UI Events & Focus Handling, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla1.9alpha8

People

(Reporter: neil, Assigned: peterv)

References

()

Details

(Keywords: access, regression)

Attachments

(1 file, 1 obsolete file)

Since about the 28th (I had to ask ajschult to work out a regression window because I've been away) it is impossible to tab past a disabled <input> (of whatever type) or <textarea> although <button> and <select> are OK.
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a7pre) Gecko/2007070304 Minefield/3.0a7pre

Using the builds from http://hourly-archive.localgho.st/win32.html, I verified this regressed between 20070627_1935 and 20070627_2053, making this a regression from bug 237964.
Keywords: access
Summary: Can't tab paste disabled inputs or textareas → Can't tab past disabled inputs or textareas
Attached patch v1 (obsolete) — Splinter Review
This fixes the testcase, but I need to think a bit more about the interaction between the disabled and contentEditable attributes.
Assignee: nobody → peterv
Status: NEW → ASSIGNED
Comment on attachment 271219 [details] [diff] [review]
v1

IsEditableRoot() returns true for input and textarea, but we shouldn't ignore the disabled attribute for those.
Attachment #271219 - Flags: superreview?(jonas)
Attachment #271219 - Flags: review?(jonas)
Hmm.. why should we ignore the disabled attribute when contenteditable is set to true? And why does this fix this bug since most of the time contenteditable is not set?
(In reply to comment #4)
> Hmm.. why should we ignore the disabled attribute when contenteditable is set
> to true?

Because you want to edit them and so they you should be able to tab to them.

> And why does this fix this bug since most of the time contenteditable
> is not set?

The contenteditable patch changed IsFocusable so that it ignores the disabled attribute for editable roots. Inputs or textareas are editable roots, but we should not ignore the disabled attribute on them (and tabindex should be -1 for disabled inputs/textareas).
I should probably have given you more context:

Index: content/html/content/src/nsGenericHTMLElement.cpp
===================================================================
RCS file: /Users/peterv/source/cvs.mozilla.org/cvsroot/mozilla/content/html/content/src/nsGenericHTMLElement.cpp,v
retrieving revision 1.723
diff -u -p -u -1 -0 -r1.723 nsGenericHTMLElement.cpp
--- content/html/content/src/nsGenericHTMLElement.cpp	8 Jul 2007 07:08:08 -0000	1.723
+++ content/html/content/src/nsGenericHTMLElement.cpp	9 Jul 2007 08:40:37 -0000
@@ -3306,23 +3306,26 @@ nsGenericHTMLElement::RemoveFocus(nsPres
   }
 }
 
 PRBool
 nsGenericHTMLElement::IsFocusable(PRInt32 *aTabIndex)
 {
   PRInt32 tabIndex = 0;   // Default value for non HTML elements with -moz-user-focus
   GetTabIndex(&tabIndex);
 
   PRBool disabled;
-  if (IsEditableRoot()) {
+  if (IsEditableRoot() && GetContentEditableValue() == eTrue) {
+    // Ignore the disabled attribute in editable contentEditable roots.
     disabled = PR_FALSE;
     if (!HasAttr(kNameSpaceID_None, nsGkAtoms::tabindex)) {
+      // The default value for tabindex should be 0 for editable
+      // contentEditable roots.
       tabIndex = 0;
     }
   }
   else {
     // Just check for disabled attribute on all HTML elements
     disabled = HasAttr(kNameSpaceID_None, nsGkAtoms::disabled);
     if (disabled) {
       tabIndex = -1;
     }
   }
Comment on attachment 271219 [details] [diff] [review]
v1

>+  if (IsEditableRoot() && GetContentEditableValue() == eTrue) {

So can you remove the IsEditableRoot() part here then? Isn't the only reason that an htmlelement is IsEditableRoot() that it's a textarea/input or it has GetContentEditableValue() == eTrue? And we don't want the former to enter this path?

r/sr=me either way.
Attachment #271219 - Flags: superreview?(jonas)
Attachment #271219 - Flags: superreview+
Attachment #271219 - Flags: review?(jonas)
Attachment #271219 - Flags: review+
(In reply to comment #6)
> Isn't the only reason
> that an htmlelement is IsEditableRoot() that it's a textarea/input or it has
> GetContentEditableValue() == eTrue?

Yes, but not all elements with contenteditable=true are editable roots (or "hosts" as HTML5 calls it). See http://www.whatwg.org/specs/web-apps/current-work/#contenteditable
Attached patch v1.1Splinter Review
I ended up changing IsEditableRoot to only return PR_TRUE for designMode and contentEditable (and also removed the unused FindEditableRoot). Also includes a mochitest. Sorry for the double-review :-/.
Attachment #271219 - Attachment is obsolete: true
Attachment #271887 - Flags: superreview?(jonas)
Attachment #271887 - Flags: review?(jonas)
Attachment #271887 - Flags: superreview?(jonas)
Attachment #271887 - Flags: superreview+
Attachment #271887 - Flags: review?(jonas)
Attachment #271887 - Flags: review+
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9beta1
Duplicate of this bug: 389656
Component: Keyboard: Navigation → User events and focus handling
You need to log in before you can comment on or make changes to this bug.