1250323 - Don't hide overflow-y on the pseudo class panel

Status: RESOLVED FIXED

(DevTools :: Inspector: Rules, defect, P1)

Description

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

The pseudoclass panel shrinks if we have it opened and adjust the height of the toolbox.

Comment 2

[magicp]

Both Toolbox and Browser Toolbox (remote)

Comment 3

[magicp]

(In reply to Gabriel Luong [:gl] (ΦωΦ) from comment #0)
> The pseudoclass panel shrinks if we have it opened and adjust the height of
> the toolbox.

Another STR: Expand ruleview-expander(twisty)

Comment 4

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

Attachment: 1250323.patch [1.0]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=70bf8790245e

Comment 5

[Patrick Brosset <:pbro>]

Comment on attachment 8725430 [details] [diff] [review]
1250323.patch [1.0]

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

This fixes the issue correctly for me.
I just have a few comments about the structure of the CSS that I'd like to see addressed before R+'ing.

::: devtools/client/themes/rules.css
@@ +28,5 @@
>  
> +/* Rule View Toolbar */
> +
> +.devtools-sidebar-toolbar {
> +  display: flex;

The devtools-sidebar-toolbar class is used in other panels too, not just the rule-view, so if 'display:flex' makes sense for *all* of these panels, then this rule should go in a more general CSS file. 
If, instead, it is only required for the rule-view, then you should scope the selector with #sidebar-panel-ruleview for example.

Right now, it applies to the computed-view too, but the rule is defined in rules.css, which is awkward.

@@ +38,5 @@
> +}
> +
> +#ruleview-toolbar {
> +  display: flex;
> +  flex-direction: row;

'row' is the initial value for flex-direction, so it isn't needed here.

@@ +39,5 @@
> +
> +#ruleview-toolbar {
> +  display: flex;
> +  flex-direction: row;
> +  height: 23px;

I see 24px being a pretty consistent value across devtools for various tab/tool-bars. Why 23px here?

@@ +52,5 @@
> +}
> +
> +#pseudo-class-panel {
> +  display: flex;
> +  flex-direction: row;

'row' is the initial value for flex-direction, so it isn't needed here.

@@ +53,5 @@
> +
> +#pseudo-class-panel {
> +  display: flex;
> +  flex-direction: row;
> +  position: relative;

position:relative seems like a hack needed for something, but I can't figure out why. Can you add a comment above the property that explains why it's needed.

@@ +55,5 @@
> +  display: flex;
> +  flex-direction: row;
> +  position: relative;
> +  height: 24px;
> +  outline: 0;

Why outline:0 here? This is just a simple div, it's not getting an outline by default, right?

@@ +59,5 @@
> +  outline: 0;
> +  overflow: hidden;
> +  transition-property: height;
> +  transition-duration: 150ms;
> +  transition-timing-function: ease;

Or use the shorthand:
transition: height 150ms ease;

Comment 6

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

Comment on attachment 8725430 [details] [diff] [review]
1250323.patch [1.0]

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

::: devtools/client/themes/rules.css
@@ +28,5 @@
>  
> +/* Rule View Toolbar */
> +
> +.devtools-sidebar-toolbar {
> +  display: flex;

Fixed. I decided on removing .devtools-sidebar-toolbar and adding that to the toolbar container for Rules / Computed and Font views.

@@ +38,5 @@
> +}
> +
> +#ruleview-toolbar {
> +  display: flex;
> +  flex-direction: row;

Fixed. Removed flex-direction: row

@@ +39,5 @@
> +
> +#ruleview-toolbar {
> +  display: flex;
> +  flex-direction: row;
> +  height: 23px;

The ruleview-toolbar needs to be 1px smaller than the container (24px), otherewise, the parent height grows to 25px.

@@ +52,5 @@
> +}
> +
> +#pseudo-class-panel {
> +  display: flex;
> +  flex-direction: row;

Fixed. Remove flex-direction: row

@@ +53,5 @@
> +
> +#pseudo-class-panel {
> +  display: flex;
> +  flex-direction: row;
> +  position: relative;

Removed position: relative, turns out it is not needed anymore.

@@ +55,5 @@
> +  display: flex;
> +  flex-direction: row;
> +  position: relative;
> +  height: 24px;
> +  outline: 0;

We had an outline there before because of an erroneous placed tabindex. That has been removed now and we can remove the outline: 0 as well.

@@ +59,5 @@
> +  outline: 0;
> +  overflow: hidden;
> +  transition-property: height;
> +  transition-duration: 150ms;
> +  transition-timing-function: ease;

Fixed. Used shorthand

Comment 7

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

Attachment: 1250323.patch [2.0]

Comment 8

[Pulsebot]

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

Comment 9

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

That test is still failing, so I'm backing out everything from that original push:
https://hg.mozilla.org/integration/fx-team/rev/215615d0cae1

Comment 10

[Pulsebot]

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

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/24c1ce7bb2ab