1308227 - Add a CSS Grid accordion container to the layout panel

Status: RESOLVED FIXED

(DevTools :: Inspector, defect, P3)

Description

[Gabriel Luong [:gl] (ΦωΦ)]

The purpose of this bug is to do the following:
(1) Add an accordion react component to layout/components/. I think we should reuse the accordion component and its styles that are already available in debugger.html. Since we don't have a proper way of sharing these widgets right now, it is ok to simply copy and duplicate it into layout/components/ for the time being until we figure out how to make these widgets more reusable throughout the entire devtools.
(2) Use the debugger.html accordion component to add an accordion into the layout panel with the Grid heading.

Comment 1

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: 1308227.patch [1.0]

This adds the Accordion component from debugger.html project into inspector/components. It is assumed we would remove this once we have a better way to reuse and share components amongst the projects. There are some slight modifications such as not being able to use the svg-inline-react component to inline svg, and reusing theme-twisty to get the arrow. 

This also adds l10n support and a layout.css for the layout view.

Comment 2

[Tim Nguyen :ntim]

Comment on attachment 8800104 [details] [diff] [review]
1308227.patch [1.0]

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

A few comments on the CSS, everything else looks great!

::: devtools/client/inspector/components/Accordion.css
@@ +6,5 @@
> +  background-color: var(--theme-body-background);
> +  width: 100%;
> +}
> +
> +.accordion ._header {

I don't understand why we're using an underscore in front of header

@@ +10,5 @@
> +.accordion ._header {
> +  background-color: var(--theme-toolbar-background);
> +  border-bottom: 1px solid var(--theme-splitter-color);
> +  cursor: pointer;
> +  font-size: 11px;

font-size: 11px is basically hardcoding the OSX native font-size, which likely won't work well on other platforms.
Can this be removed?

@@ +18,5 @@
> +
> +  -webkit-user-select: none;
> +  -moz-user-select: none;
> +  -ms-user-select: none;
> +  -o-user-select: none;

According to MDN, -o-user-select was never supported. Edge and Firefox support -webkit-user-select.
I'm not sure if we need to support chrome like debugger.html, but if we do, I would only keep -webkit-, -moz- and unprefixed. If we don't, I'd only keep -moz-.

@@ +23,5 @@
> +  user-select: none;
> +}
> +
> +.accordion ._header:hover {
> +  background-color: var(--theme-selection-color);

This is not a good choice for contrast in dark theme. --theme-selection-color is white, and the dark theme text color is white as well.

How about --theme-selection-background-semitransparent or --theme-tab-toolbar-hover  ?

@@ +27,5 @@
> +  background-color: var(--theme-selection-color);
> +}
> +
> +.accordion ._header:hover svg {
> +  fill: var(--theme-comment-alt);

We don't need this.

@@ +30,5 @@
> +.accordion ._header:hover svg {
> +  fill: var(--theme-comment-alt);
> +}
> +
> +.accordion ._content {

Why the underscore?

@@ +32,5 @@
> +}
> +
> +.accordion ._content {
> +  border-bottom: 1px solid var(--theme-splitter-color);
> +  font-size: 11px;

For the same reason as above, I would remove font-size: 11px

Comment 3

[Julian Descottes [:jdescottes]]

Comment on attachment 8800104 [details] [diff] [review]
1308227.patch [1.0]

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

Gabriel: Patch seems to work great (after adding the missing files :) ) but I need more time to review the React parts as I'm not super familiar with.
I'll finish the review tomorrow, but could you update your patch with the missing utils/ folder and properties file?

Thanks!

::: devtools/client/inspector/components/Accordion.css
@@ +13,5 @@
> +  cursor: pointer;
> +  font-size: 11px;
> +  padding: 5px;
> +  transition: all 0.25s ease;
> +  width: 100%;

The padding 5px + width 100% makes the header overflow and forces an horizontal scrollbar. box-sizing: border-box?

::: devtools/client/inspector/layout/components/Grid.js
@@ +8,5 @@
> +const { DOM: dom, createClass } = require("devtools/client/shared/vendor/react");
> +
> +const Grid = createClass({
> +
> +  displayName: "App",

displayName: "Grid"?

::: devtools/client/inspector/layout/moz.build
@@ +6,5 @@
>  DIRS += [
>      'actions',
>      'components',
>      'reducers',
> +    'utils',

The utils folder is missing from the patch.

From reading the patch I assume it contains a l10n.js which uses a new properties file (also missing). Mocked it for my current review.

Comment 4

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: 1308227.patch [2.0]

Comment 5

[Gabriel Luong [:gl] (ΦωΦ)]

I have moved Accordion.css and Accordion.js into layout/components. This is a duplicate of what is upstream in the debugger.html project without using inline Svg and replacing the arrow with the theme-twisty. This is merely a stop gap until we figure out how to share these core widgets within our tools, and I don't want this to block the grid inspector. The decision to move it into layout/components instead of shared/ is to really make it clear that this is not permanent and will be removed once we have a better solution.

That said Julian we should probably arrange a vidyo to talk about how to make the inspector be able to consume a webpack bundle that may contain these devtool core widgets like the Accordion as part of devtools.html. I would assume the current style problems in Accordion.css is also a problem with the current  debugger.html and will be addressed upstream.

Comment 6

[Gabriel Luong [:gl] (ΦωΦ)]

It is a great pain since we don't have a sane way to share these components yet, but this is probably the best work around for the time being, and working on sharing these components are an immediate concern that we should address this quarter.

Comment 7

[Julian Descottes [:jdescottes]]

(In reply to Gabriel Luong [:gl][1 biz day review guarantee] (ΦωΦ) from comment #5)
> I have moved Accordion.css and Accordion.js into layout/components. This is
> a duplicate of what is upstream in the debugger.html project without using
> inline Svg and replacing the arrow with the theme-twisty. This is merely a
> stop gap until we figure out how to share these core widgets within our
> tools, and I don't want this to block the grid inspector. The decision to
> move it into layout/components instead of shared/ is to really make it clear
> that this is not permanent and will be removed once we have a better
> solution.
> 

Sorry I missed the fact that this was code taken from debugger.html and went straight to review the code :)

We need to discuss:
- short term: how to share the Accordion widget with debugger.html (do we resynchronize, do we fork and forget etc...)
- long term: how to reliably share widgets with debugger.html

Short term is important because if we're temporarily taking the ownership on this code, then we can't say "it will be fixed upstream" (and all the review comments on Accordion.css should be considered then). 

Or we say it's an "import" from debugger.html. In this case we need to isolate this component a bit (layout/components/accordion/ ? client/shared/component/accordion/ ?) and have an "UPGRADING" information. Or simply we could have a comment on top of each imported file that explains that the file was taken form debugger.html etc...

Just the minimum so that if someone sees an issue with the Accordion, it's clear if the fix should be contributed in mc or in debugger.html. 

I know this is just temporary, but we never know how long "temporary" ends up to be so we should still be careful. 

> That said Julian we should probably arrange a vidyo to talk about how to
> make the inspector be able to consume a webpack bundle that may contain
> these devtool core widgets like the Accordion as part of devtools.html. I

Yes, we need to agree how we share React widgets with debugger.html. I feel like Jason is the person to talk to about this right now. Let's check if anything has already been discussed on this topic, and if needed we'll setup a meeting.

Related to this, we had a similar case for the splitter: initially taken from debugger.html, then enhanced for the inspector use case and reintegrated in debugger.html [1]. Not saying that we should necessarily go this way for all UI widgets though...

[1] https://github.com/devtools-html/debugger.html/pull/805/

> would assume the current style problems in Accordion.css is also a problem
> with the current debugger.html and will be addressed upstream.

Yes and no. For instance the box-sizing: border-box suggestion I mentioned is already fixed in debugger.html because there is a * { box-sizing: border-box; } in another stylesheet.

Comment 8

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: 1308227.patch [3.0]

I went ahead with adding the disclaimers for Accordion.js and Accordion.css. I am expecting to probably have something that can replace this soon from our discussion, but for the time being, I just want a quick win by landing this.

Comment 9

[Julian Descottes [:jdescottes]]

Comment on attachment 8800985 [details] [diff] [review]
1308227.patch [3.0]

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

Looks good to me! 

Before landing, please fix the CSS issue I mentioned and make sure there are no others remaining due to the * {box-sizing} rule (or change the scope of the rule, both work for me).

::: devtools/client/inspector/layout/utils/l10n.js
@@ +5,5 @@
> +"use strict";
> +
> +const { LocalizationHelper } = require("devtools/shared/l10n");
> +const STRINGS_URI = "devtools/locale/layout.properties";
> +const L10N = new LocalizationHelper(STRINGS_URI);

[personal opinion]
Not convinced about the added value of having a helper here instead of importing 

> const { LocalizationHelper } = require("devtools/shared/l10n");
> const L10N = new LocalizationHelper("devtools/locale/layout.properties");

where necessary. I'll leave that up to you!

::: devtools/client/themes/inspector.css
@@ +19,5 @@
>    --breadcrumbs-border-color: #454d5d;
>  }
>  
> +* {
> +  box-sizing: border-box;

Looks like this impacts a bit the layout. For instance, the markup view toolbar is no longer of the same height as the sidebar tabs for instance. This comes from the following property: 

> .tabs .tabs-navigation {
>   height: 23px;
> }

from tabbar.css. Since this is only used by the inspector we could also simply fix it here (should be 24px). 

I *think* this is the only discrepancy but please try to review the UI carefully on your side as well. Other option is to only apply this rule for the accordion.

Comment 10

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: 1308227.patch [4.0]

Comment 11

[Gabriel Luong [:gl] (ΦωΦ)]

https://hg.mozilla.org/integration/fx-team/rev/e7b9263ae9f1356fa228c14b806db2509723b64a
Bug 1308227 - Add a CSS Grid accordion container to the layout panel r=jdescottes

Comment 12

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

I had to back this out for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=11928695&repo=fx-team

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

Comment 13

[Gabriel Luong [:gl] (ΦωΦ)]

https://hg.mozilla.org/integration/fx-team/rev/45f9fc18cf3dab70ba8d1982f40fea9246ae60b9
Bug 1308227 - Add a CSS Grid accordion container to the layout panel r=jdescottes

Comment 14

[Carsten Book [:Tomcat]]

sorry had to back this out for conflicts like warning: conflicts while merging devtools/client/inspector/inspector.xhtml! (edit, then use 'hg resolve --mark') when merging to m-c

Comment 15

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/fx-team/rev/dd3eb044bb51
Backed out changeset 45f9fc18cf3d for causing merge conflicts with mozilla-central

Comment 16

[Patrick Brosset <:pbro>]

(In reply to Pulsebot from comment #15)
> Backout by [e-mail]:
> https://hg.mozilla.org/integration/fx-team/rev/dd3eb044bb51
> Backed out changeset 45f9fc18cf3d for causing merge conflicts with
> mozilla-central
Tomcat has merged m-c to fx-team just now, so I pulled from fx-team, and am now rebasing Gabriel's patch on top. I'll attach it as soon as I'm done.

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/fx-team/rev/78bd3dbc3dde
Add a CSS Grid accordion container to the layout panel r=jdescottes

Comment 18

[Julian Descottes [:jdescottes]]

(In reply to Julian Descottes [:jdescottes] from comment #9)
> Comment on attachment 8800985 [details] [diff] [review]
> 1308227.patch [3.0]
> 
> Review of attachment 8800985 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good to me! 
> 
> Before landing, please fix the CSS issue I mentioned and make sure there are
> no others remaining due to the * {box-sizing} rule (or change the scope of
> the rule, both work for me).
> 
> [...]
> 
> ::: devtools/client/themes/inspector.css
> @@ +19,5 @@
> >    --breadcrumbs-border-color: #454d5d;
> >  }
> >  
> > +* {
> > +  box-sizing: border-box;
> 
> Looks like this impacts a bit the layout. For instance, the markup view
> toolbar is no longer of the same height as the sidebar tabs for instance.
> This comes from the following property: 
> 
> > .tabs .tabs-navigation {
> >   height: 23px;
> > }
> 
> from tabbar.css. Since this is only used by the inspector we could also
> simply fix it here (should be 24px). 
> 
> I *think* this is the only discrepancy but please try to review the UI
> carefully on your side as well. Other option is to only apply this rule for
> the accordion.

Gabriel, I see you haven't addressed my comment about * {box-sizing: border-box} in inspector.css. 

Can you address this in a follow up?

Comment 19

[Julian Descottes [:jdescottes]]

I didn't see you updated the tabbar height to 24px, my bad!

Comment 20

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/78bd3dbc3dde