344850 - Tabbing from <radio> or <textbox> with tabindex fails

Status: RESOLVED FIXED

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

Description

[Stephen Donner [:stephend] Not actively reading bugmail]

Build ID: 2006-07-14-23

In Composer, I can't tab from the "Alternate Text" radio button into its adjacent textfield.

Comment 1

[Stephen Donner [:stephend] Not actively reading bugmail]

Complete steps:

1. In Composer, click on Insert | Image...
2. Tab to "Alternate Text"

Expected Results:

Take me to the adjacent textfield

Actual Results:

Won't tab futher.

Comment 2

[Serge Gautherie (:sgautherie)]

[Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.8.1b1) Gecko/20060717 SeaMonkey/1.1a] (nightly) (W98SE)

I confirm.

And Shift-Tab is not working from 'Tooltip' either.

Comment 3

[[e-mail]]

SeaMonkey 1.0.0 has the bug, although Mozilla 1.7 does not, but that's still a big regression range, anyone care to start narrowing it down?

Comment 4

[Serge Gautherie (:sgautherie)]

[Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.8.1b1) Gecko/20060717 SeaMonkey/1.1a] (nightly) (W98SE)

I don't think it's related,
but (anyway) I get the following error each time the "Insert > Image..." dialog opens
[
Error: redeclaration of const MOZILLA_CONTENT_PACK
Source File: chrome://help/content/contextHelp.js
Line: 40
]

I don't seem to get the error with the other menu items...

Comment 5

[Serge Gautherie (:sgautherie)]

[Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.7.13) Gecko/20060414] (release) (W98SE)


In this release, Tab and Shit-Tab work;
I was only kind of surprised that the (2 buttons) radio group comes "after" its associated textfield.
[Anyway, now I know what behaviour(s) to look for.]

***

[
Error: redeclaration of const MOZILLA_CONTENT_PACK
Source File: chrome://help/content/contextHelp.js
Line: 24
]
already there: that part is not a (recent) regression.

As a reminder (for now), 3 bugs have comments with this const name:
bug 162432, bug 271992, bug 260058.

Comment 6

[Serge Gautherie (:sgautherie)]

**Summary:

I found 3 change/regression timeframes...
I searched and used the (+/- rarely) available Windows builds.
If further narrowing of the timeframes is needed, you'll have to do it with Linux builds.!.

**Details:

*Initially:
 *Working (like v1.7.13).

(...)
2004-04-10-11-trunk : Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.7b) Gecko/20040410
2004-04-18-10-trunk : Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.8a) Gecko/20040418
2004-06-25-09-trunk : Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.8a2) Gecko/20040625
2004-06-26-09-trunk : Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.8a2) Gecko/20040626

*After that build,
 *Much like the current behaviour, but it seems to need 2 Tab to leave the first textarea...

2004-07-12-15-trunk : Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.8a2) Gecko/20040712

*After that build,
 *Tab "cycle": Location > Tooltip > Alt.Text (area) > 'Tab_Title'.
 *Shift-Tab "cycle": Tooltip < Alt.Text (area) < Cancel < Help < Location.
 *File and the radio group are +/- ignored (I did look precisely.)

2004-08-19-10-trunk : Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.8a3) Gecko/20040819
2004-09-02-07-trunk : Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.8a3) Gecko/20040902
2005-01-05-07-trunk : Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.8a6) Gecko/20050105
2005-02-12-06-trunk : Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.8b) Gecko/20050212
2005-03-04-07-trunk : Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.8b2) Gecko/20050304
2005-03-12-06-trunk : Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.8b2) Gecko/20050312
2005-03-14-06-trunk : Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.8b2) Gecko/20050314

*After that build,
 *Shift-Tab doesn't work in the (3) textareas.
 *Tab doesn't work on the radio group.

2005-03-15-07-trunk : Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.8b2) Gecko/20050315
2005-03-16-06-trunk : Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.8b2) Gecko/20050316
2005-03-20-06-trunk : Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.8b2) Gecko/20050320
2005-04-04-07-trunk : Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.8b2) Gecko/20050404
2005-04-05-07-trunk : Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.8b2) Gecko/20050405
[Before: http://archive; After: Ftp://ftp]
2005-07-05-07-trunk : Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.8b2) Gecko/20050705 SeaMonkey/1.0a
(...)

