60210 - Image manager shows 2 "top" tabs

Status: RESOLVED FIXED

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

Description

[Chuck R.]

I have build 2000111404. When I went to Tasks, Privacy and Security, Image
Manager, View sites that can/not... the image manager appeared.

It has 3 tabs. The first and third which showed up as "top" (they were both
white) so I didn't know which tab was really showing.

Reproduce: 100%

Comment 1

[Asa Dotzler [:asa]]

Confirmed with 111508 mozilla trunk build on NT.  I'm also seeing it on today's
mac and linux builds. Updating Platform, OS and component

Comment 2

[Stephen P. Morse]

Now that's real strange.  There is javascript to select the third tab when the 
dialog is brought up as the image viewer.  But for some reason, the original 
first tab is also being selected.  Sounds like a regression somewhere in xul.

However we can easily side-step whatever xul problem there is by explicitly 
adding two lines that deselect the first tab.  Attaching patch.

Comment 3

[Stephen P. Morse]

Attachment: Patch to de-select the first tab

Comment 4

[dbragg]

Looks good.  I noticed there's another CookieViewer.js file in
/extensions/wallet/cookieviewer/resources/content

If that file is obsolete it should be removed.

r=dbragg

Comment 5

[dbragg]

Nevermind.  That other file is a placeholder.

Comment 6

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

You should never have to set the selected attributes yourself.

When the dialog loads, set the selectedTab property of the tabcontrol to be the
images tab or the cookies tab. This will ensure everything else happens
automatically. 

Comment 7

[Stephen P. Morse]

OK, that works.  Attaching new patch.

Comment 8

[Stephen P. Morse]

Attachment: Revised patch

Comment 9

[dbragg]

I thought that's what wasn't working in the first place.  Whatever.  r=dbragg  

Comment 10

[Stephen P. Morse]

What wasn't working in the first place was setting the "select" property on the 
<tab>.  The change that ben suggested (and that is in the latest patch) is to 
set the "selectTab" property on the <tabcontrol>.

Thanks for the review.

Comment 11

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

Is element2 declared outside this anywhere? (A peek in lxr showed it wasn't)...
you should replace

element2 = document.getElem...

with var element2 = document.getElem...

if not, to prevent js warnings (and prevent the variable from contaminating the
global scope).

Comment 12

[Stephen P. Morse]

Attachment: revised patch to include "var"

Comment 13

[Stephen P. Morse]

New patch checked in.  dbragg, could you please re-review the new patch.  
Thanks.

Comment 14

[dbragg]

r=dbragg

Comment 15

[Stephen P. Morse]

Oops, my comment above about "new patch checked in" was in error.  I meant to 
say "new patch attached".  According to the rules, nothing can be checked in 
until it is re-reviewed and re-approved.

Now it has been re-reviewed.  Ben, please re-approve.  Thanks.

Comment 16

[Stephen P. Morse]

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

Comment 17

[Mike Kaply [:mkaply]]

Attachment: Might as well fix the cookietab to use selectedTab while we're here

Comment 18

[Stephen P. Morse]

mkappy, I appreciate your wanting to help but I'm afraid it's going to backfire.  
The current bug report deals with the image manager tabs and that is what my 
patch is fixing.  If you want to fix the cookie manager as well (which currently 
isn't broken), please open a new bug on that one.  This bug has been in the 
review queue for 12 days now and if you change the patch then we will have to 
start the review process all over.

Comment 19

[Mike Kaply [:mkaply]]

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

Comment 20

[Mike Kaply [:mkaply]]

My apologies for confusing the issue.

Based on Goodgers comment that the selected attribute should never be set 
explicitly, I thought it would be worthwhile to also fix the case where the 
cookietab is explicitly setting the selected attribute rather than setting 
selectedTab about 6 lines above this one.

Sometimes it's hard to get code fixes like this one in when they don't actually 
have a visual consequence, so I thought it might be worthwhile to fix it here 
while the code is open.

Again, sorry for the confusion.

Comment 21

[Brendan Eich [:brendan]]

Not every change to a patch requires re-review from the beginning.  If the
substance changes completely, or if most of the patch changes in a deep way
(e.g., not just tabs cleanups or comment changes), then of course.  Use your own
judgment.

/be

Comment 22

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

I agree with mkaply's suggestion. Since we're not in lockdown anymore, it
doesn't seem that it would hurt to make this change. 

Comment 23

[Stephen P. Morse]

Checked in a hybrid of kaply's patch and my patch.

Comment 24

[Stephen P. Morse]

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