147811 - JavaScript menus do not work?

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Antti Vähä-Sipilä]

From Bugzilla Helper:
User-Agent: Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.0rc3) Gecko/20020523
BuildID:    2002052306

An image has a link to a menu defined in JavaScript as follows:
<a href="javascript:window.showMenu(window.FOO);"><img SRC="FOO.jpg" ALT="foo
menu" border="0"></a>

However, nothing happens when clicking on the link. On Netscape 4.75, a menu
opens and gives a possibility to open a link from the menu.

The menu has been defined in JavaScript like this:
function onLoad() {
	loadMenus();
}

function loadMenus() {
  
window.FOO = new Menu("FOO");
FOO.addMenuItem("Foo 1", "top.window.location='http://foo.com/'");
FOO.addMenuItem("Foo 2", "top.window.location='http://foo2.com/'");
FOO.addMenuSeparator();
FOO.addMenuItem("Close", "hideMenu");

FOO.bgColor = "#FFEEFF";
FOO.menuItemBgColor = "white";
FOO.fontColor ="0000bb";
FOO.fontSize = 8;
FOO.fontWeight = "bold";

myMenu.writeMenus();
if (document.all) {
    loadMenus();
}

Reproducible: Always
Steps to Reproduce:
1. Go to a page having a JavaScript menu as described in Description
2. Click on the image link


Actual Results:  Nothing

Expected Results:  A menu should have appeared, allowing me to select a link (it
does on Netscape 4.75).

Comment 1

[Alfonso Martinez]

If you don't provide a full working example it's impossible to see what is the
problem, but it seems to be that the menus are only designed for IE and NS4

Comment 2

[Olivier Cahagne]

You use document.all, which is IE-only JavaScript code.

Propose to mark INVALID.

Comment 3

[Phil Schwartau]

Reassigning to DOM Level 0. This does look like a coding error, 
but there is not enough information to make a conclusion.

Recall that only Microsoft IE supports |document.all|.
Therefore I don't see how your code snippet

                  if (document.all) {
                    loadMenus();
                  }

would work in Netscape 4.75, as you say. Antti, would it be possible
for you to attach a reduced testcase by going to the URL of this bug,
and then the "Create a New Attachment" link? This could be just a
small menu with links pointing to public sites, just enough to see
what the problem is - 

Otherwise, we can't make a final determination; thanks -

Comment 4

[Antti Vähä-Sipilä]

Added an example URL on a publicly available site and edited out some crud that
was for IE only.

Comment 5

[Boris Zbarsky [:bzbarsky]]

function writeMenus(container) {
    if (!container && document.layers) {
        if (eval("document.width")) 
            container = new Layer(1000);
    } else if (!container && document.all) {
        if (!document.all["menuContainer"]) 
            document.writeln('<SPAN ID="menuContainer"></SPAN>');
        container = document.all["menuContainer"];
    }

So... Mozilla does not support document.layers or document.all.  So |container|
is never set.  Then later you take the |if (!container) return;| early return.
You want a document.getElementById branch here, methinks.

Comment 6

[Phil Schwartau]

Boris is correct, but we actually have another problem as well. If you
set breakpoints in Mozilla's JavaScript Debugger, you'll see that the 
writeMenus() function doesn't even get called in Mozilla.

It is invoked this way by the script:

<SCRIPT>
function onLoad() {loadMenus();}
function loadMenus(){ etc. etc. FOO.writeMenus();}
</SCRIPT>


The intention is to make the window onLoad handler call FOO.writeMenus()
for us, but this is not working. Here is a simpler example:


<SCRIPT>
function onLoad() {alert('Hello');}
</SCRIPT>


When you load this in NN4.7, you get the alertbox.
But in IE6 or Mozilla, you don't: the alert is not fired.

Let me ask Boris and Johnny: is that correct behavior?

Comment 7

[Phil Schwartau]

Note we cab modify the script to make sure the writeMenus() function
does get called, e.g. by commenting out these two lines:

/////////////////function onLoad() {
loadMenus();
///////////////////////////////////}


then we see exactly what Boris said above. Here is a longer excerpt
from the writeMenus() function: 


function writeMenus(container) {
    if (!container && document.layers) {
        if (eval("document.width")) 
            container = new Layer(1000);
    } else if (!container && document.all) {
        if (!document.all["menuContainer"]) 
            document.writeln('<SPAN ID="menuContainer"></SPAN>');
        container = document.all["menuContainer"];
    }
    if (!container && !window.delayWriteMenus) {
        window.delayWriteMenus = this.writeMenus;
        window.menuContainerBgColor = this.menuContainerBgColor;
        setTimeout('delayWriteMenus()', 3000);
        return;
    }    
    container.isContainer = "menuContainer" + menuContainers.length;

                                  etc.



Because |document.layers| and |document.all| are non-W3C syntax 
not supported in Mozilla, the first time through this function 
we end up doing 

           window.delayWriteMenus = this.writeMenus;
and        setTimeout('delayWriteMenus()', 3000);


and returning. But we end up passing through the same function again,
three seconds later. This time we make it to the line 

          container.isContainer =  etc.


This causes an error, since |container| has never been set. To see this,
Just comment out the lines I indicated above, and load the URL with the
Mozilla JavaScript Console open. Three seconds after you load, you will 
see this error in the Console:

          Error: container has no properties
          Source File: (path to)/menu.js
          Line: 98

Comment 8

[Phil Schwartau]

SUMMARY:

1. The site does not have a valid codepath for Mozilla. 
2. There is still a question in Comment #6 about the use of onLoad handlers

Comment 9

[Phil Schwartau]

Note this DOES work in Mozilla and IE6 as well as NN4.7:

<SCRIPT>
window.onload = function() {alert('Hello')}
</SCRIPT>


whereas the approach in Comment #6 does not, even if we
are careful about using lower-case spelling of "onload":


<SCRIPT>
function onload() {alert('Hello');}
</SCRIPT>

Comment 10

[Johnny Stenback (:jst)]

Mass-reassigning bugs to [e-mail]

Comment 11

[Fabian Guisset]

I fail to see why
function onLoad() { ... }
would be called when the page is loaded if there is no <body onload="onLoad();">
or some such. If NS4 did that it doesn't make sense and imho shouldn't be emulated.

Based on the comments and the analysis I suggest INVALID. Thoughts?

Comment 12

[Boris Zbarsky [:bzbarsky]]

It worked in NS4 because it mapped "function onLoad" to "window.onload".  We
don't do that.  We don't even map "function onload" to "window.onload", (which
is arguably a bug, actually). But JS is case-sensitive, so "function onLoad"
shouldn't do anything in any case.

Comment 13

[Brendan Eich [:brendan]]

> We don't even map "function onload" to "window.onload", (which is arguably a
> bug, actually).

We don't?  That is a bug.  Last I checked, though, we do.  What's a testcase?

/be

Comment 14

[Boris Zbarsky [:bzbarsky]]

Attachment: not mapping "function onload" to event handler

If I change that to

window.onload = function onload() {
 alert('aaa');
}

it works.

Comment 15

[Boris Zbarsky [:bzbarsky]]

I bet the bug there is that the function declaration just defines the property
on the JSObject and never ends up calling nsWindowSH::SetProperty

Comment 16

[Brendan Eich [:brendan]]

jst, did I regress this by redoing the way declared functions are defined (by
prolog bytecode generated for each declaration)?  (Erk, mid-air with bz saying
the same thing: "I bet the bug there is that the function declaration just
defines the property on the JSObject and never ends up calling
nsWindowSH::SetProperty").

Or do we not try calling window.onload on each load event, where we once did?

What does IE do with function onload?  The old days, sigh, may matter so little
now that we can WONTFIX every aspect of this bug (certainly, we shouldn't try to
call onLoad).

/be

Comment 17

[Johnny Stenback (:jst)]

Brendan, I can't say for sure that that's what regressed it, but it sure seems
like it could be that. If I do "onload = onload;" after the function declaration
in the above testcase, the onload function is called on load, w/o that it's not.

Want a separate bug on this problem, since it's not exactly the problem
described in this bug?

Comment 18

[Brendan Eich [:brendan]]

WONTFIX implies evangelism, of course.

But we can fix things so function onload, when defined per ECMA-262 (created, so
as to remove any pre-existing proprety), by checking for 'onload' as the id
passed to nsWindowSH::AddProperty.

/be

Comment 19

[Brendan Eich [:brendan]]

jst: what does IE do?  Probably it doesn't call a function onload (guessing,
based on naive ECMA implementation).

We're violating compatibility, OTOH, and every little bit hurts, even if the
pain is worth it (as in the case of dropping onLoad support). 
nsWindowSH::AddProperty can be modified easily to fix the bug (separate bug is
ok, or keep talking here; I'm not picky).

/be

Comment 20

[Johnny Stenback (:jst)]

Hmm, I was expecting IE to call the onload function by just defining it, but it
doesn't! And even worse, if you define a function named onload, there doesn't
seem to be anything you can do to the window object to make IE call that
function on load, i.e. setting window.onload = onload doesn't do anything in
this case (however, if no onload function is defined, then setting window.onload
to some other function does cause that other function to be called on load).

Ok. Let's keed talking here for now, since we have testcases etc here already.

Comment 21

[Brendan Eich [:brendan]]

W3C DOM working group owes us a DOM level 0 spec, dammit.

I think we should restore compatibility with what we had before the ECMA
changes, or the DOM-via-XPConnect changes, or whatever changes stopped function
onload(){} at top level from defining window.onload.  jst, care to do an
AddProperty patch, so we can see how big it is?

/be

Comment 22

[Johnny Stenback (:jst)]

Attachment: Register event handlers on global function declaration, not just when setting properties.

Comment 23

[Johnny Stenback (:jst)]

Attachment: Same thing for non-global scopes where event listeners belong.

Comment 24

[Brendan Eich [:brendan]]

Comment on attachment 135605 [details] [diff] [review]
Same thing for non-global scopes where event listeners belong.

+  if (!JSVAL_IS_STRING(id) || flags & JSRESOLVE_ASSIGNING) {

Nit: overparenthesize the & expression (I do this to avoid brainprint for the
case where parens are needed: == or != operators with &|^ operands).

Thanks for doing this, looks nice and small, and may be worthwhile even if IE
screws the pooch.

/be

Comment 25

[Johnny Stenback (:jst)]

Thanks for the reviews, fix checked in. Leaving bug open to figure out if more
is needed here.

Comment 26

[Johnny Stenback (:jst)]

I had to back out the change since it caused tinderbox to go orange due to a
testing goofup on my end. The fix is correct, but I did most of my testing using
mfcembed and the one time I did start Mozilla to see that things were still
working I must have started the wrong build, since this patch makes Mozilla
crash on startup. The problem is that we're now registering event handlers while
compiling scripts in the XUL documents, and that causes us to try to atomize a
JS string using a null cx since there's no script running at the time. Patch
coming up that protects against doing that.

Comment 27

[Johnny Stenback (:jst)]

Attachment: Don't try to internalize JS strings on a null cx (diff -w)

Comment 28

[Brendan Eich [:brendan]]

Comment on attachment 135768 [details] [diff] [review]
Don't try to internalize JS strings on a null cx (diff -w)

Sure -- I wouldn't have caught this by reviewing.  But who runs mfcembed these
days? ;-)

/be

Comment 29

[Christopher Aillon (sabbatical, not receiving bugmail)]

Comment on attachment 135768 [details] [diff] [review]
Don't try to internalize JS strings on a null cx (diff -w)

yeah, what are you doing running mfcembed?  :)
r=caillon

Comment 30

[Johnny Stenback (:jst)]

Thanks! (Hey, I used mfcEmbed since I can't debug mozilla while running an
installed version! :-)

Fix checked in again.

Comment 31

[Hermann Schwab]

partly wfm Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.6b) Gecko/20031118

As the fix got checked in 11/17/2003 17:59, I was assuming BuildID 2003111808
would contain it. The testcase from Attachment: is working, I don´t see a menu
however, using the URL from the URL: field: http://www.iki.fi/avs/mozjs.html 
Netscape 4.79 shows the menu.

Comment 32

[Boris Zbarsky [:bzbarsky]]

See comment 12 and comment 16.  The site needs evangelizing no matter what.

Comment 33

[Andrew Hagen]

Last post here was over five years ago. 

The URL is dead. The site http://www.asiantuntijat.org/~avs/ looks like it works, although it doesn't seem to have any Javascript menus. 

Resolving this as fixed as there was a check-in.