Skip to content

Apply CSS shorthand expansion to IE<9 only #1953

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

Closed
wants to merge 1 commit into from
Closed

Apply CSS shorthand expansion to IE<9 only #1953

wants to merge 1 commit into from

Conversation

syranide
Copy link
Contributor

Following up on #1886 and #1901, perhaps this is an edge-case that is unlikely to have a significant real-word impact, but faster is better (and a step towards isolating IE8 fixes). :)

@zpao Feel free to do as you want, but perhaps this is a solution for starting small and going from there.

Unsetting shorthand bug is reproducible on IE when document mode is 8 or less (we don't actually support IE<8, but whatever). According to all information I can find (and common sense), this only applies to IE<8 and no other browser. http://bugs.jquery.com/ticket/12385 jamietre/CsQuery#93

Warning: #2407 need to verify if this PR actually surfaces that problem even more

@syranide
Copy link
Contributor Author

If you want, I could turn the IE8-detection into feature detection instead. Which perhaps makes more sense as per the discussion in the other PR. EDIT: But, React does use docoument.documentMode in a lot of places for the event system.

@sebmarkbage
Copy link
Collaborator

Yes, feature detection, please. Two wrongs doesn't make a right.

@syranide
Copy link
Contributor Author

@sebmarkbage Well is (safe) UA-detection for a nonsensical error in a legacy browser really worth feature testing? I feel it makes a temporary workaround a lot more permanent when there is no one who knows if it's actually necessary, or when it can be safely removed (but perhaps we don't want to "find out in production" either as I assume no one definitively knows if other browsers are affected...).

We use document.documentMode in a bunch of other places when we're keenly aware that only old IE is a culprit.

But say (repeat) the word and I'll do it :)

@syranide syranide self-assigned this Oct 1, 2014
@syranide
Copy link
Contributor Author

@sebmarkbage I changed it to use feature detection instead.

@syranide
Copy link
Contributor Author

Somewhat related and important: #2407

var styleFloatAccessor = 'cssFloat';
if (ExecutionEnvironment.canUseDOM) {
try {
// IE8 throws "Invalid argument." if resetting shorthand style properties.
document.createElement('div').style.font = '';
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why does this throw? All the info online I can find indicate that it silently does the wrong thing.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

>>document.createElement('div').style.font = '';
  "Invalid argument." (error)
>>document.createElement('div').style.fontSize = '';
  ""

@syranide
Copy link
Contributor Author

I'm gonna go with blocked until #2407 is resolved, at first glance it seems that #2407 should be fixed by the current lack of a feature test (but apparently isn't for some reason), and adding this feature test would make it more prevalent.

@sophiebits
Copy link
Collaborator

(That was me merging, GitHub seemed a little confused.)

sophiebits added a commit to sophiebits/react that referenced this pull request Aug 20, 2015
In conjunction with facebook#1953, fixes facebook#2407. This seems to be all of the shorthand style properties that IE8 supports, excluding a few nonstandard ones.
@zpao
Copy link
Member

zpao commented Aug 20, 2015

Does this now hold the record for "oldest PR merged"?

@sophiebits
Copy link
Collaborator

I think so.

@sophiebits
Copy link
Collaborator

I've got my eye on #1510 though…

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

Successfully merging this pull request may close these issues.

5 participants