Closed Bug 1250323 Opened 4 years ago Closed 4 years ago

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

Categories

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

defect

Tracking

(firefox47 fixed)

RESOLVED FIXED
Firefox 47
Tracking Status
firefox47 --- fixed

People

(Reporter: gl, Assigned: gl)

References

Details

Attachments

(1 file, 1 obsolete file)

The pseudoclass panel shrinks if we have it opened and adjust the height of the toolbox.
Duplicate of this bug: 1250892
Priority: -- → P1
Both Toolbox and Browser Toolbox (remote)
(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)
Blocks: 1252640
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;
Attachment #8725430 - Flags: review?(pbrosset)
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
Attachment #8725430 - Attachment is obsolete: true
Attachment #8725801 - Flags: review?(pbrosset)
Attachment #8725801 - Flags: review?(pbrosset) → review+
That test is still failing, so I'm backing out everything from that original push:
https://hg.mozilla.org/integration/fx-team/rev/215615d0cae1
Flags: needinfo?(gl)
Flags: needinfo?(gl)
https://hg.mozilla.org/mozilla-central/rev/24c1ce7bb2ab
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 47
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.