Scope in event handlers

[David Faure]

For a long time, KHTML has been pushing a wrong scope on event handlers defined in JS (foo.onclick=myfunc).
It should only be done for event handlers defined in HTML (onclick="...").

Please consider the patch below for Safari, which simply moves the code down to the right class.

There is another way of fixing that problem, if we can assume that an HTML event listener
will always only be used for the same node, which I think is the case. Then we could set the scope
when defining the function (i.e. in parseCode()) instead of doing it for each event.
But this would require passing the Node via many methods (khtml_part, kjs_proxy, html_doc etc.)
so it would be a much bigger patch - but if we agree that it would be correct we could do it
in both KHTML and Safari. What do you think?

Testcase attached, see in particular the 4th column.

----------  Forwarded Message  ----------

Subject: kdelibs/khtml
Date: Thursday 10 March 2005 18:54
From: David Faure <faure at kde.org>
To: kde-cvs at kde.org
Cc: khtml-cvs at kde.org

CVS commit by faure: 

Move "pushing element/form/doc onto the scope"
 from JSEventListener::handleEvent to JSLazyEventListener::handleEvent so that
 it only happens with listeners set from html attributes, not from JS - in which
 case the function already got a correct scope when created.
Testcase: events/eventhandlerscope.{html,js}


  M +8 -0      ChangeLog   1.398
  M +22 -11    ecma/kjs_events.cpp   1.93


--- kdelibs/khtml/ChangeLog  #1.397:1.398
@@ -1,2 +1,10 @@
+2005-03-10  David Faure  <faure at kde.org>
+
+        * ecma/kjs_events.cpp (handleEvent): Move "pushing element/form/doc onto the scope"
+        from JSEventListener::handleEvent to JSLazyEventListener::handleEvent so that
+        it only happens with listeners set from html attributes, not from JS - in which
+        case the function already got a correct scope when created.
+        Testcase events/eventhandlerscope.{html,js}
+
 2005-03-04  Allan Sandfeld Jensen <kde at carewolf.com>
 

--- kdelibs/khtml/ecma/kjs_events.cpp  #1.92:1.93
@@ -79,13 +79,5 @@ void JSEventListener::handleEvent(DOM::E
     // Set "this" to the event's current target
     Object thisObj = Object::dynamicCast(getDOMNode(exec,evt.currentTarget()));
-    ScopeChain oldScope = listener.scope();
-    if ( thisObj.isValid() ) {
-      ScopeChain scope = oldScope;
-      // Add the event's target element to the scope
-      // (and the document, and the form - see KJS::HTMLElement::eventHandlerScope)
-      static_cast<DOMNode*>(thisObj.imp())->pushEventHandlerScope(exec, scope);
-      listener.setScope( scope );
-    }
-    else {
+    if ( !thisObj.isValid() ) {
       if ( m_hackThisObj.isValid() ) { // special hack for Image
         thisObj = m_hackThisObj;
@@ -110,6 +102,4 @@ void JSEventListener::handleEvent(DOM::E
     guard.stop();
 
-    listener.setScope( oldScope );
-
     window->setCurrentEvent( 0 );
     interpreter->setCurrentEvent( 0 );
@@ -158,5 +148,26 @@ void JSLazyEventListener::handleEvent(DO
   parseCode();
   if (!listener.isNull()) {
+
+    KHTMLPart *part = ::qt_cast<KHTMLPart *>(static_cast<Window*>(win.imp())->part());
+    KJSProxy *proxy = 0;
+    if (part)
+      proxy = part->jScript();
+    KJS::ScriptInterpreter *interpreter = static_cast<KJS::ScriptInterpreter *>(proxy->interpreter());
+    ExecState *exec = interpreter->globalExec();
+    ScopeChain oldScope = listener.scope();
+    Object thisObj = Object::dynamicCast(getDOMNode(exec,evt.currentTarget()));
+    if ( thisObj.isValid() ) {
+      ScopeChain scope = oldScope;
+      // Add the event's target element to the scope
+      // (and the document, and the form - see KJS::HTMLElement::eventHandlerScope)
+      // ### we could do this in parseCode, once and for all, if only we
+      // had the node in the constructor (would require passing it through KHTMLPart)
+      static_cast<DOMNode*>(thisObj.imp())->pushEventHandlerScope(exec, scope);
+      listener.setScope( scope );
+    }
+
     JSEventListener::handleEvent(evt);
+
+    listener.setScope( oldScope );
   }
 }


-------------------------------------------------------

-- 
David Faure, faure at kde.org, sponsored by Trolltech to work on KDE,
Konqueror (http://www.konqueror.org), and KOffice (http://www.koffice.org).
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://mail.kde.org/pipermail/khtml-devel/attachments/20050311/b28e67c7/eventhandlerscope.html