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

Remove allow transparency #10823

Merged
merged 5 commits into from Sep 28, 2017
Merged

Conversation

nhunzaker
Copy link
Contributor

allowtransparency is an Internet Explorer-only attribute that controls the background transparency of an iFrame. When set to true, it respects the background color of the iFrame. When set to false, it sets the background color to that of the document.

This feature was removed in IE9. Browsers beyond this point just let you set the background of the document body to transparent, making the attribute unnecessary.

Should some ambitious/poor developer manage to get IE7/8 to work with React 16.x, they can still use allowtransparency="true", since React now supports unrecognized attributes.

How I verified this change

Check out the MSDN Example on allowTransparency

IE8 looks like this:

30827527-6a6687c8-a208-11e7-9c3f-e3f13f1699af

Every other browser:

30827538-6f3d6406-a208-11e7-973b-6883ac692272

Copy link
Contributor

@jquense jquense left a comment

Choose a reason for hiding this comment

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

nice 👍

Copy link
Contributor

@aweary aweary left a comment

Choose a reason for hiding this comment

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

🙌

| `as=(string 'true')`| (changed)| `"true"` |
| `as=(string 'false')`| (changed)| `"false"` |
| `as=(string 'on')`| (changed)| `"on"` |
| `as=(string 'off')`| (changed)| `"off"` |
Copy link
Contributor

Choose a reason for hiding this comment

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

Why did these change? Was it just outdated or something?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Huh. Good question. I don't know. I made this file by exporting it from the browser. I'll see what's up.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe you're not using latest Chrome? I recently updated those (see blame).

@nhunzaker
Copy link
Contributor Author

Trying to re-record, but I'm hitting this locally:

screen shot 2017-09-27 at 11 47 13 am

I'm not really sure where unpkg is even referenced. Any ideas? Otherwise I could just revert that section by hand.

@gaearon
Copy link
Collaborator

gaearon commented Sep 27, 2017

We need to update unpkg URLs in the fixture to use the new URLs.

/dist/react.js => /umd/react.development.js

etc

@nhunzaker
Copy link
Contributor Author

@gaearon Thanks. Now I'm questioning my grep technique 😨. I've updated the URLS and the snapshot.

@gaearon
Copy link
Collaborator

gaearon commented Sep 27, 2017

Why is CI failing? 😥

@nhunzaker
Copy link
Contributor Author

nhunzaker commented Sep 27, 2017

I rebased and force pushed my branch. Maybe it threw off CI.

`allowtransparency` is an Internet Explorer-only attribute that
controls the background transparency of an iFrame. When set to true,
it respects the background color of the iFrame. When set to false, it
sets the background color to that of the document.

This feature was removed in IE9 - falling out of React's support
commitment.

Developers that have somehow figured out how to get IE8 to work with
React 16.x can still use `allowtransparency="true"`, since React now
supports unrecognized attributes.
@nhunzaker
Copy link
Contributor Author

Neat. An empty commit did the trick! CI passes!

@ljharb
Copy link
Contributor

ljharb commented Nov 16, 2017

Was this change included in the v16.1 release notes?

@gaearon
Copy link
Collaborator

gaearon commented Nov 16, 2017

Probably forgot it, no. Happy to take changelog PR.

@nhunzaker
Copy link
Contributor Author

I'll take the blame :). @ljharb So I better understand the ramifications of this change, our understanding was that this was =< IE8. Did this cause any functional changes on your end?

@ljharb
Copy link
Contributor

ljharb commented Nov 16, 2017

@nhunzaker it caused jsx-eslint/eslint-plugin-react#1538 to be filed, is all.

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.

None yet

6 participants