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

VERIFIED FIXED

Status

()

Core
Disability Access APIs
P2
major
VERIFIED FIXED
11 years ago
10 years ago

People

(Reporter: Simon Bates, Assigned: Aaron Leventhal)

Tracking

(Blocks: 1 bug)

Trunk
x86
Windows XP
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.9 +
in-testsuite ?

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: Simple patch. Needed for ARIA when img is used for child widgets)

Attachments

(2 attachments)

(Reporter)

Description

11 years ago
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
(Reporter)

Comment 1

11 years ago
Created attachment 303562 [details]
focus-img-tabindex-minus-one.html
(Assignee)

Comment 2

11 years ago
Created attachment 303652 [details] [diff] [review]
Simple patch -- check if tabindex attr present
Attachment #303652 - Flags: superreview?(mats.palmgren)
Attachment #303652 - Flags: review?(mats.palmgren)
Sorry, I thought
-1 means not focusable
0, focusable but not navigable
>0 defines navigation order

Isn't it true?
(Assignee)

Updated

11 years ago
Blocks: 343213
Severity: normal → major
Flags: blocking1.9?
Whiteboard: Simple patch. Seeking r+. Needed for ARIA when img is used for child widgets

Updated

11 years ago
Flags: blocking1.9? → blocking1.9+
Priority: -- → P2
(Assignee)

Comment 5

11 years ago
Mats, this is marked blocking. How about some review love?
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?
Attachment #303652 - Flags: superreview?(mats.palmgren)
Attachment #303652 - Flags: superreview+
Attachment #303652 - Flags: review?(mats.palmgren)
Attachment #303652 - Flags: review+
Whiteboard: Simple patch. Seeking r+. Needed for ARIA when img is used for child widgets → Simple patch. Needed for ARIA when img is used for child widgets
Flags: in-testsuite?
(Assignee)

Updated

11 years ago
Attachment #303652 - Flags: approval1.9?

Comment 7

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

You are blocking - approval not needed.  Land away
Attachment #303652 - Flags: approval1.9? → approval1.9+
checked in

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

Status: NEW → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED
So...  the review included a request for a test.  I assume that's being worked on, still?
Marco, since Aaron is away several days then could you care about mochitest for this?

Updated

11 years ago
Depends on: 424304

Comment 11

11 years ago
(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.
Filed bug 425087 to followup on the remaining issues from comment 6.
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
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.