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)
Core
DOM: HTML Parser
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
|
jst
:
superreview+
|
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...
Assignee | ||
Comment 1•22 years ago
|
||
Comment 2•22 years ago
|
||
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•22 years ago
|
||
Attachment #102845 -
Attachment is obsolete: true
Assignee | ||
Comment 4•22 years ago
|
||
Attachment #103079 -
Attachment is obsolete: true
Assignee | ||
Comment 5•22 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•22 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•22 years ago
|
||
milestone changed to mozilla1.3alpha
Target Milestone: --- → mozilla1.3alpha
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.
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 11•22 years ago
|
||
Marking topembed+ as per topembed triage.
Assignee | ||
Comment 12•22 years ago
|
||
Attachment #103080 -
Attachment is obsolete: true
Assignee | ||
Comment 13•22 years ago
|
||
Assignee | ||
Comment 14•22 years ago
|
||
hey adam, which parts of the IsSameEvent(...) comparison do you think should be case-insensitive? -- rick
Comment 15•22 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•22 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•22 years ago
|
||
Attachment #104663 -
Attachment is obsolete: true
Comment 18•22 years ago
|
||
Hi Rick, I think your patch is missing nsScriptEventManager.h/.cpp can you provide those?
Assignee | ||
Comment 19•22 years ago
|
||
Attachment #104904 -
Attachment is obsolete: true
Comment 20•22 years ago
|
||
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•22 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 22•22 years ago
|
||
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•22 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•22 years ago
|
||
Comment on attachment 105126 [details] [diff] [review] includes nsScriptManager.h and .cpp r=adamlock
Attachment #105126 -
Flags: review+
Assignee | ||
Comment 25•22 years ago
|
||
This patch addresses jst and adams comments...
Attachment #105126 -
Attachment is obsolete: true
Assignee | ||
Comment 26•22 years ago
|
||
These changes address jst and adams comments
Attachment #104664 -
Attachment is obsolete: true
Assignee | ||
Comment 27•22 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•22 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•22 years ago
|
||
Comment 30•22 years ago
|
||
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•22 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•22 years ago
|
||
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
Assignee | ||
Comment 33•22 years ago
|
||
Comment on attachment 105868 [details] [diff] [review] XPConnect.cpp sr=rpotts@netscape.com
Attachment #105868 -
Flags: superreview+
Comment 34•22 years ago
|
||
My fix is checked in, marking fixed.
Status: NEW → RESOLVED
Closed: 22 years ago
Resolution: --- → FIXED
Comment 35•22 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•22 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•22 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•21 years ago
|
||
Ashish, can you verify this as fixed?
Blocks: 190852
QA Contact: moied → ashishbhatt
Comment 40•21 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.
Description
•