Helen V. Holmes (:helenvholmes) (:✨) Reporter
	Description • 9 years ago

      No description provided.

	Jen Fong-Adwent [:ednapiranha]
	Comment 1 • 9 years ago

Brian, do you see any initial issues that could happen from moving hbox, vbox, etc to divs and flexbox? If not, I'm going to try my hand on just moving all those parts in toolbox-options.xul into html.

	Jen Fong-Adwent [:ednapiranha]
	Comment 2 • 9 years ago

(In reply to Jen Fong-Adwent [:ednapiranha] from comment #1)
> Brian, do you see any initial issues that could happen from moving hbox,
> vbox, etc to divs and flexbox? If not, I'm going to try my hand on just
> moving all those parts in toolbox-options.xul into html.

Edit: not toolbox-options.xul but toolbox.xul

	Brian Grinstead [:bgrins]
	Comment 3 • 9 years ago

(In reply to Jen Fong-Adwent [:ednapiranha] from comment #2)
> (In reply to Jen Fong-Adwent [:ednapiranha] from comment #1)
> > Brian, do you see any initial issues that could happen from moving hbox,
> > vbox, etc to divs and flexbox? If not, I'm going to try my hand on just
> > moving all those parts in toolbox-options.xul into html.
> 
> Edit: not toolbox-options.xul but toolbox.xul

Feel free to give it a try, you might run into some sizing issues with things like the split console and the deck above it which use a flex of 1000 and 1 respectively.

Things like the notificationbox and deck elements will be harder since they have behaviors attached to them.

	Brian Grinstead [:bgrins]
	Comment 4 • 9 years ago

Renaming this to what I think the intent of this bug is after discussing.  Feel free to change if I'm misunderstanding.

	Jen Fong-Adwent [:ednapiranha]
	Comment 5 • 9 years ago

Initial version of toolbox tabs with most XUl removed

	Jen Fong-Adwent [:ednapiranha]
	Comment 6 • 9 years ago

First pass at removing some XUL elements. A few notes on this patch:

1. I removed the flex attributes for the label and radio and it didn't appear to do too much to the end result (see screenshot). Let me know if there is a scenario where the current changes would break that I might have missed

2. I could not get chrome:// assets to display in an <img> element and couldn't find much documentation on XUL <image> and what kind of quirks it has that img doesn't. So I left that as is but I'll need your help in figuring that one out.

3. Removed -moz-box-align: center in toolbars.css since it was already declared higher up.

	Brian Grinstead [:bgrins]
	Comment 7 • 9 years ago

Comment on attachment 8716074 [details] [diff] [review]
Bug1245921.patch

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

::: devtools/client/framework/toolbox.js
@@ +1102,5 @@
>  
>      radio.addEventListener("command", () => {
>        this.selectTool(id);
>      });
>  

Removing this functionality (and the cropping / flexing below) is preventing tabs from shrinking as the toolbox gets small.  They also seem to be smaller to start with by default.

@@ +1118,5 @@
>        radio.appendChild(image);
>      }
>  
>      if (toolDefinition.label && !toolDefinition.iconOnly) {
>        let label = this.doc.createElement("label");

Can you try converting all of the tab-related elements being built here to the HTML namespace?  Maybe something like radio -> div, spacer -> div, image -> img, label -> label.

If you didn't convert the namespace on the "img" tag that might explain why assets weren't loading.

@@ +1126,5 @@
>  
>      if (!toolDefinition.bgTheme) {
>        toolDefinition.bgTheme = "theme-toolbar";
>      }
> +    let toolboxPanel = this.doc.createElement("div");

Why put the panels in divs here?  Seems unrelated to this change and might cause issues (these aren't associated with the tabs but are rather the containers that tools are loaded in).  I'd tackle that in another issue / patch.

Regardless, this would need to be doing doc.createElementNS(HTML_NS, "div")

::: devtools/client/themes/toolbars.css
@@ +890,5 @@
>    -moz-margin-start: 4px;
>    opacity: 0.6;
>    max-height: 16px;
>    width: 16px; /* Prevents collapse during theme switching */
> +  margin-left: 10px;

This'd need to be margin-inline-start and margin-inline-end instead of left/right... but I don't think these changes are needed anyway (see comments in toolbox.js).

	Jen Fong-Adwent [:ednapiranha]
	Comment 8 • 9 years ago

Still WIP but wanted to get your thoughts on what you think might be going on with resizing a parent XUL element and having children that are XHTML / using flexbox.

In the exact same code but wrapped in a non-XUL #toolbox-tabs element, I get what I expect - the tabs to collapse even if the children have text that is longer than the collapsed width: http://codepen.io/anon/pen/xZQqKv

But in the current patch, the only difference (that I can see) is that #toolbox-tabs is a XUL element and it doesn't seem to allow me to go beyond breaking the width of the longest piece of text: https://dl.dropboxusercontent.com/u/1913694/resize.mov

The only way I've been able to get it to work in this situation is to make the label pos. abs and then the tab collapses as expected. Any ideas?

	Brian Grinstead [:bgrins]
	Comment 9 • 8 years ago

Sorry for the delay.  I've seen the patch and confirmed the issue in Comment 8 but haven't had a chance to try and work around it

	Brian Grinstead [:bgrins]
	Comment 10 • 8 years ago

Here's a WIP where I took the position: abs idea from comment 8 and expanded on it by wrapping the entire contents inside an absolutely positioned element.  It seems to work fine, although I'd like some additional feedback on the approach and to see if there's any cases where this is failing.

The absolute positioning is somewhat unfortunate since it makes it inflexible later on if we wanted to do something like give each tab only the width it needs (so 'console' would shrink down to be only as wide as the text, which 'style editor' would remain larger because of the longer label).  But at least it's matching current behavior.  I'm not sure if there's a way to get around that until we switch entirely to CSS flex for the tab bar (which may not be actually too terribly hard once this part is done).

	Brian Grinstead [:bgrins]
	Comment 11 • 8 years ago

Comment on attachment 8717192 [details] [diff] [review]
Bug1245921.patch

See Comment 10

	Tim Nguyen :ntim
	Comment 12 • 8 years ago

Comment on attachment 8723122 [details] [diff] [review]
tabs-as-html-WIP.patch

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

I don't like the idea of using position: abs, but if it's the only way we can switch the tabs to HTML (without switching everything), then it's fine.

I've noticed one issue with this patch though, the tab right borders seem to shift a bit when switching tabs.

	Tim Nguyen :ntim
	Comment 13 • 8 years ago

Simple style move.

	Brian Grinstead [:bgrins]
	Comment 14 • 8 years ago

Comment on attachment 8770988 [details] [diff] [review]
Part 1: Move toolbox styles from toolbars.css to toolbox.css

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

This looks fine but can you move this into a separate bug that blocks Bug 1285745 instead of doing it here?

::: devtools/client/themes/toolbox.css
@@ +2,5 @@
>  /* This Source Code Form is subject to the terms of the Mozilla Public
>   * License, v. 2.0. If a copy of the MPL was not distributed with this
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
> +.theme-light, .theme-dark {

Can you make this :root instead?  Just in case a new variable gets added that doesn't also get added to firebug at least there will be a default value.

@@ +295,3 @@
>  
>  #command-button-paintflashing::before {
> +  background-image: var(--command-paintflashing-image);

What changed on these lines?

	Tim Nguyen :ntim
	Comment 17 • 8 years ago

Setting needinfo as remainder to post patch.

	Marco Mucci [:MarcoM]
	Comment 20 • 8 years ago

Hi Tim, want to confirm if you are still working on this bug or can one of our team members complete it?

	Tim Nguyen :ntim
	Comment 21 • 8 years ago

(In reply to Marco Mucci [:MarcoM] from comment #20)
> Hi Tim, want to confirm if you are still working on this bug or can one of
> our team members complete it?

Hello Marco, I had discussed this with bgrins, and the plan was to do the whole toolbox in one go because of a XUL layout issue that affected the HTML tabs. Feel free to re-assign this bug.

	Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]
	Comment 22 • 8 years ago

I am going to create a react component for this. It will have the advantage that it can be used for both the toolbox and the sidebar.

One issue is that we should be following Marco Zehe's tab accessibility rules (titles need to be a tags for arrow key navigation, space should select a tab etc.)

The repository will live at https://github.com/MikeRatcliffe/firefox-devtools-toolbar

	Jan Honza Odvarko [:Honza] (always need-info? me)
	Comment 23 • 8 years ago

(In reply to Michael Ratcliffe [:miker] [:mratcliffe] from comment #22)
> I am going to create a react component for this. It will have the advantage
> that it can be used for both the toolbox and the sidebar.
Is the component about tabs?

If yes, note that there is already React component for tabs,
see: devtools/client/shared/components/tabs 
(it's being used for the sidebar as well as for HTTPi and JSON viewer)
I think we should share it.

Honza

	Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]
	Comment 24 • 8 years ago

(In reply to Jan Honza Odvarko [:Honza] from comment #23)
> (In reply to Michael Ratcliffe [:miker] [:mratcliffe] from comment #22)
> > I am going to create a react component for this. It will have the advantage
> > that it can be used for both the toolbox and the sidebar.
> Is the component about tabs?
> 
> If yes, note that there is already React component for tabs,
> see: devtools/client/shared/components/tabs 
> (it's being used for the sidebar as well as for HTTPi and JSON viewer)
> I think we should share it.
> 
> Honza

Yes, I know about that component and will simply build upon it.

There are a couple of issues regarding accessibility that I need to fix e.g. arrow keys and space to select tabs but of course I am building on that component.

	Brian Grinstead [:bgrins]
	Comment 25 • 8 years ago

(In reply to Michael Ratcliffe [:miker] [:mratcliffe] from comment #24)
> (In reply to Jan Honza Odvarko [:Honza] from comment #23)
> > (In reply to Michael Ratcliffe [:miker] [:mratcliffe] from comment #22)
> > > I am going to create a react component for this. It will have the advantage
> > > that it can be used for both the toolbox and the sidebar.
> > Is the component about tabs?
> > 
> > If yes, note that there is already React component for tabs,
> > see: devtools/client/shared/components/tabs 
> > (it's being used for the sidebar as well as for HTTPi and JSON viewer)
> > I think we should share it.
> > 
> > Honza
> 
> Yes, I know about that component and will simply build upon it.
> 
> There are a couple of issues regarding accessibility that I need to fix e.g.
> arrow keys and space to select tabs but of course I am building on that
> component.

Not sure if this is what you are already saying, but please make any accessibility fixes on the tabs component directly so we can take advantage of that in other places.

	gasolin@mozilla.com
	Comment 26 • 8 years ago

In sidebar tabs spec in Bug 1292054 , Helen said sidebar tab and toolbox tab will have different behaviour. (different motion, different additional menu..)

If we use react, maybe we could create a base sidebar component and extend it as SidebarTab and ToolboxTab? 

at least we should treat tab item in same format (in current code base toolbox tabbar use `radio`(WIP use `div`) and sidebar tab use `li`) so we can reuse most of code and reduce the maintainance effort.

	Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]
	Comment 27 • 8 years ago

(In reply to Fred Lin [:gasolin] from comment #26)
> In sidebar tabs spec in Bug 1292054 , Helen said sidebar tab and toolbox tab
> will have different behaviour. (different motion, different additional
> menu..)
> 

It really isn't a huge difference... more a choice of menu icon and options that would need to be configurable anyway... styles are covered by CSS.

> If we use react, maybe we could create a base sidebar component and extend
> it as SidebarTab and ToolboxTab? 
> 
> at least we should treat tab item in same format (in current code base
> toolbox tabbar use `radio`(WIP use `div`) and sidebar tab use `li`) so we
> can reuse most of code and reduce the maintainance effort.

Yes, they totally need to be in the same format.

	Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]
	Comment 28 • 8 years ago

In working on this I discovered that I was pretty much duplicating the contents of toolsidebar.js inside toolbox.js.

Because we use all tabs in the same way (click one and open an iframe) we should move toolsidebar.js into tabbar.js and make it more generic.

I have logged bug 1297651 to do this.

	Greg Tatum [:gregtatum] Assignee
	Comment 29 • 8 years ago

Hey Mike, I'm working on Helen's toolbox tab mockups as part of the bug fixing project. I see you have a few things broken out for them. I'm going to churn lots of code here if I'm working on it. Mind if I take over this bug? I know you're doing some stuff with the inspector tabs and you were looking to share some work. I think these tabs might end up being a bit more involved with some of the things that Helen and I have discussed.

	Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]
	Comment 30 • 8 years ago

(In reply to Greg Tatum [:gregtatum] [@gregtatum] from comment #29)
> Hey Mike, I'm working on Helen's toolbox tab mockups as part of the bug
> fixing project. I see you have a few things broken out for them. I'm going
> to churn lots of code here if I'm working on it. Mind if I take over this
> bug? I know you're doing some stuff with the inspector tabs and you were
> looking to share some work. I think these tabs might end up being a bit more
> involved with some of the things that Helen and I have discussed.

Sure, assigned to you.

	Greg Tatum [:gregtatum] Assignee
	Comment 31 • 8 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=3205604c1376

	Greg Tatum [:gregtatum] Assignee
	Comment 32 • 8 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=fbafd7656184

	Greg Tatum [:gregtatum] Assignee
	Comment 33 • 8 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=61328f51a405

	Greg Tatum [:gregtatum] Assignee
	Comment 36 • 8 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d3c27ad42ee3

	Greg Tatum [:gregtatum] Assignee
	Comment 37 • 8 years ago

Apparently mratcliffe is not valid for mozreview and you were never actually pinged on this bug. I'm need infoing you just to double be sure this went through. Ugh.

	Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]
	Comment 38 • 8 years ago

(In reply to Greg Tatum [:gregtatum] [@gregtatum] from comment #37)
> Apparently mratcliffe is not valid for mozreview and you were never actually
> pinged on this bug. I'm need infoing you just to double be sure this went
> through. Ugh.

You need to use "miker"... I can't add it myself because you somehow still have a draft open.

	Greg Tatum [:gregtatum] Assignee
	Comment 39 • 8 years ago

Thanks, I'm failing at reviewboard apparently. It should be published now and good to go.

Also there are a few more failing tests I'm cleaning up... there is an error around adding additional tools.

	Greg Tatum [:gregtatum] Assignee
	Comment 40 • 8 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=85e84cb14132

	Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]
	Comment 43 • 8 years ago
mozreview-review

Comment on attachment 8812337 [details]
Bug 1245921 - Turn toolbox toolbar into a React component r+miker

https://reviewboard.mozilla.org/r/92782/#review97626

	Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]
	Comment 44 • 8 years ago
mozreview-review

Comment on attachment 8812338 [details]
Bug 1245921 - Update tests for toolbox toolbar;

https://reviewboard.mozilla.org/r/94116/#review97628

::: devtools/client/framework/test/browser_toolbox_options_disable_buttons.js:64
(Diff revision 2)
>      "#enabled-toolbox-buttons-box input[type=checkbox]")];
>    let toolboxButtonNodes = [...doc.querySelectorAll(".command-button")];
> -  toolboxButtonNodes.push(doc.getElementById("command-button-frames"));
> -  let toggleableTools = toolbox.toolboxButtons;
>  
> +  // toolboxButtonNodes.push(doc.getElementById("command-button-frames"));

Did you mean to delete this?

	Greg Tatum [:gregtatum] Assignee
	Comment 45 • 8 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=cdf656f866f6

	Greg Tatum [:gregtatum] Assignee
	Comment 46 • 8 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9376e7942f13

	Greg Tatum [:gregtatum] Assignee
	Comment 47 • 8 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=7bc0e7036043

	Alexandre Poirot [:ochameau]
	Comment 48 • 8 years ago

You look very close Greg, just a couple of oranges left!
Do not hesitate to shout if you are stuck and need help.

	Greg Tatum [:gregtatum] Assignee
	Comment 49 • 8 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=74c62c208d4f

	Greg Tatum [:gregtatum] Assignee
	Comment 53 • 8 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=abfa2838f5a3

	Greg Tatum [:gregtatum] Assignee
	Comment 54 • 8 years ago

I could probably use some help at this point. I can't get any of the remaining tests to reproduce. I have tried it on a Mac, Windows 10, and Ubuntu. I even tried slowing down my Ubuntu virtual machine, and tried --run-until-failure with no success. These are consistently failing on try.

	Jarda Snajdr [:jsnajdr]
	Comment 55 • 8 years ago

It seems that React has serious trouble when the document has an iframe with a nested document, when there are React components mounted in both documents, and when the components install event listeners.

I looked at the two failing tests in Netmonitor - browser_net_reload-button.js and browser_net_reload-markers.js. Both of them look up the "Reload" button in Netmonitor UI, and call the click() method on it. Somehow, the click is incorrectly handled and leads to test failure.

What's happening?

When a React component has a onClick listener, the event listener is not added to each DOM element separately. Instead, one shared listener is added to the owning document, and the React event system dispatches the event to the right component, depending on the event's target.

If there are two documents, one parent and one child, and there is at least one component with onClick listener mounter in each of them, then there are two listeners - one on parent document, one on the child. This leads to each onClick event being dispatched two times.

	Greg Tatum [:gregtatum] Assignee
	Comment 56 • 8 years ago

Well this is scary. If I strip out the actual loading of react in the toolbar, then the netmonitor suddenly works. This does appear to be the issue.

https://gist.github.com/gregtatum/20c085f3dbcd10e858e8cbb2d64af590

	Jarda Snajdr [:jsnajdr]
	Comment 57 • 8 years ago

If I create a document with iframe and a click listener:

<div>Click me!</div>
<iframe src="frame-sub.html"></iframe>
<script>
  document.addEventListener("click", e => {
    console.log("top frame click:", document, e);
  });
</script>

And frame-sub.html also has a listener:

<div>Click me!</div>
<script>
  document.addEventListener("click", e => {
    console.log("sub frame click:", document, e);
  });
</script>

Then clicks in the iframe don't propagate to the parent document - only one listener fires each time, depending on where I click.

It's different in XUL, however, at least in devtools. Both listeners are fired there. So it might be XUL weirdness.

Note that React is aware of the fact that components can be mounted in different documents, and has explicit support for it - the event listeners are tracked per-document. So we're likely not really abusing React in any way - it's just XUL that delivers an unwelcome surprise.

	Alexandre Poirot [:ochameau]
	Comment 58 • 8 years ago

I'm not sure but note that this behavior difference may be related to be running in privileged mode. It might not be related to XUL.

	Jarda Snajdr [:jsnajdr]
	Comment 59 • 8 years ago

In privileged iframes, a mouse click event is dispatched both in the parent and child frame. This happens e.g. when clicking in devtools - about:devtools-toolbox and tool-specific iframes like netmonitor.xul.

For unprivileged content iframes, however, clicking inside the child iframe dispatches a click event only inside the child iframe. Not in the parent one.

Smaug, is it really true? Is the EventTargetChainItem list constructed differently in these two cases? Is there some flag to force the privileged iframe to use the same approach as content iframes?

React event system relies on the fact that the event is dispatched only in one of the both documents.

	Greg Tatum [:gregtatum] Assignee
	Comment 60 • 8 years ago

This also seems to be a XUL parent -> XUL iframe issue, as tools like the memory tool with memory.xhtml seem to be unaffected.

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 61 • 8 years ago

(In reply to Jarda Snajdr [:jsnajdr] from comment #59)
> In privileged iframes, a mouse click event is dispatched both in the parent
> and child frame. This happens e.g. when clicking in devtools -
> about:devtools-toolbox and tool-specific iframes like netmonitor.xul.
> 
> For unprivileged content iframes, however, clicking inside the child iframe
> dispatches a click event only inside the child iframe. Not in the parent one.
> 
> Smaug, is it really true?
Yes. That has been the behavior since the beginning of privileged DOM, IIRC.
In the web events of course must not leak from iframes to the container document, but browser chrome wants to get
all the events, so events propagate from iframes (and from top level pages) to the <xul:browser> or whatever happens to be the "container" element in privileged code.

> Is the EventTargetChainItem list constructed
> differently in these two cases?
Yes. http://searchfox.org/mozilla-central/rev/ac40ca3ec39efe85bfb111274c10ee4ceea5bb7a/dom/base/nsGlobalWindow.cpp#3281,3312
and http://searchfox.org/mozilla-central/rev/ac40ca3ec39efe85bfb111274c10ee4ceea5bb7a/dom/base/nsGlobalWindow.cpp#3232,3247,3252,3256
and this all relying on 
http://searchfox.org/mozilla-central/rev/ac40ca3ec39efe85bfb111274c10ee4ceea5bb7a/dom/base/nsFrameLoader.cpp#2335-2339,2349


> Is there some flag to force the privileged
> iframe to use the same approach as content iframes?
Not atm


> React event system relies on the fact that the event is dispatched only in
> one of the both documents.
Right. So nsFrameLoader could perhaps check if mOwnerContent has some attribute in
http://searchfox.org/mozilla-central/rev/ac40ca3ec39efe85bfb111274c10ee4ceea5bb7a/dom/base/nsFrameLoader.cpp#2335 and if so, 
fallback to the non-chrome code path.

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 62 • 8 years ago

(Random wondering. Has any performance measurements happened around this XUL->HTML conversion. We've seen regressions before when such conversions were done.)

	Greg Tatum [:gregtatum] Assignee
	Comment 63 • 8 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f09781b95af4

	Jarda Snajdr [:jsnajdr]
	Comment 64 • 8 years ago

(In reply to Olli Pettay [:smaug] from comment #62)
> (Random wondering. Has any performance measurements happened around this
> XUL->HTML conversion. We've seen regressions before when such conversions
> were done.)

After we migrated a big part of Netmonitor, there are some talos regressions, tracked in bug 1321749. But I attribute them to suboptimal React/Redux code rather than to XUL vs HTML. Is there a reason why the same thing implemented in HTML should be slower than XUL?

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 65 • 8 years ago

XUL has things like loading from the cache (which is binary format, no parsing needed), and prototype cache which helps when opening a new window since the basic DOM structure isn't re-loaded at all, but just cloned.

	Greg Tatum [:gregtatum] Assignee
	Comment 66 • 8 years ago

Well, changing the XUL behavior for event dispatching broke all things on my try run, so it looks like that might not be a viable approach. I didn't have any odd behavior when I ran through it on my machine, and did fix the bug (so that's a shame).

https://gist.github.com/gregtatum/352af65d739d8895dedc2a8d8ea6016d

Time to look for another approach.

	Greg Tatum [:gregtatum] Assignee
	Comment 67 • 8 years ago

smaug: The performance impacts between XUL and HTML have been brought up before, and we have some people on our team working on optimizing our tool loading. It's something we're aware of and mindful of, although we're moving full-steam ahead on incorporating only web technologies because of other benefits. I'm not aware of anyone tracking specific metrics for performance regressions (although maybe this exists and I'm not aware of it) But there are efforts to re-optimize as we bring on this new code.

	Jarda Snajdr [:jsnajdr]
	Comment 68 • 8 years ago

The <iframe useContentEventHandling=true> has one undesired behavior - the "load" event on this iframe never fires. Used for example here when opening the Console:

http://searchfox.org/mozilla-central/source/devtools/client/webconsole/panel.js#55

Olli, is there a way to fix this?

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 69 • 8 years ago

Ah, we need also a change to http://searchfox.org/mozilla-central/rev/51aa673e28802fe6ea89f4793721fc55334a6ac8/dom/base/nsGlobalWindow.cpp#3520-3525
There one could check that if docshell is typeChrome and element has useContentEventHandling=true, then dispatch load event.

	Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]
	Comment 73 • 8 years ago
mozreview-review

Comment on attachment 8820421 [details]
Bug 1245921 - Monkey patch ReactDOM event system for XUL; r+miker

https://reviewboard.mozilla.org/r/99906/#review101786

The patch itself looks great... thanks for the descriptive comments.

The try run is full of failures now so I will grant r+ when that is fixed.

	Greg Tatum [:gregtatum] Assignee
	Comment 86 • 8 years ago
mozreview-review

Comment on attachment 8820421 [details]
Bug 1245921 - Monkey patch ReactDOM event system for XUL; r+miker

https://reviewboard.mozilla.org/r/92780/#review101982

	Greg Tatum [:gregtatum] Assignee
	Comment 87 • 8 years ago

No idea how to re-flag you on reviewboard, but I got the tests passing on the React monkey patch. I had to move it inside of ReactDOM due to it being loader agnostic.

	Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]
	Comment 88 • 8 years ago
mozreview-review

Comment on attachment 8820421 [details]
Bug 1245921 - Monkey patch ReactDOM event system for XUL; r+miker

https://reviewboard.mozilla.org/r/99906/#review102020

You don't need to needinfo me... when you push a new / updated patch to review I will know.

On the same note, if you are not ready for review just turn off the review flag in the patch inside the bug.

Great job!

	Pulsebot
	Comment 91 • 8 years ago

Pushed by gtatum@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/e82113b4fe65
Monkey patch ReactDOM event system for XUL; r+miker r=miker
https://hg.mozilla.org/integration/autoland/rev/b105a99a50cf
Turn toolbox toolbar into a React component r+miker r=miker

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 92 • 8 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/e82113b4fe65
https://hg.mozilla.org/mozilla-central/rev/b105a99a50cf

	Jarda Snajdr [:jsnajdr]
	Comment 93 • 8 years ago

This Perfherder graph shows that landing this bug caused a significant performance regression in DAMP:

https://treeherder.mozilla.org/perf.html#/graphs?timerange=5184000&series=%5Bmozilla-central,edaec66500db21d37602c99daa61ac983f21a6ac,1,1%5D&series=%5Bmozilla-central,9663a154caf9b713fb951c8db24518c7e0ceddce,1,1%5D&zoom=1482970350426.9663,1483549144544.9438,250,412.6086691151495&selected=%5Bmozilla-central,edaec66500db21d37602c99daa61ac983f21a6ac,156199,65580021,1%5D

This is the pushlog that includes the patches from this bug:

https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=9104708cc3ac0ccfe4cf5d518e13736773c565d7&tochange=a06f92099a5d8edeb05e5971967fe8d6cd4c593c

For some reason, it escaped the attention of sheriffs and no alert/bug was created.

Greg, can you please have a look at what might be the cause? Is it the React component for toolbox tabs? Or the Proxy in the ReactDOM monkeypatch?

BTW I'd like to work on finishing the useContentEventHandling patch soon, as I think it's a much cleaner solution. If it improved performance, even better.

	Petruta Horea [:phorea], Desktop QA
	Comment 94 • 8 years ago

I performed exploratory testing on latest Nightly 53.0a1 under Win 10 64-bit, Mac OS X 10.11.5 and Ubuntu 16.04 64-bit.
I haven't found any new regressions, but I won't mark it as verified until the dependencies are fixed.

Are there any specific manual tests I should perform next? Thank you!

	Joel Maher ( :jmaher ) (UTC -8)
	Comment 95 • 8 years ago

this perf regression from comment 93 is now on aurora, not sure if we wanted to accept it or work on it.

	Greg Tatum [:gregtatum] Assignee
	Comment 96 • 7 years ago

I think we can accept it. This was de-XULing work that changed the underlying implementation. Alexandre Poirot has been doing follow-up work on fixing startup performance issues with our toolbox that will help mitigate these introduced perf regressions.

	Petruta Horea [:phorea], Desktop QA
	Comment 97 • 7 years ago

Marking as verified based on above comments.