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

RESOLVED FIXED in mozilla1.3alpha

Status

()

Core
HTML: Parser
RESOLVED FIXED
15 years ago
14 years ago

People

(Reporter: rpotts (gone), Assigned: rpotts (gone))

Tracking

({topembed+})

Trunk
mozilla1.3alpha
topembed+
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 9 obsolete attachments)

41.67 KB, patch
rpotts (gone)
: review+
rpotts (gone)
: superreview+
Details | Diff | Splinter Review
1.01 KB, patch
Details | Diff | Splinter Review
6.67 KB, patch
rpotts (gone)
: superreview+
Details | Diff | Splinter Review
(Assignee)

Description

15 years ago
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...
(Assignee)

Comment 1

15 years ago
Created attachment 102845 [details] [diff] [review]
very preliminary WIP
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
(Assignee)

Comment 3

15 years ago
Created attachment 103079 [details] [diff] [review]
WIP v1.2 -- correctly supports arguments passed to event handlers.
Attachment #102845 - Attachment is obsolete: true
(Assignee)

Comment 4

15 years ago
Created attachment 103080 [details] [diff] [review]
WIP v1.3 - the *correct* patch :-)
Attachment #103079 - Attachment is obsolete: true
(Assignee)

Comment 5

15 years ago
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
(Assignee)

Comment 6

15 years ago
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

Comment 7

15 years ago
milestone changed to mozilla1.3alpha
Target Milestone: --- → mozilla1.3alpha

Updated

15 years ago
Blocks: 176349

Comment 8

15 years ago
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.

Comment 9

15 years ago
Created attachment 103999 [details] [diff] [review]
XPConnect.xpp

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.

Comment 10

15 years ago
Bulk adding topembed keyword.  Gecko/embedding needed.
Keywords: topembed

Comment 11

15 years ago
Marking topembed+ as per topembed triage.
Keywords: topembed → topembed+
(Assignee)

Comment 12

15 years ago
Created attachment 104663 [details] [diff] [review]
v1.4 -- Changes to support <SCRIPT FOR= EVENT= ...>
Attachment #103080 - Attachment is obsolete: true
(Assignee)

Comment 13

15 years ago
Created attachment 104664 [details] [diff] [review]
v1.4 -- changes to XPConnect.cpp
(Assignee)

Comment 14

15 years ago
hey adam,

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

-- rick

Comment 15

15 years ago
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.
(Assignee)

Comment 16

15 years ago
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(...)
(Assignee)

Comment 17

15 years ago
Created attachment 104904 [details] [diff] [review]
v1.5 -- This patch addresses adam's comments
Attachment #104663 - Attachment is obsolete: true

Comment 18

15 years ago
Hi Rick, I think your patch is missing nsScriptEventManager.h/.cpp can you
provide those?
(Assignee)

Comment 19

15 years ago
Created attachment 105126 [details] [diff] [review]
includes nsScriptManager.h and .cpp
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 21

15 years ago
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+
(Assignee)

Comment 23

15 years ago
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 24

15 years ago
Comment on attachment 105126 [details] [diff] [review]
includes nsScriptManager.h and .cpp

r=adamlock
Attachment #105126 - Flags: review+
(Assignee)

Comment 25

15 years ago
Created attachment 105362 [details] [diff] [review]
v1.7 - Changes to support <SCRIPT FOR= EVENT= ...>

This patch addresses jst and adams comments...
Attachment #105126 - Attachment is obsolete: true
(Assignee)

Comment 26

15 years ago
Created attachment 105364 [details] [diff] [review]
v1.7 -- Changes to XPConnect.cpp

These changes address jst and adams comments
Attachment #104664 - Attachment is obsolete: true
(Assignee)

Comment 27

15 years ago
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+
(Assignee)

Comment 28

15 years ago
Comment on attachment 105362 [details] [diff] [review]
v1.7 - Changes to support <SCRIPT FOR= EVENT= ...>

this patch has been checked into the trunk.

-- rick
(Assignee)

Comment 29

15 years ago
Created attachment 105539 [details] [diff] [review]
patch for nsGlobalWindow.cpp (was missing from the v1.7 patch)
Comment on attachment 105539 [details] [diff] [review]
patch for nsGlobalWindow.cpp (was missing from the v1.7 patch)

sr=jst
Attachment #105539 - Flags: superreview+
(Assignee)

Comment 31

15 years ago
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...

Comment 32

15 years ago
Created attachment 105868 [details] [diff] [review]
XPConnect.cpp

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

Updated

15 years ago
Attachment #105364 - Attachment is obsolete: true
(Assignee)

Comment 33

15 years ago
Comment on attachment 105868 [details] [diff] [review]
XPConnect.cpp

sr=rpotts@netscape.com
Attachment #105868 - Flags: superreview+

Comment 34

15 years ago
My fix is checked in, marking fixed.
Status: NEW → RESOLVED
Last Resolved: 15 years ago
Resolution: --- → FIXED

Comment 35

15 years ago
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?
(Assignee)

Comment 36

15 years ago
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


Comment 37

15 years ago
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.
this caused bug 184159

Comment 39

15 years ago
Ashish, can you verify this as fixed?
Blocks: 190852
QA Contact: moied → ashishbhatt

Comment 40

14 years ago
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.