Scope in event handlers

[Maciej Stachowiak]

On Mar 11, 2005, at 8:07 AM, David Faure wrote:

> 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?

I think it is indeed the right fix to figure out the scope based on the 
node the handler is attached to. Specifically, trying this test case in 
other browsers is what convinced me:

<div id="foo" onclick="alert(customProperty)">foo</div>
<div id="bar">bar</div>
<script>
document.getElementById("foo").customProperty="foo";
document.getElementById("bar").customProperty="bar";
document.getElementById("bar").onclick = 
document.getElementById("foo").onclick;
</script>

When you click on the "bar" div, the alert says "foo". In other words, 
the scope mangling is done statically at the time the handler is 
declared in an attribute, and even if you move it to another node it 
won't change.

Here is my patch to do this (sorry for not sending it sooner):

  
And here's a patch that is needed to correct a mistake in my original 
patch (it was wrong about handling body elements, since they put their 
scope


  


>
> 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).
> <eventhandlerscope.html>
> _______________________________________________
> Khtml-devel at kde.org
> https://mail.kde.org/mailman/listinfo/khtml-devel

-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://mail.kde.org/pipermail/khtml-devel/attachments/20050311/b961455f/attachment-0003.html
-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: scope-change.patch.txt
Url: http://mail.kde.org/pipermail/khtml-devel/attachments/20050311/b961455f/scope-change.patch-0001.txt
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://mail.kde.org/pipermail/khtml-devel/attachments/20050311/b961455f/attachment-0004.html
-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: scope-crash-fix.patch.txt
Url: http://mail.kde.org/pipermail/khtml-devel/attachments/20050311/b961455f/scope-crash-fix.patch-0001.txt
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://mail.kde.org/pipermail/khtml-devel/attachments/20050311/b961455f/attachment-0005.html