Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Fixes #12956 - Improve cloneFixAttributes function #1034

Closed
wants to merge 2 commits into
from

Conversation

Projects
None yet
4 participants
Member

markelog commented Nov 18, 2012

Events bounded via addEventListener will not be cloned in IE, events bounded via attachEvent will, but in IE9-10, jQuery does not use attachEvent method and even if it did, clear(merge)Attributes hack would not work for those events anyway.

So for new IE, there is no reason to use this hack.

Expando property is not cloned in IE9-10 and it might not exist at all, but removeAttribute method is expensive to use, it would be better to check for existence of expando before trying to remove it.

I would perform clear(merge)Attributes hack only if expando property is exist, but it would affect user events bounded via attachEvent, i'm wondering - is it a real issue?

/cc @dmethvin

Owner

dmethvin commented Nov 19, 2012

This makes sense too. We only want to use the clearAttributes/mergeAttributes junk on oldIE.

Member

markelog commented Nov 20, 2012

@dmethvin

We only want to use the clearAttributes/mergeAttributes junk on oldIE

Do you?

From where i'm standing this is not true. If so, and this –

but it would affect user events bounded via attachEvent, i'm wondering - is it a real issue?

is not a problem, you can eliminate clear(merge)Attributes hack altogether, by using detachEvent,
which will speed up cloning process and solve #9646.

Owner

dmethvin commented Nov 20, 2012

For IE9 and higher, we use addEventListener for our own needs, so we don't need it there. It's possible that someone is using attachEvent on their own elements in IE and then cloning the element, is that the case you were concerned about? That is really undefined territory. Anything attached directly with either attachEvent or addEventListener is basically something the user would need to worry about.

Member

rwaldron commented Nov 21, 2012

Two things... First, is there a ticket for this? Second, the patch needs to be rebased—Thanks! :)

Member

markelog commented Nov 25, 2012

@rwldrn

First

No, do you need one?

Second

Wait a bit, i will rebase shortly

Member

rwaldron commented Nov 25, 2012

No, do you need one?

We've been over this, yes, please make tickets for bugs/features/changes... everything

Member

markelog commented Nov 26, 2012

We've been over this, yes, please make tickets for bugs/features/changes... everything

Yeah, we did, many times, but i ask because your policy often changes, and sometimes, you push commits without tickets... So, as i understand – de jure i have to, but not de facto.

Member

markelog commented Nov 26, 2012

Member

rwaldron commented Nov 26, 2012

If you're referring to 4fed8eb, @Krinkle and I discussed this in person and the change itself is trivial.

This still needs to be rebased.

Member

markelog commented Nov 26, 2012

If you're referring to 4fed8eb, @Krinkle and I discussed this in person and the change itself is trivial.

By "you" i did not mean you specifically, i mean team does this – 1, 2, 3, 4...

This still needs to be rebased.

Before i do, i want to try couple of things with this code.

Member

rwaldron commented Nov 26, 2012

  1. Doesn't change any jQuery source
  2. Doesn't change any jQuery source
  3. @dmethvin is the leader of the project and has more understanding of the entire code base then anyone else in the world.
  4. I agree that this should've had a ticket.
Member

markelog commented Nov 26, 2012

Doesn't change any jQuery source

So?

@dmethvin is the leader of the project and has more understanding of the entire code base then anyone else in the world.

And?

These examples are not the only one, but never mind, i can create tickets if you like.

Member

markelog commented Nov 26, 2012

@rwldrn Although, if issue is purely with code it would be nice if it was discussed only at github, it's tediously to converse in different places

Member

rwaldron commented Nov 26, 2012

"Github Issues" is inadequate for our needs. I'm not interested in arguing about this any further.

Owner

dmethvin commented Nov 26, 2012

I'm not sure what is being argued, but @Orkel has been working on some of these tickets for a while. They just got tangled up with some other pull requests in the same vicinity.

Member

markelog commented Nov 28, 2012

This still needs to be rebased.

done.

It's possible that someone is using attachEvent on their own elements in IE and then cloning the element, is that the case you were concerned about?

Yep.

Anything attached directly with either attachEvent or addEventListener is basically something the user would need to worry about.

OK, then let's try new approach, i added more tests, just in case.

We are just in IE 6-8 territory here now, right? Can we just use dest.detachEvent( "on"+e, data.handle ) directly?

Owner

markelog replied Nov 28, 2012

We can, but jQuery.removeEvent also deals with oldIE memory leaks.

That's a mighty bold statement!

Owner

markelog replied Nov 28, 2012

That's just how I roll!
I can rephrase it to "almost every", just like here.

Owner

dmethvin commented Nov 28, 2012

So what should we do with gh-1036? Do you want to rebase that after this lands?

Member

markelog commented Nov 28, 2012

@dmethvin

Do you want to rebase that after this lands?

or vice-versa, whatever you decide...

Member

mikesherov commented Nov 29, 2012

@jquerybot retest

leoken pushed a commit to leoken/jquery that referenced this pull request Dec 10, 2012

Fixes #1034 - Check for style.removeAttribute before calling it
Fixes issue in non IE browsers that happen to come down this path

@dmethvin dmethvin closed this in 93e1892 Dec 12, 2012

mescoda pushed a commit to mescoda/jquery that referenced this pull request Nov 4, 2014

Fixes #1034 - Check for style.removeAttribute before calling it
Fixes issue in non IE browsers that happen to come down this path

mescoda pushed a commit to mescoda/jquery that referenced this pull request Nov 4, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment