1266420 - Create an HTML replacement for Sidebar toggle button

Status: VERIFIED FIXED

(DevTools :: General, enhancement, P1)

Description

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

Sidebar toggle button appears in panel's toolbar and its responsibility is to show and hide the side bar. This button is currently implemented in XUL and we want to use HTML instead.

This is an adept for React component that could be reused across all panels (those that have a sidebar).

Honza

Comment 1

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

Attachment: bug1266420.patch

I tried to implement the button as a simple React component, patch attached.

- The component maintains a simple boolean state expanded/collapsed.
- The component lives within inspector dir, but as soon as it's being used in other panels, we can move it into shared dir.

Honza

Comment 2

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

Forgot to mention, the patch from bug 1259819 is needed.

Honza

Comment 3

[Brian Grinstead [:bgrins]]

Comment on attachment 8764482 [details] [diff] [review]
bug1266420.patch

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

::: devtools/client/inspector/components/sidebar-toggle.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/. */
> +
> +#inspector-pane-toggle-box {

I don't think we should add a new style sheet for this.  It's already relying on styles in inspector.css, let's just add the line-height rule on #inspector-pane-toggle next to those.  If/when we use this as a shared component let's look at making those styles shared (either through a common css file or it's own css file).

::: devtools/client/inspector/components/sidebar-toggle.js
@@ +7,5 @@
> +"use strict";
> +
> +const { DOM, createClass, PropTypes } = require("devtools/client/shared/vendor/react");
> +
> +loader.lazyRequireGetter(this, "ViewHelpers", "devtools/client/shared/widgets/view-helpers", true);

ViewHelpers is unused

@@ +10,5 @@
> +
> +loader.lazyRequireGetter(this, "ViewHelpers", "devtools/client/shared/widgets/view-helpers", true);
> +
> +// Shortcuts
> +const { button } = DOM;

That's only used once, DOM.button() seems fine, but I don't care either way

@@ +16,5 @@
> +/**
> + * Sidebar toggle button. This button is used to exapand
> + * and collapse Sidebar.
> + */
> +var SidebarToggle = createClass({

Can you add a small test for this component?

@@ +51,5 @@
> +   */
> +  updateCollapseAttribute: function () {
> +    let node = this.refs.button;
> +    if (this.state.collapsed) {
> +      node.setAttribute("pane-collapsed", "");

Can we make this thing be a class instead (and update all references across devtools/client that use it)?

Comment 4

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

Attachment: bug1266420.patch

(In reply to Brian Grinstead [:bgrins] from comment #3)
> Comment on attachment 8764482 [details] [diff] [review]
> bug1266420.patch
> 
> Review of attachment 8764482 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: devtools/client/inspector/components/sidebar-toggle.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/. */
> > +
> > +#inspector-pane-toggle-box {
> 
> I don't think we should add a new style sheet for this.  It's already
> relying on styles in inspector.css, let's just add the line-height rule on
> #inspector-pane-toggle next to those.  If/when we use this as a shared
> component let's look at making those styles shared (either through a common
> css file or it's own css file).
Done

> 
> ::: devtools/client/inspector/components/sidebar-toggle.js
> @@ +7,5 @@
> > +"use strict";
> > +
> > +const { DOM, createClass, PropTypes } = require("devtools/client/shared/vendor/react");
> > +
> > +loader.lazyRequireGetter(this, "ViewHelpers", "devtools/client/shared/widgets/view-helpers", true);
> 
> ViewHelpers is unused
Removed

> 
> @@ +10,5 @@
> > +
> > +loader.lazyRequireGetter(this, "ViewHelpers", "devtools/client/shared/widgets/view-helpers", true);
> > +
> > +// Shortcuts
> > +const { button } = DOM;
> 
> That's only used once, DOM.button() seems fine, but I don't care either way
OK, I kept it.

> 
> @@ +16,5 @@
> > +/**
> > + * Sidebar toggle button. This button is used to exapand
> > + * and collapse Sidebar.
> > + */
> > +var SidebarToggle = createClass({
> 
> Can you add a small test for this component?
Done, a mochitest is part of the patch

> 
> @@ +51,5 @@
> > +   */
> > +  updateCollapseAttribute: function () {
> > +    let node = this.refs.button;
> > +    if (this.state.collapsed) {
> > +      node.setAttribute("pane-collapsed", "");
> 
> Can we make this thing be a class instead (and update all references across
> devtools/client that use it)?
Good point, done.

Thanks!
Honza

Comment 5

[Brian Grinstead [:bgrins]]

Comment on attachment 8765949 [details] [diff] [review]
bug1266420.patch

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

So there's some code in the shared widgets.css referring to [pane-collapsed], which makes me think it'd be best if we went through and updated all references in devtools to consistently use the class instead of the attribute.  I know that's a pain, but I think it's the only way we'll be consistent and it's generally best this way.

Fine to do that in a second patch, but please do it in this bug since I think it might actually cause a layout issue in the inspector due to: .devtools-responsive-container > .devtools-sidebar-tabs:not([pane-collapsed])

Also FYI this patch doesn't apply locally

::: devtools/client/inspector/inspector-panel.js
@@ +1209,5 @@
>     */
>    onPaneToggleButtonClicked: function (e) {
>      let sidePaneContainer = this.panelDoc.querySelector("#inspector-sidebar-container");
> +    let button = this.panelDoc.querySelector("#inspector-pane-toggle");
> +    let isVisible = !button.classList.contains("pane-collapsed");

Seems like it'd be better to query this._sidebarToggle state than the DOM it builds?

Comment 6

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

Attachment: bug1266420.patch

(In reply to Brian Grinstead [:bgrins] from comment #5)
> Comment on attachment 8765949 [details] [diff] [review]
> bug1266420.patch
> 
> Review of attachment 8765949 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> So there's some code in the shared widgets.css referring to
> [pane-collapsed], which makes me think it'd be best if we went through and
> updated all references in devtools to consistently use the class instead of
> the attribute.  I know that's a pain, but I think it's the only way we'll be
> consistent and it's generally best this way.
> 
> Fine to do that in a second patch, but please do it in this bug since I
> think it might actually cause a layout issue in the inspector due to:
> .devtools-responsive-container > .devtools-sidebar-tabs:not([pane-collapsed])
Yeah, this touches even other panels, but I agree it's better, done

> 
> Also FYI this patch doesn't apply locally
Note that you need patch from bug 1259819

> 
> ::: devtools/client/inspector/inspector-panel.js
> @@ +1209,5 @@
> >     */
> >    onPaneToggleButtonClicked: function (e) {
> >      let sidePaneContainer = this.panelDoc.querySelector("#inspector-sidebar-container");
> > +    let button = this.panelDoc.querySelector("#inspector-pane-toggle");
> > +    let isVisible = !button.classList.contains("pane-collapsed");
> 
> Seems like it'd be better to query this._sidebarToggle state than the DOM it
> builds?
Yes, good point, done.

Honza

Comment 7

[Brian Grinstead [:bgrins]]

(In reply to Jan Honza Odvarko [:Honza] from comment #6)
> > Also FYI this patch doesn't apply locally
> Note that you need patch from bug 1259819

You might want to reverse that dependency since this is pretty much ready to land, but up to you

Comment 8

[Brian Grinstead [:bgrins]]

Comment on attachment 8766302 [details] [diff] [review]
bug1266420.patch

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

The code all looks good, but I've still not been able to test locally due to: unable to find 'devtools/client/inspector/components/moz.build' for patching (even with the dependency applied).

::: devtools/client/inspector/inspector-panel.js
@@ +491,5 @@
>     * Add the expand/collapse behavior for the sidebar panel.
>     */
>    setupSidebarToggle: function () {
> +    let SidebarToggle = this.React.createFactory(this.browserRequire(
> +      "devtools/client/inspector/components/sidebar-toggle"));

I feel like we should go ahead and make this a shared component now.  There are other places that'll need this and that will cause less churn / new files in the inspector folder and easier history traversal.

Comment 9

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

Attachment: bug1266420.patch

(In reply to Brian Grinstead [:bgrins] from comment #8)
> Comment on attachment 8766302 [details] [diff] [review]
> bug1266420.patch
> 
> Review of attachment 8766302 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> The code all looks good, but I've still not been able to test locally due
> to: unable to find 'devtools/client/inspector/components/moz.build' for
> patching (even with the dependency applied).
Fixed, this patch no longer depends on the one from bug 1259819.

> 
> ::: devtools/client/inspector/inspector-panel.js
> @@ +491,5 @@
> >     * Add the expand/collapse behavior for the sidebar panel.
> >     */
> >    setupSidebarToggle: function () {
> > +    let SidebarToggle = this.React.createFactory(this.browserRequire(
> > +      "devtools/client/inspector/components/sidebar-toggle"));
> 
> I feel like we should go ahead and make this a shared component now.  There
> are other places that'll need this and that will cause less churn / new
> files in the inspector folder and easier history traversal.
Yes, agree. So, I also reintroduced sidebar-toggle.css and put all related styles into it.

Updated patch attached.

Honza

Comment 10

[Brian Grinstead [:bgrins]]

Comment on attachment 8766693 [details] [diff] [review]
bug1266420.patch

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

::: devtools/client/shared/components/sidebar-toggle.css
@@ +1,1 @@
> +/* vim:set ts=2 sw=2 sts=2 et: */

I guess this is fine for now but we should also discuss the CSS loading strategy - I don't think adding a new CSS link to each tool that wants to use a component is great

@@ +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/. */
> +
> +#inspector-pane-toggle-box {

Could you use display: flex instead of the hardcoded value?  Also, since this is not part of the component this should move back into the inspector CSS file if it's required.

::: devtools/client/shared/components/sidebar-toggle.js
@@ +55,5 @@
> +
> +    return (
> +      button({
> +        ref: "button",
> +        id: "inspector-pane-toggle",

This shouldn't be called inspector-pane-toggle (maybe sidebar-toggle), and it should be a class instead of an ID so it could have more than one per page.  The the css names also should be updated to match

Comment 11

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

Attachment: bug1266420.patch

(In reply to Brian Grinstead [:bgrins] from comment #10)
> Comment on attachment 8766693 [details] [diff] [review]
> bug1266420.patch
> 
> Review of attachment 8766693 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: devtools/client/shared/components/sidebar-toggle.css
> @@ +1,1 @@
> > +/* vim:set ts=2 sw=2 sts=2 et: */
> I guess this is fine for now but we should also discuss the CSS loading
> strategy - I don't think adding a new CSS link to each tool that wants to
> use a component is great
Totally agree. Do we have a bug for this?

> 
> @@ +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/. */
> > +
> > +#inspector-pane-toggle-box {
> 
> Could you use display: flex instead of the hardcoded value?
I reset the line-height (which is set in toolbars.css) using 'initial'
and set display:block on the toggle-button, which centers it properly.

> Also, since
> this is not part of the component this should move back into the inspector
> CSS file if it's required.
Done

> ::: devtools/client/shared/components/sidebar-toggle.js
> @@ +55,5 @@
> > +
> > +    return (
> > +      button({
> > +        ref: "button",
> > +        id: "inspector-pane-toggle",
> 
> This shouldn't be called inspector-pane-toggle (maybe sidebar-toggle), and
> it should be a class instead of an ID so it could have more than one per
> page.  The the css names also should be updated to match
Done


Honza

Comment 12

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

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

Honza

Comment 13

[Brian Grinstead [:bgrins]]

Comment on attachment 8767077 [details] [diff] [review]
bug1266420.patch

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

This needs rebasing

Comment 14

[Brian Grinstead [:bgrins]]

Comment on attachment 8767077 [details] [diff] [review]
bug1266420.patch

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

::: devtools/client/shared/components/sidebar-toggle.js
@@ +54,5 @@
> +    }
> +
> +    return (
> +      button({
> +        ref: "button",

Remove the ref now that it's not used

@@ +57,5 @@
> +      button({
> +        ref: "button",
> +        className: classNames.join(" "),
> +        title: title,
> +        tabindex: 0,

AFAICT having tabindex 0 and not having a tab index is the same thing for focusable elements.  I know this is how it was in the markup too, but just want to double check with Yura to see if we can remove this line.  Feel free to land in the mean time and we can do a follow up if needed.

Comment 15

[Brian Grinstead [:bgrins]]

(In reply to Brian Grinstead [:bgrins] from comment #14)
> @@ +57,5 @@
> > +      button({
> > +        ref: "button",
> > +        className: classNames.join(" "),
> > +        title: title,
> > +        tabindex: 0,
> 
> AFAICT having tabindex 0 and not having a tab index is the same thing for
> focusable elements.  I know this is how it was in the markup too, but just
> want to double check with Yura to see if we can remove this line.  Feel free
> to land in the mean time and we can do a follow up if needed.

Setting needinfo flag

Comment 16

[Marco Zehe (:MarcoZ)]

(In reply to Brian Grinstead [:bgrins] from comment #14)
> Comment on attachment 8767077 [details] [diff] [review]
> bug1266420.patch
> 
> Review of attachment 8767077 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: devtools/client/shared/components/sidebar-toggle.js
> @@ +57,5 @@
> > +      button({
> > +        ref: "button",
> > +        className: classNames.join(" "),
> > +        title: title,
> > +        tabindex: 0,
> 
> AFAICT having tabindex 0 and not having a tab index is the same thing for
> focusable elements.  I know this is how it was in the markup too, but just
> want to double check with Yura to see if we can remove this line.  Feel free
> to land in the mean time and we can do a follow up if needed.
Stealing this NI because I just stumbled upon it: Yes this line can be removed. Buttons are focusable by default and don't need tabindex.="0".

Comment 17

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

Attachment: bug1266420.patch

(In reply to Brian Grinstead [:bgrins] from comment #14)
> ::: devtools/client/shared/components/sidebar-toggle.js
> @@ +54,5 @@
> > +    }
> > +
> > +    return (
> > +      button({
> > +        ref: "button",
> 
> Remove the ref now that it's not used
Done

(In reply to Marco Zehe (:MarcoZ) from comment #16)
> > AFAICT having tabindex 0 and not having a tab index is the same thing for
> > focusable elements.  I know this is how it was in the markup too, but just
> > want to double check with Yura to see if we can remove this line.  Feel free
> > to land in the mean time and we can do a follow up if needed.
> Stealing this NI because I just stumbled upon it: Yes this line can be
> removed. Buttons are focusable by default and don't need tabindex.="0".
Removed 

Thanks!

Honza

Comment 18

[Carsten Book [:Tomcat]]

failed to apply:

renamed 1266420 -> bug1266420.patch
applying bug1266420.patch
patching file devtools/client/shared/components/test/mochitest/chrome.ini
Hunk #1 FAILED at 0
1 out of 1 hunks FAILED -- saving rejects to file devtools/client/shared/components/test/mochitest/chrome.ini.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working directory
errors during apply, please fix and qrefresh bug1266420.patch
Tomcats-MacBook-Pro-2:fx-team Tomcat$

Comment 19

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

Attachment: bug1266420.patch

Rebased on the current HEAD.
Honza

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/fx-team/rev/e27437d7fe35
Implement SidebarToggle component; r=bgrins

Comment 21

[Carsten Book [:Tomcat]]

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

Comment 22

[Ada [:adalucinet]]

Verified fixed using latest Nightly 50.0a1, across platforms [1]. 
Found 1 issue specific to RTL builds - bug 1285528. 
Based on the above results, marking here accordingly.

[1] Windows 10 64-bit, Mac OS X 10.11.1 and Ubuntu 16.04 64-bit