1259126 - Stop using a <deck> to manage selected panel in toolbox

Status: RESOLVED FIXED

(DevTools :: Framework, defect, P1)

Description

[Brian Grinstead [:bgrins]]

We are using a XUL <deck> + selectedPanel to make sure only one tool is visible at once.

https://dxr.mozilla.org/mozilla-central/source/devtools/client/framework/toolbox.xul#145
https://dxr.mozilla.org/mozilla-central/source/devtools/client/framework/toolbox.js#1355

Comment 1

[Brian Grinstead [:bgrins]]

I'd suggest we switch this to a <box> for now, and then instead of deck.selectedPanel = panel we do something like set a 'selected' attribute on the selected panel (and remove it from any others) and add a CSS rule in https://dxr.mozilla.org/mozilla-central/source/devtools/client/themes/toolbars.css to make all 

.toolbox-panel elements visibility: collapse (or display: none) by default and then set it to be visible for the [selected] one.

Comment 2

[Steve Melia]

Hi Brian i'd like to pick this one up thanks.

Comment 3

[Brian Grinstead [:bgrins]]

Alright great, let me know if you have any questions.  I found at least one test that will need to be updated: https://dxr.mozilla.org/mozilla-central/source/devtools/client/framework/test/browser_toolbox_sidebar.js#114

Comment 4

[Steve Melia]

Attachment: Broken tests

Having a problem with a broken test here, but wanted to see what you think so far.

The test you mentioned (browser_toolbox_sidebar.js) seems to be for the side tabpanel inside the inspector; as opposed to the tabs for the toolbox.

However browser_toolbox_select_event.js seems to cover this case; and i've managed to break it with some sort of timing issue; I think perhaps the inspector tab is being selected now before the toolbox has loaded or something like that. I'm investigating this but it's taking me a while to figure out.

There was another error reported by this test which seemed to be from a null inspector iframe contentWindow; so I changed the event from DOMContentLoaded to DOMFrameContentLoaded.

Interestingly, the tab panel inside the inspector (sidebar) seems to be using a XUL element tabbox so I guess this is a candidate for replacement as well? Should I open a case for this? I can't find an existing one. Is there a programme of work to replace XUL in the devtools?

Comment 5

[Brian Grinstead [:bgrins]]

