1260552 - Come up with something to replace a xul:splitter

Status: VERIFIED FIXED

(DevTools :: Shared Components, enhancement, P1)

Description

[Brian Grinstead [:bgrins]]

We need some kind of element/component to allow resizing between two other elements.

Documentation for the available options of xul splitter: https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XUL/splitter

An example usage of it is to split between the split console and the selected tool: https://dxr.mozilla.org/mozilla-central/source/devtools/client/framework/toolbox.xul#146.

One requirement is that we can use this to split two elements that are already added in the DOM so we can use it on existing code without needing to re-write them to construct their DOM in JS: https://dxr.mozilla.org/mozilla-central/source/devtools/client/inspector/inspector.xul#182

Comment 1

[Brian Grinstead [:bgrins]]

Another requirement: to be able to be hidden (i.e. one of the sections is collapsed along with the splitter).  This happens with the split console when it's not visible.  Could be done with CSS or managed within a component.

Comment 2

[Brian Grinstead [:bgrins]]

Also it needs to have hooks available to (or built in support to) store last known size as a preference so we can restore the state after a toolbox reopen

Comment 3

[Bryan Clark (DevTools PM) [:clarkbw]]

Might be worth looking at what the debugger team is using: https://github.com/jlongster/debugger.html/blob/master/js/components/SplitBox.js

Comment 4

[James Long (:jlongster)]

I've talked with Brian and we were going to collaborate but I haven't had a chance to follow-up with him. That is a good start, needs a few tweaks but I think it's a good API. We also need to figure out how to make it easy to load in non-React content into the panels.

Brian, ping me when you want to move forward with this and I can help make something that we all can use.

Comment 5

[Alexandre Poirot [:ochameau]]

Hum. There is already an horizontal split box in tree:
http://mxr.mozilla.org/mozilla-central/source/devtools/client/shared/components/h-split-box.js

Comment 6

[James Long (:jlongster)]

