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

DOMLazyTree, populate <object> before insertion into DOM #6691

Merged
merged 1 commit into from
May 5, 2016
Merged

DOMLazyTree, populate <object> before insertion into DOM #6691

merged 1 commit into from
May 5, 2016

Conversation

syranide
Copy link
Contributor

@syranide syranide commented May 3, 2016

Fix for #6629 and replaces the broken PR #6640.

Again, real-life performance implications unknown. As far as I can tell and understand it, it works as intended and considering the simplicity I fail to see how this could break anything.

@sophiebits
Copy link
Collaborator

Any idea whether video/audio source/track are affected too?

@syranide
Copy link
Contributor Author

syranide commented May 4, 2016

@spicyj I can't see why they would be, this feels like idiosyncratic behavior for <object> which kind of makes sense as plugins aren't really DOM-aware but still needs to be communicable via script. Anyway, https://jsfiddle.net/xckpxcpz/, video is not a problem in IE (and someone would surely have reported that too).

@jimfb
Copy link
Contributor

jimfb commented May 4, 2016

This looks fine to me, so unless anyone has any objections, I'm going to tentatively accept.

@syranide Do you have a demo/example that demonstrates the problem, so we can verify the fix and also see when we regress this?

@jimfb jimfb self-assigned this May 4, 2016
@syranide
Copy link
Contributor Author

syranide commented May 4, 2016

@jimfb http://dev.cetrez.com/jsx/swffail.html (jsfiddle not possible due to sandboxing)

On IE (lazy populate) bgcolor will not be seen during creation and thus show up as a plain white box rather than blue.

@jimfb
Copy link
Contributor

jimfb commented May 4, 2016

👍 Great, thanks @syranide!

Reposting the relevant code here, just so it doesn't get lost if the file on dev.cetrez.com goes away.

<script>
var Hello = React.createClass({
  render: function() {
    return (
      React.createElement('object', {type: 'application/x-shockwave-flash', data: 'awd', width: 200, height: 200}, [
        React.createElement('param', {name: 'movie', value: 'awd'}),
        React.createElement('param', {name: 'bgcolor', value: '#0000ff'})
      ])
    );
  }
});

ReactDOM.render(
  React.createElement(Hello),
  document.getElementById('container')
);

document.body.style.background = '#cc0000';
</script>

// <param> nodes immediately upon insertion into the DOM, so <object>
// must also be populated prior to insertion into the DOM.
if (tree.node.nodeType === 11 ||
tree.node.nodeName && tree.node.nodeName.toLowerCase() === 'object') {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we do tree.node.nodeType === 1 && tree.node.nodeName === 'OBJECT' && (tree.node.namespaceURI == null || tree.node.namespaceURI === DOMNamespaces.html)? Otherwise I suppose I'm good with this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure thing, that seems like a better test (and no toLowerCase() 👍). I'm just curious though, elsewhere we use the test I implemented above, is that just a transitional thing and to be replaced?

Copy link
Contributor Author

@syranide syranide May 5, 2016

Choose a reason for hiding this comment

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

Oh right, I don't think this will work for true XHTML-documents? nodeName is reported with the case as-is there IIRC? (hence toLowerCase())

@sophiebits
Copy link
Collaborator

Thanks for sending, just one note inline and then feel free to merge.

@sophiebits sophiebits added this to the 15.0.x milestone May 4, 2016
@syranide
Copy link
Contributor Author

syranide commented May 5, 2016

Updated the PR but #6691 (comment) needs an answer. PS. I also took the liberty of adding constants for the nodeTypes like sporadically done elsewhere.

@sophiebits
Copy link
Collaborator

You're right, let's keep the toLowerCase. Forgot about the XHTML case.

@sophiebits sophiebits merged commit 2af4765 into facebook:master May 5, 2016
@sophiebits
Copy link
Collaborator

Thanks!

@syranide syranide deleted the ielesslazy2 branch May 6, 2016 08:10
zpao pushed a commit that referenced this pull request May 10, 2016
@zpao zpao modified the milestones: 15.0.x, 15.0.3 May 10, 2016
@zpao zpao modified the milestones: 15.0.3, 15.y.z May 20, 2016
@zpao zpao modified the milestones: 15.1.0, 15.y.z May 20, 2016
@syranide
Copy link
Contributor Author

Ok, so apparently this still doesn't work in IE9 and ooooh weeee, it's seemingly impossible to make it work in IE9! The only way is to create the object and params together using innerHTML (swfobject has an explicit code path for that too). But AFAIK that's not really realistic and IE9 is on the shortlist so I doubt it's worth the headache.

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.

4 participants