Skip to content

Wrap setInnerHTML in Windows 8 apps (Fixes #441) #2799

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

Merged
merged 2 commits into from
Jan 8, 2015
Merged

Wrap setInnerHTML in Windows 8 apps (Fixes #441) #2799

merged 2 commits into from
Jan 8, 2015

Conversation

stkb
Copy link
Contributor

@stkb stkb commented Jan 2, 2015

See #441

If React is running in a Windows 8 metro app, creates a version of setInnerHTML that uses execUnsafeLocalFunction to bypass the restrictive html-insertion security policy of that javascript runtime.

@@ -28,6 +28,15 @@ var setInnerHTML = function(node, html) {
node.innerHTML = html;
};

// Win8 apps: Allow all html to be inserted
if (typeof MSApp !== 'undefined' && MSApp.execUnsafeLocalFunction) {
setInnerHTML = function (node, html) {
Copy link
Member

Choose a reason for hiding this comment

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

nit: no space after function keyword

Copy link
Member

Choose a reason for hiding this comment

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

Also, 2 space indent

@zpao
Copy link
Member

zpao commented Jan 5, 2015

Seems fine to me apart from the style nits.

@stkb
Copy link
Contributor Author

stkb commented Jan 5, 2015

Thanks, code style now fixed :)

zpao added a commit that referenced this pull request Jan 8, 2015
Wrap setInnerHTML in Windows 8 apps (Fixes #441)
@zpao zpao merged commit 5394acd into facebook:master Jan 8, 2015
@zpao
Copy link
Member

zpao commented Jan 8, 2015

Thanks!

@syranide
Copy link
Contributor

syranide commented Jan 8, 2015

I don't have access to W8, but could it make sense to wrap the entire transaction in execUnsafeLocalFunction rather than each invocation of setInnerHTML? I.e. is there are measurable performance overhead? I sincerely doubt it though, but you never know... especially when it's MS.

@syranide
Copy link
Contributor

syranide commented Jan 8, 2015

There's also WinJS.Utilities.setInnerHTMLUnsafe which seems like a better method to use for each invocation... unless we intend to wrap the entire transaction in MSApp.execUnsafeLocalFunction?

@stkb
Copy link
Contributor Author

stkb commented Jan 8, 2015

Regarding using WinJS.Utilities.setInnerHTMLUnsafe, I first considered that but didn't want to use a WinJS.* method, since using the WinJS library is not a requirement for writing a javascript Windows 8 app (it's perfectly possible to do it without, and if you don't use it, you still have the Windows and MSApp global objects but no WinJS). After digging deeper I found that all that function does is exactly what the new code in React.js is doing:

setInnerHTMLUnsafe = function (element, text) { 
83             msApp.execUnsafeLocalFunction(function () { 
84                 element.innerHTML = text; 
85             }); 
86         }; 

As far as the placement of the wrapping goes, I'm not familiar with the React.js codebase so just chose the safest place for it. I noticed that the setInnerHTML function is used in a couple places in the codebase (as well as some where it isn't yet and I think it should be) so I didn't want to potentially litter the code with lots of checks for MSApp. But if you can find a better place for it then fair enough. I haven't done any performance measurements but haven't noticed any sluggishness yet either.

@syranide
Copy link
Contributor

syranide commented Jan 8, 2015

@stkb Aha, good detective work on setInnerHTMLUnsafe! As for wrapping transactions higher up, I doubt it's worth it but you never know, I don't ever think you'd be able to "feel it", but it could show up in benchmarks... even though I doubt that too. It's something to consider for the future at least. :)

On another note, I think createNodesFromMarkup needs to be wrapped as well (but it shouldn't reuse setInnerHTML) for this to really work? (related #2340) I.e. if you perform an "unsafe update" it will still throw I think...

@stkb
Copy link
Contributor Author

stkb commented Jan 8, 2015

@syranide Agreed it's something in to keep in mind for the future or if anyone has performance problems in Windows apps.

I will take a look at createNodesFromMarkup...

Also forgot to say @zpao thanks for the merge :)

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.

3 participants