303428 - Focus ring can end up on an inactive tab

Status: RESOLVED FIXED

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

Description

[Brian Polidoro]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b4) Gecko/20050804 Firefox/1.0+
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b4) Gecko/20050804 Firefox/1.0+

The focus ring on the tab bar does not go away or move to the new active tab in
the scenario given.

Reproducible: Always

Steps to Reproduce:
1. Put a focus ring on the current active bu clicking on it.
2. Open a bookmark in a new tab from the bookmark menu or the bookmark toolbar.
 Do this by middle clicking or Ctrl-clicking on a bookmark or use a open in tabs
menu item to open multiple bookmarks in tabs.  Don't use a context menu or the
bookmark sidebar.
3. A new tab will become the active tab but the focus ring will remain on the
previous active tab.

Actual Results:  
Focus ring appears on an inactive tab.

Expected Results:  
Whichever is deemed more appropriate: 
The focus ring should no longer appear.
The focus ring appears on the new active tab.

Comment 1

[Martijn Wargers (dead)]

Confirmed.
Might have something to do with bug 264820.

Comment 2

[Brian Polidoro]

I also see this happens on a Mac at least in the case of clicking on a bookmark
toolbar bookmark with the apple key pressed with the focus indicator present on
the active tab.
Nightly with the same date as comment #0.
OS->ALL  

Comment 3

[Brian Polidoro]

I can't determine if this affects the bookmarks menu on the Mac because of bug
262388.

Comment 4

[Ria Klaassen (not reading all bugmail)]

This would be only a visual problem IMHO if it also activated the tabscrolling
but it doesn't.

Comment 5

[Mike Schroepfer]

Mike can you take a look?

Comment 6

[Mike Connor [:mconnor]]

Bumping to aaron after discussion on IRC, he's been handling issues in the tab
focus impl that he created for 1.5.

Comment 7

[Mike Connor [:mconnor]]

re-bumping.  Aaron, let me know tomorrow if you're not going to have cycles for
this.

Comment 8

[Aaron Leventhal]

Is this a regression from my setTimeout checkin for bug 249136 on 8/5/2005 ?
I'm planning to back that out for a better fix anyway.

Comment 9

[Aaron Leventhal]

(In reply to comment #8)
> Is this a regression from my setTimeout checkin for bug 249136 on 8/5/2005 ?
> I'm planning to back that out for a better fix anyway.

No, duh. The reporter was using a build from 8/4/2005.

Comment 10

[Aaron Leventhal]

I was able to duplicate this before, but this is WFM in the 8/17/2005 branch build.

Comment 11

[Jesse Ruderman]

Confirmed. Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b4)
Gecko/20050818 Firefox/1.0+

Comment 12

[Phil Ringnalda (:philor)]

I can still reproduce with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US;
rv:1.9a1) Gecko/20050820 Firefox/1.6a1 ID:2005082018 by Ctrl-clicking a
bookmark, though open in tabs WFM.

Comment 13

[Majken Connor [:Kensie]]

I still see it also, 

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20050820
Firefox/1.6a1

I would imagine it has to do with the fact that I have to click to get a focus
ring, ctrl-tabbing through the tabs doesn't create one. If I click somewhere
else first (i.e. right click on a bookmark or link) or even left click a
bookmark to open it in current tab, focus ring disappears.

Comment 14

[Brian Polidoro]

Using the Open in Tabs menu item is WFM now.  But middle click and ctrl-click
still shown the bug.  

Comment 15

[Aaron Leventhal]

I cannot repro with Ctrl+click using the following steps:

1. Launch Firefox, with bookmarks sidebar already open
2. Click on current tab so it is focused and shows focus ring
3. Ctrl+click on bookmark to mozilla.org

Focus ring goes away as new tab's content has focus.

Comment 16

[Brian Polidoro]

I had said in the intial bug report that the bookmark sidebar does not exhibit
the problem.  Only the bookmark menu and the bookmark toolbar do.  

I also just found that middle clicking or ctrl-clicking on the back button, the
forward button, the throbber, the home button and entries in the go menu also
show the same problem.  

Oh and possibly another bug is that the home menu item on the go menu does
nothing when middle clicked and opens the homepage in the current tab when
ctrl-clicked.  

Also when a tab has the focus ring the copy command is enabled on the edit menu
but doesn't copy anything because I can't paste anything into notepad even if I
have text selected in the page.  And select all is enabled but does nothing when
selected.

But I haven't searched if any of those have been filed. 
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b4) Gecko/20050821
Firefox/1.0+ ID:2005082106

Comment 17

[Aaron Leventhal]

(In reply to comment #16)
> I had said in the intial bug report that the bookmark sidebar does not exhibit
> the problem.  Only the bookmark menu and the bookmark toolbar do.  

Okay, I spaced that. It's dain bramage from too many late nights getting ready
for this release.

So, SetContentState(EVENT_STATE_FOCUS) is possibly not doing its job.

Another way to repro this is to focus a tab, and then hit ctrl+b twice. The
current tab has a focus outline even though it's the content with focus.

Comment 18

[Majken Connor [:Kensie]]

Is this left over from/regression to mconnor's reorder tabs thing? (I do seem to
recall he had some issues with the focus rings when he was implementing it, but
that was a while ago)

