Opened 8 years ago
Closed 8 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 8 years ago by
comment:2 Changed 8 years ago by
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 8 years ago by
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 8 years ago by
Milestone: | tbd → 1.9 |
---|---|
Priority: | undecided → low |
comment:5 Changed 8 years ago by
Milestone: | 1.9 → 1.9.2 |
---|
Presumably you meant 1.9.2, since 1.9.0 was already released.
comment:6 Changed 8 years ago by
yup, thanks for catching that.
if no one has objections, i will land this tomorrow
comment:7 Changed 8 years ago by
Owner: | set to liucougar |
---|---|
Status: | new → assigned |
comment:8 Changed 8 years ago by
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 8 years ago by
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 8 years ago by
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)
comment:11 Changed 8 years ago by
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 8 years ago by
pushed a new patch to the pull request to move the fix to on.selector()
comment:15 Changed 8 years ago by
Resolution: | → fixed |
---|---|
Status: | assigned → closed |
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?