56447 - Cookie and Image UI sometimes has two tabs selected

Status: VERIFIED DUPLICATE of bug 60210

(Core :: Graphics: Image Blocking, defect, P3)

Description

[Jeffrey Baker]

On Linux 2000-10-12-21-MTrunk, the cookie and image UI sometimes comes up with
two tabs selected.

1) Pick Tasks->Privacy->Images->View Sites

Result: the dialog appears with both the first and third tabs selected.

Expected Result: only the third tab is selected.

My patch for this problem is attached.  I wonder if the selection is supposed to
automatically work when you set the index attribute of a tabcontrol?

Comment 1

[Jeffrey Baker]

Attachment: Patch to force tabpanels to deselect themselves

Comment 2

[Blake Ross]

morse needs to review this.

Comment 3

[Stephen P. Morse]

Very strange bug.  It's obviously a glitch in the underlaying xul processing 
that is allowing this double-tab selection.  This is simply a work-around to 
avoid the underlaying problem.

But I am in agreement with this work-around.

r=morse
a=morse (as module owner)

Comment 4

[Jeffrey Baker]

Alright, but once this gets checked in I won't mark it fixed, but rather
reassign to myself and mark future.  I'll investigate XUL tabcontrol handling on
the trunk.

Comment 5

[Ben Goodger (use ben at mozilla dot org for email)]

This patch may fix the symptom, but the best fix for this is to use tabcontrol's
'selectedTab' property:

tabControl.selectedTab = document.getElementById("imagesTab");

where 'tabControl' is the <tabcontrol>'s DOM node. The <xul:tabcontrol>'s code
will sort everything else out for you.

Comment 6

[Jeffrey Baker]

Attachment: Second patch using correct tabcontrol wizardry

Comment 7

[Jeffrey Baker]

I have attached a second patch which uses tabcontrol.selectedTab to set the
active tab panel.  Tested on Linux trunk build.  Please give the new patch a
quick review.

Note that there is *no* way I would have ever discovered the selectedTab trick
by myself.  The XUL reference at http://www.mozilla.org/xpfe/xulref/ makes no
mention of it.  Indeed, http://www.mozilla.org/xpfe/xptoolkit/tabs.html
specifically states that the correct way to make a tab active is by setting the
index attribute.  The string "selectedTab" does not appear on the mozilla.org
site anywhere.

I guess the only reference I can rely on are the XUL XML files themselves.(?)

Comment 8

[Stephen P. Morse]

r=morse for the second patch.

Comment 9

[Ben Goodger (use ben at mozilla dot org for email)]

Yes, our <tabcontrol> implementation changed recently (in the past couple of 
months) to be completely defined in XBL, with an enhanced content node API. 

I'll send out a message to xpfe describing the new features...

As for the newest patch, they call me Dr Pedant... the cleanest way to do this 
is like so:

var tabID = !window.arguments[0] ? "cookiesTab" : "imagesTab";
element = document.getElementById("cookieViewerTabControl");
element.selectedTab = document.getElementById(tabID);

(I saw you doing a comparison of window.arguments[0] with string "0", js makes 
these be the same in a simple equality comparison, it's only if you do 
window.arguments[0] === "0" [identity comparison] that type matters), so 
!window.arguments[0] is the same as (window.arguments[0] == "0")

Comment 10

[Stephen P. Morse]

I would argue that using
    !window.arguments[0]
instead of
     window.arguments[0] == "0"
is a bad idea.  It hides the intent of the code and so is much less readable.
 

Comment 11

[Ben Goodger (use ben at mozilla dot org for email)]

The meaning of the parameter as a deck index is no longer really relevant, as 
the integer passed to the dialog is no longer used this way, rather it is now a 
boolean flag representing which tab to set as the selectedTab. 

If you wanted to make the meaning particularly explicit, you'd pass either 
"images" or "cookies" as the parameter to the dialog, and do a check for either 
of those, e.g. 

var tabID = window.arguments[0] == "cookies" ? "cookieTab" : "imagesTab";

Comment 12

[Jeffrey Baker]

My goal in making the patch was to change as few lines as possible, therefore
having the best chance of slipping the fix into the 6.0 product.  Therefore I
left the wanky parameter passing and if/else logic in there.

Comment 13

[Blake Ross]

This won't be accepted for rtm.

Comment 14

[Ben Goodger (use ben at mozilla dot org for email)]

Really, I don't much care either way. !foo is just as readable to me as anything 
else. 

alternately, the expression would just read

var tabID = window.arguments[0] == "0" ? "cookiesTab" : "imagesTab";

Comment 15

[Stephen P. Morse]

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

Comment 16

[Blake Ross]

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

Comment 17

[Scott Brodmerkle]

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

Comment 18

[R.K.Aa.]

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

Comment 19

[Matthew T (active 1999-2002)]

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

Comment 20

[Stephen P. Morse]

*** This bug has been marked as a duplicate of 60210 ***

Comment 21

[R.K.Aa.]

verified dup of now fixed bug 60210 "Image manager shows 2 "top" tabs"
(Linux SEA 2000121906)