(In reply to Alexandre Poirot [:ochameau] from comment #5)
> Hum. There is already an horizontal split box in tree:
> http://mxr.mozilla.org/mozilla-central/source/devtools/client/shared/
> components/h-split-box.js

It's very, very basic. I started using that in the new debugger but had to write my own. Nick wrote that really quickly and I'm sure he'd be fine to replace that with whatever we come up with. The way it works make it hard to have layouts that are split 3+ times.

Comment 7

[[e-mail]]

Update url for 

SplitBox
https://github.com/jlongster/debugger.html/blob/master/public/js/components/SplitBox.js

and h-split-box

https://github.com/mozilla/gecko-dev/blob/master/devtools/client/shared/components/h-split-box.js

Comment 8

[Jan Honza Odvarko [:Honza] (always need-info? me)]

(In reply to Fred Lin [:gasolin] from comment #7)
> Update url for 
> 
> SplitBox
> https://github.com/jlongster/debugger.html/blob/master/public/js/components/
> SplitBox.js
> 
> and h-split-box
> 
> https://github.com/mozilla/gecko-dev/blob/master/devtools/client/shared/
> components/h-split-box.js
Yep I am building the new splitter on top of SplitBox (which in turn is built on top of h-split-box.js).

Honza

Comment 9

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Attachment: bug1260552.patch

First version of the Splitter component (in shared/components dir).

* Demo page available: http://janodvarko.cz/devtools/splitter.html/
* Source for the demo: https://github.com/janodvarko/splitter.html

I am now working on integrating the component into the Inspector panel (replacing <xul:splitter>)

Honza

Comment 10

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Attachment: bug1260552-splitter.patch

Implementation of SplitBox based on the one coming from debugger.html

Honza

Comment 11

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Attachment: bug1260552-panel.patch

Integration of SplitBox into the Inspector panel.

- Removing xul:splitter from inspector.xul
- Removing all remaining xul elements from inspector.xul! (as soon as bug 1294186 is fixed we might try to rename the file to inspector.(x)html to see what happens.
- The browser_inspector_pane-toggle-04.js fails for me. Patrick, what this test is supposed to do, any tips why it fails?
- inspector.css is setting height of .devtools-main-conten to `calc(100% - 1px);`, which is not nice. I couldn't figure out what takes the 1 pixel...

Honza

Comment 12

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=9063dc068f85

Honza

Comment 13

[James Long (:jlongster)]

I'll check this on more on Monday, sorry, but at a quick glance this looks pretty good.

Comment 14

[Patrick Brosset <:pbro>]

(In reply to Jan Honza Odvarko [:Honza] from comment #10)
> Created attachment 8782432 [details] [diff] [review]
> bug1260552-splitter.patch
> 
> Implementation of SplitBox based on the one coming from debugger.html
> 
> Honza
How are we dealing with component sharing with debugger.html? The Draggable and SplitBox components were created as part of the debugger.html project, and live in that repo. Now we're copying them in the main devtools repo to use them in the inspector. Will debugger.html use them from here, or will still be integrated as one built artifact that has all its dependencies built-in?

Comment 15

[Patrick Brosset <:pbro>]

Comment on attachment 8782435 [details] [diff] [review]
bug1260552-panel.patch

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

Thanks Honza. This works generally nicely. I have made a few comments in the code. And I have noticed a few important things while testing that I think we should fix:

- the markup-view min-width is gone somehow. So it's possible to resize the sidebar so big that the whole markup tree is going to be entirely hidden, making it impossible (or really hard) to grab the splitter again and resize the sidebar,
- none of the sidebar panels work anymore. They are all empty, like when you select a comment node for example,
- I was able to make the splitter jump erratically while dragging. Not sure how to reproduce this easily, but I switched to various docking positions a few times, and then just dragged the splitter around. In horizontal layout (when the sidebar is to the right of the markup-view), then if I drag slowly to the right, the splitter starts to jump like crazy all over the place, and an offset is introduced between the mouse position and the splitter position.
See this gif: https://dl.dropboxusercontent.com/u/714210/inspector-splitter-jumps.gif
Although the frame rate isn't high enough to really show what the effect is like.

::: devtools/client/inspector/inspector-panel.js
@@ +107,5 @@
>  
>    this._target.on("will-navigate", this._onBeforeNavigate);
>    this._detectingActorFeatures = this._detectActorFeatures();
>  
> +  this.onResizeSplitterBox = this.onResizeSplitterBox.bind(this);

Please move this up to line 107, so it's part of the same block of code where all the other function bindings are done.

@@ +412,5 @@
>    get browserRequire() {
>      return this._toolbox.browserRequire;
>    },
>  
> +  get InspectorTabPanel() {

Why the initial capital letter? We usually just use camelCase.

@@ +425,5 @@
> +  /**
> +   * Build Splitter located between the main and side area of
> +   * the Inspector panel.
> +   */
> +  setupSplitter: function() {

eslint nit: missing space between function and (

@@ +432,5 @@
> +
> +    this.panelWin.addEventListener("resize", this.onResizeSplitterBox, true);
> +
> +    let splitter = SplitBox({
> +      className: 'inspector-sidebar-splitter',

Please use only double-quotes

@@ +435,5 @@
> +    let splitter = SplitBox({
> +      className: 'inspector-sidebar-splitter',
> +      initialWidth: 350,
> +      initialHeight: 350,
> +      minSize: 300,

I don't understand these numbers, initialWidth/Height and minSize.
Bug 1291972 recently landed and set adapted the min sidebar size to 350px, so why 300 here?
Also, can you use constants to describe these numbers instead? So they have names and comments.

Btw, this reminds me that when you worked on the sidebar refactoring to HTML, we decided to leave the allTabs menu to a follow-up bug, right? What's the plan for this? Before, users were able to resize the sidebar to something smaller (don't remember, maybe 200px?), because we had an overflow mechanism on the tabs and showed an allTabs drop-down menu to access the hidden ones. Now, we can't do that anymore. It'd be great to minimize as much as we can the negative effects devtools.html is having on the UX. This is a refactoring project that should be mostly invisible to users. Do you know when/if someone is working on this? Maybe the people in Taipei can help with this?

@@ +441,5 @@
> +      rightControl: true,
> +      left: this.InspectorTabPanel({
> +        id: "inspector-main-content"
> +      }),
> +      right: this.InspectorTabPanel({

I see you've used the names left and right, does that mean that the sidebar will always be to the right? Does SplitBox support RTL?
Currently, the inspector does support RTL. Not everything works perfectly, but a lot of effort has been done, and a lot of bugs have been filed, so I'd like this to support RTL right from the start too.
Maybe it does, but at least left should be renamed as start and right as end to be more direction-agnostic.

@@ +444,5 @@
> +      }),
> +      right: this.InspectorTabPanel({
> +        id: "inspector-sidebar-container"
> +      })
> +    })

eslint nit: missing semicolon.

@@ +454,5 @@
> +  /**
> +   * If Toolbox width is less than 600 px, the splitter changes its mode
> +   * to `horizontal` to support portrait view.
> +   */
> +  onResizeSplitterBox: function() {

eslint nit: missing space between function and (

@@ +456,5 @@
> +   * to `horizontal` to support portrait view.
> +   */
> +  onResizeSplitterBox: function() {
> +    let box = this.panelDoc.getElementById("inspector-splitter-box");
> +    this._splitter.setState({

Hm, I was surprised at first thinking that 'onResizeSplitterBox' actually meant that the splitter was moving. So I thought that you were doing this every time the splitter moved.
But this function actually is an event handler for window resize, which makes a lot more sense.
I think you should rename 'onResizeSplitterBox' to 'onWindowResize'. At least reading the function name just tells you when it is called, not what we expect to do inside it (which might change in the future, if we ever decide to do other things when the window gets resized).

@@ +457,5 @@
> +   */
> +  onResizeSplitterBox: function() {
> +    let box = this.panelDoc.getElementById("inspector-splitter-box");
> +    this._splitter.setState({
> +      vert: (box.clientWidth > 600)

Please move 600 as a constant with a name and comment so it's clearer.

@@ +479,5 @@
> +        // value is really useful at a time depending on the current
> +        // orientation (vertical/horizontal).
> +        // Having both is supported by the splitter component.
> +        width = 450;
> +        height = 450;

Also move 450 as a constant.

@@ +485,5 @@
> +
> +      this._splitter.setState({
> +        width: width,
> +        height: height
> +      });

Simpler with ES6:
this._splitter.setState({width, height});

@@ +493,5 @@
> +    let onSaveSize = () => {
> +      let state = this._splitter.state;
> +      Services.prefs.setIntPref("devtools.toolsidebar-width.inspector", state.width);
> +      Services.prefs.setIntPref("devtools.toolsidebar-height.inspector", state.height);
> +    }

eslint nit: missing semicolon.

@@ +497,5 @@
> +    }
> +
> +    this.sidebar.on("show", onLoadSize);
> +    this.sidebar.on("hide", onSaveSize);
> +    this.sidebar.on("destroy", onSaveSize);

We never do
this.sidebar.off("show", onLoadSize);
this.sidebar.off("hide", onSaveSize);
this.sidebar.off("destroy", onSaveSize);
this should be called in a tearDown somewhere, or in destroy.

@@ +503,5 @@
> +
> +  /**
> +   * Splitter clean up.
> +   */
> +  teardownSplitter: function() {

eslint nit: space between function and (

@@ +519,5 @@
>  
>      let defaultTab = Services.prefs.getCharPref("devtools.inspector.activeSidebar");
>  
>      if (!Services.prefs.getBoolPref("devtools.fontinspector.enabled") &&
> +       defaultTab == "sidebar-panel-fontinspector") {

Why do the tab IDs need to change (you added the "sidebar-panel-" prefix)?

It's not a huge deal, but we store these in a pref to remember which panel the user last selected. So with this change, the next time someone opens the inspector and had, say, the pref set to "animationinspector", then the rule-view will open instead.

Do we absolutely have to change these IDs?

@@ +1303,5 @@
>     * state and tooltip.
>     */
>    onPaneToggleButtonClicked: function (e) {
>      let sidePaneContainer = this.panelDoc.querySelector("#inspector-sidebar-container");
> +    sidePaneContainer = sidePaneContainer.closest(".controlled");

Why no directly:
let sidePaneContainer = this.panelDoc.querySelector(".controlled");
Can there be several controlled elements?

::: devtools/client/inspector/inspector.xul
@@ +29,1 @@
>          xmlns:html="http://www.w3.org/1999/xhtml">

There are no more XUL elements in this document. And we have both xmlns and xmlns:html pointing to the same URL. I think we can simplify this and remove xmlns:html and remove all html: prefixes throughout the document.

Also, did you try changing inspector.xul inspector.xhtml in this patch? I think this is intended to be done in a later bug, but just wondering, maybe it's a simple change to do here.

@@ +30,5 @@
>  
>    <script type="application/javascript;version=1.8"
>            src="chrome://devtools/content/shared/theme-switching.js"/>
> +
> +  <html:div flex="1" class="inspector-responsive-container theme-body inspector">

The flex="1" attribute is probably not needed anymore now that this isn't XUL anymore, and we use CSS flexbox.

@@ +33,5 @@
> +
> +  <html:div flex="1" class="inspector-responsive-container theme-body inspector">
> +
> +    <!-- Main Panel Content -->
> +    <html:div flex="1" id="inspector-main-content" class="devtools-main-content">

Same here.

@@ +56,2 @@
>        </html:div>
> +      <html:div flex="1" id="markup-box" />

Same here.

@@ +61,5 @@
>        </html:div>
> +    </html:div>
> +
> +    <!-- Splitter -->
> +    <html:div id="inspector-splitter-box">

I don't know how the new splitter works, but if I'm just looking at the markup, I'm wondering why this is called a splitter *box*, instead of just a splitter.
After all, there's nothing in this element, and it sits between inspector-main-content and inspector-sidebar-container, so reading this file, I feel like this *is* the splitter.
Box implies that something will go inside it. Maybe that's what happens later, in JS, though.

::: devtools/client/inspector/test/browser_inspector_pane-toggle-02.js
@@ +11,5 @@
>    info("Open the inspector in a side toolbox host");
>    let {toolbox, inspector} = yield openInspectorForURL("about:blank", "side");
>  
>    let panel = inspector.panelDoc.querySelector("#inspector-sidebar-container");
> +  panel = panel.closest(".controlled");

Searching for a parent with class controlled seems a bit backward here. Why we would even look for inspector-sidebar-container first?

I don't know what controlled means here, but if it's always the sidebar that's controlled by the splitter, then why not just:

let panel = inspector.panelDoc.querySelector(".controlled");

Same comment for the next 3 tests changed in this patch.

::: devtools/client/themes/inspector.css
@@ +23,5 @@
> +
> +/* The main panel layout. This area consists of a toolbar, markup view
> +  and breadcrumbs bar. */
> +.devtools-main-content {
> +  height: calc(100% - 1px);

I don't understand why this needs to be 100%-1px. But maybe we could use 100% and overflow:hidden ?

Comment 16

[Jan Honza Odvarko [:Honza] (always need-info? me)]

(In reply to Patrick Brosset <:pbro> from comment #14)
> (In reply to Jan Honza Odvarko [:Honza] from comment #10)
> > Created attachment 8782432 [details] [diff] [review]
> > bug1260552-splitter.patch
> > 
> > Implementation of SplitBox based on the one coming from debugger.html
> > 
> > Honza
> How are we dealing with component sharing with debugger.html? The Draggable
> and SplitBox components were created as part of the debugger.html project,
> and live in that repo. Now we're copying them in the main devtools repo to
> use them in the inspector. Will debugger.html use them from here, or will
> still be integrated as one built artifact that has all its dependencies
> built-in?
We've quickly chatted about this with James and I think that debugger.html should use the one from shared/components. James, is that correct?

Honza

Comment 17

[James Long (:jlongster)]

Yes, for now anything local in the debugger.html is temporary because there didn't exist anything else yet. We already have a process for copying over libraries from m-c that we need, so as soon as something comes available that should replace something, we can pull it over and work on using it instead.

Comment 18

[James Long (:jlongster)]

Long-term, I think when more things are using webpack we can share this stuff by simply publishing to npm.

Comment 19

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Attachment: bug1260552-panel.patch

(In reply to Patrick Brosset <:pbro> from comment #15)
> Comment on attachment 8782435 [details] [diff] [review]
> bug1260552-panel.patch
> 
> Review of attachment 8782435 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Thanks Honza. This works generally nicely. I have made a few comments in the
> code. And I have noticed a few important things while testing that I think
> we should fix:
> 
> - the markup-view min-width is gone somehow. So it's possible to resize the
> sidebar so big that the whole markup tree is going to be entirely hidden,
> making it impossible (or really hard) to grab the splitter again and resize
> the sidebar,
Fixed

> - none of the sidebar panels work anymore. They are all empty, like when you
> select a comment node for example,
Ah, last minute change, fixed.

> - I was able to make the splitter jump erratically while dragging. Not sure
> how to reproduce this easily, but I switched to various docking positions a
> few times, and then just dragged the splitter around. In horizontal layout
> (when the sidebar is to the right of the markup-view), then if I drag slowly
> to the right, the splitter starts to jump like crazy all over the place, and
> an offset is introduced between the mouse position and the splitter position.
> See this gif:
> https://dl.dropboxusercontent.com/u/714210/inspector-splitter-jumps.gif
> Although the frame rate isn't high enough to really show what the effect is
> like.
I couldn't repro this, but I think I fixed this. Please re-test.

> 
> ::: devtools/client/inspector/inspector-panel.js
> @@ +107,5 @@
> >  
> >    this._target.on("will-navigate", this._onBeforeNavigate);
> >    this._detectingActorFeatures = this._detectActorFeatures();
> >  
> > +  this.onResizeSplitterBox = this.onResizeSplitterBox.bind(this);
> 
> Please move this up to line 107, so it's part of the same block of code
> where all the other function bindings are done.
Done


> 
> @@ +412,5 @@
> >    get browserRequire() {
> >      return this._toolbox.browserRequire;
> >    },
> >  
> > +  get InspectorTabPanel() {
> 
> Why the initial capital letter? We usually just use camelCase.
It's name of the imported component, just like React or ReactDOM above
But, both works for me, just let me know if you prefer camelCase even in this case.

> 
> @@ +425,5 @@
> > +  /**
> > +   * Build Splitter located between the main and side area of
> > +   * the Inspector panel.
> > +   */
> > +  setupSplitter: function() {
> 
> eslint nit: missing space between function and (
Fixed (all eslint issues should be fixed)

> 
> @@ +435,5 @@
> > +    let splitter = SplitBox({
> > +      className: 'inspector-sidebar-splitter',
> > +      initialWidth: 350,
> > +      initialHeight: 350,
> > +      minSize: 300,
> 
> I don't understand these numbers, initialWidth/Height and minSize.
> Bug 1291972 recently landed and set adapted the min sidebar size to 350px,
> so why 300 here?
True, minSize changed to 350

> Also, can you use constants to describe these numbers instead? So they have
> names and comments.
Done

> 
> Btw, this reminds me that when you worked on the sidebar refactoring to
> HTML, we decided to leave the allTabs menu to a follow-up bug, right? What's
> the plan for this? Before, users were able to resize the sidebar to
> something smaller (don't remember, maybe 200px?), because we had an overflow
> mechanism on the tabs and showed an allTabs drop-down menu to access the
> hidden ones. Now, we can't do that anymore. It'd be great to minimize as
> much as we can the negative effects devtools.html is having on the UX. This
> is a refactoring project that should be mostly invisible to users. Do you
> know when/if someone is working on this? Maybe the people in Taipei can help
> with this?
Bug 1281789, patch waiting for your review.

> 
> @@ +441,5 @@
> > +      rightControl: true,
> > +      left: this.InspectorTabPanel({
> > +        id: "inspector-main-content"
> > +      }),
> > +      right: this.InspectorTabPanel({
> 
> I see you've used the names left and right, does that mean that the sidebar
> will always be to the right? Does SplitBox support RTL?
> Currently, the inspector does support RTL. Not everything works perfectly,
> but a lot of effort has been done, and a lot of bugs have been filed, so I'd
> like this to support RTL right from the start too.
> Maybe it does, but at least left should be renamed as start and right as end
> to be more direction-agnostic.
Done, the following prop names are used:
* startPanel
* endPanel
* endPanelControl

RTL is supported now.

> 
> @@ +456,5 @@
> > +   * to `horizontal` to support portrait view.
> > +   */
> > +  onResizeSplitterBox: function() {
> > +    let box = this.panelDoc.getElementById("inspector-splitter-box");
> > +    this._splitter.setState({
> 
> Hm, I was surprised at first thinking that 'onResizeSplitterBox' actually
> meant that the splitter was moving. So I thought that you were doing this
> every time the splitter moved.
> But this function actually is an event handler for window resize, which
> makes a lot more sense.
> I think you should rename 'onResizeSplitterBox' to 'onWindowResize'. At
> least reading the function name just tells you when it is called, not what
> we expect to do inside it (which might change in the future, if we ever
> decide to do other things when the window gets resized).
True, I renamed it to `onPanelWindowResize` if that's ok.

> 
> @@ +457,5 @@
> > +   */
> > +  onResizeSplitterBox: function() {
> > +    let box = this.panelDoc.getElementById("inspector-splitter-box");
> > +    this._splitter.setState({
> > +      vert: (box.clientWidth > 600)
> 
> Please move 600 as a constant with a name and comment so it's clearer.
Done

> 
> @@ +479,5 @@
> > +        // value is really useful at a time depending on the current
> > +        // orientation (vertical/horizontal).
> > +        // Having both is supported by the splitter component.
> > +        width = 450;
> > +        height = 450;
> 
> Also move 450 as a constant.
Done

> 
> @@ +485,5 @@
> > +
> > +      this._splitter.setState({
> > +        width: width,
> > +        height: height
> > +      });
> 
> Simpler with ES6:
> this._splitter.setState({width, height});
Done

> @@ +497,5 @@
> > +    }
> > +
> > +    this.sidebar.on("show", onLoadSize);
> > +    this.sidebar.on("hide", onSaveSize);
> > +    this.sidebar.on("destroy", onSaveSize);
> 
> We never do
> this.sidebar.off("show", onLoadSize);
> this.sidebar.off("hide", onSaveSize);
> this.sidebar.off("destroy", onSaveSize);
> this should be called in a tearDown somewhere, or in destroy.
Done

> @@ +519,5 @@
> >  
> >      let defaultTab = Services.prefs.getCharPref("devtools.inspector.activeSidebar");
> >  
> >      if (!Services.prefs.getBoolPref("devtools.fontinspector.enabled") &&
> > +       defaultTab == "sidebar-panel-fontinspector") {
> 
> Why do the tab IDs need to change (you added the "sidebar-panel-" prefix)?
Yeah, that was part of the last minute change that broke the side panels.
I removed this and introduced a new 'idPrefix' for the InspectorTabPanel
component. The component doesn't always need the prefix and so, now it's 
customizable through the prop.


> @@ +1303,5 @@
> >     * state and tooltip.
> >     */
> >    onPaneToggleButtonClicked: function (e) {
> >      let sidePaneContainer = this.panelDoc.querySelector("#inspector-sidebar-container");
> > +    sidePaneContainer = sidePaneContainer.closest(".controlled");
> 
> Why no directly:
> let sidePaneContainer = this.panelDoc.querySelector(".controlled");
> Can there be several controlled elements?
Yes, there could be in the future and I wanted to avoid a collision.
But, I simplified the code and used:

let sidePaneContainer = this.panelDoc.querySelector(
  "#inspector-splitter-box .controlled");

> 
> ::: devtools/client/inspector/inspector.xul
> @@ +29,1 @@
> >          xmlns:html="http://www.w3.org/1999/xhtml">
> 
> There are no more XUL elements in this document. And we have both xmlns and
> xmlns:html pointing to the same URL. I think we can simplify this and remove
> xmlns:html and remove all html: prefixes throughout the document.
There are blockers. E.g. the all-tabs-menu (Bug 1281789) is built on top of framework/menu module that is still using XUL internally and requires <xul:popupset> element in the doc. I created Bug 1297412 to fix this. Also, there is bug 1292592, the inspector is using SourceEditor (Fred Lin is looking at it)

So, I actually set the default xmlns to "xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul" again. Note that, till the file uses *.xul extension the default NS (used by document.createElement) is always XUL no matter what you say in the markup.


> @@ +30,5 @@
> >  
> >    <script type="application/javascript;version=1.8"
> >            src="chrome://devtools/content/shared/theme-switching.js"/>
> > +
> > +  <html:div flex="1" class="inspector-responsive-container theme-body inspector">
> 
> The flex="1" attribute is probably not needed anymore now that this isn't
> XUL anymore, and we use CSS flexbox.
Fixed (and also the other two occurrences)

> @@ +61,5 @@
> >        </html:div>
> > +    </html:div>
> > +
> > +    <!-- Splitter -->
> > +    <html:div id="inspector-splitter-box">
> 
> I don't know how the new splitter works, but if I'm just looking at the
> markup, I'm wondering why this is called a splitter *box*, instead of just a
> splitter.
> After all, there's nothing in this element, and it sits between
> inspector-main-content and inspector-sidebar-container, so reading this
> file, I feel like this *is* the splitter.
> Box implies that something will go inside it. Maybe that's what happens
> later, in JS, though.
Yes, totally agree, I don't like it either. The problem is that we don't have one root (application) React component that would render the rest of the UI using whole hierarchy of child components. At this point we are just rendering bits and pieces using React while the rest is plain HTML markup. When React comp is rendered it always needs a container (a parent node) to be rendered into - and this box is the parent node for <SplitBox /> React component.

> 
> ::: devtools/client/inspector/test/browser_inspector_pane-toggle-02.js
> @@ +11,5 @@
> >    info("Open the inspector in a side toolbox host");
> >    let {toolbox, inspector} = yield openInspectorForURL("about:blank", "side");
> >  
> >    let panel = inspector.panelDoc.querySelector("#inspector-sidebar-container");
> > +  panel = panel.closest(".controlled");
> 
> Searching for a parent with class controlled seems a bit backward here. Why
> we would even look for inspector-sidebar-container first?
> 
> I don't know what controlled means here, but if it's always the sidebar
> that's controlled by the splitter, then why not just:
> 
> let panel = inspector.panelDoc.querySelector(".controlled");
I would like to use safer CSS selector.
I used "#inspector-splitter-box .controlled", but no strong opinion,
I can remove "#inspector-splitter-box" if you think it's better

> ::: devtools/client/themes/inspector.css
> @@ +23,5 @@
> > +
> > +/* The main panel layout. This area consists of a toolbar, markup view
> > +  and breadcrumbs bar. */
> > +.devtools-main-content {
> > +  height: calc(100% - 1px);
> 
> I don't understand why this needs to be 100%-1px. But maybe we could use
> 100% and overflow:hidden ?
This is puzzling me I can't explain why it's needed. I tried 'overflow:hidden', but the entire panel content jumps 1 px up and back when the Inspector panel is opened. Using the calc() is better.
I created a comment explaining why this is there.

Thanks for the review!
Honza

Comment 20

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Attachment: bug1260552-splitter.patch

Some minor changes:
- rename left/right to startPanel/endPanel
- Support for RTL

Honza

Comment 21

[James Long (:jlongster)]

I've been at a conference, sorry :( I will find time tomorrow to look at this. Sorry for not being good at reviewing.

Comment 22

[Patrick Brosset <:pbro>]

Comment on attachment 8784856 [details] [diff] [review]
bug1260552-panel.patch

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

All my comments from the previous review were addressed so I don't have much problems with the code changes. However I think there's a performance problem that we must address before landing this.
I wasn't able to reproduce the crazy jumping problem I described before, so indeed you managed to fix this, but now I see the splitter move really slowly. In facts it janks really hard when I move it fast, instead of moving smoothly following the mouse as it moves. I'm not going to do a gif screen recording of this because the framerate on the gif isn't going to show the problem, I guess you can reproduce locally. I tested on a fairly powerful desktop PC with windows 10.

::: devtools/client/inspector/inspector-panel.js
@@ +450,5 @@
> +
> +    this._splitter = this.ReactDOM.render(splitter,
> +      this.panelDoc.getElementById("inspector-splitter-box"));
> +
> +    // Persis splitter state in preferences.

s/Persis/Persist

@@ +451,5 @@
> +    this._splitter = this.ReactDOM.render(splitter,
> +      this.panelDoc.getElementById("inspector-splitter-box"));
> +
> +    // Persis splitter state in preferences.
> +    this.sidebar.on("show", this._onInitSplitterState);

I think the callback would be better named onSidebarShown.
And the one for hide and destroy below, would be better named onSidebarHidden.

::: devtools/client/inspector/rules/rules.js
@@ +1600,5 @@
>    isSidebarActive: function () {
>      if (!this.view) {
>        return false;
>      }
> +

This change seems unrelated. Should be removed from this patch.

::: devtools/client/inspector/test/browser_inspector_pane-toggle-05.js
@@ +10,5 @@
>  
>  add_task(function* () {
>    let {inspector} = yield openInspectorForURL("about:blank", "side");
> +  let panel = inspector.panelDoc.querySelector("#inspector-splitter-box .controlled");
> +  panel = panel.closest(".controlled");

Is this a left over from before, or do you actually need to select a parent ".controlled" element of another ".controlled" element?

Comment 23

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Attachment: bug1260552-panel.patch

(In reply to Patrick Brosset <:pbro> from comment #22)
> Comment on attachment 8784856 [details] [diff] [review]
> bug1260552-panel.patch
> 
> Review of attachment 8784856 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> All my comments from the previous review were addressed so I don't have much
> problems with the code changes. However I think there's a performance
> problem that we must address before landing this.
> I wasn't able to reproduce the crazy jumping problem I described before, so
> indeed you managed to fix this, but now I see the splitter move really
> slowly. In facts it janks really hard when I move it fast, instead of moving
> smoothly following the mouse as it moves. I'm not going to do a gif screen
> recording of this because the framerate on the gif isn't going to show the
> problem, I guess you can reproduce locally. I tested on a fairly powerful
> desktop PC with windows 10.
This is weird, it feels ok on my machine. What exact page do you use?

All the other comments fixed.


One more thing. The test `browser_inspector_pane-toggle-04.js` fails for me. The dimensions of the hidden panel are different when switching into portrait mode, but it doesn't seem to be a problem. Remove?

Honza

Comment 24

[Julian Descottes [:jdescottes]]

Attachment: splitter-collapse-resize.gif

(In reply to Jan Honza Odvarko [:Honza] from comment #23)
> 
> One more thing. The test `browser_inspector_pane-toggle-04.js` fails for me.
> The dimensions of the hidden panel are different when switching into
> portrait mode, but it doesn't seem to be a problem. Remove?
> 

I wrote this test, so I had a quick look. The test might have to be updated but I can trigger a bug when following the scenario of the mochitest. 

Have a look at the attached GIF. If you collapse the panel and then resize the window to force the layout to switch to vertical, the collapsed panel becomes visible (which is the bug this test was supposed to cover, but again this might have to be updated if the togglePane implementation changed).

Comment 25

[Patrick Brosset <:pbro>]

(In reply to Jan Honza Odvarko [:Honza] from comment #23)
> (In reply to Patrick Brosset <:pbro> from comment #22)
> > However I think there's a performance
> > problem that we must address before landing this.
> > I wasn't able to reproduce the crazy jumping problem I described before, so
> > indeed you managed to fix this, but now I see the splitter move really
> > slowly. In facts it janks really hard when I move it fast, instead of moving
> > smoothly following the mouse as it moves.
> This is weird, it feels ok on my machine. What exact page do you use?
I reproduce this on basically any page. It does depend on the page currently being inspected to some extent. For instance if the amount of markup and rules displayed in the inspector is big, it janks even more. But from my testing the splitter is janky when I move it even when the inspector is almost empty, albeit far less.
Note that the current splitter is not perfect either, moving it isn't as smooth as it could be, but the new implementation makes it worse for me.
In fact after testing some more, both implementations seem equally bad when the markup-view contains a lot of nodes. But that's an edge case this bug doesn't really have to worry too much about. At the very least, we should get similar performance when moving the splitter on medium markup-view content size.

I use https://www.amazon.com/ as a test, keeping the body node selected, not expanding any other nodes, and having the rule-view selected in the sidebar. With DevEdition, I can move the splitter pretty smoothly, the framerate is almost high enough for me to feel like the splitter is exactly following my mouse. 
With fx-team and this patch applied however, the lag is clearly noticeable, as I move my mouse from right to left to make the rule-view bigger, the splitter halts and jumps to catch up with my mouse like 3 or 4 times during the length of the resizing (I move across about 1000px). So it's a really big lag that gives the impression that the inspector is really slow.

Note that I test on a desktop PC: Dell Precision T1700, Intel Core i7 3.6GHz, 32GB RAM. So hardware isn't an issue here.

Comment 26

[Patrick Brosset <:pbro>]

Comment on attachment 8786371 [details] [diff] [review]
bug1260552-panel.patch

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

Some rebasing needed for this patch apparently (it didn't apply cleanly on today's pull of fx-team).
Also, see my last comment about the performance problem I'm seeing.
And also see jdescottes' comment about one of the tests you changed.

With these cleared, I can R+ the patch.

::: devtools/client/inspector/inspector-panel.js
@@ +103,5 @@
>    this.onPaneToggleButtonClicked = this.onPaneToggleButtonClicked.bind(this);
>    this._onMarkupFrameLoad = this._onMarkupFrameLoad.bind(this);
> +  this._onPanelWindowResize = this._onPanelWindowResize.bind(this);
> +  this.onSidebarShown = this.onSidebarShown.bind(this);
> +  this.onSidebarHidden = this.onSidebarHidden.bind(this);

We're not consistent in this file whether we use an underscore prefix or not for event handlers and private methods.
Personally I don't think the leading underscore is important at all, but I think we should just take a decision one way or the other.
While you're working on this, do you think you could add another patch that changes all _on to on or the other way around?

Comment 27

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Attachment: bug1260552-panel.patch

(In reply to Julian Descottes [:jdescottes] from comment #24)
> I wrote this test, so I had a quick look. The test might have to be updated
> but I can trigger a bug when following the scenario of the mochitest. 
Thanks for the gif!

The new patch should fix the problem (and the test passes for me now).


(In reply to Patrick Brosset <:pbro> from comment #25)
> I use https://www.amazon.com/ as a test, keeping the body node selected, not
> expanding any other nodes, and having the rule-view selected in the sidebar.
> With DevEdition, I can move the splitter pretty smoothly, the framerate is
> almost high enough for me to feel like the splitter is exactly following my
> mouse. 
I am still looking at this issue. So far, not sure how to make it faster.


(In reply to Patrick Brosset <:pbro> from comment #26)
> ::: devtools/client/inspector/inspector-panel.js
> @@ +103,5 @@
> >    this.onPaneToggleButtonClicked = this.onPaneToggleButtonClicked.bind(this);
> >    this._onMarkupFrameLoad = this._onMarkupFrameLoad.bind(this);
> > +  this._onPanelWindowResize = this._onPanelWindowResize.bind(this);
> > +  this.onSidebarShown = this.onSidebarShown.bind(this);
> > +  this.onSidebarHidden = this.onSidebarHidden.bind(this);
> 
> We're not consistent in this file whether we use an underscore prefix or not
> for event handlers and private methods.
> Personally I don't think the leading underscore is important at all
Yeah, I have the same feeling.

I renamed this._onPanelWindowResize to this.onPanelWindowResize so, at least the patch is consistent.

> While you're working on this, do you think you could add another patch that
> changes all _on to on or the other way around?
Sure, I'll do it.

The patch is also rebased on today's HEAD

Honza

Comment 28

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Attachment: bug1260552-panel.patch

Sorry, this one is rebased.
Honza

Comment 29

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Attachment: bug1260552-panel.patch

Yet, one update (removing '_' from method name)

Honza

Comment 30

[Jan Honza Odvarko [:Honza] (always need-info? me)]

I've been playing with the splitter CSS to make the dragging smother, but not much luck so far.
Any tips how to proceed here?

Honza

Comment 31

[James Long (:jlongster)]

Comment on attachment 8784857 [details] [diff] [review]
bug1260552-splitter.patch

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

This looks awesome. So sorry I sucked at reviewing this. A few nits but nothing big. I'll r+ with comments. Thanks a lot for doing this!

::: devtools/client/shared/components/splitter/split-box.css
@@ +47,5 @@
> +  z-index: 1;
> +}
> +
> +.split-box.vert > .splitter {
> +  min-width: calc(var(--devtools-splitter-inline-start-width) +

Are these variables that we have already defined?

::: devtools/client/shared/components/splitter/split-box.js
@@ +19,5 @@
> +  propTypes: {
> +    // Custom class name. You can use more names separated by a space.
> +    className: PropTypes.string,
> +    // Initial size of controlled panel.
> +    initialSize: PropTypes.number,

I see `initialSize` here, but `this.props.initialWidth`, `this.props.initialHeight`, and `this.props.size` is used below. Which is it?

Seems like it should be `initialSize` the the local state should just have a `size` property.

@@ +40,5 @@
> +  getDefaultProps() {
> +    return {
> +      splitterSize: 5,
> +      vert: true,
> +      endPanelControl: false,

nit: trailing comma

@@ +54,5 @@
> +    let size = this.props.size;
> +    return {
> +      vert: this.props.vert,
> +      width: this.props.initialWidth || size,
> +      height: this.props.initialHeight || size,

nit: trailing comma

@@ +173,5 @@
> +
> +    return (
> +      dom.div({
> +        className: classNames.join(" "),
> +        style: style },

nit: it seems strange to line the properties up with children, it'd probably be good to move this whole object down so that the children are indented 2 more spaces. But that's just a style preference.

@@ +182,5 @@
> +        ) : null,
> +        Draggable({
> +          className: "splitter",
> +          style: splitterStyle,
> +          onStart: () => this.onStartMove(),

Since you are using `createClass`, you can just pass `this.onStartMove` directly, and same for `onStopMove`

Comment 32

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Thanks James!

Did you integrate it with Debugger.html?
How fast is it for you?
Is it the same/slower as before?

Honza

Comment 33

[James Long (:jlongster)]

Haven't had time yet. I'll try to do it soon. It's using similar techniques as before so I bet it's about the same.

Comment 34

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Attachment: bug1260552-splitter.patch

(In reply to James Long (:jlongster) from comment #31)
> Comment on attachment 8784857 [details] [diff] [review]
> bug1260552-splitter.patch
> 
> Review of attachment 8784857 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This looks awesome. So sorry I sucked at reviewing this. A few nits but
> nothing big. I'll r+ with comments. Thanks a lot for doing this!
> 
> ::: devtools/client/shared/components/splitter/split-box.css
> @@ +47,5 @@
> > +  z-index: 1;
> > +}
> > +
> > +.split-box.vert > .splitter {
> > +  min-width: calc(var(--devtools-splitter-inline-start-width) +
> 
> Are these variables that we have already defined?
Yes, these come from `client\themes\splitters.css`

> 
> ::: devtools/client/shared/components/splitter/split-box.js
> @@ +19,5 @@
> > +  propTypes: {
> > +    // Custom class name. You can use more names separated by a space.
> > +    className: PropTypes.string,
> > +    // Initial size of controlled panel.
> > +    initialSize: PropTypes.number,
> 
> I see `initialSize` here, but `this.props.initialWidth`,
> `this.props.initialHeight`, and `this.props.size` is used below. Which is it?
> Seems like it should be `initialSize` the the local state should just have a
> `size` property.
Fixed (this.props.size renamed to this.props.initialSize). This props is only
used to initialize initialWidth and initialHeight.

> 
> @@ +40,5 @@
> > +  getDefaultProps() {
> > +    return {
> > +      splitterSize: 5,
> > +      vert: true,
> > +      endPanelControl: false,
> 
> nit: trailing comma
Fixed

> 
> @@ +54,5 @@
> > +    let size = this.props.size;
> > +    return {
> > +      vert: this.props.vert,
> > +      width: this.props.initialWidth || size,
> > +      height: this.props.initialHeight || size,
> 
> nit: trailing comma
Fixed

> 
> @@ +173,5 @@
> > +
> > +    return (
> > +      dom.div({
> > +        className: classNames.join(" "),
> > +        style: style },
> 
> nit: it seems strange to line the properties up with children, it'd probably
> be good to move this whole object down so that the children are indented 2
> more spaces. But that's just a style preference.
Done

> 
> @@ +182,5 @@
> > +        ) : null,
> > +        Draggable({
> > +          className: "splitter",
> > +          style: splitterStyle,
> > +          onStart: () => this.onStartMove(),
> 
> Since you are using `createClass`, you can just pass `this.onStartMove`
> directly, and same for `onStopMove`
Done

Thanks!
Honza

Comment 35

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Attachment: bug1260552-tests.patch

There are some test failing on try.

* devtools/client/inspector/test/browser_inspector_search-filter_context-menu.js
This one is puzzling, some comments:
- It looks like context menu items are using different implementation of `goUpdateCommand` since XUL->HTML?
- The test is calling (once):
  let onContextMenuPopup = once(searchContextMenu, "popupshowing");
  and waiting (twice)?:
  yield onContextMenuPopup;
  ...
  yield onContextMenuPopup;
  Am I missing something?
- The paste menu item state depends on the current clipboard content, it's a bit dangerous
- The test expects cmd_selectAll to be enabled even if the input field is empty?
- The attached patch just disables checking of clipboard menu-items (for now)

  This test comes from bug bug 1294937 (regression after Inspector Search box XUL -> HTML)

@gasolin, gl?


* devtools/client/inspector/rules/test/browser_rules_colorUnit.js
(and other tests sending VK_RETURN key event)
- I added `win.focus()` before sending an event to `win`


* devtools/client/inspector/fonts/test/browser_fontinspector_theme-change
I had to use `timeout()`, my feeling is that it's because PREVIEW_UPDATE_DELAY, see fonts.js, but I might be wrong.
@Patrick?

There are couple more failing, I am looking into it.

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

Honza

Comment 36

[Patrick Brosset <:pbro>]

(In reply to Jan Honza Odvarko [:Honza] from comment #30)
> I've been playing with the splitter CSS to make the dragging smother, but
> not much luck so far.
> Any tips how to proceed here?
I found something that makes it a lot faster for me, and it has nothing to do with CSS.

I was first worried about the reflows that are caused on each mouse move event because of this code in split-box.js:

      let innerOffset = x - win.mozInnerScreenX;
      size = endPanelControl ?
        (node.offsetLeft + node.offsetWidth) - innerOffset :
        innerOffset - node.offsetLeft;

win.mozInnerScreenX, node.offsetLeft and node.offsetWidth are always the same once you start dragging, and accessing them everytime probably causes reflows that slow the animation down.
You can instead calculate them in onStartMove and then just used a stored value.

But that doesn't make a lot of difference for me.

What does, however, is making the panels next to the splitter inert. For instance, when you make the sidebar bigger, you drag the splitter towards the left. And by doing so, your mouse goes faster than the splitter can follow it, and therefore ends up generating mouseover events on the markupview. Now, doing this shows the highlighter on whatever node you happen to be hovering over.
This has 2 effects:
- it slows down quite a lot the animation in my case
- it distracts users when all the want is drag the panel, not highlight nodes.

I've made a quick test, in onStartMove:

    [...doc.querySelectorAll(".controlled, .uncontrolled")].forEach(n => {
      n.style.pointerEvents = "none";
    });

and in onStopMove:

    [...doc.querySelectorAll(".controlled, .uncontrolled")].forEach(n => {
      n.style.pointerEvents = "auto";
    });

I think maybe instead we should just add/remove a class on the container that addds the pointer-events property instead, but this was enough for me to test and I got a much smoother dragging animation as a result.

Comment 37

[Patrick Brosset <:pbro>]

(In reply to Jan Honza Odvarko [:Honza] from comment #35)
> * devtools/client/inspector/fonts/test/browser_fontinspector_theme-change
> I had to use `timeout()`, my feeling is that it's because
> PREVIEW_UPDATE_DELAY, see fonts.js, but I might be wrong.
> @Patrick?
In what way exactly did this test fail? Can you include a try build url where it did fail (i.e. without your timeout)? I don't really understand how your patch could cause a failure here. And PREVIEW_UPDATE_DELAY is only used when the text field in the font-inspector has its value updated, which is not the case in this test.
This test waits for the fontinspector-updated event which should be enough.

Comment 38

[Patrick Brosset <:pbro>]

Comment on attachment 8786785 [details] [diff] [review]
bug1260552-panel.patch

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

The interdiff makes sense. The only last comment I had was the performance one, but I've added a potential solution above, and it's going to be in the other patch anyway. So I can R+ this one.

Comment 39

[Patrick Brosset <:pbro>]

Comment on attachment 8787166 [details] [diff] [review]
bug1260552-splitter.patch

See comment 36 about perf.

Comment 40

[Patrick Brosset <:pbro>]

(In reply to Patrick Brosset <:pbro> from comment #38)
> Comment on attachment 8786785 [details] [diff] [review]
> bug1260552-panel.patch
> 
> Review of attachment 8786785 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> The interdiff makes sense. The only last comment I had was the performance
> one, but I've added a potential solution above, and it's going to be in the
> other patch anyway. So I can R+ this one.
Obviously this R+ is also assuming you get a green try build.

Comment 41

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Attachment: bug1260552-perf.patch

(In reply to Patrick Brosset <:pbro> from comment #36)
> What does, however, is making the panels next to the splitter inert.
Nice catch!

New patch attached

Honza

Comment 42

[Jan Honza Odvarko [:Honza] (always need-info? me)]

(In reply to Patrick Brosset <:pbro> from comment #37)
> (In reply to Jan Honza Odvarko [:Honza] from comment #35)
> > * devtools/client/inspector/fonts/test/browser_fontinspector_theme-change
> > I had to use `timeout()`, my feeling is that it's because
> > PREVIEW_UPDATE_DELAY, see fonts.js, but I might be wrong.
> > @Patrick?
> In what way exactly did this test fail? Can you include a try build url
> where it did fail (i.e. without your timeout)? I don't really understand how

Here is the try build:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=41d30781efba&selectedJob=26758149

Honza

Comment 43

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Attachment: bug1260552-tests.patch

Here is an update, but the following tests still fail:

* devtools/client/inspector/rules/test/browser_rules_edit-selector_09.js
* devtools/client/inspector/rules/test/browser_rules_edit-property-order.js
They fail because of the same reason (looks like focus might be the issue).

* devtools/client/shared/components/test/mochitest/test_tabs_accessibility.html 
I am looking into this one.

Honza

Comment 44

[Jan Honza Odvarko [:Honza] (always need-info? me)]

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

Honza

Comment 45

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Attachment: bug1260552-tests.patch

Fix for the following test included:
devtools/client/shared/components/test/mochitest/test_tabs_accessibility.html 

Honza

Comment 46

[Patrick Brosset <:pbro>]

Comment on attachment 8787601 [details] [diff] [review]
bug1260552-tests.patch

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

Some investigation below about browser_fontinspector_theme-change.js

::: devtools/client/inspector/fonts/test/browser_fontinspector_theme-change.js
@@ +53,5 @@
>    info("Waiting for font-inspector to update.");
>    yield onUpdated;
> +
> +  // Wait for preview update delay PREVIEW_UPDATE_DELAY, see fonts.js
> +  yield waitForTime(200);

I've been investigating this issue. We shouldn't just fix it by adding a timeout here.
I'm not yet 100% sure of the root cause, but I know where the problem occurs at least.

In fonts.js, onThemeChanged is called many times when the theme changes, instead of just once as you might expect. It's actually called once per frame that changes theme I believe.
Which is why you see a condition in this function like:
if (frame === this.chromeDoc.defaultView) {
to filter out events that are not related to the fontinspector frame.

But it turns out that with your patch, for some reasons, this condition evaluates to true more than once when we change themes.
And because that's called asynchronously, this messes up with the running of the test, and we're not catching the right fontinspector-updated events as a result.
So, yes, if you add some timeout to the test, then that will fix it, but really we need to go down to the root cause of why we're getting several times in this IF statement.

As a temporary measure, if you add this inside the IF:

      // Avoid calling update several times.
      if (newTheme === this.currentTheme) {
        return;
      }
      this.currentTheme = getTheme();

then that fixes the problem. This basically just checks that once we've run the callback with the new theme once, we don't try and run it again.

Upon further investigation, I saw that the callback is called twice with frame being inspector.xul, while without your patch, it's called only once with this frame.
So something you changed in inspector.xul makes the theme-switcher mechanism emit 2 events for the same frame.

Comment 47

[Patrick Brosset <:pbro>]

Comment on attachment 8787589 [details] [diff] [review]
bug1260552-perf.patch

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

Nice. Thanks.

Comment 48

[Jan Honza Odvarko [:Honza] (always need-info? me)]

(In reply to Patrick Brosset <:pbro> from comment #46)
> Upon further investigation, I saw that the callback is called twice with
> frame being inspector.xul, while without your patch, it's called only once
> with this frame.
Thanks for the analysis.

The callback is called twice because the theme-switching.js file is loaded twice in the inspector.xul frame. And it's loaded twice because the default NS has been changed to XHTML

I.e.:
<window html="http://www.w3.org/1999/xhtml">

I don't know why that happens.

For now, I reverted the default NS back to XUL (it's just the default, all the markup is using html: prefix so, it's really HTML). In any case, I am quite positive that this will disappear when we change the file name to xhtml.

Honza

Comment 49

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Attachment: bug1260552-side-min-width.patch

I was also discussing min-width of the Inspector side-bar with Joe. Since the all-tabs-menu is now inplace we can change the min-width so, the user can save space (without collapsing it entirely).

I used min=50px and initial width=300px

Honza

Comment 50

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=0e35fb3e2fb8

Honza

Comment 51

[Jan Honza Odvarko [:Honza] (always need-info? me)]

(In reply to Jan Honza Odvarko [:Honza] from comment #48)
> (In reply to Patrick Brosset <:pbro> from comment #46)
> > Upon further investigation, I saw that the callback is called twice with
> > frame being inspector.xul, while without your patch, it's called only once
> > with this frame.
> Thanks for the analysis.
> 
> The callback is called twice because the theme-switching.js file is loaded
> twice in the inspector.xul frame. And it's loaded twice because the default
> NS has been changed to XHTML
> 
> I.e.:
> <window html="http://www.w3.org/1999/xhtml">
Sorry, I meant:
<window xmlns="http://www.w3.org/1999/xhtml">

Honza

Comment 52

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Attachment: bug1260552-side-min-width.patch

Patch updated.

I've also changed initial dimensions for the sidebar, but let me know if new patch is needed for those changes.

Honza

Comment 53

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Attachment: bug1260552-tests.patch

- Clipboard menu items in Inspector context menu are enabled
- Make sure the focus is right before sending click/key events
- Use XUL as the default namespace in inspector.xul to avoid double load of theme-switching.js file (see also comment #48)
- Make sure the inspector splitter-box has occupies the entire available vertical area (use 'vh' units)

Try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=4496196c258c

I am seeing memleaks in for test
devtools/client/inspector/rules/test/browser_rules_edit-selector_09.js

https://treeherder.mozilla.org/#/jobs?repo=try&revision=4496196c258c&selectedJob=26974352

I can't reproduce the problem locally, but is it possible that the test just doesn't finish properly? (even if not saying timeout error)


Honza

Comment 54

[[e-mail]]

seems honza fixed the context menu related tests

Comment 55

[Patrick Brosset <:pbro>]

Comment on attachment 8788361 [details] [diff] [review]
bug1260552-tests.patch

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

I'd like to make sure you understand the cut/copy/paste test changes, because I don't.
So, please let me know what's going on here, and I can quickly R+.

I suppose you're going to fold the various patches here, right? We don't want to land a patch that has failing tests.

::: devtools/client/framework/test/browser_toolbox_textbox_context_menu.js
@@ +28,5 @@
>    is(cmdDelete.getAttribute("disabled"), "true", "cmdDelete is disabled");
>    is(cmdSelectAll.getAttribute("disabled"), "", "cmdSelectAll is enabled");
> +  is(cmdCut.getAttribute("disabled"), "", "cmdCut is enabled");
> +  is(cmdCopy.getAttribute("disabled"), "", "cmdCopy is enabled");
> +  is(cmdPaste.getAttribute("disabled"), "", "cmdPaste is enabled");

So, why are these enabled now? And why was selectAll already enabled before? Did you go to the bottom of this? I remember testing this quickly last week but couldn't figure out what was happening. For starters, it seems like when running the test, we get different results than when testing manually.
The field should be empty here, so why would cut/copy/paste/selectAll be enabled?

::: devtools/client/inspector/inspector.xul
@@ +24,5 @@
>    <!ENTITY % fontinspectorDTD SYSTEM "chrome://devtools/locale/font-inspector.dtd"> %fontinspectorDTD;
>    <!ENTITY % layoutviewDTD SYSTEM "chrome://devtools/locale/layoutview.dtd"> %layoutviewDTD;
>  ]>
>  
> +<window xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul"

I don't understand these NS changes to be honest. But assuming that we're only going to live with a XUL document for a short period of time and that it fixes our problems, I'm fine with this.
If you do understand the root cause of the influence of namespaces over the theme system, then I think an inline comment would be nice.

::: devtools/client/inspector/rules/test/browser_rules_authored_color.js
@@ +52,5 @@
>      // Validating the color change ends up updating the rule view twice
>      let onRuleViewChanged = waitForNEvents(view, "ruleview-changed", 2);
> +    let win = spectrum.element.ownerDocument.defaultView;
> +    win.document.documentElement.focus();
> +    EventUtils.sendKey("RETURN", win);

You have these 3 lines repeated in a few tests, maybe it's worth wrapping this into a helper function in head.js so you can make each test simpler.
In fact even let spectrum = cPicker.specttrum; could be moved there, as far as I can see, the spectrum variable is only used here.

function sendKeyToColorPicker({spectrum}, key) {
  let doc = spectrum.element.ownerDocument;
  doc.documentElement.focus();
  EventUtils.sendKey(key, doc.defaultView);
}

With this, you can simply do
sendKeyToColorPicker(cPicker, "RETURN");
in all these tests.

::: devtools/client/inspector/rules/test/browser_rules_cubicbezier-revert-on-ESC.js
@@ +95,5 @@
>    let onHidden = bezierTooltip.tooltip.once("hidden");
>    let onModifications = view.once("ruleview-changed");
> +  let win = widget.parent.ownerDocument.defaultView;
> +  win.document.documentElement.focus();
> +  EventUtils.sendKey("ESCAPE", win);

Ah, you also use the same thing for the cubic-bezier tooltip, so I still suggest using that helper function I mentioned, but changing it a bit so it works with all tooltips.

::: devtools/client/inspector/rules/test/browser_rules_edit-selector_09.js
@@ +108,5 @@
>    // Escape the new property editor after editing the selector
>    let onBlur = once(view.styleDocument.activeElement, "blur");
> +  let win = view.styleWindow;
> +  win.document.documentElement.focus();
> +  EventUtils.synthesizeKey("VK_ESCAPE", {}, win);

Ok, after all, it looks like you use this with any frame. So the helper function should rather be something like:

function focusAndSendKey(window, key) {
  window.document.documentElement.focus();
  EventUtils.sendKey(key, window);
}

::: devtools/client/inspector/test/browser_inspector_search-filter_context-menu.js
@@ +36,5 @@
>    is(cmdDelete.getAttribute("disabled"), "true", "cmdDelete is disabled");
>    is(cmdSelectAll.getAttribute("disabled"), "", "cmdSelectAll is enabled");
> +  is(cmdCut.getAttribute("disabled"), "", "cmdCut is enabled");
> +  is(cmdCopy.getAttribute("disabled"), "", "cmdCopy is enabled");
> +  is(cmdPaste.getAttribute("disabled"), "", "cmdPaste is enabled");

Same question as for the other context_menu test before.

Comment 56

[Patrick Brosset <:pbro>]

Comment on attachment 8788352 [details] [diff] [review]
bug1260552-side-min-width.patch

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

Generally fine, but see my comments/questions below.

::: devtools/client/inspector/inspector-panel.js
@@ +36,5 @@
>  const TOOLBOX_L10N = new LocalizationHelper("devtools/locale/toolbox.properties");
>  
>  // Sidebar dimensions
> +const INITIAL_SIDEBAR_HEIGHT = 200;
> +const INITIAL_SIDEBAR_WIDTH = 350;

I was wondering why the initial width went from 450px to 350px, but after looking at release (where we still have the old sidebar splitter and tabs), I realized that its default value used to be 350px.
We changed it recently only to account for the fact that we didn't have the allTabs menu in your HTML rework of the tabbar.
So I'm fine to change it back to 350px.

We discussed about this on IRC and you mentioned to some tests might require a large sidebar to work. If that is the case, then I suggest setting the pref in the test before the toolbox opens, instead of changing values here if needed. You can use:
yield pushPref("devtools.toolsidebar-width.inspector", 500);

I'm wondering why the height is different from the width now? We used to only have one size for both directions.

@@ +42,4 @@
>  
>  // If the toolbox width is smaller than given amount of pixels,
>  // the sidebar automatically switches from 'landscape' to 'portrait' mode.
> +const PORTRAIT_MODE_WIDTH = 700;

As discussed on IRC, 700 was our previous value anyway, and it was only changed temporarily to 600 by an earlier patch in this bug. So, fine.

::: devtools/client/themes/inspector.css
@@ +161,5 @@
>    properly visible. The value can be decreased when bug 1281789
>    is fixed and the all-tabs-menu is available again. */
>  #inspector-sidebar-container {
>    overflow: hidden;
> +  min-width: 50px;

As discussed on IRC, I agree that we should do this change. I would just like to make sure however that you've tested each of our panels when the sidebar is 50px. Do they adapt OK? Do they force a horizontal scrollbar at some stage?
I think they should do their best to resize with the sidebar, but after some limit where things would become unreadable/unusable, then there should be a min-width on the panel container that forces an overflow-x to become scroll.

Comment 57

[Jan Honza Odvarko [:Honza] (always need-info? me)]

(In reply to Patrick Brosset <:pbro> from comment #56)
> I'm wondering why the height is different from the width now? We used to
> only have one size for both directions.
Fixed both is now 350px

> ::: devtools/client/themes/inspector.css
> @@ +161,5 @@
> >    properly visible. The value can be decreased when bug 1281789
> >    is fixed and the all-tabs-menu is available again. */
> >  #inspector-sidebar-container {
> >    overflow: hidden;
> > +  min-width: 50px;
> 
> As discussed on IRC, I agree that we should do this change. I would just
> like to make sure however that you've tested each of our panels when the
> sidebar is 50px. Do they adapt OK? Do they force a horizontal scrollbar at
> some stage?
> I think they should do their best to resize with the sidebar, but after some
> limit where things would become unreadable/unusable, then there should be a
> min-width on the panel container that forces an overflow-x to become scroll.
I don't know what we call readable, but my feeling is that the panel become fairly unreadable yet before the width is only 50px. I think that the point here is to allow the user to quickly make space.
In any case this could be a follow up (just no to make this bug even bigger).

Updated patches will follow.

Honza

Comment 58

[Jan Honza Odvarko [:Honza] (always need-info? me)]

(In reply to Patrick Brosset <:pbro> from comment #55)
> Comment on attachment 8788361 [details] [diff] [review]
> bug1260552-tests.patch
> 
> Review of attachment 8788361 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I'd like to make sure you understand the cut/copy/paste test changes,
> because I don't.
> So, please let me know what's going on here, and I can quickly R+.
> 
> I suppose you're going to fold the various patches here, right? We don't
> want to land a patch that has failing tests.
Yes, updated patches will follow.

> 
> ::: devtools/client/framework/test/browser_toolbox_textbox_context_menu.js
> @@ +28,5 @@
> >    is(cmdDelete.getAttribute("disabled"), "true", "cmdDelete is disabled");
> >    is(cmdSelectAll.getAttribute("disabled"), "", "cmdSelectAll is enabled");
> > +  is(cmdCut.getAttribute("disabled"), "", "cmdCut is enabled");
> > +  is(cmdCopy.getAttribute("disabled"), "", "cmdCopy is enabled");
> > +  is(cmdPaste.getAttribute("disabled"), "", "cmdPaste is enabled");
> 
> So, why are these enabled now? And why was selectAll already enabled before?
> Did you go to the bottom of this? I remember testing this quickly last week
> but couldn't figure out what was happening. For starters, it seems like when
> running the test, we get different results than when testing manually.
My feeling is that the test is wrong (manual testing just gives different results than what is in the test). Let me yet dig into this a bit more.


> > +<window xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul"
> 
> I don't understand these NS changes to be honest.
I have no idea why that happens. It might be just weird consequence of mixing XUL and HTML. 
Since this is short term solution, it isn't worth to waste time on this.

> But assuming that we're
> only going to live with a XUL document for a short period of time and that
> it fixes our problems, I'm fine with this.
Good

> ::: devtools/client/inspector/rules/test/browser_rules_authored_color.js
> @@ +52,5 @@
> >      // Validating the color change ends up updating the rule view twice
> >      let onRuleViewChanged = waitForNEvents(view, "ruleview-changed", 2);
> > +    let win = spectrum.element.ownerDocument.defaultView;
> > +    win.document.documentElement.focus();
> > +    EventUtils.sendKey("RETURN", win);
> 
> You have these 3 lines repeated in a few tests, maybe it's worth wrapping
> this into a helper function in head.js
Done


Honza

Comment 59

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Attachment: bug1260552-splitter.patch

Comment 60

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Attachment: bug1260552-panel.patch

Here is the second (and last) patch.

I managed to fix almost all failing tests but, there are still some failures on try. The Inspector code is quite complex and there are no clues in the test-log what could be wrong. I need help from someone who wrote the code/tests. Patrick, can you look at the following failures?

1) devtools/client/inspector/rules/test/browser_rules_edit-selector_09.js
It says there are leaked windows in debug version of Firefox.

2) devtools/client/inspector/test/browser_inspector_pane-toggle-04.js
This seems to be OSX only, might be related to the min-width: 50px?

3) devtools/client/inspector/rules/test/browser_rules_filtereditor-appears-on-swatch-click.js and devtools/client/inspector/rules/test/browser_rules_eyedropper.js
Both these tests finishes too early. Appending: `yield waitForTick()` fixes the problem, but we should find the real reason.

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

Honza

Comment 61

[[e-mail]]

Quick test the patch and overall works great! I'll try turn .xul to .html based on it and find whats missing in bug 1292592 (de-xul sourceeditor).

During test I found the `hide sidebar animation` is missing when tap the sidebar toggle. But `show sidebar animation` still works. We can fix it in another bug anyway.

Comment 62

[Jan Honza Odvarko [:Honza] (always need-info? me)]

(In reply to Jan Honza Odvarko [:Honza] from comment #60)
> 2) devtools/client/inspector/test/browser_inspector_pane-toggle-04.js
> This seems to be OSX only, might be related to the min-width: 50px?
So, yes setting min-height (portrait mode) to 0 fixes the test failure.
Also the test on Windows need to asynchronously wait for recalc layout otherwise it passes (false positive). That's why I couldn't reproduce this on Win OS.

Honza

diff --git a/devtools/client/inspector/test/browser_inspector_pane-toggle-04.js --- a/devtools/client/inspector/test/browser_inspector_pane-toggle-04.js
+++ b/devtools/client/inspector/test/browser_inspector_pane-toggle-04.js
@@ -43,16 +43,18 @@ add_task(function* () {

   ok(panel.classList.contains("pane-collapsed"), "The panel is in collapsed sta   let currentPanelHeight = panel.getBoundingClientRect().height;
   let currentPanelMarginBottom = panel.style.marginBottom;

   info("Resizing window to switch to the vertical layout.");
   hostWindow.resizeTo(300, 800);

+  // Wait for async recalc layout.
+  yield waitForTick();
+
   // Check the panel is collapsed, and still has the same dimensions.
   ok(panel.classList.contains("pane-collapsed"), "The panel is still collapsed"   is(panel.getBoundingClientRect().height, currentPanelHeight,
     "The panel height has not been modified when changing the layout.");
   is(panel.style.marginBottom, currentPanelMarginBottom,
     "The panel margin-bottom has not been modified when changing the layout.");
   info("Restoring window original size.");

Comment 63

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Attachment: bug1260552-panel.patch

Update for:

* devtools/client/framework/test/browser_toolbox_textbox_context_menu.js
This test just opens a context menu on the screen. The modification sets focus to the Inspector's search box, because cut/copy/paste item state depends on what's currently focused (the focus element should be predictable not random).

* devtools/client/inspector/test/browser_inspector_search-filter_context-menu.js
The change sets focus to the Inspector's search box before opening the context menu.

Note that both tests can fail locally because checking cmd_paste command state depends on the current value in the clipboard (try should have an empty clipboared however).

---

I am still stuck with the test failures mentioned in comment #60

Honza

Comment 64

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Attachment: bug1260552-panel.patch

Introducing/using emptyClipboard() helper API.

Honza

Comment 65

[Patrick Brosset <:pbro>]

Comment on attachment 8789234 [details] [diff] [review]
bug1260552-panel.patch

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

The test changes look good. I'm currently trying to investigate the leaks in browser_rules_edit-selector_09.js. My hope for today is to at least manage to reproduce it locally, which will be the first challenge.

Comment 66

[Patrick Brosset <:pbro>]

(In reply to Jan Honza Odvarko [:Honza] from comment #60)
> 1) devtools/client/inspector/rules/test/browser_rules_edit-selector_09.js
> It says there are leaked windows in debug version of Firefox.
I'm taking a look at this one for now. I'll let you know if I find anything.

Comment 67

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Attachment: bug1260552-temp.patch

(In reply to Patrick Brosset <:pbro> from comment #66)
> (In reply to Jan Honza Odvarko [:Honza] from comment #60)
> > 1) devtools/client/inspector/rules/test/browser_rules_edit-selector_09.js
> > It says there are leaked windows in debug version of Firefox.
> I'm taking a look at this one for now. I'll let you know if I find anything.
Thanks

I am yet attaching a temporary patch containing some changes that eliminates test failures. These could help use to track down the real problem (not supposed to land in this state).

* browser_rules_edit-selector_09.js
I used yield waitForTime(2000); at the end of the test and it seems to helped (in one of my try pushes, I can't repro locally). This might be the same reason as e.g. for browser_rules_eyedropper.js

* browser_rules_eyedropper.js, browser_rules_filtereditor-appears-on-swatch-click.js
Using waitForNextTick() at the end helps.
It looks like the test is finishing too early.

* browser_inspector_pane-toggle-04.js
This one just removes the line that tests the height prop of the panel. This fails on OSX and passes on others if waitFortTick is used after resizeTo(800, 300); 

Try push:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=1a7d8e5abf91

Honza

Comment 68

[Jan Honza Odvarko [:Honza] (always need-info? me)]

(In reply to Jan Honza Odvarko [:Honza] from comment #67)
> Created attachment 8789310 [details] [diff] [review]
> * browser_inspector_pane-toggle-04.js
> This one just removes the line that tests the height prop of the panel. This
> fails on OSX and passes on others if waitFortTick is used after
> resizeTo(800, 300); 
Just to note that I commented out wrong line in this test so, it fails on the try run.

Honza

Comment 69

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Attachment: bug1260552-tests.patch

So, here is a patch that fixes some mysterious test failures that I haven't been able to reproduce locally (tried Win/Mac/Linux builds, but no luck).
I am quite convinced that the real problem is with the tests not with the Splitter component implementation/integration.

The fixes are more of a workaround since I can't explain where the problem is.
Also, I am not able to fix the devtools/client/inspector/rules/test/browser_rules_edit-selector_09.js test - Try says there are memory leaks, see [1]). The test is just finishing too soon.

I tried `toolbox.destroy()`  or `Cu.forceCC/GC()` but, no luck. Only waiting for 2 sec really helps.

Also, I test devtools/client/inspector/test/browser_inspector_pane-toggle-04.js fails (behaves differently on OSX and Win). I can't found any workaround. Perhaps bigger refactoring of this test is needed, since the splitter markup is significantly different than before.

Try with this patch included:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=439de8cc9aab&selectedJob=27206166

There is also this orange: 
devtools/client/debugger/test/mochitest/browser_dbg_addon-workers-dbg-enabled.j
Irrelevant?

---

I spent good amount of time looking for better fixes, but very little gain. I suggest to file a follo up bug for:

1) The leaking test (browser_rules_edit-selector_09.js)
2) The layout test (browser_inspector_pane-toggle-04.js)

... and move forward with the Inspector HTML migration. This bug is already blocking Code mirror integration and I'd really like to move it forward to.

Honza


[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=8e92846a7d37&selectedJob=27212829

Comment 70

[Julian Descottes [:jdescottes]]

Just a heads up, I launched a try run of your patches on top of my current GC/CC patches, to see if it changed anything [1]. 
Looks like it doesn't, but my try syntax includes the clipboard test suite, which seems to be consistently failing on :
- inspector/computed/test/browser_computed_search-filter_context-menu.js
- inspector/rules/test/browser_rules_search-filter_context-menu.js
- inspector/test/browser_inspector_search-filter_context-menu.js

[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=ec5c82e69397ca83056e64e8023eee250d9a9c4e

Comment 71

[Alexandre Poirot [:ochameau]]

It looks like it mess up with key shortcuts. 
I see devtools/client/inspector/rules/test/browser_rules_edit-selector_09.js open the webconsole on ESCAPE. Could you confirm this?

I realized that only after digging into memory leak which relates the the split webconsole being opened and not correctly destroyed at test end. Ideally the test framework would take care of that while closing the toolboxes.

Comment 72

[Brian Grinstead [:bgrins]]

(In reply to Alexandre Poirot [:ochameau] from comment #71)
> I realized that only after digging into memory leak which relates the the
> split webconsole being opened and not correctly destroyed at test end.
> Ideally the test framework would take care of that while closing the
> toolboxes.

Yeah, if the issue is that split console gets left open, we should have shared-head.js do `Services.prefs.clearUserPref("devtools.toolbox.splitconsoleEnabled");` in a cleanup function like here: https://dxr.mozilla.org/mozilla-central/source/devtools/client/framework/test/shared-head.js#94-99.

There's other tests that are doing this individually, and their code could generally be removed if it was done in shared-head: https://dxr.mozilla.org/mozilla-central/search?q=Services.prefs.clearUserPref(%22devtools.toolbox.splitconsoleEnabled%22)%3B&redirect=true

Comment 73

[Alexandre Poirot [:ochameau]]

The split webconsole should be destroyed during toolbox.destroy, but the thing is that it is still initializing while the test ends. Locally it often throws:
  TypeError: this._networkRequests is null

That's because the webconsole is destroyed while it is still initializing. We do a getCachedMessages requests which takes some time to process and resolves after toolbox.destroy is called.

So, I don't think we can improve the test framework easily about that.
We may listen for all toolId+"init" event on gDevTools and ensure waiting for all toolId+"ready" before destroying stuff in the test framework, but that's sound rather complex.
We may also consider doing that in production code to avoid some race when stressing our tools?

Comment 74

[Brian Grinstead [:bgrins]]

(In reply to Alexandre Poirot [:ochameau] from comment #73)
> The split webconsole should be destroyed during toolbox.destroy, but the
> thing is that it is still initializing while the test ends. Locally it often
> throws:
>   TypeError: this._networkRequests is null
> 
> That's because the webconsole is destroyed while it is still initializing.
> We do a getCachedMessages requests which takes some time to process and
> resolves after toolbox.destroy is called.
> 
> So, I don't think we can improve the test framework easily about that.
> We may listen for all toolId+"init" event on gDevTools and ensure waiting
> for all toolId+"ready" before destroying stuff in the test framework, but
> that's sound rather complex.
> We may also consider doing that in production code to avoid some race when
> stressing our tools?

Yeah, that doesn't sound like a good situation.  Not sure what the right thing to do is here, but just a couple thoughts: I wouldn't want to prevent a buggy tool from being able to prevent / delay toolbox shutdown due to it never resolving as ready (esp when considering addon panels).  Generally I'd prefer to be as responsive as possible when someone presses the close button or key shortcut, at least as far as visually removing the toolbox and then being able to re-open it ASAP.

Comment 75

[Jan Honza Odvarko [:Honza] (always need-info? me)]

(In reply to Alexandre Poirot [:ochameau] from comment #71)
> It looks like it mess up with key shortcuts. 
> I see devtools/client/inspector/rules/test/browser_rules_edit-selector_09.js
> open the webconsole on ESCAPE. Could you confirm this?
Yes, I can confirm, the Console is opened.

I can see it clearly when putting:
yield waitForTime(10000) at the end of `testEditSelector` or `testAddMatchedRule`

Interestingly, the test fails if I put `waitForTime` at the end of `testEditSelector`. I wouldn't expect that.


Honza

Comment 76

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Attachment: bug1260552-panel.patch

An update that should fix the EXCAPE problem (wrong focus management).
This shouldn't affect the mem-leaks, let's see what the Try says.

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

Honza

Comment 77

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Btw. order of patches:

1) bug1260552-splitter.patch
2) bug1260552-panel.patch
3) bug1260552-tests.patch

Honza

Comment 78

[Alexandre Poirot [:ochameau]]

(In reply to Jan Honza Odvarko [:Honza] from comment #76)
> Created attachment 8790705 [details] [diff] [review]
> bug1260552-panel.patch
> 
> An update that should fix the EXCAPE problem (wrong focus management).
> This shouldn't affect the mem-leaks, let's see what the Try says.

browser_rules_edit-selector_09.js leak was related to this console opening in middle of the test, so I'm expecting it to be gone. I can look into any other leak if there is still some after this patch.

Comment 79

[Alexandre Poirot [:ochameau]]

Looks like in only fails on devtools/server/tests/browser/browser_perf-realtime-markers.js
which is bug 1294009. I've just requested some more run on most test to ensure there is no intermittents.

Comment 80

[Julian Descottes [:jdescottes]]

(In reply to Julian Descottes [:jdescottes] from comment #70)
> Just a heads up, I launched a try run of your patches on top of my current
> GC/CC patches, to see if it changed anything [1]. 
> Looks like it doesn't, but my try syntax includes the clipboard test suite,
> which seems to be consistently failing on :
> - inspector/computed/test/browser_computed_search-filter_context-menu.js
> - inspector/rules/test/browser_rules_search-filter_context-menu.js
> - inspector/test/browser_inspector_search-filter_context-menu.js
> 
> [1]
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=ec5c82e69397ca83056e64e8023eee250d9a9c4e

Still investigating those. In each failing test, the test code was modified in order to focus the search input in the beginning of the test.

The tests fail both locally and on try, but for different reasons apparently.

On try, this makes the cut, copy & paste commands enabled in the context menu, while the test expects them to be disabled. Not sure why cut&copy become available, as the input is empty when the test starts.

Locally only the paste command is enabled (and using emptyClipboard() does not seem to help. In the meantime "select all" is disabled, since the input is empty, while the test expects it to be enabled.

Also related, browser_toolbox_textbox_context_menu.js fails for me locally on OSX, while it is succesful on try.

Comment 81

[Jan Honza Odvarko [:Honza] (always need-info? me)]

(In reply to Alexandre Poirot [:ochameau] from comment #78)
> (In reply to Jan Honza Odvarko [:Honza] from comment #76)
> > Created attachment 8790705 [details] [diff] [review]
> > bug1260552-panel.patch
> > 
> > An update that should fix the EXCAPE problem (wrong focus management).
> > This shouldn't affect the mem-leaks, let's see what the Try says.
> 
> browser_rules_edit-selector_09.js leak was related to this console opening
> in middle of the test, so I'm expecting it to be gone. I can look into any
> other leak if there is still some after this patch.
I can confirm, I don't see mem-leaks now, thanks Alex!

Honza

Comment 82

[Jan Honza Odvarko [:Honza] (always need-info? me)]

(In reply to Julian Descottes [:jdescottes] from comment #80)

Thanks for helping with this Julian!

> Still investigating those. In each failing test, the test code was modified
> in order to focus the search input in the beginning of the test.
> 
> The tests fail both locally and on try, but for different reasons apparently.
> 
> On try, this makes the cut, copy & paste commands enabled in the context
> menu, while the test expects them to be disabled. Not sure why cut&copy
> become available, as the input is empty when the test starts.
I've been observing the same results. I also tried to put `waitForTime` in the test code to see the menu at the screen. Cut/Copy items were really enabled even if there is nothing in the input box. 

When I right-clicked in the input-box manually (while the test is waiting on `waitForTime`) the menu appeared again with Cut/Copy properly disabled and the test properly finished. This made me think that the focus was wrong and not set to the input box.

I was also playing with `Toolbox._updateTextboxMenuItems`, but this seems to work properly.

Also, I was playing with:
let controller = searchBox.ownerDocument.commandDispatcher.getControllerForCommand("cmd_copy");
is(controller.isCommandEnabled("cmd_copy"), true, "cmd_copy is enabled");

And `isCommandEnabled` really returned true (which is unexpected since there is nothing in the searchbox). Could it be that the document has wrong controller associated? The process that decides whether these menu-items are enabled or not just seem to be wrong.

Honza

Comment 83

[Julian Descottes [:jdescottes]]

Made some more progress. First of all, before your patches the 3 failing tests here were really inaccurate. 
They aim to compare the context menu of a field before/after setting a value in the field, but the first state of the context menu was always taken before actually focusing the field. So the context menu was not actually related to the input field. This led to inconsistent menu items being enabled, such as "selectAll" while the input is empty. 

So I think it makes sense, since we force the focus in the field first, to update the initial asserts.
"selectAll" should be disabled, since the inputs are empty. 

"paste" is a bit more problematic, because we can't properly control the clipboard. The emptyClipboard helper actually only seems to clear the native clipboard on Windows (see Bug 666254). We can either assert Paste only on windows, or not assert it at all. In favor of the #1 option at the moment. 

New try run with my changes applied: 
- https://treeherder.mozilla.org/#/jobs?repo=try&revision=5037c14b0c14

Comment 84

[Jan Honza Odvarko [:Honza] (always need-info? me)]

(In reply to Julian Descottes [:jdescottes] from comment #83)
> Made some more progress. First of all, before your patches the 3 failing
> tests here were really inaccurate. 
> They aim to compare the context menu of a field before/after setting a value
> in the field, but the first state of the context menu was always taken
> before actually focusing the field. So the context menu was not actually
> related to the input field. This led to inconsistent menu items being
> enabled, such as "selectAll" while the input is empty. 
> 
> So I think it makes sense, since we force the focus in the field first, to
> update the initial asserts.
> "selectAll" should be disabled, since the inputs are empty. 
> 
Thanks for the analysis Julian! (it goes inline with what I have been observing)

> "paste" is a bit more problematic, because we can't properly control the
> clipboard. The emptyClipboard helper actually only seems to clear the native
> clipboard on Windows (see Bug 666254). We can either assert Paste only on
> windows, or not assert it at all. In favor of the #1 option at the moment. 
#1 sounds good to me.

> 
> New try run with my changes applied: 
> - https://treeherder.mozilla.org/#/jobs?repo=try&revision=5037c14b0c14


Honza

Comment 85

[Julian Descottes [:jdescottes]]

Looks like I forgot to call emptyClipboard in one test: https://treeherder.mozilla.org/#/jobs?repo=try&revision=711484c01658

Comment 86

[Julian Descottes [:jdescottes]]

Try looks green with the last patch applied. 

Honza: I can either submit for review or let you fold my patch [1] with yours? 
Let me know how you prefer to proceed.

[1] https://hg.mozilla.org/try/rev/10ebec133080afa4378a4ec67f7cfcd7d081253f

Comment 87

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Attachment: bug1260552-panel.patch

Reabase

Comment 88

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Attachment: bug1260552-tests.patch

(In reply to Julian Descottes [:jdescottes] from comment #86)
> Try looks green with the last patch applied. 
> 
> Honza: I can either submit for review or let you fold my patch [1] with
> yours? 
> Let me know how you prefer to proceed.
> 
> [1] https://hg.mozilla.org/try/rev/10ebec133080afa4378a4ec67f7cfcd7d081253f
I merged your changes with mine. Updated patch is attached.

There is one more failing test I forgot.
devtools/client/inspector/test/browser_inspector_pane-toggle-04.js
See the patch, one check commented out. I couldn't figure out why `panel.getBoundingClientRect().height` is different on Win and Mac.

Try push for the current patches:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=c1fdf2194b00

Honza

Comment 89

[Julian Descottes [:jdescottes]]

(In reply to Jan Honza Odvarko [:Honza] from comment #88)
> Created attachment 8791246 [details] [diff] [review]
> bug1260552-tests.patch
> 
> (In reply to Julian Descottes [:jdescottes] from comment #86)
> > Try looks green with the last patch applied. 
> > 
> > Honza: I can either submit for review or let you fold my patch [1] with
> > yours? 
> > Let me know how you prefer to proceed.
> > 
> > [1] https://hg.mozilla.org/try/rev/10ebec133080afa4378a4ec67f7cfcd7d081253f
> I merged your changes with mine. Updated patch is attached.
> 
> There is one more failing test I forgot.
> devtools/client/inspector/test/browser_inspector_pane-toggle-04.js
> See the patch, one check commented out. I couldn't figure out why
> `panel.getBoundingClientRect().height` is different on Win and Mac.
> 
> Try push for the current patches:
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=c1fdf2194b00
> 
> Honza

I think the remaining failure is linked to the visibility: collapse rule that was added to the selector #inspector-splitter-box .controlled.pane-collapsed in inspector.css 

The test is measuring the height of the hidden panel while the layout is changing. The goal of the test was to ensure that a collapsed panel could not appear by switching layout / resizing the container, since the panel was purely hidden using negative margins. Now it is hidden using "visibility: collapse", so I think it's safe to remove this test.

I ran a small try run removing the visibility collapse, and the test is passing [1], but I still think we should remove this test now, for the reasons mentioned above.

[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=7f5807c6bef6687eb011e38e0c6f182e8e42d543&selectedJob=27481425

Comment 90

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Attachment: bug1260552-tests.patch

(In reply to Julian Descottes [:jdescottes] from comment #89)
> but I still think we should remove this test now, for the
> reasons mentioned above.
Agree, I've been suggesting the same in comment #23

Tree is closed, I'll post a link to Try push as soon as it's open again.

Honza

Comment 91

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Try push:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=80d934816849

Honza

Comment 92

[Jan Honza Odvarko [:Honza] (always need-info? me)]

The last try is green, let's try to land this thing!

Honza

Comment 93

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/fx-team/rev/e7da6eac37d3
Implement Splitter React component. r=jlongster, r=pbro
https://hg.mozilla.org/integration/fx-team/rev/e8de82ab570d
Replace xul:spliter by html:splitter. r=pbro
https://hg.mozilla.org/integration/fx-team/rev/c6564e0c0491
Fix test failures. r=me

Comment 94

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/e7da6eac37d3
https://hg.mozilla.org/mozilla-central/rev/e8de82ab570d
https://hg.mozilla.org/mozilla-central/rev/c6564e0c0491

Comment 95

[Petruta Horea [:phorea], Desktop Test Engineering]

Hello Honza,

For the manual verification of this bug, is there something else to cover besides the splitting methods in Dev Tools (as specified in the Demo from comment 9)?

Thank you,
Petruta

Comment 96

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backed out for failing devtools tests on Linuxe 32 bit debug, e.g. browser_rules_completion-existing-property_01.js:

https://hg.mozilla.org/mozilla-central/rev/8d211b6a94f603ef81ffcdee3a2b484c2ecd21cd
https://hg.mozilla.org/mozilla-central/rev/65f32c651eb766d3e6be7e55aafe42c2f8a47305
https://hg.mozilla.org/mozilla-central/rev/8dc7461d078ee8681c0a751ea02a69611fff1954

First run with jobs + containing failures: https://treeherder.mozilla.org/#/jobs?repo=fx-team&revision=dd0619214bc367e85630a917a95de4d3db9ed9b6

Take a look at the dt5+6+7:
E.g. dt5: https://treeherder.mozilla.org/logviewer.html#?job_id=11607446&repo=fx-team

20:59:04     INFO -  116 INFO TEST-PASS | devtools/client/inspector/rules/test/browser_rules_authored_color.js | changing the color preserved the unit for rgb -
20:59:04     INFO -  117 INFO Adding a new tab with URL: data:text/html;charset=utf-8,%3Cstyle%20type%3D%22text%2Fcss%22%3E%0A%20%20%20%20%20%20%23testid%20%7B%20%20color%3A%20%23F0C%3B%7D%20%0A%20%20%20%20%20%20%3C%2Fstyle%3E%0A%20%20%20%20%20%20%3Cdiv%20id%3D%22testid%22%20class%3D%22testclass%22%3EStyled%20Node%3C%2Fdiv%3E
20:59:04     INFO -  118 INFO Tab added and finished loading
20:59:04     INFO -  119 INFO Loading the helper frame script chrome://mochitests/content/browser/devtools/client/inspector/rules/test/doc_frame_script.js
20:59:04     INFO -  120 INFO Opening the inspector
20:59:04     INFO -  121 INFO Opening the toolbox
20:59:04     INFO -  122 INFO Toolbox opened and focused
20:59:04     INFO -  123 INFO Need to wait for the inspector to update
20:59:04     INFO -  124 INFO Waiting for actor features to be detected
20:59:04     INFO -  125 INFO Selecting the ruleview sidebar
20:59:04     INFO -  126 INFO Selecting the node for '#testid'
20:59:04     INFO -  127 INFO Getting the spectrum colorpicker object
20:59:04     INFO -  128 INFO Setting the new color
20:59:04     INFO -  129 INFO Applying the change
20:59:04     INFO -  130 INFO Waiting for rule-view to update
20:59:04     INFO -  131 INFO Waiting for the style to be applied on the page
20:59:04     INFO -  132 INFO Sending message Test:WaitForComputedStylePropertyValue to content
20:59:04     INFO -  133 INFO Expecting message Test:WaitForComputedStylePropertyValue from content
20:59:04     INFO -  134 INFO Waiting for event: 'ruleview-changed' on [object Object].
20:59:04     INFO -  135 INFO Got event: 'ruleview-changed' on [object Object].
20:59:04     INFO -  136 INFO Got event: 'ruleview-changed' on [object Object].
20:59:04     INFO -  137 INFO TEST-PASS | devtools/client/inspector/rules/test/browser_rules_authored_color.js | changing the color preserved the unit for hex -
20:59:04     INFO -  138 INFO Leaving test bound
20:59:04     INFO -  139 INFO TEST-UNEXPECTED-FAIL | devtools/client/inspector/rules/test/browser_rules_authored_color.js | This test exceeded the timeout threshold. It should be rewritten or split up. If that's not possible, use requestLongerTimeout(N), but only as a last resort. -

The next one is a permafail:
21:03:13     INFO -  262 INFO TEST-PASS | devtools/client/inspector/rules/test/browser_rules_completion-existing-property_01.js | Popup is closed -
21:03:13     INFO -  263 INFO Leaving test bound
21:03:13     INFO -  264 INFO Removing tab.
21:03:13     INFO -  265 INFO Waiting for event: 'TabClose' on [object XULElement].
21:03:13     INFO -  266 INFO Got event: 'TabClose' on [object XULElement].
21:03:13     INFO -  267 INFO Tab removed and finished closing
21:03:13     INFO -  268 INFO TEST-UNEXPECTED-FAIL | devtools/client/inspector/rules/test/browser_rules_completion-existing-property_01.js | This test exceeded the timeout threshold. It should be rewritten or split up. If that's not possible, use requestLongerTimeout(N), but only as a last resort. -

dt6 has: TEST-UNEXPECTED-FAIL | devtools/client/inspector/layout/test/browser_layout_editablemodel.js | This test exceeded the timeout threshold. It should be rewritten or split up. If that's not possible, use requestLongerTimeout(N), but only as a last resort. - 

dt7 has: devtools/client/inspector/computed/test/browser_computed_search-filter_escape-keypress.js | This test exceeded the timeout threshold. It should be rewritten or split up. If that's not possible, use requestLongerTimeout(N), but only as a last resort.

Comment 97

[Joel Maher ( :jmaher ) (UTC -8)]

as a note when this landed we saw some talos regressions in damp:
== Change summary for alert #3271 (as of September 15 2016 19:58 UTC) ==

Regressions:

  4%  damp summary windows7-32 pgo e10s     215.11 -> 224.3
  3%  damp summary windows7-32 opt e10s     274.23 -> 283.8

For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=3271

and the backout saw the improvements.

Comment 98

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Attachment: bug1303390.patch

Include patch from bug 1303390 into this bug (makes tests more stable).
Already R+ by bgrins

Honza

Comment 99

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Attachment: bug1262443.patch

Include patch (1/2) from bug 1262443 (makes tests more stable)
Already R+ from :pbro

Honza

Comment 100

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Attachment: bug1262443-tests.patch

Include patch (2/2) from bug 1262443 (makes tests more stable)
Already R+ from :pbro

Honza

Comment 101

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Patches should be applied in this order:

1) bug1260552-splitter.patch
2) bug1260552-panel.patch
3) bug1260552-tests.patch
4) bug1303390.patch
5) bug1262443.patch
6) bug1262443-tests.patch

Here is try push from today:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=ffc7b61d70fc
Looks good to me.


Another try push (the same set of patches) + Talos:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=dac041887725

Honza

Comment 102

[Jan Honza Odvarko [:Honza] (always need-info? me)]

@Brian, the try push + Talos finished.

Honza

Comment 103

[Brian Grinstead [:bgrins]]

OK, ongoing talos comparison link: https://treeherder.mozilla.org/perf.html#/compare?originalProject=fx-team&originalRevision=8a1efb73e742&newProject=try&newRevision=dac041887725&framework=1&filter=damp&showOnlyImportant=0

Comment 104

[Brian Grinstead [:bgrins]]

(In reply to Joel Maher ( :jmaher) from comment #97)
> as a note when this landed we saw some talos regressions in damp:
> == Change summary for alert #3271 (as of September 15 2016 19:58 UTC) ==
> 
> Regressions:
> 
>   4%  damp summary windows7-32 pgo e10s     215.11 -> 224.3
>   3%  damp summary windows7-32 opt e10s     274.23 -> 283.8
> 
> For up to date results, see:
> https://treeherder.mozilla.org/perf.html#/alerts?id=3271
> 
> and the backout saw the improvements.

We don't have PGO results from this try push but as far as I can tell there are no regressions flagged in https://treeherder.mozilla.org/perf.html#/compare?originalProject=fx-team&originalRevision=8a1efb73e742&newProject=try&newRevision=dac041887725&framework=1&filter=damp&showOnlyImportant=0

This is what I see for windows7 opt e10s:

  1.41% damp summary windows7-32 opt e10s 279.00 -> 282.95

Still a bit of a regression but not flagged.  Could be that the XUL removal being folded into this series is helping here. Looks like this should be good to go from DAMP's perspective

Comment 105

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Ok, let's try to land.

Honza

Comment 106

[Carsten Book [:Tomcat]]

needs rebasing because does not apply cleanly like:

Hunk #1 FAILED at 29
1 out of 6 hunks FAILED -- saving rejects to file devtools/client/inspector/inspector-panel.js.rej
patching file devtools/client/inspector/inspector.xul
Hunk #1 succeeded at 13 with fuzz 2 (offset 0 lines).
patch failed, unable to continue (try -v)
patch failed, rejects left in working directory
errors during apply, please fix and qrefresh bug1260552-panel.patch

Comment 107

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Attachment: bug1260552-panel.patch

Rebasing

Comment 108

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/fx-team/rev/3f1d76375421
Implement Splitter React component; r=jlongster, pbro
https://hg.mozilla.org/integration/fx-team/rev/dd2b2926a163
Replace xul:spliter by html:splitter; r=pbro
https://hg.mozilla.org/integration/fx-team/rev/f6cdaa677d14
Fixing test failures; r=me

Comment 109

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/3f1d76375421
https://hg.mozilla.org/mozilla-central/rev/dd2b2926a163
https://hg.mozilla.org/mozilla-central/rev/f6cdaa677d14

Comment 110

[Cristian Comorasu, QA [:ccomorasu], Releae Desktop QA - Please contact [e-mail]]

I reproduced this bug using Fx 48.0a1, build ID:  20160328030215, on Windows 10 x64.
I can confirm this issue is fixed, I verified using Fx 52.0a1, build ID: 20161003030438 and Fx 51.0a1, build ID: 20161003004005, on Windows 10 x64, mac OS X 10.10.5 and Ubuntu 14.04 LTS.

Cheers!