(In reply to Steve Melia from comment #4)
> Created attachment 8735641 [details] [diff] [review]
> Broken tests
> 
> Having a problem with a broken test here, but wanted to see what you think
> so far.
> 
> The test you mentioned (browser_toolbox_sidebar.js) seems to be for the side
> tabpanel inside the inspector; as opposed to the tabs for the toolbox.
> 
> However browser_toolbox_select_event.js seems to cover this case; and i've
> managed to break it with some sort of timing issue; I think perhaps the
> inspector tab is being selected now before the toolbox has loaded or
> something like that. I'm investigating this but it's taking me a while to
> figure out.

Thanks, I'll take a look at the patch.

> Interestingly, the tab panel inside the inspector (sidebar) seems to be
> using a XUL element tabbox so I guess this is a candidate for replacement as
> well? Should I open a case for this? I can't find an existing one. Is there
> a programme of work to replace XUL in the devtools?

I'm using Bug 1259121 to track all related work.  And I think that'd fall within the scope of Bug 1259819, although 1259819 is a broader problem.  Partly because it's not just a <deck> but a <tabbox> which has <tabs/> and <tabpanels /> as children and has more complex behavior (see: https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XUL/tabbox).  If you'd like to give it a try then me or Patrick (pbro) could help answer questions as you run into them, but fair warning that it's really not an easy bug and you might learn more about XUL than you ever wanted to :).

There are also others that are better scoped, like Bug 1259812 (for the 'breadcrumbs' widget in the inspector) and I'm also in the process of breaking up work into smaller pieces in this planning doc: https://docs.google.com/document/d/1_5aerWTN_GVofr6YQVjmJlaGfZ4nv5YKZmdGHewfTpE/edit

Comment 6

[Brian Grinstead [:bgrins]]

(In reply to Steve Melia from comment #4)
> There was another error reported by this test which seemed to be from a null
> inspector iframe contentWindow; so I changed the event from DOMContentLoaded
> to DOMFrameContentLoaded.

Huh, hadn't heard of that event before.  Interesting

Comment 7

[Brian Grinstead [:bgrins]]

(In reply to Brian Grinstead [:bgrins] from comment #6)
> (In reply to Steve Melia from comment #4)
> > There was another error reported by this test which seemed to be from a null
> > inspector iframe contentWindow; so I changed the event from DOMContentLoaded
> > to DOMFrameContentLoaded.
> 
> Huh, hadn't heard of that event before.  Interesting

We'd need to check that in various host modes (like bottom dock, side dock, and window docked) to make sure the event works properly in those cases

Comment 8

[Brian Grinstead [:bgrins]]

The test looks like it could be racy.  It'd be nice to convert it to the add_task pattern and use shared helper functions like: https://dxr.mozilla.org/mozilla-central/source/devtools/client/inspector/test/browser_inspector_destroy-before-ready.js#10.

Here's a sample of what I think could replace the entirety of that test and be testing the same thing:

add_task(function*() {
  let toolbox = yield openNewTabAndToolbox("data:text/html;charset=utf-8,test select events");

  yield new Promise(resolve => {
    toolbox.once("select", (event, id) => {
      is(id, "inspector", "inspector selected");
      resolve();
    });
    toolbox.selectTool("inspector");
  });

  yield new Promise(resolve => {
    toolbox.once("select", (event, id) => {
      is(id, "webconsole", "webconsole selected");
      resolve();
    });
    toolbox.selectTool("webconsole");
  });

  yield new Promise(resolve => {
    toolbox.once("select", (event, id) => {
      is(id, "styleeditor", "styleeditor selected");
      resolve();
    });
    toolbox.selectTool("styleeditor");
  });

  yield new Promise(resolve => {
    toolbox.once("inspector-selected", () => {
      is(toolbox.currentToolId, "inspector", "inspector selected");
      resolve();
    });
    toolbox.selectTool("inspector");
  });

  yield new Promise(resolve => {
    toolbox.once("webconsole-selected", () => {
      is(toolbox.currentToolId, "webconsole", "webconsole selected");
      resolve();
    });
    toolbox.selectTool("webconsole");
  });

  yield new Promise(resolve => {
    toolbox.once("styleeditor-selected", () => {
      is(toolbox.currentToolId, "styleeditor", "styleeditor selected");
      resolve();
    });
    toolbox.selectTool("styleeditor");
  });

  yield closeToolboxAndTab(toolbox);
});

Comment 9

[Brian Grinstead [:bgrins]]

Comment on attachment 8735641 [details] [diff] [review]
Broken tests

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

This looks generally good to me, thanks.  See Comment 8 for an idea on how to refactor that failing test (of course that suggestion could be refactored further but I think it covers the same assertions as it does now without timing issues).

::: devtools/client/framework/toolbox.js
@@ +1307,5 @@
> +    }
> +
> +    let tab = this.doc.getElementById(id);
> +    tab.setAttribute("selected", "true");
> +    tab.setAttribute("aria-selected", "true");

Yura, does it make sense to set aria-selected on *both* the selected tab, and the container for the panel that is visible (i.e. the Inspector tab plus the box that contains the Inspector panel's iframe)?

The current behavior before this patch is setting it on the tab itself and then using a <xul:deck>.selectedPanel setter to choose which box is visible.

@@ +1360,5 @@
>      // found.
>      tabstrip.selectedItem = tab || tabstrip.childNodes[0];
>  
>      // and select the right iframe
> +    this.selectItem(".toolbox-panel", "toolbox-panel-" + id);

If we *don't* end up wanting to set aria-selected on both the tab and the container, then I'd suggest we don't need the selectItem function and it's logic could be moved inline when the tab is selected, with only a smaller portion of it moved inline here.

Comment 10

[Steve Melia]

Attachment: Update: Refactored test and changed styling for DOMFrameContentLoaded

OK I've refactored the test an added test cases for the different hosts.

This has resolved the problem with this test; I then identified another problem, to cut a long story short it looks like
- DOMContentLoaded on an iframe may fire before the iframe.contentWindow is available
- the replacement event i've used, DOMFrameContentLoaded will not fire if display:none

This affects e.g. browser_toolbox_view_source_01.js which ends up calling loadTool rather than selectTool; thus not displaying the tab.

Changing the CSS to visibility:collapse seems to be a workaround; i'm not sure if there's a bug here with DOMFrameContentLoaded, it looks like an FF specific event anyway. I'll have a look at creating a minimal test case for this?

So tests in /devtools/client/framework are now all passing.

Comment 11

[Brian Grinstead [:bgrins]]

Comment on attachment 8736067 [details] [diff] [review]
Update: Refactored test and changed styling for DOMFrameContentLoaded

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

Re-flagging feedback from Comment 9

Comment 12

[Brian Grinstead [:bgrins]]

(In reply to Steve Melia from comment #10)
> Changing the CSS to visibility:collapse seems to be a workaround; i'm not
> sure if there's a bug here with DOMFrameContentLoaded, it looks like an FF
> specific event anyway. I'll have a look at creating a minimal test case for
> this?
> 
> So tests in /devtools/client/framework are now all passing.

Do we *need* to use DOMFrameContentLoaded to get this to work or is it an optimization?  We could make that change in a separate bug if it's the latter

Comment 13

[Steve Melia]

Attachment: Update: don't use DOMFrameContentLoaded now we have visibility:collapse

Well; now that i'm using visibility:collapse instead of display:none; browser_toolbox_select_event and browser_toolbox_view_source_01 both work fine with DOMContentLoaded; so i've changed it back to that.

I don't fully understand what's happening with display:none; and/or the race condition, which occurred in the mochitests only - not during a manual test, and  was fixed by using DOMFrameContentLoaded. 

That's not necessarily a problem! But it did feel more correct to use display:none since afaik visibility:collapse should only be used in a table...

Comment 14

[Yura Zenevich [:yzen] (please use "needinfo")]

Comment on attachment 8736182 [details] [diff] [review]
Update: don't use DOMFrameContentLoaded now we have visibility:collapse

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

My guess is that you wanted me to take a look at ARIA part, which looks reasonable. On the side note, it would be ideal if we stuck to using display none or visibility hidden as it proved to be the most reliable from a11y perspective as well.

Comment 15

[Brian Grinstead [:bgrins]]

Comment on attachment 8736182 [details] [diff] [review]
Update: don't use DOMFrameContentLoaded now we have visibility:collapse

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

Looking good, thanks.  A few more notes

::: devtools/client/framework/test/browser_toolbox_select_event.js
@@ +19,5 @@
> +  yield isSelected("inspector");
> +  yield isSelected("webconsole");
> +  yield isSelected("jsdebugger");
> +  yield isSelected("styleeditor");
> +  yield toolbox.destroy();

What about not destroying the toolbox in between the first and second set (which would test a distinct case that is re-selecting a tool that's already been initialized)?

@@ +42,2 @@
>  
> +  gBrowser.removeTab(tab);

These two lines can be removed:

gBrowser.removeTab(tab);
finish();

@@ +46,5 @@
> +  /**
> +   * Assert that selecting the given toolId raises a select event
> +   * @param {toolId} Id of the tool to test
> +   */
> +  function isSelected(toolId) {

So I think there's another case that we want to check which is that the toolbox emits "inspector-selected" in addition to "select" with the ID of inspector.

I'd say keep this function (although maybe renamed to testSelectEvent) and add a new one (testToolSelectEvent or similar) that is called instead of the third set of calls to isSelected

::: devtools/client/framework/toolbox.js
@@ +1299,5 @@
> +   *        CSS selector for the collection of items to unselect
> +   * @param {string} id
> +   *        The Id of the item to select
> +   */
> +  selectItem: function(collectionSelector, id) {

Nit: I'd prefer it slightly it took a DOM collection instead of a selector string then looped through the set and removed selected attribute and aria-selected on all nodes, except for the one that needs it.  Something similar to this:

selectSingleNode: function(collection, id) {
  [...collection].forEach(node => {
    if (node.id === id) {
      node.setAttribute("selected", "true");
      node.setAttribute("aria-selected", "true");
    } else {
      node.removeAttribute("selected");
      node.removeAttribute("aria-selected");
    }
  }
}
Then called via:

this.selectSingleNode(this.doc.querySelectorAll(".devtools-tab"),
                      "toolbox-tab-" + id);

This is slightly different behavior - it would remove attributes on more than one node if they somehow got in that state, and also would remove the [aria-selected] attribute if somehow it was set but not [selected].

@@ +1306,5 @@
> +      selected.removeAttribute("selected");
> +      selected.setAttribute("aria-selected", "false");
> +    }
> +
> +    let tab = this.doc.getElementById(id);

The variable name 'tab' should be something more generic

::: devtools/client/themes/toolbars.css
@@ +836,5 @@
>  
> +.toolbox-panel {
> +  display: -moz-box;
> +  -moz-box-flex: 1;
> +  visibility: collapse;

Let's stick with display: none as in Comment 14 if the tests are passing with it.

Comment 16

[Steve Melia]

Attachment: Update: implemented some feedback

See attached implementing your suggestions - regarding the tests the three sections test different hostType as mentioned in Comment 7; within which I test tab reselection. The toolbox destroy seems necessary to allow for reopening with a different host type? Perhaps the test is too long?

I added in the tool specific event to match the scope of the original test - apologies for missing this.

So - this patch is working great; but unfortunately, if I use display:none the tests fail (...view_source_01.js) as described in Comment 10; I don't think i've explained this very well so let me restate my observations around https://dxr.mozilla.org/mozilla-central/source/devtools/client/framework/toolbox.js#1289

- When using display:none to hide the tab panels; waiting on the frame to load with DOMContentLoaded before initialising the tool; ...view_source_01.js fails with "TypeError: this.panelWin is undefined" {file: "resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/debugger/panel.js" line: 18} because the event has fired even though the contentWindow is still null.

- When using display:none; waiting on the frame to load with DOMFrameContentLoaded, ...view_source_01.js times out waiting for the jsdebugger to open - it appears that the event never fires for this tab, I haven't figured out why yet.

Annoyingly, the ...select_event.js test works fine in all scenarios; and everything works OK with visibility: collapse. (as in my patch)

Comment 17

[Brian Grinstead [:bgrins]]

Comment on attachment 8737000 [details] [diff] [review]
Update: implemented some feedback

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

Nice!  I made a couple of local changes for eslint (to import all shared-head globals from the head.js file and to fix an indentation thing in the test).  Here's a try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=9e96baddf7e3

Comment 18

[Brian Grinstead [:bgrins]]

(In reply to Steve Melia from comment #16)
> - When using display:none; waiting on the frame to load with
> DOMFrameContentLoaded, ...view_source_01.js times out waiting for the
> jsdebugger to open - it appears that the event never fires for this tab, I
> haven't figured out why yet.
> 
> Annoyingly, the ...select_event.js test works fine in all scenarios; and
> everything works OK with visibility: collapse. (as in my patch)

Works for me, visibility: collapse is what the <deck> was using so it's not a change from the current behavior regarding Comment 14

Comment 19

[Brian Grinstead [:bgrins]]

Try looks good, going to check in

Comment 20

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/133d8859d7b5

Comment 21

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/133d8859d7b5