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.

(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

(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.

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

Initial version of toolbox tabs with most XUl removed

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.

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).

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?

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

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).

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

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.

Simple style move.

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?

Setting needinfo as remainder to post patch.

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

(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.

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

(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

(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.

(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.

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.

(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.

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.

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.

(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.

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.

(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.

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.

Comment on attachment 8812337 [details] Bug 1245921 - Turn toolbox toolbar into a React component r+miker https://reviewboard.mozilla.org/r/92782/#review97626

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?

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

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.

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.

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

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.

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.

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.

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

(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.

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

(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?

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.

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.

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.

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?

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.

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.

Comment on attachment 8820421 [details] Bug 1245921 - Monkey patch ReactDOM event system for XUL; r+miker https://reviewboard.mozilla.org/r/92780/#review101982

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.

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!

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

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

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.

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!

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

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.

Marking as verified based on above comments.