Bug Tracker

Opened 10 years ago

Closed 9 years ago

Last modified 9 years ago

#5539 closed bug (invalid)

Event target is wrong on DOMCharacterDataModified

Reported by: hesselink Owned by: hesselink
Priority: low Milestone: 1.4
Component: event Version: 1.3.2
Keywords: Cc:
Blocked by: Blocking:

Description

If you bind the DOMCharacterDataModified event, the event object your handler gets has the wrong target (the parent node instead of the text node). This is because of these lines:

// check if target is a textnode (safari)
if ( event.target.nodeType == 3 )
  event.target = event.target.parentNode;

Since there are other events, like DOMNodeInsertedIntoDocument and DOMNodeRemovedFromDocument that might want a text node as a target, I added the following to fix it:

 && event.type.slice(0,3) != "DOM"

Change History (14)

comment:1 Changed 9 years ago by dmethvin

That's a tough one, I'm not sure you want event.target pointing to a text node in most cases.

comment:2 Changed 9 years ago by Rick Waldron

Owner: changed from brandon to hesselink
Status: newpending

Please provide a distilled and reduced jsFiddle test case, thanks!

comment:3 Changed 9 years ago by hesselink

Status: pendingnew

I'm not sure what you want me to do, or why. Was my explanation not clear enough?

If you bind a DOMCharacterDataModified event (using jQuery.bind), and a text node has its character data modified, you get an event, but not with the text node as a target, but its parent. This is not what you generally want, because there can be multiple text nodes in the parent, and you have no way to figure out which one it is.

This is generally what you want for all DOM mutation events. That is why my fix changed the jQuery behaviour (which, according to the source comment, works around a bug in Safari) changed only the behaviour of events where the name starts with DOM. So this doesn't change anything in 'most cases', only in a few very specific ones (i.e. DOM mutation events).

I'm not sure how me writing code in jsFiddle (which I'm not familiar with) would be a good investment of my or your time. If you can explain why it is, please do.

comment:5 Changed 9 years ago by Rick Waldron

Keywords: needsreview added
Priority: majorlow

Creating and submitting a jsFiddle that illustrates the behavior that you're reporting saves us the time of trying to recreate your issue based on your description. It also expedites the review process for your issue - faster in, faster out.

Additionally, jQuery does not have documented support for the DOM MutationEvents and therefore cannot be accountable for unexpected behavior when attempting to use them.

comment:6 Changed 9 years ago by hesselink

Is there any chance you will accept this fix if I take the time to learn this tool and write a test case, given that you state that there is no documented support for DOM mutation events?

comment:7 Changed 9 years ago by Rick Waldron

I can't guarantee anything, but I can say with confidence that contributions are taken into further consideration with more frequence then just requests

comment:8 Changed 9 years ago by hesselink

Allright, here it is: http://jsfiddle.net/58gD4/1/

comment:9 Changed 9 years ago by Rick Waldron

Resolution: invalid
Status: newclosed

Thanks for that.

I've further reduced the test here: http://jsfiddle.net/rwaldron/RfHZP/2/

I'd like to note that you're binding an event handler to the div, so logically - the div is your target.

Additionally, it's not wise for jQuery core to normalize APIs that have been deprecated in the standards bodies. See here: http://dev.w3.org/2006/webapi/DOM-Level-3-Events/html/DOM3-Events.html#events-mutationevents

Again, thank you for taking the time to put this test case together.

comment:10 Changed 9 years ago by Rick Waldron

#6785 is a duplicate of this ticket.

comment:11 Changed 9 years ago by hesselink

Couldn't you have decided that before I invested time in the test case? It added nothing useful if you had just read the ticket. The event bubbles, by the way, so the target is not always the node it is bound to.

I must say I am very dissatisfied with how jquery has treated me. I had a fix that worked for me. I thought I'd be a good citizen and contribute the patch back. I've been ignored for 11 months, then I received an automated response that I had to write a test, only to spend time on it and promptly have the bug marked invalid. This is no way to treat your community.

You could have read the bug 11 months ago, and told me you wouldn't fix it because it is about DOM mutation events. You could have read the bug before telling me to write a test case. You could have told me you probably wouldn't accept the patch. Instead, you led me on, acted disinterested, and made it very unlikely I'll ever contribute again.

Throughout the process, I've felt I was working against the jquery people, who didn't want my contribution. I'd like to work with them, helping them understand the issue, and them helping me get a patch into shape. This whole thing left a very bad taste in my mouth.

comment:12 Changed 9 years ago by ajpiano

hesselink, the jQuery project is very happy to have contributions from the community. The reply you received was not automated, but rather the result of a recent, concerted effort to deal with the over 1500 backlogged ticket on the bug tracker, Part of that effort includes having standards for what the people handling ticket triage will and will not handle, and that entails having a test case of the behaviour. Just because this patch was not accepted does not mean your contributions, in general, are unwelcome. It is rather indicative of the simple fact that just because something works for a particular user in a particular application, that does not mean it is necessarily an appropriate fix to be landed in the library that will be distributed to all users, everywhere.

Yes, we could have rejected the ticket out of hand just because it had anything to do with mutation events, but with a solid test case, at least we have something to refer back to in the future. And we know that it is problematic that there are backlogged bugs, like this one - but dealing with them now is better than leaving them completely ignored. No one was leading you on, and there is a big difference between "not wanting your contribution" and "not adding support for this particular issue."

The new ticket triaging effort has, in fact, been spearheaded by community members who are doing much of the review and analysis of tickets, in coordination with a number of team members. Thus, I hope you will reconsider your decision to remove yourself from this process entirely, and reevaluate whether you really think that the interaction you had with respect to this ticket is truly "no way to treat the community," in light of the facts that I have shared.

Last edited 9 years ago by ajpiano (previous) (diff)

comment:13 Changed 9 years ago by hesselink

I understand the need for some process. However, in this case I feel that you traded my time for your process, which doesn't seem to add anything. This doesn't sound appealing for me, which is why I said I wouldn't contribute in the future.

I specifically asked if there was any chance you would accept this patch with a test case, since you already said jQuery didn't 'support' DOM mutation events. The reply could just have said: the chance we'll accept a patch concerning DOM mutation evens is very low. I wouldn't have wasted my time on a test case, and the outcome would have been the same.

I want to make very clear that my annoyance is not about my patch not being accepted. We have a local fork of jQuery which we will keep using, regardless of this patch being accepted. I'm annoyed with the strict adherence to process, without any regard for the specific contents of the bug.

comment:14 Changed 9 years ago by dmethvin

Keywords: needsreview removed
Note: See TracTickets for help on using tickets.