Closed Bug 174404 Opened 22 years ago Closed 22 years ago

support the (optional) FOR and EVENT attributes in the SCRIPT tag...

Categories

(Core :: DOM: HTML Parser, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla1.3alpha

People

(Reporter: rpotts, Assigned: rpotts)

References

Details

(Keywords: topembed+)

Attachments

(3 files, 9 obsolete files)

41.67 KB, patch
rpotts
: review+
rpotts
: superreview+
Details | Diff | Splinter Review
1.01 KB, patch
Details | Diff | Splinter Review
6.67 KB, patch
rpotts
: superreview+
Details | Diff | Splinter Review
If a script tag contains 'FOR' and 'EVENT' attributes then treat it as an event
handler rather than an inline script block.

This means that script compilation should be defered...
Attached patch very preliminary WIP (obsolete) — Splinter Review
I assume you mean the MS extensions described at

http://msdn.microsoft.com/workshop/author/dhtml/reference/properties/event.asp
and
http://msdn.microsoft.com/workshop/author/dhtml/reference/properties/htmlfor_0.asp

?  If I read that documentation correctly, the htmlFor attribute can be assigned
a Node _or_ and id, no?
OS: Windows 2000 → All
Hardware: PC → All
Attachment #102845 - Attachment is obsolete: true
Attachment #103079 - Attachment is obsolete: true
hey boris,

it looks like the htmlFor attribute is of type STRING so i don't believe it can
hold a Node by reference...  i *think* this is correct :-)  the documentation sux...

-- rick
oops... the patch i attached has a typo which means that if you call an event
handler with more than 10 arguments you will crash...

oh well... it's a minor flaw and i'll fix it in the next version :-)

-- rick
milestone changed to mozilla1.3alpha
Target Milestone: --- → mozilla1.3alpha
Blocks: grouper
A couple of minor nits discovered in the patch:

1) IsSameEvent needs to do case insensitive name comparisons
2) You probably want to pass argArray, not &argArray in the CallEventHandler fn.

I'll submit an updated diff of XPConnect.cpp which makes use of David's variant
conversion service.
Attached patch XPConnect.xpp (obsolete) — Splinter Review
diff -u doesn't exactly make things clear, but the patch adds code to create an
array of jsval values using the IDispatch support service which are passed in
the call to Invoke on the script event handler.
Bulk adding topembed keyword.  Gecko/embedding needed.
Keywords: topembed
Marking topembed+ as per topembed triage.
Keywords: topembedtopembed+
Attachment #103080 - Attachment is obsolete: true
Attached patch v1.4 -- changes to XPConnect.cpp (obsolete) — Splinter Review
hey adam,

which parts of the IsSameEvent(...) comparison do you think should be
case-insensitive?

-- rick
The EVENT tag needs to be case insensitive to handle someEvent even if the value
in the tag is "SomeEvent", "SOMEEVENT" or whatever.

So I think the part that says "aEventName.Equals(mEventName)" needs to be case
insensitive.
hey adam,

you're right... it seems that the event-binding is MUCH looser than i thought :-(

apparently, *only* the object name is case sensitive and the number of arguments
is  NOT even considered in the binding process...

so, i'll update the patch to reflect these changes to IsSameEvent(...)
Attachment #104663 - Attachment is obsolete: true
Hi Rick, I think your patch is missing nsScriptEventManager.h/.cpp can you
provide those?
Attachment #104904 - Attachment is obsolete: true
Comment on attachment 104664 [details] [diff] [review]
v1.4 -- changes to XPConnect.cpp

+    nsAutoString eventName(bstrName.m_str);

I see no reason why this couldn't be a nsDependentString in stead of an
nsAutoString, that way we'd save the cost of the nsAutoString constructor and
the string copy.

Other than that, sr=jst
Attachment #104664 - Flags: superreview+
Comment on attachment 105126 [details] [diff] [review]
includes nsScriptManager.h and .cpp

Patch looks pretty good, but I have some nits and a few questions;

nsIScriptEventManager nsIScriptEventHandler need documenting.

How does nsScriptEventManager cope with dynamic HTML? It looks like it creates
a list of script elements during construction, but it's possible that more
could be added. Perhaps this should be deferred to the first call to
FindEventHandler()?

No newline at end of nsScriptEventManager.h

nsIScriptEventHandler::IsSameEvent still has an argcount param. Is this
necessary anymore?

David has defined a native JSVal typedef in idl to pass through JS values (see
nsIDispatchSupport in bug 173146). Perhaps nsIScriptEventHandler::Invoke should
use one too instead of casting to a void *?

Hardcoded '10' in Invoke. Could it be made a const?

Should Invoke really compile the event handler function each time, or cache it
for performance? What happens to the compiled funcObject each time, is it
garbage collected or leaking?
Comment on attachment 105126 [details] [diff] [review]
includes nsScriptManager.h and .cpp

- In nsIScriptEventHandler.idl:

+  boolean IsSameEvent(in AString aObjectName,
+		       in AString aEventName,
+		       in unsigned long aArgCount );
+
+  void Invoke(in nsISupports aTargetObject,
+	       in voidPtr aArgs,
+	       in unsigned long aArgCount );

Extra space before the closing paren.

- In nsScriptEventManager.h

\ No newline at end of file

Add a newline at the end of the file.

sr=jst
Attachment #105126 - Flags: superreview+
answers to various comments...

>jst: 
>(From update of attachment 104664 [details] [diff] [review])
>+    nsAutoString eventName(bstrName.m_str);

Yes.  I'll change that to a nsDependentString.  I was just a little aftaid at
first ;-)

