Create an HTML replacement for Sidebar toggle button

VERIFIED FIXED in Firefox 50

Status

enhancement
P1
normal
VERIFIED FIXED
3 years ago
Last year

People

(Reporter: Honza, Assigned: Honza)

Tracking

unspecified
Firefox 50
Dependency tree / graph

Firefox Tracking Flags

(firefox50 verified)

Details

(Whiteboard: [devtools-html])

Attachments

(1 attachment, 6 obsolete attachments)

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
Severity: normal → enhancement
Whiteboard: [devtools-html]
Flags: qe-verify+
Priority: -- → P1
QA Contact: alexandra.lucinet
Assignee: nobody → mratcliffe
Status: NEW → ASSIGNED
Iteration: --- → 49.1 - May 9
Iteration: 49.1 - May 9 → 49.2 - May 23
Iteration: 49.2 - May 23 → 49.3 - Jun 6
Iteration: 49.3 - Jun 6 → ---
Priority: P1 → P2
Assignee: mratcliffe → nobody
Status: ASSIGNED → NEW
Assignee: nobody → odvarko
Status: NEW → ASSIGNED
Iteration: --- → 50.2
Priority: P2 → P1
Posted patch bug1266420.patch (obsolete) — Splinter Review
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
Attachment #8764482 - Flags: review?(bgrinstead)
Forgot to mention, the patch from bug 1259819 is needed.

Honza
Depends on: 1259819
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)?
Attachment #8764482 - Flags: review?(bgrinstead)
Posted patch bug1266420.patch (obsolete) — Splinter Review
(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
Attachment #8764482 - Attachment is obsolete: true
Attachment #8765949 - Flags: review?(bgrinstead)
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?
Attachment #8765949 - Flags: review?(bgrinstead)
Posted patch bug1266420.patch (obsolete) — Splinter Review
(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
Attachment #8765949 - Attachment is obsolete: true
Attachment #8766302 - Flags: review?(bgrinstead)
(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 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.
Attachment #8766302 - Flags: review?(bgrinstead)
Posted patch bug1266420.patch (obsolete) — Splinter Review
(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
Attachment #8766302 - Attachment is obsolete: true
Attachment #8766693 - Flags: review?(bgrinstead)
No longer depends on: 1259819
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
Attachment #8766693 - Flags: review?(bgrinstead)
Posted patch bug1266420.patch (obsolete) — Splinter Review
(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
Attachment #8766693 - Attachment is obsolete: true
Attachment #8767077 - Flags: review?(bgrinstead)
Comment on attachment 8767077 [details] [diff] [review]
bug1266420.patch

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

This needs rebasing
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.
Attachment #8767077 - Flags: review?(bgrinstead) → review+
(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
Flags: needinfo?(yzenevich)
(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".
Flags: needinfo?(yzenevich)
Posted patch bug1266420.patch (obsolete) — Splinter Review
(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
Attachment #8767077 - Attachment is obsolete: true
Attachment #8767663 - Flags: review+
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$
Flags: needinfo?(odvarko)
Keywords: checkin-needed
Rebased on the current HEAD.
Honza
Attachment #8767663 - Attachment is obsolete: true
Flags: needinfo?(odvarko)
Attachment #8767688 - Flags: review+

Comment 20

3 years ago
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/fx-team/rev/e27437d7fe35
Implement SidebarToggle component; r=bgrins
Keywords: checkin-needed

Comment 21

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/e27437d7fe35
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 50

Updated

3 years ago
Depends on: 1285449
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
Status: RESOLVED → VERIFIED
QA Whiteboard: [qe-dthtml]
Flags: qe-verify+

Updated

Last year
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.