Comment 7

[Serge Gautherie (:sgautherie)]

(In reply to comment #6)
>  *File and the radio group are +/- ignored (I did look precisely.)

Ah: 'I did _not_ look precisely'.

Comment 8

[Mats Palmgren (inactive)]

Attachment: Testcase #1

Comment 9

[Mats Palmgren (inactive)]

Attachment: Testcase #2a

Comment 10

[Mats Palmgren (inactive)]

Attachment: Testcase #2b (as #2a but without tabindex)

Comment 11

[Mats Palmgren (inactive)]

Attachment: Testcase #3 (simple <textbox> only)

Comment 12

[Mats Palmgren (inactive)]

Attachment: Patch rev. 1

There are two separate bugs here.
1. when we have a radio control with a tabindex attribute (making it
   tabbable) we get into trouble because the radio always puts back
   focus on the radiogroup when it is focused. 
http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/toolkit/content/widgets/radio.xml&rev=1.23&root=/cvsroot&mark=433,437#433
   So, on the next TAB we start again at the radiogroup,
   finding the tabbable radio item that puts back focus on the group...
2. for anonymous content with a tabindex, like for example the html:input
   inside a textbox with a tabindex we have a simliar situation for
   Shift-TAB (the textbox puts the focus back on the html:input).

For the reported Composer dialog it would be enough to move the tabindex
attribute to the radiogroup (to fix 1), but I have included a fix that tries
to solve the problem when individual <radio> items are tabbable in case
someone wants to use that for some reason.
http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/editor/ui/dialogs/content/EdImageOverlay.xul&rev=1.10&root=/cvsroot&mark=72,91,96#71

With this patch I get the expected behaviour with both forward/backward
tabbing on the reported dialog, the testcases and a lot of other testing
I did on Linux. I also (briefly) tested Firefox on Windows and Camino on
MacOSX.

About the "control" property - it seems to me this have never worked so
we could just rename it to "radioGroup" (which is implemented) in the IDL
file instead, what do you think?
http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/dom/public/idl/xul/nsIDOMXULSelectCntrlItemEl.idl&rev=1.3&root=/cvsroot&mark=55#43

Comment 13

[Boris Zbarsky [:bzbarsky]]

I'm not sure I can sanely review the XUL changes; one of the Neils would be bette there...  I can try to do the ESM changes, but again it would be better if someone more familiar with the code reviewed; I'd be happy to sr, but I don't feel like I know this code well enough to do the r.

Comment 14

[Mats Palmgren (inactive)]

Comment on attachment 231513 [details] [diff] [review]
Patch rev. 1

Neil, can you review the XUL changes please?
Bryner, can you review the ESM changes please?

Comment 15

[[e-mail]]

Comment on attachment 231513 [details] [diff] [review]
Patch rev. 1

>+      <property name="control">
readonly="true" perhaps?

Comment 16

[[e-mail]]

(In reply to comment #12)
>About the "control" property - it seems to me this have never worked so
>we could just rename it to "radioGroup" (which is implemented) in the IDL
>file instead, what do you think?
I think that idea is worse than the reverse of renaming "radioGroup" to be "control", but your patch works too.

Comment 17

[Brian Ryner (not reading)]

Comment on attachment 231513 [details] [diff] [review]
Patch rev. 1

This code can be tricky and I don't have time to do a thorough review on it.  I'd suggest Aaron Leventhal.

Comment 18

[Mats Palmgren (inactive)]

Comment on attachment 231513 [details] [diff] [review]
Patch rev. 1

Aaron, can you review the nsEventStateManager changes please?

Comment 19

[Aaron Leventhal]

Mats, I think you need an sr= more than a second review. I don't really want to or have time to touch this right now.

Comment 20

[Mats Palmgren (inactive)]

Comment on attachment 231513 [details] [diff] [review]
Patch rev. 1

I need review on the ESM changes and superreview on the whole patch.
Boris, I'm sorry my requests seems to land on your or roc's plate
every time, but I don't know anyone else who knows this code.

Comment 21

[Boris Zbarsky [:bzbarsky]]

Mats, I'll try to figure this code out enough to review, but that will probably take time... :(  If it doesn't happen in the next week or so, it probably won't till after Sept 3, at least.

Comment 22

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 231513 [details] [diff] [review]
Patch rev. 1

roc, if you're at all familiar with this code, could you review?  This is the bug I was telling you about the other day...

Comment 23

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 231513 [details] [diff] [review]
Patch rev. 1

I can't give this code good review, but generally I feel it's better to do what one can rather than block things indefinitely. Especially when the contributor is someone like Mats who I'm confident will chase down any regressions :-)

Comment 24

[[e-mail]]

Comment on attachment 231513 [details] [diff] [review]
Patch rev. 1

>+    if (startRadioGroup &&
>+        NS_SUCCEEDED(startRadioGroup->GetSelectedItem(getter_AddRefs(radio))) &&
>+        radio) {
>+      nsIContent* start = nsnull;
>+      CallQueryInterface(radio, &start);
>+      if (start) {
>+        // Start traversing at the selected <radio> item.
>+        aStartFrame = start ?
>+          mPresContext->GetPresShell()->GetPrimaryFrameFor(start) : nsnull;
I just noticed... nsIContent is a refcounted interface, so you should do something like this:
if (startRadioGroup &&
    NS_SUCCEEDED(startRadioGroup->GetSelectedItem(getter_AddRefs(radio)))) {
  nsCOMPtr<nsIContent> start(do_QueryInterface(radio));
  if (start) {
    // Start traversing at the selected <radio> item.
    aStartFrame = mPresContext->GetPresShell()->GetPrimaryFrameFor(start);

>       else if ((aIgnoreTabIndex || mCurrentTabIndex == tabIndex) &&
>-          currentContent != aStartContent) {
>+               currentContent != aStartContent) {
Weird indentation change here.

Comment 25

[Serge Gautherie (:sgautherie)]

Mats, could you post a new patch with comment 24 suggestions ?

Comment 26

[Mike Schroepfer]

Is this patch still relevant?  Issue still there? Stephen or Mats can you catch us up?

Comment 27

[Stephen Donner [:stephend] Not actively reading bugmail]

(In reply to comment #26)
> Is this patch still relevant?  Issue still there? Stephen or Mats can you catch
> us up?

Schrep: I don't know about the patch, but the bug's still there; just waiting on Mats' follow-up patch/reply.

Comment 28

[Reed Loden [:reed]]

Mats, ping?

Comment 29

[Samuel Sidler (old account; do not CC)]

A regression in accessibility on the trunk with a testcase and an almost-completed patch isn't blocking?

Re-requesting blocking. There's no reason we shouldn't fix this.

Comment 30

[Robert Sayre]

> A regression in accessibility on the trunk with a testcase and an
> almost-completed patch isn't blocking?

Last patch was in 2006. Would love to get this in, of course.

Mats, ping?

Comment 31

[Stephen Donner [:stephend] Not actively reading bugmail]

Zach, any chance you could look at this?

Comment 32

[Jason Levine]

I would imagine that there's no chance of getting this into 1.9.1 (FF 3.5), but it's an old bug that's still biting people, including in a web app we use daily.  Any chance of getting this bugfix rolling?

Comment 33

[[e-mail]]

This seems to have been fixed for 1.9.2 by the focus manager rewrite although I can't seem to tab backwards from any of the text fields for some reason.

Comment 34

[Neil Deakin]

Attachment: when tabbing backwards, skip nodes that would redirect their focus

Comment 35

[Neil Deakin]

http://hg.mozilla.org/mozilla-central/rev/5cb576a2923a

Comment 36

[Neil Deakin]

This is already fixed in 1.9.2.