407421 - [FIX]Named windows/tabs are randomly targeted from windows that target them

Status: RESOLVED FIXED

(Core :: General, enhancement)

Description

[Daniel Ceregatti]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8.1.11) Gecko/20071127 Firefox/2.0.0.11
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8.1.11) Gecko/20071127 Firefox/2.0.0.11

When multiple tabs are opened via the following method:

<a target="namedWindow" ... >

And one clicks another link that targets said named window, the tab/window that firefox picks is random. 

Reproducible: Always

Steps to Reproduce:
Close all firefox windows (for the sake of keeping a sane environment to test this). Open a firefox window. Browse to http://sh.nu/focus.php

Click the "Click here to open a new unnamed window" link. This will open a new window (or tab, in my case. I'll use tabs from now on). Return to the original tab, and repeat this 3 times. You now have 4 tabs open. Let's call them tabs 1 through 4.

In tabs 2, 3, and 4, click the "Click here to name this window." link. This will run a javascript that sets the window name.

Go back to tab 1 and click the "Click here to open a new named window." link and watch the icon for tabs 2-4 to see which gets reloaded (There is a sleep (1) in the focus.php script that will allow you to see loading animation in the tab). Tab 2 _should_ be the one that get re-loaded.

Now focus tab 2, then focus tab 1, and click the "Click here to open a new named window.". Tab 3 will now reload.

Now focus tab 3, then focus tab 1, and click the "Click here to open a new named window.". Tab 4 will now reload.

Now focus tab 4, then focus tab 1, and click the "Click here to open a new named window.". Tab 2 will now reload.

Seeing the pattern?

There are other permutations that cause a random tab to reload the named window. Suffice it to say that the behavior is completely unpredictable.
Actual Results:  
A random tab/window is targeted.

Expected Results:  
A specific, as yet to be defined, tab/window should be targeted

I suggest that the behavior should be to use the last focussed tab/window with the targeted name, or the very first spawned tab/window with the targeted name (This is the observed IE7 bahavior), or the spawned last tab with the targeted name.

Comment 1

[Daniel Ceregatti]

This was not always the way it behaved. It seems to have been doing the expected behavior in 2.0(.0.0), and changed to the observed behavior at 2.0.0.1

Comment 2

[Boris Zbarsky [:bzbarsky]]

I don't see quite the behavior comment 0 describes on trunk, and I don't know what would have changed from 2.0 to 2.0.0.1 to affect this (other than perhaps UI code).

But it would indeed be nice to improve the sorting of nsXULWindow::mTargetableShells.  Right now any time a tab is switched to or from it gets moved to the end of the list, basically.

Looking at the UI code, the pattern is to first set the tab being switched away from as content-targetable, then to set the tab being switched to as content-primary.  Which means that it might make sense to put a newly-added entry at the start (instead of the end) of the list if one of two things holds:

1)  It's primary
2)  There is no primary (since that means it presumably just stopped being
    primary).

Comment 3

[Boris Zbarsky [:bzbarsky]]

In fact, that approach guarantees that when opening a tab in the background it'll get added at the end, and that when switching tabs the list will end up with the new tab first, then the tab being switched away from, then all other tabs.

In other words, we maintain the invariant that the currently-selected tab is the first entry in the list.  When changing tabs, we first take the first entry out the the list, then put it back as the first entry, then move some other entry to the front.  Therefore the entries are always listed in "most recently focused first" order.  Doing the other suggestions from comment 0 would actually be a good bit more work, for questionable benefit.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Attachment: Something like this

jst, what do you think about the branch prospects here?  My first instinct is to not mess with the branch...

Comment 5

[Johnny Stenback (:jst)]

Comment on attachment 292195 [details] [diff] [review]
Something like this

r+sr=jst. Doesn't seem like a big issue, so I wouldn't worry about this on the branch.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 292195 [details] [diff] [review]
Something like this

Requesting 1.9 approval.  This changes the ordering of the targetable frames array, so we select a slightly saner target when we have multiple targets with the same name.  Only changes the ordering, so the worst regression risk is that the targets we select won't be as good as the ones we select now.  I don't foresee that happening.

Comment 7

[Mike Schroepfer]

Comment on attachment 292195 [details] [diff] [review]
Something like this

bz thanks for the explanation..

Comment 8

[Boris Zbarsky [:bzbarsky]]

Fixed.