>adam:
>
>nsIScriptEventManager nsIScriptEventHandler need documenting.

Yes.  I'll add some comments describing these interfaces... at least i got the
right license in the headers!!

>
>How does nsScriptEventManager cope with dynamic HTML? It looks like it creates
>a list of script elements during construction, but it's possible that more
>could be added. Perhaps this should be deferred to the first call to
>FindEventHandler()?

It is my understanding that the collection of script elements is 'live'.  So if
new script elements are added to the document, then they will *also* be added to
the collection.

That's why I walk the collection from last to first -- in order to pick up the
most recent event definitions.

>No newline at end of nsScriptEventManager.h

I'll fix that in the next patch :-)

>
>nsIScriptEventHandler::IsSameEvent still has an argcount param. Is this
>necessary anymore?

I left the argcount parameter there in case we wanted to leverage this interface
for firing plugin events in the future... From an API perspective it makes sense
to pass in the argument count along with the array...

>
>David has defined a native JSVal typedef in idl to pass through JS values (see
>nsIDispatchSupport in bug 173146). Perhaps nsIScriptEventHandler::Invoke should
>use one too instead of casting to a void *?

I chose 'void*' because that seemed to be the current convention used in
nsIScriptContext...  I agree that it's not very typesafe ;-)

Perhaps we should open up a separate bug to replace 'void*' with 'JSVal*' in all
JS related interfaces...

>
>Hardcoded '10' in Invoke. Could it be made a const?

Sure I can make it a 'const'.  The only reason it's hardcoded is because it's an
implementation detail that i figured no one cared about :-)

Since it's just the number of arguments at which point the heap will be used
instead of the stack (for temporary storage)...  I figured that no one would
care to change it...

>
>Should Invoke really compile the event handler function each time, or cache it
>for performance? What happens to the compiled funcObject each time, is it
>garbage collected or leaking?

Since compiling and caching the handler function is an implementation detail of
the nsHTMLScriptEventHandler, i figured that i'd hold off until it seemed this
was necessary...

Caching the JSObjects is trickier because it required rooting them.  This could
lead to massive leaks if they are not correctly unrooted.  Currently, each
function object is GCed as necessary...

I'll put together a new patches addressing these comments

-- rick
Comment on attachment 105126 [details] [diff] [review]
includes nsScriptManager.h and .cpp

r=adamlock
Attachment #105126 - Flags: review+
This patch addresses jst and adams comments...
Attachment #105126 - Attachment is obsolete: true
Attached patch v1.7 -- Changes to XPConnect.cpp (obsolete) — Splinter Review
These changes address jst and adams comments
Attachment #104664 - Attachment is obsolete: true
Comment on attachment 105362 [details] [diff] [review]
v1.7 - Changes to support <SCRIPT FOR= EVENT= ...>

moving r= and sr= forward from previous patch...
Attachment #105362 - Flags: superreview+
Attachment #105362 - Flags: review+
Comment on attachment 105362 [details] [diff] [review]
v1.7 - Changes to support <SCRIPT FOR= EVENT= ...>

this patch has been checked into the trunk.

-- rick
Comment on attachment 105539 [details] [diff] [review]
patch for nsGlobalWindow.cpp (was missing from the v1.7 patch)

sr=jst
Attachment #105539 - Flags: superreview+
Comment on attachment 105539 [details] [diff] [review]
patch for nsGlobalWindow.cpp (was missing from the v1.7 patch)

i've just checked this patch into the trunk too...
Attached patch XPConnect.cppSplinter Review
Updated patch for XPConnect.cpp removes a level of indentation and removes some
test cruft. Rick can you r/sr this?
Attachment #103999 - Attachment is obsolete: true
Attachment #105364 - Attachment is obsolete: true
Comment on attachment 105868 [details] [diff] [review]
XPConnect.cpp

sr=rpotts@netscape.com
Attachment #105868 - Flags: superreview+
My fix is checked in, marking fixed.
Status: NEW → RESOLVED
Closed: 22 years ago
Resolution: --- → FIXED
Note bug 179918, "<script FOR="window"> elements don't load in Mozilla"

Bug 179918 comment #7 has two URLs: one using <script FOR="window">,
and one not. The URL using it does not load the <script> element,
using a Mozilla trunk build from today, 2002-11-15.

Should this bug be reopened and bug 179918 made a duplicate?
well... not executing the <script FOR="window"> is the *correct* behavior as far
as i can tell.  that was the whole point of this bug ;-)

i think the BIG question is what IE does with this script??  does it execute it?
 and if so, when??

if it turns out that our behavior is different from IEs, the i think we should
open up a bug to address that issue.

but as far as i can tell, having mozilla execute the script tag was a bug in the
first place...

-- rick


IE6 loads "<script FOR="window"> elements. To see this, load 
http://www.pawella.com/index.html, which has this element:

<script type="text/javascript" language="JavaScript" FOR="window">

This element defines, among other things, an onload handler |laden|
for the frameset at this site. Now try this javascript:URL

               javascript: alert(typeof laden)

You will see that the function |laden| was successfully loaded in IE6.
In Mozilla, you get |undefined| since the script element wasn't loaded.
The site doesn't work properly in Mozilla as a consequence.
Ashish, can you verify this as fixed?
Blocks: 190852
QA Contact: moied → ashishbhatt
Please note that this only supports the <SCRIPT FOR="" EVENT=""> syntax for
custom mozilla builds with ActiveX support enabled.  It will NOT work for any of
the binaries distributed by mozilla.org, and it is unlikely that mozilla will
ever support this syntax by default.  Please see Bug 222117.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: