417760 - cannot focus() img elements with tabindex="-1"

Status: VERIFIED FIXED

(Core :: Disability Access APIs, defect, P2)

Description

[Simon Bates]

To reproduce:

* open the attached test file focus-img-tabindex-minus-one.html
* click on the button 'focus img tabindex="-1"'

Expected:

* focus to go to the img with tabindex="-1"

Actual:

* focus does not move

Tested with:

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b4pre) Gecko/2008021504 Minefield/3.0b4pre

Comment 1

[Simon Bates]

Attachment: focus-img-tabindex-minus-one.html

Comment 2

[Aaron Leventhal]

Attachment: Simple patch -- check if tabindex attr present

Comment 3

[alexander :surkov (:asurkov)]

Sorry, I thought
-1 means not focusable
0, focusable but not navigable
>0 defines navigation order

Isn't it true?

Comment 4

[Aaron Leventhal]

No, that's not right. See this table:
http://developer.mozilla.org/en/docs/Key-navigable_custom_DHTML_widgets#The_solution:_changes_to_standard_behavior_of_tabindex

Comment 5

[Aaron Leventhal]

Mats, this is marked blocking. How about some review love?

Comment 6

[Mats Palmgren (inactive)]

Comment on attachment 303652 [details] [diff] [review]
Simple patch -- check if tabindex attr present

This looks good. r+sr=mats
Please add a mochitest as well, calling focus() on a few different elements
and verify it has focus (or not) using 'activeElement' for example.
At least something simple that covers this bug.

That said, there are still a couple of issues in this code, although these
can probably wait to post-1.9:
1. we should return false if !IsInDoc(), not just for image map:
http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/content/html/content/src/nsHTMLImageElement.cpp&rev=1.224&root=/cvsroot&mark=442#436
   and a better test is probably what nsGenericHTMLElement::IsFocusable does:
http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/content/html/content/src/nsGenericHTMLElement.cpp&rev=1.756&root=/cvsroot&mark=3119-3127,3133#3116

2. this block should be removed?:
http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/content/html/content/src/nsHTMLImageElement.cpp&rev=1.224&root=/cvsroot&mark=461-464#436
   <input type="image"> is not handled by this method, if that
   was the assumption

We should refactor these methods (post-1.9) such that a subclass
IsFocusable() can re-use common code from a superclass...

Please file a followup bug?

Comment 7

[Mike Schroepfer]

Comment on attachment 303652 [details] [diff] [review]
Simple patch -- check if tabindex attr present

You are blocking - approval not needed.  Land away

Comment 8

[alexander :surkov (:asurkov)]

checked in

/cvsroot/mozilla/content/html/content/src/nsHTMLImageElement.cpp,v  <--  nsHTMLImageElement.cpp
new revision: 1.225; previous revision: 1.224
done

Comment 9

[Boris Zbarsky [:bzbarsky]]

So...  the review included a request for a test.  I assume that's being worked on, still?

Comment 10

[alexander :surkov (:asurkov)]

Marco, since Aaron is away several days then could you care about mochitest for this?

Comment 11

[Marco Zehe (:MarcoZ)]

(In reply to comment #9)
> So...  the review included a request for a test.  I assume that's being worked
> on, still?

I've created follow-up bug 424304 for this Mochitest. I'll work on it there.

Comment 12

[Mats Palmgren (inactive)]

Filed bug 425087 to followup on the remaining issues from comment 6.

Comment 13

[Carsten Book [:Tomcat]]

verified fixed using the testcase and Mozilla/5.0 (Windows; U; Windows NT 5.2; en-US; rv:1.9pre) Gecko/2008042705 Minefield/3.0pre ID:2008042705