Skip to content
New issue

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

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix for #4707/#7101 #24

Closed
wants to merge 4 commits into from
Closed

Fix for #4707/#7101 #24

wants to merge 4 commits into from

Conversation

csnover
Copy link
Member

@csnover csnover commented Sep 30, 2010

Original commit for #4707 was incomplete and would only not clobber existing filters if one of those existing filters happened to be the opacity filter. New bug #7101 was raised about this. This patch ensures existing filters are not clobbered even if opacity filter does not exist.

Because my previous ajax pull request didn’t get committed upstream, this pull request is a bit fucked and includes 4 of my commits… sorry about that. I will try to learn for the future how to use this thing properly so it only pulls one commit instead of a range. :|

@jeresig
Copy link
Member

jeresig commented Sep 30, 2010

Landed! In the future you should make a separate branch for each bug fix that you make - this will avoid problems like this (and make it easier for me to land the change).

@tpreusse
Copy link

tpreusse commented Oct 3, 2010

I just notices you used
style.filter +' '+ opacity
this might have some disadvantages over using
filter +' '+ opacity
and could lead to different behaviour depending on whether a alpha filter has been applied before or not since for the true case
filter.replace(ralpha, opacity)
local filter variable is used and not style.filter

Thomas

@csnover
Copy link
Member Author

csnover commented Oct 3, 2010

filter is defined as style.filter || "" so there should be no behavioural difference unless style.filter is somehow null or undefined. It may be a better idea from a code size standpoint to use filter instead, though.

@tpreusse
Copy link

tpreusse commented Oct 3, 2010

You'r right I was looking at 1.4.2 where it was defined as
var filter = style.filter || jQuery.curCSS( elem, "filter" ) || "";

@lock lock bot locked as resolved and limited conversation to collaborators Jan 20, 2019
This pull request was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants