363934 - keyboard tab bar navigation broken

Status: RESOLVED FIXED

(Firefox :: Tabbed Browser, defect)

Description

[Peter van der Woude [:Peter6]]

Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9a1) Gecko/20061214 Minefield/3.0a1 ID:2006121406 [cairo]

repro:
open multiple tabs
focus 1 tab
use leftarrow|rightarrow|home|end keys to navigate

result:
it works only the first time , nothing happens when you hit any of the above tabs after that

http://bonsai.mozilla.org/cvsquery.cgi?module=PhoenixTinderbox&date=explicit&mindate=1165984140&maxdate=1166020919

->Bug 330705

Comment 1

[Martijn Wargers (dead)]

Isn't this fixed now that I partially backed out the patch from bug 330705?

Comment 2

[Peter van der Woude [:Peter6]]

Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9a1) Gecko/20061214 Minefield/3.0a1 ID:2006121422 [cairo]

No

Comment 3

[Peter van der Woude [:Peter6]]

it works only the first time , nothing happens when you hit any of the above
<del>tabs</del> keys after that

Comment 4

[Martijn Wargers (dead)]

Attachment: patch

This fixes it, but I'm not really sure why.
Apparently, the adding of a functional blur method for xul elements, which I did in bug 330705, has caused this bug.

Comment 5

[Peter van der Woude [:Peter6]]

additional:

if you quickly rightarrow, you can get to other tabs.
So the focus is properly applied, but lost due to one or another timeout.

Comment 6

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #4)
> This fixes it, but I'm not really sure why.
> Apparently, the adding of a functional blur method for xul elements, which I
> did in bug 330705, has caused this bug.

Perhaps due to http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/toolkit/content/widgets/tabbrowser.xml&rev=1.216#669 ? </random guess>

Comment 7

[:Gavin Sharp [email: [e-mail]]]

Or perhaps the delayed setFocus stuff further down in updateCurrentBrowser.

Comment 8

[Martijn Wargers (dead)]

Attachment: real patch

(In reply to comment #6)
> Perhaps due to

Yes, thanks!
That is the cause of the bug. When the tab is the focusedElement, it gets blurred now. The extra check I added prevents that. I copied it more or less from below, where a similar check is used.

This should be very safe, but I wonder why this blur() line is even there. Maybe because of Mac problems showing the outline through the background tab or something?

Comment 9

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #8)
> This should be very safe, but I wonder why this blur() line is even there.
> Maybe because of Mac problems showing the outline through the background tab or
> something?

It seems to have been added by attachment 152579 [details] [diff] [review] from bug 175893, by Aaron. Maybe he remembers what it's for?

Comment 10

[Frank Wein [:mcsmurf]]

Also affects SeaMonkey tabbrowser btw.

Comment 11

[Frank Wein [:mcsmurf]]

Filed Bug 364969 for xpfe/.

Comment 12

[Martijn Wargers (dead)]

Comment on attachment 248784 [details] [diff] [review]
real patch

Oops, sorry, the patch should use document.commandDispatcher.focusedElement
instead of this.mCurrentBrowser.focusedElement.

Comment 13

[Martijn Wargers (dead)]

Attachment: patch

Ok, this patch is for xpfe and toolkit. I haven't tested the xpfe version (my seamonkey build not working right now).
I had an irc chat with Aaron on this. Basically he agreed that we should try to remove this code, but he suggested to me that I should file a new bug on that, and put the accompanying patch for the removal in there.

Comment 14

[[e-mail]]

Comment on attachment 251687 [details] [diff] [review]
patch

It's not obvious so I won't ask you to alter the patch but mCurrentTab hasn't been changed yet so that we don't technically need to check parent nodes here.

Comment 15

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 251687 [details] [diff] [review]
patch

Maybe add a comment explaining the check? ("Only blur the focused element if it isn't another tab, to avoid breaking keyboard tab navigation"?)

Comment 16

[:Gavin Sharp [email: [e-mail]]]

Er, s/another/a/, I guess.

Comment 17

[Martijn Wargers (dead)]

Attachment: patch with extra comment

Comment 18

[Martijn Wargers (dead)]

Checking in toolkit/content/widgets/tabbrowser.xml;
/cvsroot/mozilla/toolkit/content/widgets/tabbrowser.xml,v  <--  tabbrowser.xml
new revision: 1.217; previous revision: 1.216
done
Checking in xpfe/global/resources/content/bindings/tabbrowser.xml;
/cvsroot/mozilla/xpfe/global/resources/content/bindings/tabbrowser.xml,v  <--  t
abbrowser.xml
new revision: 1.174; previous revision: 1.173
done

Checked in on trunk.

I filed bug 367806 on actually removing this code again.