Tabbing order does not follow specified tabindex order

RESOLVED FIXED

Status

()

defect
P2
major
RESOLVED FIXED
12 years ago
5 months ago

People

(Reporter: mats, Assigned: mats)

Tracking

(Blocks 1 bug, {html4, regression, testcase})

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

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 1 obsolete attachment)

Posted file Testcase
Tabbing order does not follow specified tabindex order.

STEPS TO REPRODUCE
1. open the attached testcase
2. press TAB to navigate through the elements

ACTUAL RESULTS
2 then 0 then location bar then 1

EXPECTED RESULTS
1 then 2 then 0 then location bar

PLATFORMS AND BUILDS TESTED
Bug occurs Firefox 2007-05-31 on Linux
Bug does NOT occur in Firefox 2007-04-14-04 on Linux
Bug occurs in Firefox 2007-04-15-04 on Linux

I'm guessing it's a regression from bug 144000
Flags: blocking1.9?
Flags: blocking1.9? → blocking1.9+
Posted file Tab and Shift+Tab differs (obsolete) —
1) Click on 'pick' element
2)Than if you tabbing ( TAB ) through this page you get next order:
pick,green, blue
3)Than if you tabbing back ( Shift+TAB ) you get next order:
blue, red, green, pick

Expected behaviour:
2) pick, green, red, blue 3) blue, red, green, pick


The code must be next:
You must have sort all nodes by next rule:
Exclude all DOM nodes with tabIndex===-1 or tabIndex===undefined
Push all enabled nodes with tabIndex>=1 to the TABARRAY
Sort array by ascending tabIndex
currNode -- the node that has focus now


function nextTabNode( currNode ) {
 if( currNode.TABARRAY.length > 0 ) {//If we have child that can be tabbed
   return currNode.TABARRAY[0]//return first tab child
   }

 while( currNode.parentNode ) {
   //Check if we not last tab element
   if( currNode.MYTABINDEX +1 < currNode.parentNode.TABARRAY.length ) {
     //return next tab element
     return currNode.parentNode.TABARRAY[ currNode.MYTABINDEX +1 ];
     }

   //go up to the parent and try to tab to its next element
   currNode= currNode.parentNode;
   }

 //There is no next tab element. FF can tab else where
 return null;
 }

For back tabbing use next condition:
...
   if( currNode.MYTABINDEX -1 >= 0 ) {
     return currNode.parentNode.TABARRAY[ currNode.MYTABINDEX -1 ];
     }
...
Uri, can you please take a look at this?
I'm swamped with work, but I'll try to find time to look at this sometime this week. However, if I see I'm not getting to it, we might need to just back out 144000. Sorry.
Can someone confirm this bug is still reproducible? I used to be able to reproduce with the second testcase but now i'm not.

And if someone could better explain how the first testcase is supposed to work, that would be nice. There are so many 0s 1s and 2s I just get confused.
I can reproduce using the original testcase (not sure what the 2nd testcase tries to test)
Comment on attachment 269996 [details]
Tab and Shift+Tab differs

marking this testcase obsolete as it was apparently fixed some time ago
Attachment #269996 - Attachment is obsolete: true
Priority: -- → P4
Taking. I hope I don't have to backout bug 144000, but find some better solution.
Assignee: nobody → Olli.Pettay
Depends on: 382192
After fighting with this some time, I think finding a better fix for bug 144000
is the right thing to do. That would fix bug 382192 too.
In the long term, I think that fixing this bug by making Selection code not suck so much is the right thing to do (I actually believe that the fix for 144000 is the correct fix). However, given the coming release, I agree that backing out 144000 is probably our best option right now.
Posted patch wip1Splinter Review
Hang on, I've also been looking at this for a couple of days now, while
reviewing bug 382192...  I think something like this actually solves
all problems without having to backout bug 144000.
I will comment some more on bug 382192 in a bit.
Posted patch wip2Splinter Review
FWIW, same as "wip1" adding one hunk from Uri's patch in bug 382192:
the improved focusability tests which allows for focusing an ancestor
when the caret moves in to it (which fixes bug 376369). I think this is a good
thing but there is a problem: the caret gets stuck in an <input> and can't
be moved outside just using the arrow keys. So I think we should wait with
this bit and fix it separately (in bug 376369 I guess).
Comment on attachment 288626 [details] [diff] [review]
wip1

BTW, the "selectionContent == endSelectionContent" is a bit lame,
what it should test is if the selection is collapsed...
I'm testing wip2...
Wip2 seems to work fine here, and fixes bug 382192 too (not bug 376369).
Is there still something else you're going to change (than comment #12)?
If Uri could review this, great, otherwise I can too.
Please get the tests in when landing.
Flags: in-testsuite?
Btw, since mac handles tab-pressing in a bit different way, the tests may need to
do something similar what is done in http://lxr.mozilla.org/seamonkey/source/content/events/test/test_bug238987.html
Mats, we're you going to ask review for the patch?
Mats, assigning to you since you have the patch.
Hope it is ok.
Assignee: Olli.Pettay → mats.palmgren
Mats, any news on this?
IMO, this should be higher than P4
->P2
Priority: P4 → P2
Flags: tracking1.9+ → blocking1.9+
Duplicate of this bug: 409588
Duplicate of this bug: 422532
Just a note -- this badly breaks keyboard navigation for forms on Wikipedia and other MediaWiki-based sites (eg http://en.wikipedia.org/wiki/Special:Userlogin )
just to mention that this is still an issue in the latest version 3 beta 5, and it's a serious keyboard accessibility problem...
Mats, any update? Otherwise I think we should just back out bug 144000.
I'm inclined to agree with the backout; it feels like we traded brokenness here, and I'd rather take the one that doesn't regress us.
(In reply to comment #25)
> I'm inclined to agree with the backout; it feels like we traded brokenness
> here, and I'd rather take the one that doesn't regress us.
> 

Agreed - Uri can you help us here?  
Patch to back out bug 144000 now attached to that bug. Still needs testing and a couple of eyes to look at it before committing.
(In reply to comment #26)
> (In reply to comment #25)
> > I'm inclined to agree with the backout; it feels like we traded brokenness
> > here, and I'd rather take the one that doesn't regress us.
> > 
> 
> Agreed - Uri can you help us here?  
> 

Sorry. I suggested backing out 144000 a while ago, but was asked to hold. I'm traveling now, an generally have very little Mozilla bandwidth these weeks. Thanks for picking this up, Johnny.
It's really not clear what's the exact right behavior here with the testcase in this bug, but backing out the fix for bug 144000, using the backout patch in that bug, does get us back to working like Firefox 2 is wrt tabindex on this page. I fired off some tryserver builds with that fix too, so maybe others can help test that as well once they're available...
Mac build looks good on Wikipedia login form!
tried all the things mentioned on my duplicate bug https://bugzilla.mozilla.org/show_bug.cgi?id=409588 and the test build works as it should on WinXP with a fresh profile.
Fixed by backing out bug 144000.
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Component: Keyboard: Navigation → User events and focus handling
You need to log in before you can comment on or make changes to this bug.