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

RESOLVED FIXED in mozilla1.9beta3

Status

()

Core
General
--
enhancement
RESOLVED FIXED
11 years ago
10 years ago

People

(Reporter: Daniel Ceregatti, Assigned: bz)

Tracking

({regression})

unspecified
mozilla1.9beta3
regression
Points:
---
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(1 attachment)

(Reporter)

Description

11 years ago
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.
Component: Tabbed Browser → DOM
Product: Firefox → Core
QA Contact: tabbed.browser → general
(Reporter)

Comment 1

11 years ago
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

Updated

11 years ago
Keywords: regression
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).
Blocks: 326009
Status: UNCONFIRMED → NEW
Component: DOM → XP Miscellany
Ever confirmed: true
QA Contact: general → brendan
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.
Created attachment 292195 [details] [diff] [review]
Something like this

jst, what do you think about the branch prospects here?  My first instinct is to not mess with the branch...
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
Attachment #292195 - Flags: superreview?(jst)
Attachment #292195 - Flags: review?(jst)
Summary: Named windows/tabs are randomly targeted from windows that target them → [FIX]Named windows/tabs are randomly targeted from windows that target them
Target Milestone: --- → mozilla1.9 M11
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.
Attachment #292195 - Flags: superreview?(jst)
Attachment #292195 - Flags: superreview+
Attachment #292195 - Flags: review?(jst)
Attachment #292195 - Flags: review+
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.
Attachment #292195 - Flags: approval1.9?

Comment 7

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

bz thanks for the explanation..
Attachment #292195 - Flags: approval1.9? → approval1.9+
Fixed.
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Flags: in-testsuite?
Resolution: --- → FIXED

Updated

10 years ago
Component: XP Miscellany → General
QA Contact: brendan → general
You need to log in before you can comment on or make changes to this bug.