Lighthouse has a new layout. Prefer the old one? Return to the old layout, and switch back any time from the link at the top of each page.

Wrong this in $w.dispatchEvent for on* callbacks

#68

OK, not 100% sure about this one (I've been working all night on hacking envjs), but in $w.dispatchEvent(), when it's calling "on___" callbacks associated with an event, you use "this" where it really should be "this" (_this probably refers in the scope to window or document, I haven't checked)

I will put the code of both calls made for events attached through addEventListener() & on__ callbacks, so you can compare (the error probably comes from code copied from the first block):

        if ( this.uuid && $events[this.uuid][event.type] ) {
            var _this = this;
            $events[this.uuid][event.type].forEach(function(fn){
                $debug('calling event handler '+fn+' on target '+_this);
                fn.call( _this, event );
            });
        }

        if (this["on" + event.type]) {
            $debug('calling event handler '+event.type+' on target '+this);
            // Right under here:
            this["on" + event.type].call(_this, event);
        }

It really should be

           this["on" + event.type].call(this, event);
so in the scope of the callback "this" will be the element associated with the callback

Reported by Jonathan R.-Brochu · May 22nd, 2009 @ 02:54 AM

State: resolved
Milestone: 1.1 Release
Assigned to: nobody

Activity

  1. Jonathan R.-Brochu
    Jonathan R.-Brochu

    Sorry, in the first paragraph please don't mind the messed-up formatting, so you should read:

    [...] you use "_this" where it really should be "this" [...]
    

    May 22nd, 2009 @ 03:06 AM

  2. gleneivey
    gleneivey
    • Tag changed from event to event, scope

    I think I fixed this (in a different way) in all of the changes I made to the scoping of window objects and frames. Also, note that even if the proposed change does get the right "this" for event handlers specified by HTML attributes, it doesn't build a scope chain including the other DOM elements that surround the event's target.

    What is the proper procedure for getting verification that the current top-of-tree addresses this problem and for closing this ticket?

    August 22nd, 2009 @ 04:58 AM

  3. Jonathan R.-Brochu
    Jonathan R.-Brochu

    (I apologize in advance in case I seem a bit lost, I haven't followed or played with EnvJS in a while)

    First, I just realized I never mentioned this bug was for the code in "src/window/event.js"

    I just checked Thatcher's branch (is it the main one?), and the commit 9bc5f5d603d911ea8fe8d52f806e296a0dcde7f3 on 2009-08-18 effectively makes this bug irrelevant, as the change in scoping eliminated the problem. Well, providing the new scope handling is conform.

    I looked a bit at "test/unit/events.js" and the likes, and out-of-my-head here's how I think I would check it. Please note though, it hasn't been tested:

    test("Check that 'this' in event handlers refers to the object receiving the event", function() {
        expect(4);
    
        // get a few objects the event will bubble up to
        var img = document.getElementById('eventsFrame')
          .contentDocument.getElementById('theIMG');
        var a = document.getElementById('eventsFrame')
          .contentDocument.getElementById('theA');
        var p = document.getElementById('eventsFrame')
          .contentDocument.getElementById('theP');
        var li = document.getElementById('eventsFrame')
          .contentDocument.getElementById('theLI');
    
        // add handlers
        addHdlr = function(elem, id) {
            elem.addEventListener('click', function(event){
                try{
                    ok( event.target === img && this === elem,
                        "Scope: 'this' refers to element '" + id + "'");
                }catch(e){print(e);}
            });
        }
        addHdlr(img, "theIMG");
        addHdlr(a, "theA");
        addHdlr(p, "theP");
        addHdlr(li, "theLI");
    
        // create and dispatch event
        __click__(img);    
    });
    

    August 28th, 2009 @ 01:40 PM

  4. Thatcher
    Thatcher
    • Milestone set to 1.0 Release
    • State changed from new to open

    I added the test in a comment block in unit/events.js so we can verify the test is correct and make sure the tests passing before closing the ticket. as a note it does fail, so either the test is incorrect or the behavior (or both;))

    September 9th, 2009 @ 07:06 PM

  5. Thatcher
    Thatcher
    • Milestone changed from 1.0 Release to 1.1 Release

    September 9th, 2009 @ 07:26 PM

  6. Thatcher
    Thatcher

    changed milestone since I'm trying to do an offical 1.0.0 tag and this is important but fits best with 1.1 goals

    September 9th, 2009 @ 07:27 PM

  7. gleneivey
    gleneivey

    I believe this issue is resolved in commit 66c2765 which I have just pushed to thatcher//master.

    I was messing with test/unit/events.js for something else (adding testing of 'onunload' events), and noticed the commented-out test at the end with a pointer to this ticket. Unfortunately, the description of the problem here and the expected results in the test case aren't correct. JavaScript code that is in the literal value assigned to an event-handler HTML attribute in the page text itself is supposed to execute in a context where this refers to the DOM object for the HTML element where the handler is defined, as described above. (I actually implemented this behavior some time ago, but didn't know about this ticket to clear it then.) However, event handlers that are defined in the normal flow of JavaScript execution and assigned to the DOM object's event handler by JavaScript (i.e., element.onload=aFunction or element.addEventListener()) execute with the this that is appropriate to the scope in which they are defined. The test case above creates handlers with addEventListner() and so should not pass when executed in env.js or a real browser.

    I modified my existing test cases so that the handlers specified as HTML attributes check for the correct values of their this references. I also noticed, though, that the proposed test case is the only place (at least that I'm aware of) where we employ addEventListener() in the unit tests. So, I re-purposed and refactored the proposed test to focus on this, and it is included in my commit referenced above.

    Hopefully u'all will agree that this ticket can be closed,

    glen
    

    September 20th, 2009 @ 06:32 PM

  8. Thatcher
    Thatcher
    • State changed from open to resolved

    thanks for following up on this glen

    November 12th, 2009 @ 05:15 AM

Please Sign in or create a free account to add a new ticket.

With your very own profile, you can contribute to projects, track your activity, watch tickets, receive and update tickets through your email and much more.