lite query engine matches() fails when the node is a text node

[liucougar]

this is noticed in our app because sometimes firefox would have a text node as event.target, and this breaks lite-query engine matches

[Kitson Kelly]

This is a pretty fundamental change in something that has been there for years. I think we need to be very careful about patching this. I am not saying it is wrong, I am just saying there is a huge risk for other regressions.

Can we see a specific example of where this breaks?

[liucougar]

the same test case does work correctly in acme, only breaks in lite

I think the patch is low risk: if you pass in a text node, the current implementation will throw error: Value does not implement interface Element.

if someone encountered this, they would for sure see the same error

as to test case: i tried to make a simple test case, but I failed to do that. if you want, i could send you info how to see it in our app.

[bill]

Makes sense to me. If the patch changed the matches() return value from true to false then it might break existing applications, but since matches() currently just throws an error I don't see any risk.

The argument to make the lite engine and acme consistent also makes sense.

[bill]

Presumably you meant 1.9.2, since 1.9.0 was already released.

[liucougar]

yup, thanks for catching that.

if no one has objections, i will land this tomorrow

[Bryan Forbes]

I have an objection. I have only seen this in Webkit when using event delegation with the 'mousewheel' event. Since this really isn't a problem with the engine and more of a problem with Webkit emitting events on improper targets or not including matches() on text nodes. In addition, this is such a corner case that I'd rather put a note in documentation than make a fix to this that does not meet DOM standards.

[liucougar]

I encountered this bug in firefox 21, when using event delegation with 'click' event.

could you enlighten me why this does not meet standards? I consulted MDN, and could not find anything specifically talking about text node.

according to ​http://www.w3.org/TR/DOM-Level-2-Events/: event.target is of type EventTarget? (​http://www.w3.org/TR/DOM-Level-2-Events/events.html#Events-Event-target).

EventTarget? interface is defined as having addEventListener/removeEventListener/dispatchEvent (​http://www.w3.org/TR/DOM-Level-2-Events/events.html#Events-EventTarget), all of which exist on text node in firefox

so it seems to me the standard does not rule out the possibility of event.target being a text node. also, i am seeing this issue in firefox, so it's not limited to webkit

[Kris Zyp]

I think this seems like a good thing to fix, but could we assuage the concerns that were mentioned by moving the check for text nodes to dojo/on.js's selector function (basically adding an or'ed condition to the while loop of eventTarget.nodeType == 3)

[Bryan Forbes]

I like kzyp's idea: move the fix to the delegation code as that's really where the problem is in my mind. Text nodes don't implement matches, matchesSelector, or webkitMatchesSelector (because they can't be targeted by a CSS query) and if they are targeted by an event, we should be running the query engine on their parent. In a previous project, I had done this:

on(node, 'mousewheel', function (evt) {
    var target = evt.target;

    if (target.nodeType !== 1) {
        target = target.parentNode;
    }
    ...
});

It seems we could do something similar in on.selector().

[liucougar]

pushed a new patch to the pull request to move the fix to on.selector()

[liucougar]

anyone have comments on this?

[liucougar <liucougar@…>]

In 13cdcc934d44915bb51304937a6782a81d79c235/dojo:

Error: Processor CommitTicketReference failed

Unsupported version control system "git": Can't find an appropriate component, maybe the corresponding plugin was not enabled? 

[Kris Zyp <kriszyp@…>]

In 3f100b1950ea771c3d6254ff2d5230ece5a012c2/dojo:

Error: Processor CommitTicketReference failed

Unsupported version control system "git": Can't find an appropriate component, maybe the corresponding plugin was not enabled?