1259819 - Create an HTML replacement for the sidebar tabs widget

Status: VERIFIED FIXED

(DevTools :: Framework, enhancement, P1)

Description

[Brian Grinstead [:bgrins]]

Attachment: sidebar-menu-overflow.png

The sidebar tabs widget used for example in the inspector is using XUL elements:  https://dxr.mozilla.org/mozilla-central/source/devtools/client/framework/sidebar.js.

For Bug 1259121 we should come up with an HTML replacement.  One of the tricky parts is going to be handling the overflow case - right now we use a toolbarbutton with a menupopup to (see screenshot).

Comment 1

[Brian Grinstead [:bgrins]]

I seem to remember an HTML version of this for devtools.html but looking at the code now it seems to still use XUL: https://github.com/joewalker/devtools.html/blob/master/client/inspector/inspector-panel.js and https://github.com/joewalker/devtools.html/blob/1719fbb60eb3d02341724f76bc44ebad2792aecc/client/framework/content/widgets/sidebar.js

Comment 2

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

(In reply to (Unavailable until April 25) Brian Grinstead [:bgrins] from comment #1)
> I seem to remember an HTML version of this for devtools.html but looking at
> the code now it seems to still use XUL:
> https://github.com/joewalker/devtools.html/blob/master/client/inspector/
> inspector-panel.js and
> https://github.com/joewalker/devtools.html/blob/
> 1719fbb60eb3d02341724f76bc44ebad2792aecc/client/framework/content/widgets/
> sidebar.js

There is Tab React component already used for the JSON Viewer and HTTP Inspector:
https://github.com/mozilla/gecko-dev/blob/master/devtools/client/jsonview/components/reps/tabs.js

Some comments:

* It would be great to move the tabs.js into client/shared/components
* I think we could try to use it since it's already nicely working on two places (JSONv, HTTPi) as part of the XUL -> HTML effort
* Support for 'all tabs menu' is not ready and we might want to postpone this to the next phase
* The API we mostly need: show(), hide(), addTab(), 'select' event
* Support for telemetry needs to be done. It should be relatively simple (just all telemetry module callbacks)
* The side bar is used in the following panels: Inspector, Net, Scratchpad, Console
* There are several test for side panel too: 
  - client\framework\test\browser_toolbox_sidebar.js
  - client\framework\test\browser_toolbox_sidebar_events.js
  - client\framework\test\browser_toolbox_sidebar_events.js
  - client\framework\test\browser_toolbox_sidebar_overflow_menu.js
  -

Comment 3

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

Attachment: bug1259819.patch

I spent some time on this and it would be great to have some feedback at this point.

The code still needs clean up, but here is what it's done in the attached patch:

* devtools/client/framework/toolsidebar.js
There is a new ToolSidebar() implementation. It's intended as direct replacement for the current ToolSidebar() in sidebar.js file. The main difference is that it's based on React component 'Sidebar' (not a XUL widget) see below.

This is big part of the patch, but it's mostly copy of the current sidebar.js 
(sidebar.js should be removed eventually)

* devtools/shared/components/tabs/sidebar
This is the Sidebar() React component mentioned above, it's built on top of Tabs component, which is already utilized by JSON Viewer and HTTPi. The Tabs component should go to shared/components dir too. Originally I thought that ToolSidebar can be replaced directly by this component, but ToolSidebar emits bunch of events handled by the rest of the system and I think it's safer to have both.

* client/inspector/computed/components/computed-panel.js
* client/inspector/layout/components/layout-panel.js
* client/inspector/fonts/components/fontinspector-panel.js
* client/inspector/rules/components/rules-panel.js
These are just plain React components that generates HTML markup for individual panels. It's just copy-paste of markup that lived in inspector.xul

This represents second big part of the patch, but it's just essentially copy-paste thing.

* client/inspector/inspector-panel.js
This module now inserts individual side panels components (mentioned above) into the new ToolSidebar().

There is also new onResize handler that sets height of the entire side bar so, overflow (i.e. the v-scrollbar works as expected). I couldn't find CSS way to do this.

Also the logic responsible for storing sidebar width is modified a bit.

* devtools/client/inspector/inspector.xul
All the side panels markup is removed. It's replace by React components (mentioned above)

* devtools/client/shared/components/tabs/frame-panel.js
Support for side panels with iframe. This is intended for the Animation panel, which lived in an iframe (TBD). We want to remove iframes, but it should be done in a follow up.

--

Missing:
- Localization, all DTD strings need to go to *.properties
- The browser Inspector is broken (not sure why, any tips welcome)
- Animation panel
- Code clean up


Honza

Comment 4

[Brian Grinstead [:bgrins]]

(In reply to Jan Honza Odvarko [:Honza] from comment #3)
> Created attachment 8755899 [details] [diff] [review]
> bug1259819.patch
> 
> I spent some time on this and it would be great to have some feedback at
> this point.
> 
> The code still needs clean up, but here is what it's done in the attached
> patch:
> 
> * devtools/client/framework/toolsidebar.js
> There is a new ToolSidebar() implementation. It's intended as direct
> replacement for the current ToolSidebar() in sidebar.js file. The main
> difference is that it's based on React component 'Sidebar' (not a XUL
> widget) see below.
> 
> This is big part of the patch, but it's mostly copy of the current
> sidebar.js 
> (sidebar.js should be removed eventually)

What is needed to remove sidebar.js (or make changes to it so that ToolSidebar is unnecessary)?  It would be easier to follow this patch I think if I saw a diff on top of Sidebar instead of a duplicate version of it.  See comment below.

> * devtools/shared/components/tabs/sidebar
> This is the Sidebar() React component mentioned above, it's built on top of
> Tabs component, which is already utilized by JSON Viewer and HTTPi. The Tabs
> component should go to shared/components dir too. Originally I thought that
> ToolSidebar can be replaced directly by this component, but ToolSidebar
> emits bunch of events handled by the rest of the system and I think it's
> safer to have both.

If the main difference is that one emits a bunch of extra events I feel like we should consolidate on a single implementation and either live with the events in the other, or provide a 'quiet' option when initializing.  Less code that way, and a single widget to update for bugfixes, accessibility, etc.

> * client/inspector/computed/components/computed-panel.js
> * client/inspector/layout/components/layout-panel.js
> * client/inspector/fonts/components/fontinspector-panel.js
> * client/inspector/rules/components/rules-panel.js
> These are just plain React components that generates HTML markup for
> individual panels. It's just copy-paste of markup that lived in inspector.xul
> 
> This represents second big part of the patch, but it's just essentially
> copy-paste thing.

I'm curious for Patrick's feedback here as far as moving this markup into React.  Moving the markup into JS seems great to me in general since we can move strings out of dtd files and into property files which helps with dt.html.
 
> * devtools/client/shared/components/tabs/frame-panel.js
> Support for side panels with iframe. This is intended for the Animation
> panel, which lived in an iframe (TBD). We want to remove iframes, but it
> should be done in a follow up.

Note that we've had problems when removing iframes - see Bug 1238133 and it's blockers.  A couple of those were due to mixing XUL and CSS flex, but the last (bug 1260235) proved that it was hard to share keyboard shortcuts across multiple tools in the same frame.  Anyhow, I don't think it should be an explicit goal to remove iframes - especially for tools that are self contained like the animation inspector.  The main argument, I think, is for performance and I don't remember getting a big boost from Bug 1238133 (although I may be forgetting).

Comment 5

[Brian Grinstead [:bgrins]]

Comment on attachment 8755899 [details] [diff] [review]
bug1259819.patch

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

First pass of feedback, see comment 4 also

::: devtools/client/inspector/computed/components/computed-panel.js
@@ +13,5 @@
> +
> +/**
> + * Computed side panel
> + */
> +var ComputedPanel = createClass({

Do these strictly have to be React components, or could you add a tab that's referring to just a DOM Node?  I ask because I think we should be considering all consumers of the ToolSidebar API (including net monitor, console, scratchpad) so we will be able to migrate all of them away from that API which will let us delete that code and have a consistent component.  I'm not sure how easy switching them all to react would be.  Although it seems this isn't really "switching" the sidebar tools to use React but is rather using React to do a one-time render of the container, which is why I was wondering if it'd be easy to instead pass in a DOM Node.

::: devtools/client/inspector/fonts/components/fontinspector-panel.js
@@ +30,5 @@
> +            input({
> +              id: "font-preview-text-input",
> +              className: "devtools-textinput",
> +              type: "search",
> +              placeholder: "&previewHint;"})

These strings for all the tools will need to move into properties files and out of the dtd files (which can then be removed from being referenced in inspector.xul)

Comment 6

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

Attachment: bug1259819.patch

(In reply to Brian Grinstead [:bgrins] from comment #4)
> (In reply to Jan Honza Odvarko [:Honza] from comment #3)
> > Created attachment 8755899 [details] [diff] [review]
> > bug1259819.patch
> > 
> > I spent some time on this and it would be great to have some feedback at
> > this point.
> > 
> > The code still needs clean up, but here is what it's done in the attached
> > patch:
> > 
> > * devtools/client/framework/toolsidebar.js
> > There is a new ToolSidebar() implementation. It's intended as direct
> > replacement for the current ToolSidebar() in sidebar.js file. The main
> > difference is that it's based on React component 'Sidebar' (not a XUL
> > widget) see below.
> > 
> > This is big part of the patch, but it's mostly copy of the current
> > sidebar.js 
> > (sidebar.js should be removed eventually)
> 
> What is needed to remove sidebar.js (or make changes to it so that
> ToolSidebar is unnecessary)?  It would be easier to follow this patch I
> think if I saw a diff on top of Sidebar instead of a duplicate version of
> it.  See comment below.
We would have to migrate (XUL->HTML) all places where the XUL ToolSidebar is used (all tests, net monitor, scratchpad, webconsole).

Yeah, I see the review problem, for now it's probably best to apply the patch and compare both files... ? (toolsidebar.js and sidebar.js)


> > * devtools/shared/components/tabs/sidebar
> > This is the Sidebar() React component mentioned above, it's built on top of
> > Tabs component, which is already utilized by JSON Viewer and HTTPi. The Tabs
> > component should go to shared/components dir too. Originally I thought that
> > ToolSidebar can be replaced directly by this component, but ToolSidebar
> > emits bunch of events handled by the rest of the system and I think it's
> > safer to have both.
> 
> If the main difference is that one emits a bunch of extra events I feel like
> we should consolidate on a single implementation and either live with the
> events in the other, or provide a 'quiet' option when initializing.  Less
> code that way, and a single widget to update for bugfixes, accessibility,
> etc.
The thing is that the ToolSidebar() is EventEmitter-decorated object and Sidebar() is a React component. The codebase listens for events emitted by ToolSidebar(), but we want to use React to render the Tabs (and I don't like having React component decorated and used as event target). That's why I keep it separated. Anyway, I agree we want to merge those into one, it's just that it probably needs more progress in the migration.

> Do these strictly have to be React components, or could you add a tab that's referring
> to just DOM Node?  
Good question. I have been also thinking about it and I agree that it's good strategy to just wrap an existing DOM node. Later we can move forward and generated the content by React as well. I adopted the patch.

The patch:
- Content of the panels is not generated by React, just existing DOM nodes are wrapped. 
- Side panel titles are loaded from inspector.properties file
- The Browser console fixed!
- Code cleaned (but some parts still needs an attention)

I believe that it's very close to proper review. All seem to be working for me apart from the Animations panel. Patrick, do you have any tips why the panel doesn't work?

Honza

Comment 7

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

Attachment: bug1259819.patch

I am attaching new patch.

- Rebased on the current head
- The Animation panel fixed
- Fixed Firebug theme

Honza

Comment 8

[Brian Grinstead [:bgrins]]

I think Patrick is a better reviewer than me here.  He knows the current sidebar well, and this is affecting the inspector mostly right now.  I'm happy to review if he'd prefer, but I'll at least wait for a round of feedback before attempting to review.

Comment 9

[Patrick Brosset <:pbro>]

Comment on attachment 8760808 [details] [diff] [review]
bug1259819.patch

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

Sorry about the delay Honza.

A few general comments first, which I'm sure you know about already, but since I'm doing some testing, I might as well list the things I'm seeing here:

- The cursor shouldn't be default on the tabs, not pointer.

- If you have devtools.fontinspector.enabled set to true, then the font panel is shown in the sidebar, and that causes the following exception with your patch applied:
TypeError: this.panelDoc.getElementById(...) is null
resource://devtools/client/inspector/inspector-panel.js:429:7

- The font-size or font-family appears to be different, bigger somehow.

- The focus style (the dotted outline) around each tab extends about 3px too much to the bottom, so appears cut off (you can't see the bottom border). In fact, if you switch to the box-model tab (which has no toolbar at its top), you can see the problem better: the labels don't appear to be vertically centered, and the vertical separator lines extend below the bottom of the tabbar.

- The tabs should occupy all of the available space in the sidebar (probably using css flexbox), at least in the light and dark themes. I think with the firebug theme this might be different.
Right now they have a fixed width. Making the sidebar bigger leaves empty space to the right, and making it smaller makes the tabs wrap below, on a second line, partly hiding the content.

- Keyboard navigation for better a11y was added recently, that seems to have changed. We used to be able to use the left/right arrow keys to navigate, now only (shift)tab+enter works. The logic here is that we want toolbars to be focused, not individual items in them, so that you can quickly TAB through the whole toolbox without having to press TAB a thousand times. So if you're focused on the breadcrumbs, pressing TAB once will get you to the sidebar tabs, pressing TAB again from there will get you to the rule-view search field, not to the next tab. If you want to navigate within the tabs, then the arrow keys can be used. But TAB is reserved for quickly navigating around the main UI elements of the toolbox.

- Pressing the sidebar toggle button makes the content of the sidebar disappear quickly and then animates the sidebar.

- We used to have a min-width for the sidebar. Whether this is a good idea or not is a good question, but probably irrelevant for this patch and we should keep the min-width. One problem of not having it for instance is the content of the box-model tab gets really weird below a certain width.

- Resizing the sidebar is noticeably more janky and slow than on dev-edition right now. You can see the content of the tab resizing only at certain points in time, not smoothly as you drag the splitter. We should be really careful with this as one of the complaints we're often getting is that our tools "feel" slower than others, and having a janky UI certainly plays a bit role in this.

- We're missing the underflow/overflow mechanism that displays/hides the all-tabs menu to the right of the tabbar.

- The checkbox to include browser styles in the computed view is gone. It used to be next to the filter field, it's not there anymore.

- The animation tab doesn't take the full height of the sidebar. It's cut off somewhere after 100px or so.

- After an animation is displayed in the animation tool, if you switch tabs and come back to the animation tool, then the animation is gone and the tool seems to be reset and doesn't work anymore. This probably comes from the fact that you're using react to switch the active tab panel, and because the animation is in an iframe. Wouldn't it be a quicker/safer first step to just redo the sidebar in HTML (possibly using react to generate the initial markup) but then keep all tab panels in the DOM and just switching visually between them. Using react to re-render the active panel seems like a pretty brutal first step that might lead to hard to fix regressions. I'd prefer a smoother migration here I think.

A few comments on the general approach:

- As said just before, I'd rather prefer a simpler migration to HTML. I'm afraid with this approach you'll be running into all sorts of of bugs that will be hard and long to fix.

- Reusing the tabs component from jsonview has several effects:
  - First of all, it'd be nice to be able to merge all of our reps into one place, it feels strange to have to require a component from devtools/client/jsonview/components in the code, and it feels also weird to have to include its css.
  - The second thing is that because the styling isn't the same, you end up having to override the box-shadow that normally appears in the tab in jsonview. We should either make the styling the same (no box-shadow in jsonview), which would be better for consistency across our tools, or have the jsonview add some box-shadow. The base component should have the most common style, so the one of the tabs you find in the toolbox.

::: devtools/client/inspector/inspector-panel.js
@@ +430,5 @@
>      }
>  
>      this.layoutview = new LayoutView(this, this.panelWin);
>  
> +    if (true || this.target.form.animationsActor) {

Why add 'true ||' to this condition?

@@ +452,3 @@
>    },
>  
> +  onResizeSidebar: function () {

This handler is most probably why resizing the sidebar became so janky with this patch. We need to find a way to avoid JS altogether. Especially causing reflows during resize events.
The vbox that is next to the splitter already has a size defined, so why elements in it can't just have a 100% width?

::: devtools/client/inspector/inspector.css
@@ +3,5 @@
>   * License, v. 2.0. If a copy of the MPL was not distributed with this
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
> +#inspector-sidebar-container {
> +  overflow: hidden;

Why is this needed now? I have tried removing it and saw no changes.

::: devtools/client/inspector/inspector.xul
@@ +10,5 @@
>  <?xml-stylesheet href="chrome://devtools/skin/rules.css" type="text/css"?>
>  <?xml-stylesheet href="chrome://devtools/skin/computed.css" type="text/css"?>
>  <?xml-stylesheet href="chrome://devtools/skin/fonts.css" type="text/css"?>
>  <?xml-stylesheet href="chrome://devtools/skin/layout.css" type="text/css"?>
> +<?xml-stylesheet href="resource://devtools/client/jsonview/css/tabs.css" type="text/css"?>

See my general comment about linking to this CSS here. Maybe you could use this patch to just 'hg mv' the tabs out of jsonview and into the shared reps.

@@ +175,5 @@
>        </html:div>
>      </vbox>
>      <splitter class="devtools-side-splitter"/>
> +    <vbox id="inspector-sidebar-container">
> +      <div xmlns="http://www.w3.org/1999/xhtml"

<html:div instead of <div xmlns="http://www.w3.org/1999/xhtml"
unless I'm missing something, but isn't this why we're xmlns:html at the top of this file?

@@ +184,3 @@
>  
> +    <!-- Sidebar panel definitions -->
> +    <html:div xmlns="http://www.w3.org/1999/xhtml" id="tabpanels" style="visibility:collapse">

No need for xmlns here.

::: devtools/client/jsonview/components/reps/tabs.js
@@ +178,5 @@
>        ]).isRequired
>      },
>  
>      render: function () {
> +      return DOM.div({className: "tabPanel"},

We usually don't camelcase our css classes in devtools. tab-panel would be better. If it's going to be a shared tabpanel component we reuse in many places, it should even be devtools-tab-panel.

::: devtools/client/jsonview/css/tabs.css
@@ +41,5 @@
>  .tabs .tab-panel {
>    background-color: white;
>  }
>  
> +.tabs .tabPanel,

Oh so there's already a tab-panel class used here, so I understand why you couldn't use this class instead of tabPanel (see my previous comment). 
But having both tab-panel and tabPanel is confusing. Do you really need an extra tabPanel element inside the tab-panel container? If so, then make sure you find a more self-explanatory class name.
Looking at the DOM with the browser toolbox, it doesn't look like we need this extra element at all, it just sits between article.tab-panel and div#sidebar-panel-<something>

::: devtools/client/locales/en-US/inspector.properties
@@ +159,5 @@
>  # is visible
>  markupView.scrollInto.key=s
> +
> +# LOCALIZATION NOTE (inspector.sidebar.fontInspectorTitle):
> +# Title for side panel tab

Please be more descriptive here to facilitate the translation work:
This is the title shown in a tab in the side panel of the inspector that corresponds to the tool displaying the list of fonts used in the page.

Same for the next strings in this file.

::: devtools/client/shared/components/tabs/side-panel-wrapper.js
@@ +15,5 @@
> + * Side panel for the Inspector panel. This side panel is designed
> + * as a wrapper for exising DOM node. It's used in cases where
> + * content of the side panel is defined in DOM and all we need
> + * is just make that node visible if the panel is active (mounted).
> + */

Can you explain more in details why is this wrapper needed?

Also, about this file and the sidebar.js file next to it, why would these be in a components/tabs/ folder? The name of the folder suggests that I'm going to find tabbar components in it, but then I see something about sidebars, it may be confusing to people looking at the code for the first time. I think we need to make sure we only put in devtools/client/shared/components/ really generic and shareable things, with self-explanatory names. 
This wrapper thing seems like a very inspector-specific requirement that would probably live better in the inspector folder.
As for sidebar.js, it looks like it's more of a tabbar than a sidebar. There doesn't seem to be a reason why this should only be used in a side panel.

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

Why the long separator comment here? I don't see this used in other CSS files. Usually when I see them in the middle of files, it's a pretty clear sign that the file should be split up.

@@ +4,5 @@
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +/******************************************************************************/
> +
> +.sidePanelFrame {

No camelCase for class names please. 
.side-panel-frame instead
Also, should this be a .devtools-side-panel-frame instead? I think most of our global/reusable/top-level classes come with the devtools prefix.

@@ +12,5 @@
> +}
> +
> +.theme-dark .tabs .tabs-navigation,
> +.theme-light .tabs .tabs-navigation {
> +  box-shadow: none;

See my general comment about this.

::: devtools/client/shared/components/tabs/sidebar.js
@@ +30,5 @@
> +  },
> +
> +  // Public API
> +
> +  addTab: function (id, title, selected = false, panel, url) {

I haven't used react enough so far, but I'm not sure I've seen a lot of components that expose a public API. My understanding is that components rely on a state to change instead. Maybe the inspector should create redux store instead. It would contain only the state of the sidebar for now (but in the future could be used for the markup view too) and adding tabs, removing tabs and selecting tabs would happen with redux action creators instead.

::: devtools/client/themes/computed.css
@@ +9,5 @@
>    flex-direction: column;
>    width: 100%;
>    /* Bug 1243598 - Reduce the container height by the tab height to make room
>       for the tabs above. */
> +  height: 100%;

I didn't really understand why this change was needed, but since it appears to work fine with it, please update the comment above that is now obsolete.

::: devtools/client/themes/fonts.css
@@ +9,5 @@
>    padding-bottom: 20px;
>    width: 100%;
>    /* Bug 1243598 - Reduce the container height by the tab height to make room
>       for the tabs above. */
> +  height: 100%;

Same comment here.

::: devtools/client/themes/inspector.css
@@ +94,5 @@
>    }
>  }
>  
> +.devtools-sidebar-tabs {
> +  height: 350px;

Why this hard-coded height? Does this really need to be here? I'm guessing this needs to match the default height of the bottom-docked toolbox. What happens if you resize it and then close/open the toolbox then? What happens if you have it docked to the side instead?

I feel like we could avoid this here (since we're setting the height in JS anyway), but if not, then at the very least a comment is required to explain this magic number and explain what it should match.

::: devtools/client/themes/rules.css
@@ +33,5 @@
>    flex-direction: column;
>    width: 100%;
>    /* Bug 1243598 - Reduce the container height by the tab height to make room
>       for the tabs above. */
> +  height: 100%;

Same comment as in computed.css

Comment 10

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

Attachment: bug1259819.patch

(In reply to Patrick Brosset <:pbro> from comment #9)
> Comment on attachment 8760808 [details] [diff] [review]
> bug1259819.patch
> 
> Review of attachment 8760808 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Sorry about the delay Honza.
Thanks for the feedback!

> 
> A few general comments first, which I'm sure you know about already, but
> since I'm doing some testing, I might as well list the things I'm seeing
> here:
> 
> - The cursor shouldn't be default on the tabs, not pointer.
Fixed

> 
> - If you have devtools.fontinspector.enabled set to true, then the font
> panel is shown in the sidebar, and that causes the following exception with
> your patch applied:
> TypeError: this.panelDoc.getElementById(...) is null
> resource://devtools/client/inspector/inspector-panel.js:429:7
Fixed

> 
> - The font-size or font-family appears to be different, bigger somehow.
Fixed

> 
> - The focus style (the dotted outline) around each tab extends about 3px too
> much to the bottom, so appears cut off (you can't see the bottom border). In
> fact, if you switch to the box-model tab (which has no toolbar at its top),
> you can see the problem better: the labels don't appear to be vertically
> centered, and the vertical separator lines extend below the bottom of the
> tabbar.
Fixed

> 
> - The tabs should occupy all of the available space in the sidebar (probably
> using css flexbox), at least in the light and dark themes. I think with the
> firebug theme this might be different.
> Right now they have a fixed width. Making the sidebar bigger leaves empty
> space to the right, and making it smaller makes the tabs wrap below, on a
> second line, partly hiding the content.
Fixed

> 
> - Keyboard navigation for better a11y was added recently, that seems to have
> changed. We used to be able to use the left/right arrow keys to navigate,
> now only (shift)tab+enter works. The logic here is that we want toolbars to
> be focused, not individual items in them, so that you can quickly TAB
> through the whole toolbox without having to press TAB a thousand times. So
> if you're focused on the breadcrumbs, pressing TAB once will get you to the
> sidebar tabs, pressing TAB again from there will get you to the rule-view
> search field, not to the next tab. If you want to navigate within the tabs,
> then the arrow keys can be used. But TAB is reserved for quickly navigating
> around the main UI elements of the toolbox.
Support the left/right arrow keys is yet missing.

> 
> - Pressing the sidebar toggle button makes the content of the sidebar
> disappear quickly and then animates the sidebar.
Fixed

> 
> - We used to have a min-width for the sidebar. Whether this is a good idea
> or not is a good question, but probably irrelevant for this patch and we
> should keep the min-width. One problem of not having it for instance is the
> content of the box-model tab gets really weird below a certain width.
Fixed

> 
> - Resizing the sidebar is noticeably more janky and slow than on dev-edition
> right now. You can see the content of the tab resizing only at certain
> points in time, not smoothly as you drag the splitter. We should be really
> careful with this as one of the complaints we're often getting is that our
> tools "feel" slower than others, and having a janky UI certainly plays a bit
> role in this.
> 
> - We're missing the underflow/overflow mechanism that displays/hides the
> all-tabs menu to the right of the tabbar.
The all-tabs menu is missing entirely. Is that ok to do it as a follow up?
(I just want to keep this patch small)

> 
> - The checkbox to include browser styles in the computed view is gone. It
> used to be next to the filter field, it's not there anymore.
Fixed

> 
> - The animation tab doesn't take the full height of the sidebar. It's cut
> off somewhere after 100px or so.
Fixed

> 
> - After an animation is displayed in the animation tool, if you switch tabs
> and come back to the animation tool, then the animation is gone and the tool
> seems to be reset and doesn't work anymore. This probably comes from the
> fact that you're using react to switch the active tab panel, and because the
> animation is in an iframe. Wouldn't it be a quicker/safer first step to just
> redo the sidebar in HTML (possibly using react to generate the initial
> markup) but then keep all tab panels in the DOM and just switching visually
> between them. Using react to re-render the active panel seems like a pretty
> brutal first step that might lead to hard to fix regressions. I'd prefer a
> smoother migration here I think.
Yes, it might be the way to go, but... The panel-content re-rendering actually doesn't happen since I am using <DIV>s defined in inspector.XUL and moving them into the panel when the panel react component is mounted (i.e. panel selected). The <DIV> is moved back when unmount happens (i.e. panel unselected). The problem is that <iframe>s can't be moved in DOM without reload. But this is the only problem (!) - for the Animation panel. It works great for all the other panels.
Anyway, yes, if there is no workaround for the iframe I'd have to do what you say. But that would be rather worse than the current solution (I don't agree that it's brutal).

> 
> A few comments on the general approach:
> 
> - As said just before, I'd rather prefer a simpler migration to HTML. I'm
> afraid with this approach you'll be running into all sorts of of bugs that
> will be hard and long to fix.
> 
> - Reusing the tabs component from jsonview has several effects:
>   - First of all, it'd be nice to be able to merge all of our reps into one
> place, it feels strange to have to require a component from
> devtools/client/jsonview/components in the code, and it feels also weird to
> have to include its css.
Definitely agree, done in bug 1279215

>   - The second thing is that because the styling isn't the same, you end up
> having to override the box-shadow that normally appears in the tab in
> jsonview. We should either make the styling the same (no box-shadow in
> jsonview), which would be better for consistency across our tools, or have
> the jsonview add some box-shadow. The base component should have the most
> common style, so the one of the tabs you find in the toolbox.
Done. I adopted the base component (used in JSON Viewer) so, it uses the same styling as devtools (e.g. no box-shadow).

> 
> ::: devtools/client/inspector/inspector-panel.js
> @@ +430,5 @@
> >      }
> >  
> >      this.layoutview = new LayoutView(this, this.panelWin);
> >  
> > +    if (true || this.target.form.animationsActor) {
> 
> Why add 'true ||' to this condition?
Leftover, removed.

> 
> @@ +452,3 @@
> >    },
> >  
> > +  onResizeSidebar: function () {
> 
> This handler is most probably why resizing the sidebar became so janky with
> this patch. We need to find a way to avoid JS altogether. Especially causing
> reflows during resize events.
> The vbox that is next to the splitter already has a size defined, so why
> elements in it can't just have a 100% width?
Yes width is 100% now, but height must be set dynamically in JS. I couldn't find a way to do it in CSS.


> 
> ::: devtools/client/inspector/inspector.css
> @@ +3,5 @@
> >   * License, v. 2.0. If a copy of the MPL was not distributed with this
> >   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> >  
> > +#inspector-sidebar-container {
> > +  overflow: hidden;
> 
> Why is this needed now? I have tried removing it and saw no changes.
Otherwise the vertical scrollbar in the Computed panel is broken.

> 
> ::: devtools/client/inspector/inspector.xul
> @@ +10,5 @@
> >  <?xml-stylesheet href="chrome://devtools/skin/rules.css" type="text/css"?>
> >  <?xml-stylesheet href="chrome://devtools/skin/computed.css" type="text/css"?>
> >  <?xml-stylesheet href="chrome://devtools/skin/fonts.css" type="text/css"?>
> >  <?xml-stylesheet href="chrome://devtools/skin/layout.css" type="text/css"?>
> > +<?xml-stylesheet href="resource://devtools/client/jsonview/css/tabs.css" type="text/css"?>
> 
> See my general comment about linking to this CSS here. Maybe you could use
> this patch to just 'hg mv' the tabs out of jsonview and into the shared reps.
So, this is fixed.


> 
> @@ +175,5 @@
> >        </html:div>
> >      </vbox>
> >      <splitter class="devtools-side-splitter"/>
> > +    <vbox id="inspector-sidebar-container">
> > +      <div xmlns="http://www.w3.org/1999/xhtml"
> 
> <html:div instead of <div xmlns="http://www.w3.org/1999/xhtml"
> unless I'm missing something, but isn't this why we're xmlns:html at the top
> of this file?
Yeah, I did it, but using just the html prefix doesn't work for me. The layout is wrong. I was forced to explicitly use the xmlns attribute. Looks like a platform bug...?

> 
> @@ +184,3 @@
> >  
> > +    <!-- Sidebar panel definitions -->
> > +    <html:div xmlns="http://www.w3.org/1999/xhtml" id="tabpanels" style="visibility:collapse">
> 
> No need for xmlns here.
Yes, removed

> 
> ::: devtools/client/jsonview/components/reps/tabs.js
> @@ +178,5 @@
> >        ]).isRequired
> >      },
> >  
> >      render: function () {
> > +      return DOM.div({className: "tabPanel"},
> 
> We usually don't camelcase our css classes in devtools. tab-panel would be
> better. If it's going to be a shared tabpanel component we reuse in many
> places, it should even be devtools-tab-panel.
Fixed

> 
> ::: devtools/client/jsonview/css/tabs.css
> @@ +41,5 @@
> >  .tabs .tab-panel {
> >    background-color: white;
> >  }
> >  
> > +.tabs .tabPanel,
> 
> Oh so there's already a tab-panel class used here, so I understand why you
> couldn't use this class instead of tabPanel (see my previous comment). 
> But having both tab-panel and tabPanel is confusing. Do you really need an
> extra tabPanel element inside the tab-panel container? If so, then make sure
> you find a more self-explanatory class name.
> Looking at the DOM with the browser toolbox, it doesn't look like we need
> this extra element at all, it just sits between article.tab-panel and
> div#sidebar-panel-<something>
I cleaned this up a bit. There is now "tab-panel-box" and "tab-panel". "tab-panel-box" is parent for "tab-panel".

> 
> ::: devtools/client/locales/en-US/inspector.properties
> @@ +159,5 @@
> >  # is visible
> >  markupView.scrollInto.key=s
> > +
> > +# LOCALIZATION NOTE (inspector.sidebar.fontInspectorTitle):
> > +# Title for side panel tab
> 
> Please be more descriptive here to facilitate the translation work:
> This is the title shown in a tab in the side panel of the inspector that
> corresponds to the tool displaying the list of fonts used in the page.
> 
> Same for the next strings in this file.
Done

> 
> ::: devtools/client/shared/components/tabs/side-panel-wrapper.js
> @@ +15,5 @@
> > + * Side panel for the Inspector panel. This side panel is designed
> > + * as a wrapper for exising DOM node. It's used in cases where
> > + * content of the side panel is defined in DOM and all we need
> > + * is just make that node visible if the panel is active (mounted).
> > + */
> 
> Can you explain more in details why is this wrapper needed?
It implement the logic explained above. When a panel is selected it uses an existing <DIV> from inspector.xul as a content. When it's unselected, it moves the <DIV> back to the list of panel definitions. This way I am wrapping existing panel contents in React component (similar to what James did in the debugger the other day I guess).

> 
> Also, about this file and the sidebar.js file next to it, why would these be
> in a components/tabs/ folder? The name of the folder suggests that I'm going
> to find tabbar components in it, but then I see something about sidebars, it
> may be confusing to people looking at the code for the first time. I think
> we need to make sure we only put in devtools/client/shared/components/
> really generic and shareable things, with self-explanatory names. 
> This wrapper thing seems like a very inspector-specific requirement that
> would probably live better in the inspector folder.
> As for sidebar.js, it looks like it's more of a tabbar than a sidebar. There
> doesn't seem to be a reason why this should only be used in a side panel.
Done, sidebar renamed to tabbar.

> 
> ::: devtools/client/shared/components/tabs/sidebar.css
> @@ +2,5 @@
> > +/* This Source Code Form is subject to the terms of the Mozilla Public
> > + * License, v. 2.0. If a copy of the MPL was not distributed with this
> > + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> > +
> > +/******************************************************************************/
> 
> Why the long separator comment here? I don't see this used in other CSS
> files. Usually when I see them in the middle of files, it's a pretty clear
> sign that the file should be split up.
Removed


> 
> @@ +4,5 @@
> > + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> > +
> > +/******************************************************************************/
> > +
> > +.sidePanelFrame {
> 
> No camelCase for class names please. 
> .side-panel-frame instead
> Also, should this be a .devtools-side-panel-frame instead? I think most of
> our global/reusable/top-level classes come with the devtools prefix.
Probably yes, not done yet.

> 
> @@ +12,5 @@
> > +}
> > +
> > +.theme-dark .tabs .tabs-navigation,
> > +.theme-light .tabs .tabs-navigation {
> > +  box-shadow: none;
> 
> See my general comment about this.
So, this is fixed.

> 
> ::: devtools/client/shared/components/tabs/sidebar.js
> @@ +30,5 @@
> > +  },
> > +
> > +  // Public API
> > +
> > +  addTab: function (id, title, selected = false, panel, url) {
> 
> I haven't used react enough so far, but I'm not sure I've seen a lot of
> components that expose a public API. My understanding is that components
> rely on a state to change instead. Maybe the inspector should create redux
> store instead. It would contain only the state of the sidebar for now (but
> in the future could be used for the markup view too) and adding tabs,
> removing tabs and selecting tabs would happen with redux action creators
> instead.
Yes, exposing API from react components is not the standard. But in our case we are not using React as it's usually used for web apps. We are replacing bits and pieces in the existing code base and not having root react component that would wrap everything. I found this as a good compromise. Using Redux action creators sounds great to me and we should definitely do it at some point, but it sounds like overkill for this patch. Perhaps there is another solution? (btw. I used the same technique when implementing the NotificationBar and it nicely helped me to keep existing API and do not break the rest of the system).

> 
> ::: devtools/client/themes/computed.css
> @@ +9,5 @@
> >    flex-direction: column;
> >    width: 100%;
> >    /* Bug 1243598 - Reduce the container height by the tab height to make room
> >       for the tabs above. */
> > +  height: 100%;
> 
> I didn't really understand why this change was needed, but since it appears
> to work fine with it, please update the comment above that is now obsolete.
Comment removed.

> 
> ::: devtools/client/themes/fonts.css
> @@ +9,5 @@
> >    padding-bottom: 20px;
> >    width: 100%;
> >    /* Bug 1243598 - Reduce the container height by the tab height to make room
> >       for the tabs above. */
> > +  height: 100%;
> 
> Same comment here.
Comment removed.

> 
> ::: devtools/client/themes/inspector.css
> @@ +94,5 @@
> >    }
> >  }
> >  
> > +.devtools-sidebar-tabs {
> > +  height: 350px;
> 
> Why this hard-coded height? Does this really need to be here? I'm guessing
> this needs to match the default height of the bottom-docked toolbox. What
> happens if you resize it and then close/open the toolbox then? What happens
> if you have it docked to the side instead?
Leftover, removed.

> 
> I feel like we could avoid this here (since we're setting the height in JS
> anyway), but if not, then at the very least a comment is required to explain
> this magic number and explain what it should match.
> 
> ::: devtools/client/themes/rules.css
> @@ +33,5 @@
> >    flex-direction: column;
> >    width: 100%;
> >    /* Bug 1243598 - Reduce the container height by the tab height to make room
> >       for the tabs above. */
> > +  height: 100%;
> 
> Same comment as in computed.css
Removed

----

To summarize missing things:

* Support the left/right arrow keys to move focus
* All tabs menu (min width is set to 400px so, tabs are always visible atm)
* Use devtools-side- prefix for some CSS classes
* Fix the Animation panel that uses iframe.

Honza

Comment 11

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

Forgot to note:

> This wrapper thing seems like a very inspector-specific requirement that
> would probably live better in the inspector folder.
Agreed, I just can't load the component from devtools/client/inspector/shared/components since it isn't in list of BROWSER_BASED_DIRS defined by the BrowserLoader.

@James:
I am using BrowserLoader from the Toolbox. It uses 'useOnlyShared == true'. When I load a component from devtools/client/inspector/shared/components, I am getting error about React loaded twice since the browserRequire doesn't have this dir in BROWSER_BASED_DIRS and uses devtools.require() to load the component. What's the best to do? (it doesn't look correct to append the dir into BROWSER_BASED_DIRS)

Honza

Comment 12

[James Long (:jlongster)]

> @James:
> I am using BrowserLoader from the Toolbox. It uses 'useOnlyShared == true'.
> When I load a component from devtools/client/inspector/shared/components, I
> am getting error about React loaded twice since the browserRequire doesn't
> have this dir in BROWSER_BASED_DIRS and uses devtools.require() to load the
> component. What's the best to do? (it doesn't look correct to append the dir
> into BROWSER_BASED_DIRS)
> 
> Honza

The browser loader is a big hack until we figure out the real frontend loader story for devtools.html... so I'm ok with more hacks! But I do feel like if we keep adding entries to `BROWSER_BASED_DIRS` it's going to get huge.

What if we also automatically load browsers that match a regex with the browser loader? We could check for something like `client/.*/components`. That would allow tools to load files from there automatically with the browser loader.

Comment 13

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

(In reply to James Long (:jlongster) from comment #12)
> What if we also automatically load browsers that match a regex with the
> browser loader? We could check for something like `client/.*/components`.
> That would allow tools to load files from there automatically with the
> browser loader.
I like the idea. I filed bug 1280949 and attached a patch to it.

Honza

Comment 14

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

Attachment: bug1259819.patch

Patch updated

> * Fix the Animation panel that uses iframe.
Done

> * Support the left/right arrow keys to move focus
Done

> * All tabs menu (min width is set to 400px so, tabs are always visible atm)
I'd like to do this as a follow up if that's ok (this patch is already big)
It would be part of track #2 so, not forgotten.

> * Use devtools-side- prefix for some CSS classes
Done

@Patrick: one question, it looks like the Animation panel doesn't initialize properly. If I load e.g. google.com page I don't see any animations. I need to type something to see it. Do you have any tips what could be wrong here?

Note that you need patch from bug 1280949 to test this patch.

Honza

Comment 15

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

As bgrins suggested, I wanted to bring bug 1226877 to your attention. One of the problems with sidebars is that even though only one tabpanel is visible at any time, tabbable elements on other panels are still keyboard accessible. This should not be the case and tabpanels should be truly hidden (via visibility hidden or display none) from the user.

Comment 16

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

Attachment: bug1259819.patch

Comment 17

[Patrick Brosset <:pbro>]

(In reply to Jan Honza Odvarko [:Honza] from comment #14)
> Created attachment 8763655 [details] [diff] [review]
> bug1259819.patch
> 
> Patch updated
> 
> > * Fix the Animation panel that uses iframe.
> Done
Cool, it seems to work fine for me too.
> > * Support the left/right arrow keys to move focus
> Done
Thanks.
I do see a few inconsistencies with the current implementation still though: once one tab has focus, pressing TAB should jump to the next focusable widget inside the tab panel, not cycle through tabs.
Once any of the tab has focus, only LEFT/RIGHT should be used to switch through them, but pressing TAB on the rule-view tab for instance, should focus the rule-view search field.
> > * All tabs menu (min width is set to 400px so, tabs are always visible atm)
> I'd like to do this as a follow up if that's ok (this patch is already big)
> It would be part of track #2 so, not forgotten.
Fine by me.

> @Patrick: one question, it looks like the Animation panel doesn't initialize
> properly. If I load e.g. google.com page I don't see any animations. I need
> to type something to see it. Do you have any tips what could be wrong here?
I didn't notice anything wrong with the animation panel, it seems to work fine for me. What exactly were you expecting on google.com? I don't see any animations playing by default on that page when using Aurora either.

One more thing I noticed: It takes noticeably longer for the "computed-view" tab to be switched to. If you use the LEFT/RIGHT arrow to cycle through tabs, you can see a real speed difference when switching to the rule-view as opposed to switching to the computed-view. The computed-view takes almost 1s for me to get selected and be displayed, while all other tabs are instant.

Finally, this is a feedback I've made previously too, but I thought I'd reiterate because it is a pretty visible visual regression imo: when resizing the splitter, the sidebar container janks pretty hard. It doesn't resize smoothly as you move the splitter, and that makes the inspector feels slow.
Anything we could do about this?

Comment 18

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

(In reply to Yura Zenevich [:yzen] from comment #15)
> As bgrins suggested, I wanted to bring bug 1226877 to your attention. One of
> the problems with sidebars is that even though only one tabpanel is visible
> at any time, tabbable elements on other panels are still keyboard
> accessible. This should not be the case and tabpanels should be truly hidden
> (via visibility hidden or display none) from the user.
Thanks for the pointer!

The new (HTML based) patch uses display: none for panels that are not visible so, we should be fine. But could you please test it (using the attached patch)?

Honza

Comment 19

[Marco Zehe (:MarcoZ)]

One more question: By replacing the xul elements with HTML:div elements, we are losing a lot of semantic we got for free through XUL. So a div that looks like a tab list or tab, or tab panel, is no longer one because HTML doesn't have native tab panels. Just some CSS styling or class names don't make them tabs or the like. So, we need to make sure all the tablists, tabs, tabpanels, toolbars and other stuff that got us accessibility free for XUL, gets proper WAI-ARIA roles and states so we don't end up with unsemantic div sauce. I skimmed the patch and didn't see those added, which would significantly regress the side panel's accessibility once this lands. Am I missing something, or have these roles and states indeed not been added yet?

Comment 20

[Patrick Brosset <:pbro>]

Comment on attachment 8766696 [details] [diff] [review]
bug1259819.patch

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

Alright, this seems to work far better than when I did the first review. See my previous comment for some UX/perf/accessibility problems still.
Below are some comments about the code changes.
I have limited experience with React, so I didn't make many comments in the components, but I believe a few comments here are there are needed to make the code clearer.

::: devtools/client/inspector/components/side-panel.js
@@ +15,5 @@
> + * Side panel for the Inspector panel.
> + * This side panel is using an existing DOM node as a content.
> + */
> +var SidePanel = createClass({
> +  displayName: "SidePanel",

Iiuc, this component contains the content for a tab, right? So it doesn't really have anything to do with the fact that it's inside a sidebar in the inspector.
It happens to be a little special for the inspector, so I think it's fine to keep it insinde inspector/components/, but I think the file name side-panel.js and class and display names SidePanel aren't correct, since this component makes no assumption that it's inside a sidebar or anything.
Maybe InspectorTabPanel?

::: devtools/client/inspector/computed/computed.js
@@ +187,5 @@
>    this.searchField.addEventListener("input", this._onFilterStyles);
>    this.searchField.addEventListener("contextmenu",
>                                      this._onFilterTextboxContextMenu);
>    this.searchClearButton.addEventListener("click", this._onClearSearch);
> +  this.includeBrowserStylesCheckbox.addEventListener("click",

Shouldn't this be an `input` event instead of `click`, so it responds to keyboard events too?

::: devtools/client/inspector/inspector-panel.js
@@ +100,5 @@
>    this.onBeforeNewSelection = this.onBeforeNewSelection.bind(this);
>    this.onDetached = this.onDetached.bind(this);
>    this.onPaneToggleButtonClicked = this.onPaneToggleButtonClicked.bind(this);
>    this._onMarkupFrameLoad = this._onMarkupFrameLoad.bind(this);
> +  this.onResizeSidebar = this.onResizeSidebar.bind(this);

I think `onSidebarResize` would make more sense, but see below for another comment about the resize logic.

@@ +470,5 @@
> +    sidePaneContainer.width = Services.prefs.getIntPref(
> +      "devtools.toolsidebar-width.inspector");
> +  },
> +
> +  onResizeSidebar: function () {

This function triggers sync reflows and therefore explains why it feels so slow to move the splitter around.
I tested really quickly (probably missed edge cases) by just removing this function altogether and instead using 100% width and height in CSS on the devtools-sidebar-tabs element, and it seemed to fix the issue.
Basically, the whole sidebar is inside a vbox that has the right dimensions already, so I don't see why we would need to use JS to resize the sidePane at all, just use 100% CSS dimensions and that should make resizing much smoother.

@@ +1253,2 @@
>  
> +    // If the panel is showing, show the content immediatelly so,

s/immediatelly/immediately

::: devtools/client/inspector/inspector.css
@@ +4,5 @@
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
> +#inspector-sidebar-container {
> +  overflow: hidden;
> +  min-width: 400px;

iiuc this is the stop-gap to avoid having to deal with the all-tabs menu for now. If yes, then please add a comment in the code above this line explaining why we have this for the moment.
Also, maybe it's worth downloading a version of FF in German or French, or another language typically longer than English to see if 400px is a good minimum width.

@@ +34,5 @@
> +}
> +
> +/* Vertically center the 'Browser styles' checkbox in the
> +  Computed panel with its label. */
> +#browser-style-checkbox {

CSS related to the computed-view should go in devtools/client/themes/computed.css instead of here.
Also, instead of position:relative and a hard coded top offset, I would suggest:

display: flex;
align-items: center;

on the label container element

@@ +44,5 @@
> +#browser-style-checkbox-label {
> +  margin-right: 5px;
> +}
> +
> +#sidebar-panel-animationinspector {

This should go in the animation-panel's CSS file instead of here.
devtools/client/theme/animationinspector.css

::: devtools/client/inspector/toolsidebar.js
@@ +15,5 @@
> +Cu.import("resource://gre/modules/Task.jsm");
> +
> +/**
> + * This object represents replacement for ToolSidebar
> + * implemented in devtools/client/framework/sidebar.js module

Could you come up with a better comment here please? Readers will wonder why we're replacing it at all. 
This comment needs to explain why we have 2 implementations, and why this one is used in the inspector instead. What makes it different. And what is our migration path for the future.

@@ +30,5 @@
> +  try {
> +    this._width = Services.prefs.getIntPref("devtools.toolsidebar-width." +
> +      this._uid) || 300;
> +  } catch (err) {
> +  }

eslint disallows empty catch statements.
When you need one, you can add a comment in the catch that explains why we silence any errors.

@@ +224,5 @@
> +
> +  /**
> +   * Toggle sidebar's visibility state.
> +   */
> +  toggle: function () {

Can you remind me why we even have a toggle function here?
The sidebar in the inspector can be hidden/shown by using the collapse/expand button, and that is handled in inspector-panel.js. Why do we also need to hide/show it here too? Is it really needed?

::: devtools/client/locales/en-US/inspector.properties
@@ +345,5 @@
> +inspector.sidebar.ruleViewTitle=Rules
> +
> +# LOCALIZATION NOTE (inspector.sidebar.computedViewTitle):
> +# This is the title shown in a tab in the side panel of the Inspector panel
> +# that corresponds to the tool displaying the list of computed CSS vaules

s/vaules/values

::: devtools/client/shared/components/tabs/tabbar.css
@@ +3,5 @@
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +.theme-light .tabs .tabs-navigation,
> +.theme-dark .tabs .tabs-navigation {

Most (all?) rules in this CSS seem pretty generic, it seems like they are intended to apply to all themes. So, you should simplify the selectors, and everywhere you have:
.theme-light .something .here,
.theme-dark .something .here { ... }

Then write this instead:
.something .here { ... }

@@ +5,5 @@
> +
> +.theme-light .tabs .tabs-navigation,
> +.theme-dark .tabs .tabs-navigation {
> +  font-family: "Segoe UI";
> +  font-size: 12px;

I don't think we should have any font-family/size declarations here. Our font is set somewhere in the general theme files that are included in all tools, so I think we should just let it apply, and not override it anywhere.
I tried just disabling these 2 properties in the browser toolbox and it still looked consistent with the rest.

@@ +31,5 @@
> +  height: 23px;
> +}
> +
> +.theme-firebug .tabs .tabs-navigation {
> +  height: 24px;

How come the Firebug theme has 1 more pixel?

::: devtools/client/shared/components/tabs/tabbar.js
@@ +37,5 @@
> +      id: id,
> +      title: title,
> +      panel: panel,
> +      url: url,
> +    });

With ES6, this becomes:

tabs.push({id, title, panel, url});

@@ +139,5 @@
> +    return tab.panel;
> +  },
> +
> +  render: function () {
> +    let tabs = this.state.tabs.map(tab => {

In the browser toolbox, I see the following warning:
"Warning: Each child in an array or iterator should have a unique "key" prop. Check the render method of `Tabbar`

::: devtools/client/shared/components/tabs/tabs.css
@@ +9,5 @@
>    height: 100%;
>  }
>  
>  .tabs .tabs-navigation {
>    font-family: "Helvetica Neue",Helvetica,Arial,sans-serif;

Same comment regarding not defining fonts here.

@@ +34,3 @@
>  }
>  
> +.tabs .tab-panel-box {

I don't see this class being used in the DOM when I inspect the tabbar with the browser toolbox. Am I missing something?
I see 7 occurrences of it in CSS files in this patch, but I don't see it used anywhere in the markup.

::: devtools/client/shared/components/tabs/tabs.js
@@ +7,5 @@
>  "use strict";
>  
>  define(function (require, exports, module) {
>    const React = require("devtools/client/shared/vendor/react");
> +  const ReactDOM = require("devtools/client/shared/vendor/react-dom");

Having React, ReactDOM and DOM as globals here may be a bit confusing to new people. You're using ReactDOM only for the findDOMNode function, so maybe this would be clearer:

const React = require("devtools/client/shared/vendor/react");
const {DOM} = React;
const {findDOMNode} = require("devtools/client/shared/vendor/react-dom");

@@ +58,5 @@
>  
>      getInitialState: function () {
>        return {
> +        tabActive: this.props.tabActive,
> +        created: [],

We discussed about this on IRC because it wasn't clear to me what its purpose was.
I think you should add a comment above `created` that explains why it is used.
In particular, it should mention that because tab panels can contain iframes, we don't want them to be destroyed/recreated at every render, so they stay in the DOM as long as they have been created once, and are just hidden when needed.

And perhaps a better name for `created` would be `tabPanelsCreated`.

@@ +211,5 @@
> +          return tab;
> +        }).map((tab, index) => {
> +          let selected = selectedIndex == index;
> +          return (
> +            DOM.article({

Not sure why you chose an <article> element here rather than just a <div>. I don't really see the <article> adding any important semantics here.

::: devtools/client/themes/rules.css
@@ +31,5 @@
>    margin: 0;
>    display: flex;
>    flex-direction: column;
>    width: 100%;
>    /* Bug 1243598 - Reduce the container height by the tab height to make room

This comment is obsolete now. It should be removed.

Comment 21

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

(In reply to Marco Zehe (:MarcoZ) from comment #19)
> One more question: By replacing the xul elements with HTML:div elements, we
> are losing a lot of semantic we got for free through XUL. So a div that
> looks like a tab list or tab, or tab panel, is no longer one because HTML
> doesn't have native tab panels. Just some CSS styling or class names don't
> make them tabs or the like. So, we need to make sure all the tablists, tabs,
> tabpanels, toolbars and other stuff that got us accessibility free for XUL,
> gets proper WAI-ARIA roles and states so we don't end up with unsemantic div
> sauce. I skimmed the patch and didn't see those added, which would
> significantly regress the side panel's accessibility once this lands. Am I
> missing something, or have these roles and states indeed not been added yet?

Thanks for pointing that out. I believe semantics are missing from the patch, where we need to use some aria to get it back. ARIA has corresponding roles such as tab, tablist, tabpanel. There's also a way to connect tabs and tabpanels semantically using aria-controls.

Comment 22

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

Attachment: bug1259819.patch

(In reply to Patrick Brosset <:pbro> from comment #17)
> (In reply to Jan Honza Odvarko [:Honza] from comment #14)
> I do see a few inconsistencies with the current implementation still though:
> once one tab has focus, pressing TAB should jump to the next focusable
> widget inside the tab panel, not cycle through tabs.
> Once any of the tab has focus, only LEFT/RIGHT should be used to switch
> through them, but pressing TAB on the rule-view tab for instance, should
> focus the rule-view search field.
Should be fixed now.

> > > * All tabs menu (min width is set to 400px so, tabs are always visible atm)
> > I'd like to do this as a follow up if that's ok (this patch is already big)
> > It would be part of track #2 so, not forgotten.
> Fine by me.
Btw. it's bug 1281789

> > @Patrick: one question, it looks like the Animation panel doesn't initialize
> > properly. If I load e.g. google.com page I don't see any animations. I need
> > to type something to see it. Do you have any tips what could be wrong here?
> I didn't notice anything wrong with the animation panel, it seems to work
> fine for me. What exactly were you expecting on google.com? I don't see any
> animations playing by default on that page when using Aurora either.
OK, good.

> One more thing I noticed: It takes noticeably longer for the "computed-view"
> tab to be switched to. If you use the LEFT/RIGHT arrow to cycle through
> tabs, you can see a real speed difference when switching to the rule-view as
> opposed to switching to the computed-view. The computed-view takes almost 1s
> for me to get selected and be displayed, while all other tabs are instant.
I did improve it, please re-test.

> Finally, this is a feedback I've made previously too, but I thought I'd
> reiterate because it is a pretty visible visual regression imo: when
> resizing the splitter, the sidebar container janks pretty hard. It doesn't
> resize smoothly as you move the splitter, and that makes the inspector feels
> slow.
> Anything we could do about this?
Yeah, I know about this. I did some improvements to this too.

Honza

Comment 23

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

Ah forgot to mention, the patch depends on patch attached in bug 1266420.

Honza

Comment 24

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

Attachment: bug1259819.patch

Rebasing

Comment 25

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

(In reply to Jan Honza Odvarko [:Honza] from comment #23)
> Ah forgot to mention, the patch depends on patch attached in bug 1266420.
Bug 1266420 landed so, no other patch needed.

Honza

Comment 26

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

Attachment: bug1259819.patch

Bunch of test failures fixed on try.
(still some oranges though)

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

Honza

Comment 27

[Marco Zehe (:MarcoZ)]

Comment on attachment 8767976 [details] [diff] [review]
bug1259819.patch

I believe this is just an inter-diff to the previous patch. :-) And the patches do still not contain any WAI-ARIA roles and states.

Comment 28

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

Attachment: bug1259819.patch

(In reply to Marco Zehe (:MarcoZ) from comment #27)
> Comment on attachment 8767976 [details] [diff] [review]
> bug1259819.patch
> 
> I believe this is just an inter-diff to the previous patch. :-)
Ah, yes, new patch attached, thanks!

> And the
> patches do still not contain any WAI-ARIA roles and states.
Thanks for the pointer. This will be done as part of bug 1284509 (I just reported that)

Honza

Comment 29

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

Attachment: bug1259819.patch

Another patch update, more test failures fixed.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b4a1623c74b4&selectedJob=23377799
(looks a lot better!)


@Patrick: the current failures come mostly from the rules panel (devtools/client/inspector/rules/test/) e.g. devtools/client/inspector/rules/test/browser_rules_add-property-cancel_01.js

But, it passes on my machine. Any tips what could be the problem?

Honza

Comment 30

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

Attachment: bug1259819.patch

I think I fixed most of the failures (including the browser_rules_add-property-cancel_01.js test ), I am focusing on a mem-leak related to the Animation panel.

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

Honza

Comment 31

[Patrick Brosset <:pbro>]

Comment on attachment 8768435 [details] [diff] [review]
bug1259819.patch

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

Thanks for the new patch. Sorry for the delay.
As usual, first a few general comments:

- Keyboard handling seems to work great. Thanks.
- The performance problem I was seeing with the computed-view taking much longer to switch to than the other panels is now gone. Cool.
- Resizing the splitter seems to be jank much less than before. It's still not as smooth as with the current implementation though.
- The default size of the sidebar seems to be set to 689px for some reasons for me. Even when I change it, then close/reopen the toolbox, it forgets my custom size and auto resizes it to 689px (which I found out by looking at devtools.toolsidebar-width.inspector in about:config). The sidebar should remember the last size I resized it to and preserve this for the next times.
- There's an offset at the bottom of the rule-view and the computed-view. Something like 25px of empty margin space where the content of the rule-view and computed-view doesn't go. 
See: https://dl.dropboxusercontent.com/u/714210/sidebar-offset.png
- I've copied over a bunch of comments from my previous review because they haven't been addressed.

::: devtools/client/inspector/components/side-panel.js
@@ +14,5 @@
> +/**
> + * Side panel for the Inspector panel.
> + * This side panel is using an existing DOM node as a content.
> + */
> +var SidePanel = createClass({

I have made a comment in my last review about the name of this file and the name of this component as being a bit misleading.
All this component does is output a div with class tab-panel, and finds the right panel from the DOM and appends it.
So it makes no assumption about the fact that it's in a sidebar in the inspector. It's really just a helpful thing to deal with the fact that you've introduced react in this patch but we still have the markup for our panels in the inspector.xul document, so we need this little "hack" somehow to make it work.
So I would find a name that is self-explanatory, so it makes it easier for future maintainers to understand the goal here.
InspectorTabPanelContainer, or something like this.

Furthermore, the css class here is tab-panel, and looking at the DOM with the browser toolbox, I still see its parent having the same tab-panel class (the <article> element).
Do we need these 2 elements? You said in comment 10 that you had cleaned this up a bit, with tab-panel-box, but I don't see this class in the DOM. I just see 2 levels of tab-panel elements.

::: devtools/client/inspector/inspector-panel.js
@@ +438,5 @@
>      this.computedview = new ComputedViewTool(this, this.panelWin);
>      this.layoutview = new LayoutView(this, this.panelWin);
>  
>      if (this.target.form.animationsActor) {
> +      // xxxHonza: insertBefore: "fontinspector"

Oh right, I added this insertBefore logic 2 days ago.
But with your new API, it's far easier to deal with now anyway, no need for this insertBefore complexity anymore, you can just call addExistingTab and addFrameTab in the right order (with the font panel last) and that's it.
So I guess you can remove the comment.

@@ +479,5 @@
> +    let rect = sidePaneContainer.getBoundingClientRect();
> +
> +    let sidePane = this.panelDoc.querySelector(
> +      "#inspector-sidebar .devtools-sidebar-tabs");
> +    sidePane.style.height = rect.height + "px";

So, you've managed to remove setting the width in JS, which makes resizing smoother than it was, but I still see it being slower than with the current implementation. And I'm fairly sure that it's cause by accessing the DOM here, and setting the height on the sidePane which will trigger a reflow on resize, which is pretty bad for performance.

Did you investigate getting rid of this function altogether and just going with CSS?
If I remove the function, I get great performance, but the height is broken. I'm fairly sure this is addressable with CSS-only though.

I found something that seems to work for me:
position: relative; on the <vbox#inspector-sidebar-container>
position: absolute; top: 0; bottom: 0; on the <div#inspector-sidebar>
height: 100% on the <div.devtools-sidebar-tabs>

You already have height 100% on the other containers below that, so things should work fine.
With this in, I was able to remove the onResizeSidebar function completely and therefore get a much smoother resizing experience.

@@ +481,5 @@
> +    let sidePane = this.panelDoc.querySelector(
> +      "#inspector-sidebar .devtools-sidebar-tabs");
> +    sidePane.style.height = rect.height + "px";
> +
> +    this.sidebar._width = rect.width;

Why do you need to do this?
First of all, you shouldn't be accessing a private variable from here, and secondly: can't the sidebar deal with this on its own? It needs the know the current width for 1 reason: so that when it's hidden/destroyed, it can remember the width so it can restore it the next time it is shown. I believe it can do that on its own without having the inspector needing to tell it. It can just measure the width of the tab container, or something.

@@ +1250,2 @@
>  
> +    // If the panel is showing, show the content immediatelly so,

s/immediatelly/immediately

::: devtools/client/inspector/inspector.css
@@ +38,5 @@
>  }
> +
> +/* Vertically center the 'Browser styles' checkbox in the
> +  Computed panel with its label. */
> +#browser-style-checkbox {

Same comment as in my previous review:


CSS related to the computed-view should go in devtools/client/themes/computed.css instead of here.
Also, instead of position:relative and a hard coded top offset, I would suggest:

display: flex;
align-items: center;

on the label container element

@@ +48,5 @@
> +#browser-style-checkbox-label {
> +  margin-right: 5px;
> +}
> +
> +#sidebar-panel-animationinspector {

Same comment as in my previous review:

This should go in the animation-panel's CSS file instead of here.
devtools/client/theme/animationinspector.css

::: devtools/client/inspector/toolsidebar.js
@@ +15,5 @@
> +Cu.import("resource://gre/modules/Task.jsm");
> +
> +/**
> + * This object represents replacement for ToolSidebar
> + * implemented in devtools/client/framework/sidebar.js module

I had made a bunch of comments in my previous review that I don't see addressed here:

Could you come up with a better comment here please? Readers will wonder why we're replacing it at all. 
This comment needs to explain why we have 2 implementations, and why this one is used in the inspector instead. What makes it different. And what is our migration path for the future.

@@ +219,5 @@
> +
> +  /**
> +   * Toggle sidebar's visibility state.
> +   */
> +  toggle: function () {

I had a question about this in my previous review:

Can you remind me why we even have a toggle function here?
The sidebar in the inspector can be hidden/shown by using the collapse/expand button, and that is handled in inspector-panel.js. Why do we also need to hide/show it here too? Is it really needed?

::: devtools/client/locales/en-US/inspector.properties
@@ +345,5 @@
> +inspector.sidebar.ruleViewTitle=Rules
> +
> +# LOCALIZATION NOTE (inspector.sidebar.computedViewTitle):
> +# This is the title shown in a tab in the side panel of the Inspector panel
> +# that corresponds to the tool displaying the list of computed CSS vaules

s/vaules/values

::: devtools/client/shared/components/tabs/tabbar.css
@@ +3,5 @@
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +.theme-light .tabs .tabs-navigation,
> +.theme-dark .tabs .tabs-navigation {

Same comment as in my previous review:

Most (all?) rules in this CSS seem pretty generic, it seems like they are intended to apply to all themes. So, you should simplify the selectors, and everywhere you have:
.theme-light .something .here,
.theme-dark .something .here { ... }

Then write this instead:
.something .here { ... }

If/when we do need something different, then we'll use more specific selectors with the theme class, but for now, if they are the same rules, no need to use the theme class prefix.

@@ +5,5 @@
> +
> +.theme-light .tabs .tabs-navigation,
> +.theme-dark .tabs .tabs-navigation {
> +  font-family: "Segoe UI";
> +  font-size: 12px;

Also same comment as in my previous review:

I don't think we should have any font-family/size declarations here. Our font is set somewhere in the general theme files that are included in all tools, so I think we should just let it apply, and not override it anywhere.
I tried just disabling these 2 properties in the browser toolbox and it still looked consistent with the rest.

@@ +31,5 @@
> +  height: 23px;
> +}
> +
> +.theme-firebug .tabs .tabs-navigation {
> +  height: 24px;

How come the Firebug theme has 1 more pixel?

::: devtools/client/shared/components/tabs/tabbar.js
@@ +36,5 @@
> +    tabs.push({
> +      id: id,
> +      title: title,
> +      panel: panel,
> +      url: url,

Same as previous review:

With ES6, this becomes:
tabs.push({id, title, panel, url});

::: devtools/client/shared/components/tabs/tabs.css
@@ +9,5 @@
>    height: 100%;
>  }
>  
>  .tabs .tabs-navigation {
>    font-family: "Helvetica Neue",Helvetica,Arial,sans-serif;

Same comment regarding not defining fonts here.

@@ +34,3 @@
>  }
>  
> +.tabs .tab-panel-box {

As said previously, I don't see the tab-panel-box class being used in the DOM

::: devtools/client/shared/components/tabs/tabs.js
@@ +12,1 @@
>    const DOM = React.DOM;

Comment from my previous review:

const React = require("devtools/client/shared/vendor/react");
const {DOM} = React;
const {findDOMNode} = require("devtools/client/shared/vendor/react-dom");

@@ +58,5 @@
>  
>      getInitialState: function () {
>        return {
> +        tabActive: this.props.tabActive,
> +        created: [],

From my previous review:


We discussed about this on IRC because it wasn't clear to me what its purpose was.
I think you should add a comment above `created` that explains why it is used.
In particular, it should mention that because tab panels can contain iframes, we don't want them to be destroyed/recreated at every render, so they stay in the DOM as long as they have been created once, and are just hidden when needed.

And perhaps a better name for `created` would be `tabPanelsCreated`.

@@ +239,5 @@
> +            width: selected ? "100%" : "0",
> +          };
> +
> +          return (
> +            DOM.article({

Not sure why you chose an <article> element here rather than just a <div>. I don't really see the <article> adding any important semantics here.

::: devtools/client/themes/computed.css
@@ +9,5 @@
>    flex-direction: column;
>    width: 100%;
>    /* Bug 1243598 - Reduce the container height by the tab height to make room
>       for the tabs above. */
>    height: calc(100% - 24px);

Why did you go back to 100%-24px? This is what seems to be causing the bottom offset I mentioned in the general comments. In a previous patch, you had just 100%, and that worked fine for me.

::: devtools/client/themes/rules.css
@@ +33,5 @@
>    flex-direction: column;
>    width: 100%;
>    /* Bug 1243598 - Reduce the container height by the tab height to make room
>       for the tabs above. */
>    height: calc(100% - 24px);

Why did you go back to 100%-24px? This is what seems to be causing the bottom offset I mentioned in the general comments. In a previous patch, you had just 100%, and that worked fine for me.

Comment 32

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

Attachment: bug1259819.patch

(In reply to Patrick Brosset <:pbro> from comment #31)
> Comment on attachment 8768435 [details] [diff] [review]
> bug1259819.patch
> 
> Review of attachment 8768435 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Thanks for the new patch. Sorry for the delay.
> As usual, first a few general comments:
> 
> - Keyboard handling seems to work great. Thanks.
> - The performance problem I was seeing with the computed-view taking much
> longer to switch to than the other panels is now gone. Cool.
> - Resizing the splitter seems to be jank much less than before. It's still
> not as smooth as with the current implementation though.
> - The default size of the sidebar seems to be set to 689px for some reasons
> for me. Even when I change it, then close/reopen the toolbox, it forgets my
> custom size and auto resizes it to
Should be fixed now (see more below).

> 689px (which I found out by looking at
> devtools.toolsidebar-width.inspector in about:config). The sidebar should
> remember the last size I resized it to and preserve this for the next times.
> - There's an offset at the bottom of the rule-view and the computed-view.
> Something like 25px of empty margin space where the content of the rule-view
> and computed-view doesn't go. 
> See: https://dl.dropboxusercontent.com/u/714210/sidebar-offset.png
Should be fixed now

> - I've copied over a bunch of comments from my previous review because they
> haven't been addressed.
Ah, sorry, I somehow missed your comment #20, should be all resolved now.

> 
> ::: devtools/client/inspector/components/side-panel.js
> @@ +14,5 @@
> > +/**
> > + * Side panel for the Inspector panel.
> > + * This side panel is using an existing DOM node as a content.
> > + */
> > +var SidePanel = createClass({
> 
> I have made a comment in my last review about the name of this file and the
> name of this component as being a bit misleading.
Renamed to InspectorTabPanel.

> All this component does is output a div with class tab-panel, and finds the
> right panel from the DOM and appends it.
> So it makes no assumption about the fact that it's in a sidebar in the
> inspector. It's really just a helpful thing to deal with the fact that
> you've introduced react in this patch but we still have the markup for our
> panels in the inspector.xul document, so we need this little "hack" somehow
> to make it work.
> So I would find a name that is self-explanatory, so it makes it easier for
> future maintainers to understand the goal here.
> InspectorTabPanelContainer, or something like this.
> 
> Furthermore, the css class here is tab-panel, and looking at the DOM with
> the browser toolbox, I still see its parent having the same tab-panel class
> (the <article> element).
> Do we need these 2 elements? You said in comment 10 that you had cleaned
> this up a bit, with tab-panel-box, but I don't see this class in the DOM. I
> just see 2 levels of tab-panel elements.
tab-panel-box class removed and InspectorTabPanel() has its own class.

> 
> ::: devtools/client/inspector/inspector-panel.js
> @@ +438,5 @@
> >      this.computedview = new ComputedViewTool(this, this.panelWin);
> >      this.layoutview = new LayoutView(this, this.panelWin);
> >  
> >      if (this.target.form.animationsActor) {
> > +      // xxxHonza: insertBefore: "fontinspector"
> 
> Oh right, I added this insertBefore logic 2 days ago.
> But with your new API, it's far easier to deal with now anyway, no need for
> this insertBefore complexity anymore, you can just call addExistingTab and
> addFrameTab in the right order (with the font panel last) and that's it.
> So I guess you can remove the comment.
Done

> 
> @@ +479,5 @@
> > +    let rect = sidePaneContainer.getBoundingClientRect();
> > +
> > +    let sidePane = this.panelDoc.querySelector(
> > +      "#inspector-sidebar .devtools-sidebar-tabs");
> > +    sidePane.style.height = rect.height + "px";
> 
> So, you've managed to remove setting the width in JS, which makes resizing
> smoother than it was, but I still see it being slower than with the current
> implementation. And I'm fairly sure that it's cause by accessing the DOM
> here, and setting the height on the sidePane which will trigger a reflow on
> resize, which is pretty bad for performance.
> 
> Did you investigate getting rid of this function altogether and just going
> with CSS?
> If I remove the function, I get great performance, but the height is broken.
> I'm fairly sure this is addressable with CSS-only though.
> 
> I found something that seems to work for me:
> position: relative; on the <vbox#inspector-sidebar-container>
> position: absolute; top: 0; bottom: 0; on the <div#inspector-sidebar>
> height: 100% on the <div.devtools-sidebar-tabs>
Great catch, done.

> 
> You already have height 100% on the other containers below that, so things
> should work fine.
> With this in, I was able to remove the onResizeSidebar function completely
> and therefore get a much smoother resizing experience.
Totally agree.

> 
> @@ +481,5 @@
> > +    let sidePane = this.panelDoc.querySelector(
> > +      "#inspector-sidebar .devtools-sidebar-tabs");
> > +    sidePane.style.height = rect.height + "px";
> > +
> > +    this.sidebar._width = rect.width;
> 
> Why do you need to do this?
> First of all, you shouldn't be accessing a private variable from here, and
> secondly: can't the sidebar deal with this on its own? It needs the know the
> current width for 1 reason: so that when it's hidden/destroyed, it can
> remember the width so it can restore it the next time it is shown. I believe
> it can do that on its own without having the inspector needing to tell it.
> It can just measure the width of the tab container, or something.
So I refactored this. The logic is part of the InspectorPanel and here is copy
of a comment from the code:

  /**
   * Sidebar width is currently driven by vbox.inspector-sidebar-container
   * element, which is located at the left side of the side bar splitter.
   * It's width is changed by the splitter and stored into preferences.
   * As soon as bug 1260552 is fixed and new HTML based splitter in place
   * the width can be driven by div.inspector-sidebar element. This element
   * represents the ToolSidebar and so, the entire logic related to width
   * peristence can be done inside the ToolSidebar.
   */


> 
> @@ +1250,2 @@
> >  
> > +    // If the panel is showing, show the content immediatelly so,
> 
> s/immediatelly/immediately
Fixed

> 
> ::: devtools/client/inspector/inspector.css
> @@ +38,5 @@
> >  }
> > +
> > +/* Vertically center the 'Browser styles' checkbox in the
> > +  Computed panel with its label. */
> > +#browser-style-checkbox {
> 
> Same comment as in my previous review:
> 
> 
> CSS related to the computed-view should go in
> devtools/client/themes/computed.css instead of here.
> Also, instead of position:relative and a hard coded top offset, I would
> suggest:
> 
> display: flex;
> align-items: center;
> 
> on the label container element
Nice one, done.

> 
> @@ +48,5 @@
> > +#browser-style-checkbox-label {
> > +  margin-right: 5px;
> > +}
> > +
> > +#sidebar-panel-animationinspector {
> 
> Same comment as in my previous review:
> 
> This should go in the animation-panel's CSS file instead of here.
> devtools/client/theme/animationinspector.css
Done

> 
> ::: devtools/client/inspector/toolsidebar.js
> @@ +15,5 @@
> > +Cu.import("resource://gre/modules/Task.jsm");
> > +
> > +/**
> > + * This object represents replacement for ToolSidebar
> > + * implemented in devtools/client/framework/sidebar.js module
> 
> I had made a bunch of comments in my previous review that I don't see
> addressed here:
> 
> Could you come up with a better comment here please? Readers will wonder why
> we're replacing it at all. 
> This comment needs to explain why we have 2 implementations, and why this
> one is used in the inspector instead. What makes it different. And what is
> our migration path for the future.
Done

> 
> @@ +219,5 @@
> > +
> > +  /**
> > +   * Toggle sidebar's visibility state.
> > +   */
> > +  toggle: function () {
> 
> I had a question about this in my previous review:
> 
> Can you remind me why we even have a toggle function here?
> The sidebar in the inspector can be hidden/shown by using the
> collapse/expand button, and that is handled in inspector-panel.js. Why do we
> also need to hide/show it here too? Is it really needed?
Removed

> 
> ::: devtools/client/locales/en-US/inspector.properties
> @@ +345,5 @@
> > +inspector.sidebar.ruleViewTitle=Rules
> > +
> > +# LOCALIZATION NOTE (inspector.sidebar.computedViewTitle):
> > +# This is the title shown in a tab in the side panel of the Inspector panel
> > +# that corresponds to the tool displaying the list of computed CSS vaules
> 
> s/vaules/values
Fixed

> 
> ::: devtools/client/shared/components/tabs/tabbar.css
> @@ +3,5 @@
> > + * License, v. 2.0. If a copy of the MPL was not distributed with this
> > + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> > +
> > +.theme-light .tabs .tabs-navigation,
> > +.theme-dark .tabs .tabs-navigation {
> 
> Same comment as in my previous review:
> 
> Most (all?) rules in this CSS seem pretty generic, it seems like they are
> intended to apply to all themes. So, you should simplify the selectors, and
> everywhere you have:
> .theme-light .something .here,
> .theme-dark .something .here { ... }
> 
> Then write this instead:
> .something .here { ... }
> 
> If/when we do need something different, then we'll use more specific
> selectors with the theme class, but for now, if they are the same rules, no
> need to use the theme class prefix.
Yes, done. The only thing there is that toolbar & tabbars in Firebug theme are 24 px height (I don't know for sure why it needs one more pixel). We might want to file a follow up if we want to fix this (and make it 23px as light and dark). Also, tabs in Firebug theme are not stretched (not taking the entire horizontal space). This is also reflected in the CSS.


> 
> @@ +5,5 @@
> > +
> > +.theme-light .tabs .tabs-navigation,
> > +.theme-dark .tabs .tabs-navigation {
> > +  font-family: "Segoe UI";
> > +  font-size: 12px;
> 
> Also same comment as in my previous review:
> 
> I don't think we should have any font-family/size declarations here. Our
> font is set somewhere in the general theme files that are included in all
> tools, so I think we should just let it apply, and not override it anywhere.
> I tried just disabling these 2 properties in the browser toolbox and it
> still looked consistent with the rest.
Removed


> 
> @@ +31,5 @@
> > +  height: 23px;
> > +}
> > +
> > +.theme-firebug .tabs .tabs-navigation {
> > +  height: 24px;
> 
> How come the Firebug theme has 1 more pixel?
> 
> ::: devtools/client/shared/components/tabs/tabbar.js
> @@ +36,5 @@
> > +    tabs.push({
> > +      id: id,
> > +      title: title,
> > +      panel: panel,
> > +      url: url,
> 
> Same as previous review:
> 
> With ES6, this becomes:
> tabs.push({id, title, panel, url});
Done

> 
> ::: devtools/client/shared/components/tabs/tabs.css
> @@ +9,5 @@
> >    height: 100%;
> >  }
> >  
> >  .tabs .tabs-navigation {
> >    font-family: "Helvetica Neue",Helvetica,Arial,sans-serif;
> 
> Same comment regarding not defining fonts here.
Removed

> 
> @@ +34,3 @@
> >  }
> >  
> > +.tabs .tab-panel-box {
> 
> As said previously, I don't see the tab-panel-box class being used in the DOM
tab-panel-box removed

> 
> ::: devtools/client/shared/components/tabs/tabs.js
> @@ +12,1 @@
> >    const DOM = React.DOM;
> 
> Comment from my previous review:
> 
> const React = require("devtools/client/shared/vendor/react");
> const {DOM} = React;
> const {findDOMNode} = require("devtools/client/shared/vendor/react-dom");
Done

> 
> @@ +58,5 @@
> >  
> >      getInitialState: function () {
> >        return {
> > +        tabActive: this.props.tabActive,
> > +        created: [],
> 
> From my previous review:
> 
> 
> We discussed about this on IRC because it wasn't clear to me what its
> purpose was.
> I think you should add a comment above `created` that explains why it is
> used.
> In particular, it should mention that because tab panels can contain
> iframes, we don't want them to be destroyed/recreated at every render, so
> they stay in the DOM as long as they have been created once, and are just
> hidden when needed.
Done 

> 
> And perhaps a better name for `created` would be `tabPanelsCreated`.
I kept the name since it comes from the Accordion component (debugger.html) and it's consistent if we used the same name on both places.

> 
> @@ +239,5 @@
> > +            width: selected ? "100%" : "0",
> > +          };
> > +
> > +          return (
> > +            DOM.article({
> 
> Not sure why you chose an <article> element here rather than just a <div>. I
> don't really see the <article> adding any important semantics here.
True, div used now.

> 
> ::: devtools/client/themes/computed.css
> @@ +9,5 @@
> >    flex-direction: column;
> >    width: 100%;
> >    /* Bug 1243598 - Reduce the container height by the tab height to make room
> >       for the tabs above. */
> >    height: calc(100% - 24px);
> 
> Why did you go back to 100%-24px? This is what seems to be causing the
> bottom offset I mentioned in the general comments. In a previous patch, you
> had just 100%, and that worked fine for me.
True, removed again (I think it's related to absolute positioning).

Thanks Patrick for detailed review!

Honza

Comment 33

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

Attachment: bug1259819.patch

Yet forgot to include change related to ToolSidebar.toggle() method removal.

Honza

Comment 34

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

Try push:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e6bbe0972dd0&selectedJob=23558191

There are only oranges under dt4, (linux 64 debug), but I can't reproduce them locally. I tried Win, OSX, Linux (both debug/opt on Linux)

@Patrick do you see those on your machine?

Honza

Comment 35

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

Just managed to play around with the patch and wanted to give some suggestions regarding accessibility that is now missing since xul had it build in. Some things to watch for:

* .tabs-menu must have a role="tablist" attribute

* .tabs-menu-item must have a role="tab" attribute

* .tabs-menu-item must have an aria-controls attribute with a value referencing an id of the corresponding tab panel

* active .tabs-menu-item must have an aria-selected attribute set to "true" while the other ones have it set to "false"

* .tab-panel must have a role="tabpanel" attribute

* .tab-panel must have an aria-labelledby attribute with a value referencing an id of the corresponding tab element

For more details please see: 
https://www.w3.org/TR/2013/WD-wai-aria-practices-20130307/#tabpanel 
https://www.w3.org/TR/2013/WD-wai-aria-practices-20130307/#dualfocus

Comment 36

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

Attachment: bug1259819.patch

Patch rebased

This patch should also fix a mem leak (making sure that the animation inspector iframe is properly destroyed).

Here is the latest try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=5b972d69c227&selectedJob=23659239

I am seeing a lot of browser_dbg_listtabs-02.js failures, not sure if related.
devtools/client/debugger/test/mochitest/browser_dbg_listtabs-02.js

Honza

Comment 37

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

(In reply to Yura Zenevich [:yzen] from comment #35)
> Just managed to play around with the patch and wanted to give some
> suggestions regarding accessibility that is now missing since xul had it
> build in. Some things to watch for:
> 
> * .tabs-menu must have a role="tablist" attribute
> 
> * .tabs-menu-item must have a role="tab" attribute
> 
> * .tabs-menu-item must have an aria-controls attribute with a value
> referencing an id of the corresponding tab panel
> 
> * active .tabs-menu-item must have an aria-selected attribute set to "true"
> while the other ones have it set to "false"
> 
> * .tab-panel must have a role="tabpanel" attribute
> 
> * .tab-panel must have an aria-labelledby attribute with a value referencing
> an id of the corresponding tab element
> 
> For more details please see: 
> https://www.w3.org/TR/2013/WD-wai-aria-practices-20130307/#tabpanel 
> https://www.w3.org/TR/2013/WD-wai-aria-practices-20130307/#dualfocus
Thanks!

I filled bug 1286283 for this.

Honza

Comment 38

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

(In reply to Jan Honza Odvarko [:Honza] from comment #36)
> Created attachment 8770171 [details] [diff] [review]
> bug1259819.patch
> 
> Patch rebased
> 
> This patch should also fix a mem leak (making sure that the animation
> inspector iframe is properly destroyed).
> 
> Here is the latest try:
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=5b972d69c227&selectedJob=23659239
> 
> I am seeing a lot of browser_dbg_listtabs-02.js failures, not sure if
> related.
> devtools/client/debugger/test/mochitest/browser_dbg_listtabs-02.js
I have been running this test (Linux debug build, Win) and it fails for me even without the patch so, looks like not related.

Honza

Comment 39

[Patrick Brosset <:pbro>]

Comment on attachment 8770171 [details] [diff] [review]
bug1259819.patch

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

I see all of my comments addressed, and my manual tests didn't find anything weird.
The resizing perf is much better.
Thanks Honza for being patient with me and fixing this patch over and over again.
This is great work.
Now the next parts will be addressing the follow up bugs and migrating the splitter, which basically means migrating the whole inspector to HTML. That's going to be a tough one.
Also, I think we should start filing bugs for migrating the various panels in the inspector fully to React. Right now, with the approach chosen at the start of this bug, we have a mixed React/non-React environment, where React isn't really used to its fullest, and we shouldn't stay in this hybrid environment for too long.

::: devtools/client/inspector/toolsidebar.js
@@ +17,5 @@
> +/**
> + * This object represents replacement for ToolSidebar
> + * implemented in devtools/client/framework/sidebar.js module
> + *
> + * This new componet is part of the HTML Project aimed at

s/componet/component
Also, I think "devtools.html" is a better name than "the HTML Project", at least to me, it seems more generally recognized by people on and outside of the team.

Comment 40

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

Attachment: bug1259819.patch

(In reply to Patrick Brosset <:pbro> from comment #39)
> Comment on attachment 8770171 [details] [diff] [review]
> bug1259819.patch
> 
> Review of attachment 8770171 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I see all of my comments addressed, and my manual tests didn't find anything
> weird.
> The resizing perf is much better.
> Thanks Honza for being patient with me and fixing this patch over and over
> again.
Thank you for the reviews!

> This is great work.
> Now the next parts will be addressing the follow up bugs and migrating the
> splitter, which basically means migrating the whole inspector to HTML.
> That's going to be a tough one.
Yes, I am planning to work on the splitter now.

> Also, I think we should start filing bugs for migrating the various panels
> in the inspector fully to React. Right now, with the approach chosen at the
> start of this bug, we have a mixed React/non-React environment, where React
> isn't really used to its fullest, and we shouldn't stay in this hybrid
> environment for too long.
Absolutely agree.

> 
> ::: devtools/client/inspector/toolsidebar.js
> @@ +17,5 @@
> > +/**
> > + * This object represents replacement for ToolSidebar
> > + * implemented in devtools/client/framework/sidebar.js module
> > + *
> > + * This new componet is part of the HTML Project aimed at
> 
> s/componet/component
> Also, I think "devtools.html" is a better name than "the HTML Project", at
> least to me, it seems more generally recognized by people on and outside of
> the team.
Fixed.

I also introduced minor CSS change that fixes Tab() component usage in the HTTPi in the Console panel (Firebug theme)

So, it's ready to land I guess!

Honza

Comment 41

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

Attachment: bug1259819.patch

Patch rebased + fixing one className + three eslint errors fixed.

Try push:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=fbd7187ee14a&selectedJob=23866959

Honza

Comment 42

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/fx-team/rev/453c308dcab1
HTML Sidebar component; r=pbro

Comment 43

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Backed out for bustage: https://treeherder.mozilla.org/logviewer.html#?job_id=10560883&repo=fx-team

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

Comment 44

[Tim Nguyen :ntim]

(In reply to Wes Kocher (:KWierso) from comment #43)
> Backed out for bustage:
> https://treeherder.mozilla.org/logviewer.html#?job_id=10560883&repo=fx-team
> 
> https://hg.mozilla.org/integration/fx-team/rev/b6e311c419b5

Usually these debug-only timeouts are caused by a transition that runs very slowly on debug machines. What you might want to try here is to disable the transition only for this test.

Comment 45

[Tim Nguyen :ntim]

(The previous comment was addressed to Honza in case that wasn't clear)

Comment 46

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

Ah these is because of patch from bug 1273211 (it landed a two days ago).
Honza

Comment 47

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

Attachment: bug1259819.patch

Patch updated the test failure fixed (btw. it was test: devtools/client/inspector/test/browser_inspector_pane-toggle-05.js, introduced as part of bug 1273211). 

Honza

Comment 48

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/fx-team/rev/f249501590e6
HTML Sidebar component. r=pbro

Comment 49

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/f249501590e6

Comment 50

[Ada [:adalucinet]]

No new issues were found while verifying this fix with latest Nightly 50.0a1, under Windows 10 64-bit, Ubuntu 16.04 64-bit and Mac OS X 10.9.5