Opened 6 years ago

Closed 6 years ago

#17301 closed defect (fixed)

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

Reported by: liucougar Owned by: liucougar
Priority: low Milestone: 1.9.2
Component: Query Version: 1.9.1
Keywords: Cc:
Blocked By: Blocking:

Description

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

Change History (15)

comment:1 Changed 6 years ago by 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?

comment:2 Changed 6 years ago by 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.

comment:3 Changed 6 years ago by 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.

comment:4 Changed 6 years ago by liucougar

Milestone: tbd1.9
Priority: undecidedlow

comment:5 Changed 6 years ago by bill

Milestone: 1.91.9.2

Presumably you meant 1.9.2, since 1.9.0 was already released.

comment:6 Changed 6 years ago by liucougar

yup, thanks for catching that.

if no one has objections, i will land this tomorrow

comment:7 Changed 6 years ago by liucougar

Owner: set to liucougar
Status: newassigned

comment:8 Changed 6 years ago by 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.

comment:9 Changed 6 years ago by 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

comment:10 Changed 6 years ago by 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)

Last edited 6 years ago by Kris Zyp (previous) (diff)

comment:11 Changed 6 years ago by 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().

comment:12 Changed 6 years ago by liucougar

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

comment:13 Changed 6 years ago by liucougar

anyone have comments on this?

comment:14 Changed 6 years ago by 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? 

comment:15 Changed 6 years ago by Kris Zyp <kriszyp@…>

Resolution: fixed
Status: assignedclosed

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? 
Note: See TracTickets for help on using tickets.