112520 - Constructor not called when hidden elt is unhidden in onload

Status: RESOLVED DUPLICATE of bug 188496

(SeaMonkey :: Preferences, defect)

Description

[Philip Nemec]

Choosing the preference for images can get one of the radio button stuck on...

To reproduce - preferences / privacy & security / Images
select accept images from originating server...
select accept all

(accept images from originating server is still selected)

Comment 1

[Warner Young]

Yup, I see it on build 2001112803, Win98.

Comment 2

[Samir Gehani]

-> morse

Comment 3

[Stephen P. Morse]

Looks like a problem in the infrastructure so I'm reassigning to Hewitt.  I 
suspect that it's a regression because I'm sure this worked when I originally 
implemented it.

Below is a very simplified test case to demonstrate the problem.  Click on line 
1 or line 3 without clicking on line 2 and everything works fine.  But then 
click on line 2 and then go to line 1 or line 3.  The radio button on line 2 
remains on.

A related problem is that if you try to navigate from line to line using the 
arrow keys, you go from line 1 to line 3 and back to line 1, never landing on 
line 2.

I believe the problems are caused by the fact that line 2 is initially hidden 
and made visible when the onload handler fires.  So even though it is visible, 
the xul is not seeing it. 


<?xml-stylesheet href="chrome://communicator/skin/" type="text/css"?>

<page xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul"
      onload="document.getElementById('line2').removeAttribute('hidden');">
  <radiogroup>
    <radio label="line 1"/>
    <radio label="line 2" id="line2" hidden="true"/>
    <radio label="line 3"/>
  </radiogroup>
</page>

Comment 4

[Stephen P. Morse]

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

Comment 5

[Stephen P. Morse]

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

Comment 6

[Stephen P. Morse]

Raising severity from minor to normal.

Comment 7

[Christopher Aillon (sabbatical, not receiving bugmail)]

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

Comment 8

[Christopher Aillon (sabbatical, not receiving bugmail)]

Linux too. OS->All

Comment 9

[Stephen P. Morse]

I thought I raised the priority on this from minor to normal on 12-28-01, but it 
seems that I didn't.  Trying it again now.

Comment 10

[Jay Farrell]

Just wanted to mention I saw this on OpenVMS build 20020114 as well.

Comment 11

[Stephen P. Morse]

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

Comment 12

[Joe Hewitt (gone)]

won't have time for this soon, so giving to blake since he worked on radios recently

Comment 13

[Stephen P. Morse]

This is an embarrasing bug that IMO needs to be fixed for Mozilla 1.0.  It's 
less important for Mach V because image manager is not in the commercial 
product.

Please reconsider your removal of the 0.9.9 target milestone.

Comment 14

[Blake Ross]

Attachment: patch

Comment 15

[Blake Ross]

I attached a workaround patch for now.  After I check it in, I'll reassign this
to hyatt.  The radio's constructor is never getting called.  The workaround for
now is to default to visible and then display: none (which kills the frame)
image blocking isn't enabled.

Oh, I inverted the logic in my moz build just for testing but forgot to undo
that when I attached the patch. Fixed locally.

Comment 16

[Blake Ross]

Attachment: corrected patch

Comment 17

[Blake Ross]

morse, can you review the pref-images.xul patch please?

Comment 18

[Stephen P. Morse]

patch for pref-images.xul looks fine to me.  I see how the changing the 
default from hidden to not hidden would solve the problem.  So r=morse

However I don't understand what you accomplish by repeating the identical 
operation if you get an exception.  Why would the second attempt succeed if the 
first one didn't?  Perhaps a comment in the xul file would be in order here.

Comment 19

[Blake Ross]

The exception would probably be caused by getPref if the pref doesn't exist.  
getElementById is unlikely to throw an error.  So if getting the pref fails, we 
default to hidden.  Unless you'd rather default to shown.

Comment 20

[Stephen P. Morse]

Oh, I read the patch too quickly and thought that it was the second statement in 
the try block that was failing.  That's why I didn't understand why you 
reexecuted that statement in the catch block.  Now I understand what you are 
doing.

To prevent others from getting confused, and to avoid duplicate code, the 
following might be a cleaner way to do it:

   var enabled = false;
   try {
     enabled = parent.hPrefWindow.getPref("bool", "imageblocker.enabled");
   }
   if (!enabled) {
     document.getElementById("haveImageBlocking").setAttribute("hidden", true");
   }    

My r=morse holds if you make the above change.

Comment 21

[Stephen P. Morse]

Cleaner yet might be

   var enabled;
   try {
     enabled = parent.hPrefWindow.getPref("bool", "imageblocker.enabled");
   } catch(ex) {
     enabled = false;
   }
   if (!enabled) {
     document.getElementById("haveImageBlocking").setAttribute("hidden", true");
   }    

Comment 22

[Stephen P. Morse]

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

Comment 23

[Blake Ross]

got sr=hewitt with suggested change, checked in. now reassigning to hyatt.

Comment 24

[[not reading bugmail]]

I wonder if this is same kinda of logic would fix the Account Wizard radio
button problem in bug 109997.

Comment 25

[Boris Zbarsky [:bzbarsky]]

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

Comment 26

[Peter Trudelle]

nsbeta1+ per Nav triage team, assuming this is required for P3P

Comment 27

[Blake Ross]

I don't think this is needed for P3P...?  Even if there was some P3P UI that had
this problem, there's an easy workaround for this.

Comment 28

[Peter Trudelle]

Hmm. I thought Terri said she saw this in today's commercial build, and assumed
it must be P3P UI landing.  If it is just needed for Image Mgr, then it would
obviously not be needed for MachV.  Steve, could you please clarify why you
nominated it?

Comment 29

[Stephen P. Morse]

It's definitely not needed for p3p.  I nominated it because it was an 
embarrasement in the image-manager pref panel.  But Blake has since checked in a 
local fix for the image manager so there is no more embarassement.  Therefore 
there's no longer any embarrasement and I'll withdraw my nomination.

Comment 30

[Boris Zbarsky [:bzbarsky]]

Is this still an issue?  If so, can someone attach a testcase?

Comment 31

[[e-mail]]

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