124946 - F6 and ctrl+tab no longer correctly cycles thru frames [tho' shift+F6/ctrl+shift+tab do work]

Status: VERIFIED FIXED

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

Description

[sairuh (rarely reading bugmail)]

found using 2002.02.11.1x comm bits on win2k and linux rh7.2. n/a on mac.

1. load a page containing frames, such as http://faqs.org/ or even viewer test
#9. have a copy of the latter at http://hopey.mcom.com/tests/test9.html
2. hit F6 to cycle through the frames.

results: F6 only goes btwn the urlbar and the first frame.

expected: F6 should cycle through the urlbar and all frames.

strangely, shift+F6 works fine.

will try narrow down when this regressed.

Comment 1

[sairuh (rarely reading bugmail)]

looking at the linux builds i have, this regressed btwn the 2002.01.31.08
[working] and 2002.02.04.17 [broken] builds.

Comment 2

[sairuh (rarely reading bugmail)]

narrowed down some more, based on linux comm builds: works fine in 2002.02.01.08
build, but broken in 2002.02.01.21 build.

side note: fwiw, F6 [and shift+F6] works fine in the mail 3pane window.

Comment 3

[sairuh (rarely reading bugmail)]

at first i thought that ssu's accessibility checkins for the mail 3pane might be
involved. but then i searched and noticed he checked in around 1/29-ish, and the
builds btwn then and before the 9pm 2/1 build behaved just fine.

Comment 4

[Aaron Leventhal]

-> trudelle, for triage

Comment 5

[Blake Ross]

-> bryner

Comment 6

[sairuh (rarely reading bugmail)]

ctrl+tab is also broken --although ctrl+shift+tab works.

Comment 7

[Peter Trudelle]

nsbeta1- per ADT triage team

Comment 8

[Aaron Leventhal]

topembed+, per Chris Saari

Comment 9

[Brian Ryner (not reading)]

clearing nsbeta1- for retriage.

F6/Ctrl+tab is a big deal for keyboard navigation and accessibility, I think.

Comment 10

[Peter Trudelle]

Nav triage team needs info: Is this required for section 508, or general
accessibility?

Comment 11

[Aaron Leventhal]

This is not required for Section 508, but it is a major accessibility
regression. It's a very important keystroke!

Comment 12

[Peter Trudelle]

nsbeta1+ per Nav triage team.

Comment 13

[Kyle Yuan]

Attachment: patch

The logic of nsEventStateManager::GetNextDocShell()
is wrong when a item has not any children. In this case,
the function should return its next sibling (if exist),
rather than return nsnull.

Comment 14

[Kyle Yuan]

Hi, aaron and bryner, could you take a r= of this patch?

Comment 15

[Aaron Leventhal]

Okay, I understand that we were leaving the method too early if numChildren was
0 - it should still try to get the next sibling.

However, please start out the method with the line:
*aResult = nsnull;

So that it always sets it to null first.
This is good practice with "pure out parameters", in case the method returns early.

Do that, and I will put r=aaronl 

Comment 16

[Kyle Yuan]

Attachment: patch 2

*aResult = nsnull; was added.

This is a major accessibility bug. 
I think this patch should be checked in before 1.0 release.

Comment 17

[Brian Ryner (not reading)]

rjesup, I think you broke this in rev 1.322 (bug 117667).

Comment 18

[Brian Ryner (not reading)]

Comment on attachment 76114 [details] [diff] [review]
patch 2

r=bryner

Comment 19

[Randell Jesup [:jesup] (needinfo me)]

This was caused by my patch.

r=[e-mail]

Comment 20

[Joe Hewitt (gone)]

Comment on attachment 76114 [details] [diff] [review]
patch 2

sr=hewitt

Comment 21

[Asa Dotzler [:asa]]

Comment on attachment 76114 [details] [diff] [review]
patch 2

a=asa (on behalf of drivers) for checkin to the 1.0 trunk

Comment 22

[Aaron Leventhal]

Fixed on trunk. We are very grateful to Kyle Yuan for the patch.

Comment 23

[sairuh (rarely reading bugmail)]

this is still is broken for me, using 2002.03.27.11 commercial verif bits on win2k.

reopening.

Comment 24

[sairuh (rarely reading bugmail)]

strange, this works on http://faqs.org/ and http://warp.mcom.com/ --but for some
reason is *broken* on http://wired.com/ and viewer test #9 for frames.

Comment 25

[Aaron Leventhal]

Kyle, you can find viewer test #9 under the Mozilla Debug menu -> Viewer Demos.

Very strange intermiitent behaviors are happening. It seems to be somtimes
affected by tabbed browser mode (Accel+T). However, wired.com never seems to
work correctly.

If you need more test cases, you can contact [e-mail]. 

Comment 26

[Kyle Yuan]

I'm working on this issue. Hope it can be fixed soon.

Comment 27

[Kyle Yuan]

Attachment: additional patch

Another wrong logic in nsEventStateManager::GetNextDocShell.

Seeking next doc shell will stop, when the current item is the last child.
Fixed!

Comment 28

[Brian Ryner (not reading)]

Comment on attachment 76493 [details] [diff] [review]
additional patch

r=bryner

Comment 29

[Randell Jesup [:jesup] (needinfo me)]

I concur, r=[e-mail] for what it's worth.

Comment 30

[Joe Hewitt (gone)]

Comment on attachment 76493 [details] [diff] [review]
additional patch

sr=hewitt

Comment 31

[Asa Dotzler [:asa]]

Comment on attachment 76493 [details] [diff] [review]
additional patch

a=asa (on behalf of drivers) for checkin to the 1.0 trunk

Comment 32

[Aaron Leventhal]

checked in

Comment 33

[sairuh (rarely reading bugmail)]

hey, this looks good --tested using 2002.04.09 comm bits on linux rh7.2, win2k
and mac 10.1.3.

the only hitch i found is a "dead spot" while cycling --but only on linux. gonna
verify this one, and filed bug 136420.