Closed Bug 1238881 Opened 8 years ago Closed 8 years ago

Force React to always generate HTML elements

Categories

(DevTools :: Framework, defect)

Product:
DevTools
Component:
Framework
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(firefox46 fixed)

Status:
RESOLVED FIXED
Milestone:
Firefox 46
Tracking Flags:
Tracking Status
firefox46 --- fixed

People

(Reporter: Honza, Assigned: jlong)

References

Details

Attachments

(3 files)

bug1238881-1.patch
1.24 KB, patch
jryans
: review-
Details | Diff | Splinter Review
xhr-broken.png
40.38 KB, image/png
Details
xhr-ok.png
58.04 KB, image/png
Details 
React doesn't properly support namespaces and it always uses the default namespace for the whole document.

We are facing this issue when we want to use React in existing XUL documents - to inject HTML markup generated by React. An example is webconsole.xul document.

Our ultimate goal is to replace all XUL documents by (X)HTML documents, but this process might take some time and it should be done step by step. There are small differences between XHTML and XUL and we need to be extra careful to properly solve them.

Enabling React in XUL documents in the meantime will help us to make a progress on Reactifying the UI and go towards removing XUL.

Honza
Here is a little workaround that fixes react in webconsole.xul.

Of course, I would be happier to not have XUL at all, but this enables us to move forward with React.

Thoughts?
Honza
Flags: needinfo?(jryans)
Flags: needinfo?(jlong)
What feature is this blocking? Is it important enough to keep adding bandaids? I feel like this is the tipping point where we really need to stop and just fix our HTML. Can we invest a few weeks instead converting our XUL to XHTML so that we never have to think about this again? We don't need to *remove* XUL, we just need to switch the default namespace. All that should require is going through all the XUL elements and prepending `xul:` to them, and going through any existing `createElement` calls and changing them to `createElementNS`.