If I ctrl-click or middle click not only does it leave the focus ring, but I can
leave the focus ring on the old tab and create a focus ring on the new tab. I
can click on other tabs and creat a focus ring there. The original focus ring
goes AWAY when I click on the original tab or one of the tabs that is next to
the original tab with the focus ring.  Also if I reorder tabs, original focus
ring persists until I move a tab beside the tab with the original ring.

Comment 19

[Brian Polidoro]

For me the focus ring disappears as soon as I hit Ctrl-B.

Comment 20

[Aaron Leventhal]

When we switch focus to another window, we never call SetContentState(...,
NS_EVENT_STATE_FOCUS). Not sure where we should try to do that ... ESM's
NS_GOTFOCUS handling, or nsGlobalWindow::Focus() ? How do we avoid getting into
a circular loop?

I would think this means we get into other kinds of trouble, such as not firing
the right blur events.
Perhaps this problem is related to Brian Ryner's comments here:
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/content/events/src/nsEventStateManager.cpp#586
 586   if (!mCurrentFocus && gLastFocusedContent) {
 587     // We also need to blur the previously focused content node here,
 588     // if we don't have a focused content node in this document.
 589     // (SendFocusBlur isn't called in this case).
 

Comment 21

[Benjamin Smedberg]

I have experienced the lack of proper blur events in bug 302136... blur events
are not fired at a XUL window when we switch focus to a different window.

Comment 22

[Aaron Leventhal]

Attachment: Not sure why this causes the window to flash gray for a moment, but it fixes the focus ring problem. The gray flash would need to be remedied, however.

Comment 23

[Mike Connor [:mconnor]]

this is cosmetic only, and the fix is too risky at this stage, minusing.

Comment 24

[Neil Deakin]

*** Bug 308991 has been marked as a duplicate of this bug. ***

Comment 25

[Oliver Yeoh]

Attachment: alternative patch

Sorry for butting in but I've got an alternative patch that fixes the bug for me.

So, from what I gather... if the the focus ring is on a tab, updateCurrentBrowser() will not try to focus the content. This is perfectly valid and the behavior should not be changed. See http://lxr.mozilla.org/mozilla/source/toolkit/content/widgets/tabbrowser.xml#750

With this patch the focus ring will be on the tab that is being activated.

Comment 26

[Brian Polidoro]

I can't reproduce this bug anymore.  But having the focus ring on the new active tab sounds like it might be a good thing to have.  

Comment 27

[Jeff Walden [:Waldo]]

(restoring lost blocking request)

Comment 28

[Mike Connor [:mconnor]]

Comment on attachment 239399 [details] [diff] [review]
alternative patch

you need to make this _selectNewTab now that bug 370742 has landed.  r=me with that

Comment 29

[Nickolay_Ponomarev]

comment #28:
> you need to make this _selectNewTab now that bug 370742 has landed.  r=me with
> that
Oliver, could you post the (tested) updated patch?

Comment 30

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

Attachment: updated patch

Comment 31

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

mozilla/toolkit/content/widgets/tabbox.xml 	1.45

Comment 32

[Steffen Wilberg]

This caused bug 382766, which is a major dogfood issue. Please either fix that or back this out asap.

Comment 33

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

I've backed out this patch, because the regression it caused (bug 382766) was worse than the bug it was fixing, and because I'm not sure it was a correct change in general. It seems to me like the fix for this bug should focus on the bookmark case specifically, since it seems to do things differently, at first glance.

Comment 34

[Dão Gottwald [:dao]]

Attachment: css workaround

At least visually this should do the trick, but I don't know if this bug has other side effects. Aaron, can you tell that?

Comment 35

[Martijn Wargers (dead)]

Ah, I think I hit this bug in bug 367806. I attached a neat testcase in that bug:
https://bugzilla.mozilla.org/attachment.cgi?id=275430
and I think that's basically the same issue.

Comment 36

[Mike Connor [:mconnor]]

not an M9 blocker

Comment 37

[Dão Gottwald [:dao]]

Back to the default assignee then. Maybe somebody figures out a fix for the Core bug.

Comment 38

[Dão Gottwald [:dao]]

Actually this should be a Core bug to start with. At this point I'm not changing the component as this would mean to lose the blocking flag.

Comment 39

[Mike Connor [:mconnor]]

I'm no longer sure this blocks, but this should be a core bug.

Comment 40

[Dão Gottwald [:dao]]

Comment on attachment 283201 [details] [diff] [review]
css workaround

If the bug remains unfixed, I still think we should add [selected="true"] as a workaround for Firefox. This patch isn't up to date, though.

Comment 41

[Hussam Al-Tayeb]

(In reply to comment #0)
> The focus ring on the tab bar does not go away or move to the new active tab 

In the current builds, the focus ring stays on the old inactive tab but also appears in the new active tab if you click on the active tab. You end up with two focus rings.

Comment 42

[Neil Deakin]

This bug should have been fixed by 178324.