I'd much prefer that to this solution. There's got to be side effects to this, like existing console code that calls `createElement` expecting it to be a XUL thing.
Flags: needinfo?(jlong)
Ok I see the bug this blocks :) Is there a reason it's such high priority or can we please spend some time fixing the XUL first?
Summary: Enable React in XUL documents → Enable React in XUL documents in webconsole
(In reply to James Long (:jlongster) from comment #2)
> What feature is this blocking? Is it important enough to keep adding
> bandaids?
Blocks: Bug 1211525

I am porting Firebug gaps into devtools to make sure that Firebug users can happily switch to devtools as as soon as Firebug stops working (we don't want to lose them).

Btw. there are only two things blocking that feature:
* loader: bug 1237253
* React in XUL: this bug

> I feel like this is the tipping point where we really need to stop
> and just fix our HTML. Can we invest a few weeks instead converting our XUL
> to XHTML so that we never have to think about this again? We don't need to
> *remove* XUL, we just need to switch the default namespace. All that should
> require is going through all the XUL elements and prepending `xul:` to them,
> and going through any existing `createElement` calls and changing them to
> `createElementNS`.
This is what I tried in the first place, but it doesn't work for me. React
is still creating some elements with XUL namespace. I can attach my patch(es)
if anyone is willing to try it out.

> I'd much prefer that to this solution. There's got to be side effects to
> this, like existing console code that calls `createElement` expecting it to
> be a XUL thing.
I have been already checking the console code and it already uses
`createElementNS` to create all elements with XHTML NS.

You know that. I would be so happy if React works just fine without any workarounds and we could adapt the entire code base right now with no compromise. There are already people facing the same problem and waiting even for several weeks seem to me like why we should block them/us?

Of course if this patch has some risks I am not seeing it's different situation.
And yes it yet needs to go through testing, but I wanted to have at least one
solution here. Perhaps there are better?

Honza
(In reply to Jan Honza Odvarko [:Honza] from comment #4)
> 
> > I feel like this is the tipping point where we really need to stop
> > and just fix our HTML. Can we invest a few weeks instead converting our XUL
> > to XHTML so that we never have to think about this again? We don't need to
> > *remove* XUL, we just need to switch the default namespace. All that should
> > require is going through all the XUL elements and prepending `xul:` to them,
> > and going through any existing `createElement` calls and changing them to
> > `createElementNS`.
> This is what I tried in the first place, but it doesn't work for me. React
> is still creating some elements with XUL namespace. I can attach my patch(es)
> if anyone is willing to try it out.

That doesn't make sense. If the document is an XHTML document, it should work. You changed the default namespace to XHTML, and it still created XUL elements? Can you show me more info about that? I'm genuinely curious to learn.

> 
> > I'd much prefer that to this solution. There's got to be side effects to
> > this, like existing console code that calls `createElement` expecting it to
> > be a XUL thing.
> I have been already checking the console code and it already uses
> `createElementNS` to create all elements with XHTML NS.
> 
> You know that. I would be so happy if React works just fine without any
> workarounds and we could adapt the entire code base right now with no
> compromise. There are already people facing the same problem and waiting
> even for several weeks seem to me like why we should block them/us?
> 
> Of course if this patch has some risks I am not seeing it's different
> situation.
> And yes it yet needs to go through testing, but I wanted to have at least one
> solution here. Perhaps there are better?

It's a very big hack, and monkey-patching a core function which we will use later and wonder why it isn't working the way a normal platform API should. These kinds of things amount to a codebase that is impossible to work on because even things like platform APIs don't mean what they should anymore.
Personally, I think this is the side effect from choosing React so early on. We weren't exactly ready to fully use React yet. We still need to fix our XUL namespacing and other things, and we need to do that now. I'd very much prefer to avoid more bandaids.
Let's see if this passes tests. If it does, then maybe we could put the band-aid in place temporarily. We'll just need a follow up issue to rip it out when we're done with the work James suggests in comment #2.

I can coordinate that work and make sure that it doesn't get put on the backburner.
Comment on attachment 8706818 [details] [diff] [review]
bug1238881-1.patch

Review of attachment 8706818 [details] [diff] [review]:
-----------------------------------------------------------------

I agree with :jlong, I think this is a bad idea.  Overwriting core platform methods with different impls is quite confusing and non-obvious.  You expect a platform method to function in a certain way, and then you tear your hair out for a long time when you forget about an override like this.  It reminds me of old versions of the Prototype library that would extend core objects like Array.

What is the timeline for blocked bug 1211525?  It seems to be about making the browser console handle requests like the web console?  It's hard to follow, there are many confusing comments.

So far, I think the best path changing the default namespace like :jlong mentions, but that could take some work.  So, how critical is it for bug 1211525 to be fixed immediately?
Attachment #8706818 - Flags: review-
I have a patch up which switches the webconsole to default to the XHTML namespace instead of the XUL namespace.

Bug 1239183

Honza, do you mind testing with that patch?
Flags: needinfo?(odvarko)
(In reply to Lin Clark from comment #9)
> I have a patch up which switches the webconsole to default to the XHTML
> namespace instead of the XUL namespace.
Great, thanks for helping with this Lin!
See more below.

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #8)
> What is the timeline for blocked bug 1211525?  It seems to be about making
> the browser console handle requests like the web console?  It's hard to
> follow, there are many confusing comments.
It's primarily for the Console panel, but it'll work for the Browser as well (let's say in a second round since supporting web-dev folks is more important atm).

The feature allows inline HTTP log inspection. You might want to take a look at this docs (screenshots included): https://docs.google.com/document/d/1zQniwU_dkt-VX1qY1Vp-SWxEVA4uFcDCrtH03tGoHHM

Yes, the feature is important Firebug gap, requested many times and it must be available when Firebug stops working, which happens when e10s is on. We definitely want to support millions of Firebug users and there is yet more we want to port from Firebug. The current target for shipping e10s is Fx46.

> That doesn't make sense. If the document is an XHTML document, it should
> work. You changed the default namespace to XHTML, and it still created XUL
> elements? Can you show me more info about that? I'm genuinely curious to
> learn.
Good, please read below.

> So far, I think the best path changing the default namespace like :jlong
> mentions, but that could take some work.  So, how critical is it for bug
> 1211525 to be fixed immediately?
I don't think it's possible to change the 'default' XUL namespace (i.e. default for createElement).

Lin's patch is a step in the right direction (i.e. define xul prefix for xul namespace and make sure that document.createElementNS is used everywhere, etc.), but it isn't enough for React. AFAIK there is no way to force document.createElement() to return nodes with different namespace and React is using that function. You might also want to see bug 799937.

I believe that the only reasonable way is changing webconsole.xul to webconsole.xhtml, but it may take a while to properly fix all the little layout and css discrepancies between XUL and XHTML. We have to do this, step by step, and we shouldn't block effort on new stuff till that happens. It doesn't make much sense to me to write the UI using home-grown lib or something now and rewrite it entirely using React later. But James, you are the Lord of the Reacts so, it's your call :-)

I still believe that having document.createElement() overridden is effective strategy how to proceed now. I am talking about temporary solution and I don't agree that it's so big hack and it'll be forgotten.
But I might be wrong, so perhaps there are better/simpler ways! 


@Lin: I tested your patch and it still doesn't work for me. If you want to test on your machine, you need patch from bug 1211525 (I just attached it there for you)
https://bug1211525.bmoattachments.org/attachment.cgi?id=8707412

Honza
Flags: needinfo?(odvarko)
@Honza Thanks for attaching the patch. When I try it, I get "ReferenceError: define is not defined". Do I need to include require.js somewhere?
(In reply to Lin Clark from comment #11)
> @Honza Thanks for attaching the patch. When I try it, I get "ReferenceError:
> define is not defined". Do I need to include require.js somewhere?

Ah sorry, you need yet another patch from bug 1237253
https://bug1237253.bmoattachments.org/attachment.cgi?id=8707408

Does it help?

Honza
:jlong, please check the additional info in comment 10.
Flags: needinfo?(jlong)
Honza, I now see what you mean. I was wrong before. The entire document does indeed need to be an XHTML page with the .xhtml extension, which evidently does not load any of the XUL styles. I thought before that `createElement` used the default namespace of the page, but that's only for static markup in the XUL/XHTML/etc file.

I'm going to see if I can fix this in BrowserLoader. I'd like to see if there's a way I can make `document.createElement` always make XHTML elements for modules loaded from the browser loader, but everything else has the normal `document.createElement`.

Now that I think about it... what exact issue are you hitting, Honza? I haven't hit any issues generating divs/etc in XUL environment (besides normal styling issues, etc). You should be able to use React but internally it's creating XUL-versions of the elements. I'm curious to learn: what differences are you hitting?
(In reply to James Long (:jlongster) from comment #14)
> I'm going to see if I can fix this in BrowserLoader. I'd like to see if
> there's a way I can make `document.createElement` always make XHTML elements
> for modules loaded from the browser loader, but everything else has the
> normal `document.createElement`.
Sounds good to me.

(originally, I thought we could use a sandbox with regular HTML window and load modules including React into it)

> Now that I think about it... what exact issue are you hitting, Honza? I
> haven't hit any issues generating divs/etc in XUL environment (besides
> normal styling issues, etc). You should be able to use React but internally
> it's creating XUL-versions of the elements. I'm curious to learn: what
> differences are you hitting?
It's mostly styling issues. React is creating nodes as usual just in different NS. It means that different styles and layouts are applied to them.

---

Here are my STR how to see this behavior:

1) Apply patch from bug 1237253, https://bug1237253.bmoattachments.org/attachment.cgi?id=8707408
2) Apply patch from bug 1211525, https://bug1211525.bmoattachments.org/attachment.cgi?id=8707412
3) Run Firefox, open DevTools Toolbox, select the Console panel.
4) Load this test page: http://www.janodvarko.cz/temp/xhr-spy/ (for XHR execution, you might use any other site that executes XHR)
5) Click e.g. the first button "Get small image over XHR"
6) See the log in the Console (make sure Net filter is on), expand it by clicking on the toggle button.
7) You should see a few tabs, 'Headers' being the default.
8) Now, content of the Headers tab has wrong layout (that's because e.g. <table>, <tr>, <td> are invalid in XUL NS).

You might apply patch from this bug to see the correct layout or check out screenshots in this doc: https://docs.google.com/document/d/1zQniwU_dkt-VX1qY1Vp-SWxEVA4uFcDCrtH03tGoHHM

---

Btw. this is weird, but sometimes (looks like only when using the same page, but from localhost) Browser Inspector shows that some elements in the Headers panel content have XHTML and some XUL namespace. Specifically, the 'netInfoGroupList' is XHTML and its child 'netInfoGroup' is XUL. I couldn't understand React internals enough to track this down (but it could perhaps be weird bug in the inspector).

Honza
Honza, just a thought: have you tried doing `#mount-point * { display: block }` to change the default display mode to normal HTML instead of XUL's old flexbox model inside your component's markup?

Still a hack though.

I'll try to fix the BrowserLoader tomorrow. I wouldn't want to use iframes to load React stuff, if that's what you mean, because iframes are a pain to interact with.

We could also patch React, which has been mentioned before. Honestly, that's probably the easiest now that I think about it. I played a little bit with defining a new `document` global in BrowserLoader and using a proxy, but that doesn't work very well.
Flags: needinfo?(jlong)
(In reply to James Long (:jlongster) from comment #16)
> Honza, just a thought: have you tried doing `#mount-point * { display: block
> }` to change the default display mode to normal HTML instead of XUL's old
> flexbox model inside your component's markup?
I tried to reset the styles using something as follows:

table { display: table; }
tbody { display: table-row-group; }
tr { display: table-row; }
td { display: table-cell; }
span { display: inline; }
div { display: block; }
img { display: inline; }

It helps, but there are still little things (e.g. <img> tag seems to be broken). I don't know what else could also be broken so, this could be risky.

> Still a hack though.
Yeah

> I'll try to fix the BrowserLoader tomorrow.
Great, thanks!

> I wouldn't want to use iframes
> to load React stuff, if that's what you mean, because iframes are a pain to
> interact with.
No no I didn't mean iframes. I totally agree that we should try hard to get rid of them (or at least not use it so intensively). I rather meant something like:

var sandbox = Components.utils.Sandbox(contentPrincipal);
var win = sandbox.contentWindow;
const require = BrowserLoader("resource://devtools/client/webconsole/xhr/", win).require;
XPCOMUtils.defineConstant(this, "require", require);

I haven't thought this through, but the original idea was to load React in a content-sandbox (so, it evaluates the `document` in HTML environment) and then use it from there (so, the parent node passed into ReactDOM.render would be a node from webconsole.xul. Not sure if it make sense. And I don't even know where to get the `contentPrincipal`.

> We could also patch React, which has been mentioned before. Honestly, that's
> probably the easiest now that I think about it. I played a little bit with
> defining a new `document` global in BrowserLoader and using a proxy, but
> that doesn't work very well.

Here is my version of the React patch:

Line 19825 (in our repo):

/**
 * Dummy container used to render all markup.
 */
var dummyNode = ExecutionEnvironment.canUseDOM ? document.createElement('div') : null;

I converted it to:

var dummyNode = ExecutionEnvironment.canUseDOM ? document.createElementNS('http://www.w3.org/1999/xhtml', 'div') : null;

This helped in my case, but I am not 100% sure if this should be the only change (there are other places where document.createElement() is used, so perhaps they also need to be fixed). This looks to me a bit more risky than having the monkey patch in webconsole.xul, but I am not expert on React internals.

Honza
The content principal is an interesting idea, but I don't think we should be messing with the user's window. If anything, that means that modules might leak into the user's page (outside of devtools), but I'm not sure of how those sandboxes would work. Anyway, we can't quite do that yet because we still have privileged code.

I messed around with this a lot today and learned something *very* strange: if you do `node.innerHTML = ....`, the namespace that the new elements are created in depends on how node was inserted into the page, as well as the namespace of `node`:

* If `node` was a static element inside a .xul file, the new elements will *always* have a XUL namespace, no matter what `node` is. If `node` is XHTML it doesn't matter. Adding elements to nodes originally in the .xul file always inherits the document.
* If `node` was dynamically inserted with `appendChild`, the new elements will inherit the namespace from `node`, which is what we want.

This isn't completely related to this issue, but be aware that you'll still run into problems if you are using a static mount point to mount React stuff, even if you've explicitly made it a XHTML node. You *always* need to dynamically add the mount point. (I'm working on docs so I'll write this kind of stuff down)

---

With that said, I think I know my preference for solving this. We want both `createElement` and `innerHTML` mechanisms inside React to always create XHTML elements.

We can't do anything about `innerHTML`; you just have to make sure you are mounting it into a dynamically generated XHTML element, and it works. Notably, React by default uses `innerHTML` almost everywhere. Honza, I'm wondering if you were running into this issue? What kind of element are you mounting to?

We can patch `createElement`. React doesn't use this for much; it always uses `innerHTML` to actually add nodes to the document. But it does use it to help generate DOM strings, and other various checks, so it seems wise to make sure it's always creating XHTML. We can patch it either in React, or force a tool to override it like Honza did.

I favor patching React, because it reduces any friction for tools to start using React, and even if it's just as hacky, has zero possibility for affecting existing code. We already have specific instructions for upgrading React, and it's not that hard to patch. I'd rather put this burden on the upgrade process, instead of introduction an environment where platform APIs aren't what you think they are.

I already have a fully patch React locally working, if you all think that is the right patch. Ultimately, of course, within the year we should have much less XUL and can remove this patch.
Ugh, I could not type those last sentences. I meant "I already have a fully patched React locally working, if you all think that is the right path"
(In reply to James Long (:jlongster) from comment #18)
> We can patch `createElement`. React doesn't use this for much; it always
> uses `innerHTML` to actually add nodes to the document. But it does use it
> to help generate DOM strings, and other various checks, so it seems wise to
> make sure it's always creating XHTML. We can patch it either in React, or
> force a tool to override it like Honza did.
> 
> I favor patching React, because it reduces any friction for tools to start
> using React, and even if it's just as hacky, has zero possibility for
> affecting existing code. We already have specific instructions for upgrading
> React, and it's not that hard to patch. I'd rather put this burden on the
> upgrade process, instead of introduction an environment where platform APIs
> aren't what you think they are.
> 
> I already have a fully patch React locally working, if you all think that is
> the right patch. Ultimately, of course, within the year we should have much
> less XUL and can remove this patch.

Patching React in this way seems like a reasonable solution.
(In reply to James Long (:jlongster) from comment #18)
> The content principal is an interesting idea, but I don't think we should be
> messing with the user's window. If anything, that means that modules might
> leak into the user's page (outside of devtools), but I'm not sure of how
> those sandboxes would work. Anyway, we can't quite do that yet because we
> still have privileged code.
Totally agree, I wasn't even thinking about we should use the user content window...

> I messed around with this a lot today and learned something *very* strange:
> if you do `node.innerHTML = ....`, the namespace that the new elements are
> created in depends on how node was inserted into the page, as well as the
> namespace of `node`:
> 
> * If `node` was a static element inside a .xul file, the new elements will
> *always* have a XUL namespace, no matter what `node` is. If `node` is XHTML
> it doesn't matter. Adding elements to nodes originally in the .xul file
> always inherits the document.
> * If `node` was dynamically inserted with `appendChild`, the new elements
> will inherit the namespace from `node`, which is what we want.
> 
> This isn't completely related to this issue, but be aware that you'll still
> run into problems if you are using a static mount point to mount React
> stuff, even if you've explicitly made it a XHTML node. You *always* need to
> dynamically add the mount point. (I'm working on docs so I'll write this
> kind of stuff down)
> 
> ---
> 
> With that said, I think I know my preference for solving this. We want both
> `createElement` and `innerHTML` mechanisms inside React to always create
> XHTML elements.
> 
> We can't do anything about `innerHTML`; you just have to make sure you are
> mounting it into a dynamically generated XHTML element, and it works.
> Notably, React by default uses `innerHTML` almost everywhere.
I see, my understanding of React was that - that's why React is using the dummyNode
(I've patched to be in XHTML NS) to create nodes from the markup (by using innerHTML
of the dummyNode).

Anyway, I am keen to see what you changed in React.

> Honza, I'm
> wondering if you were running into this issue? What kind of element are you
> mounting to?
All my React generated stuff is going under `span.message-body-wrapper` node that is
dynamically created by webconsole using `document.createElementNS(XHTML_NS, "span")`.
(the span is created for every HTTP log in the Console). So, it should be fine.


> We can patch `createElement`. React doesn't use this for much; it always
> uses `innerHTML` to actually add nodes to the document. But it does use it
> to help generate DOM strings, and other various checks, so it seems wise to
> make sure it's always creating XHTML. We can patch it either in React, or
> force a tool to override it like Honza did.
> 
> I favor patching React, because it reduces any friction for tools to start
> using React, and even if it's just as hacky, has zero possibility for
> affecting existing code. We already have specific instructions for upgrading
> React, and it's not that hard to patch. I'd rather put this burden on the
> upgrade process, instead of introduction an environment where platform APIs
> aren't what you think they are.
> 
> I already have a fully patch React locally working, if you all think that is
> the right patch. Ultimately, of course, within the year we should have much
> less XUL and can remove this patch.
It's also ok for me to patch React.

Honza
(In reply to Jan Honza Odvarko [:Honza] from comment #21)
> (In reply to James Long (:jlongster) from comment #18)
> > The content principal is an interesting idea, but I don't think we should be
> > messing with the user's window. If anything, that means that modules might
> > leak into the user's page (outside of devtools), but I'm not sure of how
> > those sandboxes would work. Anyway, we can't quite do that yet because we
> > still have privileged code.
> Totally agree, I wasn't even thinking about we should use the user content
> window...

I thought that's what the content principal does. I probably misunderstood what you were getting at though.

> 
> I see, my understanding of React was that - that's why React is using the
> dummyNode
> (I've patched to be in XHTML NS) to create nodes from the markup (by using
> innerHTML
> of the dummyNode).

Only some nodes are created with `dummyNode`. I'm not entirely sure, to be honest, but there's definitely another code path that just generates a string and dump it in without `dummyNode`. It's probably something like the first render of a whole component tree, and future renders generate nodes from `dummyNode` to dynamically insert them.

> 
> Anyway, I am keen to see what you changed in React.
> 
> > Honza, I'm
> > wondering if you were running into this issue? What kind of element are you
> > mounting to?
> All my React generated stuff is going under `span.message-body-wrapper` node
> that is
> dynamically created by webconsole using `document.createElementNS(XHTML_NS,
> "span")`.
> (the span is created for every HTTP log in the Console). So, it should be
> fine.

Yeah, I'm betting that changing `dummyNode` fixed it for you because the initial render dumps in XHTML nodes (because the root node is a dynamically inserted XHTML element) and then future updates create XHTML nodes from `dummyNode`. (didn't you say that changing `dummyNode` also fixed your issues? can't remember now)

>
> It's also ok for me to patch React.
> 

We already need to change a few lines of code to force it ti include TestUtils in a production build. I think at this point we should just fork it into the mozilla org to maintain our changes. We'll make it clear that our version is only intended to be used temporarily in a XUL environment. It'll make it easier to rebase on top of future versions.
(In reply to James Long (:jlongster) from comment #22)
> I thought that's what the content principal does. I probably misunderstood
> what you were getting at though.
I was hoping we can create our own principal...

> We already need to change a few lines of code to force it ti include
> TestUtils in a production build. I think at this point we should just fork
> it into the mozilla org to maintain our changes. We'll make it clear that
> our version is only intended to be used temporarily in a XUL environment.
> It'll make it easier to rebase on top of future versions.
Yep, I support the idea.

Honza
(In reply to Jan Honza Odvarko [:Honza] from comment #23)
> (In reply to James Long (:jlongster) from comment #22)
> > I thought that's what the content principal does. I probably misunderstood
> > what you were getting at though.
> I was hoping we can create our own principal...

You need to give a window instance though. It's either going to be our existing one (which is XUL, unless it somehow forces it to not be? what would happen then?), or the main page's window (or so I thought).

> 
> > We already need to change a few lines of code to force it ti include
> > TestUtils in a production build. I think at this point we should just fork
> > it into the mozilla org to maintain our changes. We'll make it clear that
> > our version is only intended to be used temporarily in a XUL environment.
> > It'll make it easier to rebase on top of future versions.
> Yep, I support the idea.
> 
> Honza

I filed an issue to see if React is interested in any of these changes: https://bugzilla.mozilla.org/show_bug.cgi?id=1238881

And here's the fork: https://github.com/mozilla/react. The changes we are interested in is: https://github.com/mozilla/react/commit/befab4ec31ce6b567cacb6a04d94ea7936cd7ec2

I'm already landing a new version of React in bug 1224765. I'm considering just merging these bugs and landing our forked version now.
Honza, just to make sure, can you try http://jlongster.com/s/react-patched.js in place of the current react.js and see if it fixes your issue?
(In reply to James Long (:jlongster) from comment #24)
> I filed an issue to see if React is interested in any of these changes:
> https://bugzilla.mozilla.org/show_bug.cgi?id=1238881

That's this bug, so I assume you mean the issue in React repo:

https://github.com/facebook/react/issues/5856
Oops, thanks!
Hm, actually I'm not going to add the patched version in bug 1224765. We want to uplift that patch to Aurora so that the memory tool is using the production version of React (memory tool is on Aurora right?). If we need to uplift I don't want to include a patched version yet.
(In reply to James Long (:jlongster) from comment #24)
> (In reply to Jan Honza Odvarko [:Honza] from comment #23)
> > (In reply to James Long (:jlongster) from comment #22)
> > > I thought that's what the content principal does. I probably misunderstood
> > > what you were getting at though.
> > I was hoping we can create our own principal...
> 
> You need to give a window instance though. It's either going to be our
> existing one (which is XUL, unless it somehow forces it to not be? what
> would happen then?), or the main page's window (or so I thought).
We would probably have to create an iframe (e.g. within webconsole.xul) and load modules into it
(just like in a sandbox) - the code would create nodes from within the right NS and inject them into webconsole.xul. The platform also has a 'hidden' (or background) window, but those things are rather problematic (learned on irc#developers). Again I am not suggesting we should do this, but it's good to know about other options :)


(In reply to James Long (:jlongster) from comment #25)
> Honza, just to make sure, can you try
> http://jlongster.com/s/react-patched.js in place of the current react.js and
> see if it fixes your issue?
Unfortunately, I am still seeing the same problem.

(btw. in my patch I changed NS of the dummyNode, which your patch doesn't)


Honza
Attached image xhr-broken.png — 
A screenshot showing what is broken in XHR inspector (see the one-line layout for headers, it should be a table)
Attached image xhr-ok.png — 
And this screenshot shows how it should look like.
> (btw. in my patch I changed NS of the dummyNode, which your patch doesn't)

You're right. That code exists in a 3rd party module, so changing all of React's `createElement` usage wasn't enough.

I may have to take a different approach and patch the generated react.js file. That's a little annoying because it'll be harder to rebase onto new versions. At that point it's not really worth having a fork; we'll just have to do this work every time we upgrade. However, I think we've hit a pretty stable version (I don't know of anything in 0.15 that we'll really want) so there shouldn't be a need to upgrade for a while. Hopefully in the next upgrade we can just drop this hack and assume we've switched to XHTML already.
Assignee: nobody → jlong
Honza, can you try this patch? http://jlongster.com/s/react-patched.js
(In reply to James Long (:jlongster) from comment #33)
> Honza, can you try this patch? http://jlongster.com/s/react-patched.js
Works for me now!

This is great James and it'll make our XUL -> XHTML transition path easier.
Thank you for the work :)

Honza
Great! np, thanks for exposing these awkward bugs and being an early testing for heavy usage of React :) These are good problems to solve.

Blocked on the React upgrade because that's gonna conflict, and that should land today once try is green.
Depends on: 1224765
Summary: Enable React in XUL documents in webconsole → Force React to always generate HTML elements
https://hg.mozilla.org/mozilla-central/rev/63eb0ddd1cdb
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox46: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 46
[bugday-20160323]

Status: RESOLVED,FIXED -> UNVERIFIED

Comments:
STR: Not clear.
Developer specific testing

Component: 
Name			Firefox
Version			46.0b9
Build ID		20160322075646
Update Channel          beta
User Agent		Mozilla/5.0 (Windows NT 6.1; WOW64; rv:46.0) Gecko/20100101 Firefox/46.0
OS			Windows 7 SP1 x86_64

Expected Results: 
Developer specific testing

Actual Results: 
As expected
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.