964263 - Unused blankspace in css computed-view when resizing the sidebar

Status: VERIFIED FIXED

(DevTools :: Inspector, defect)

Description

[Patrick Brosset <:pbro>]

The css computed-view sidebar of the devtools inspector adapts its layout to the available space. So when you drag the splitter bar around, the layout will switch from 2 columns to 1 column, and will use ellipsis on the properties and values to make sure things are nicely displayed.

However, it's not using all the space it could, and as a result is showing less information than it could.

This happens when you move the splitter towards the right (making the computed-view smaller), the values, originally in a 2nd column, wrap down to a 2nd line below the names, but then, both the names and values have truncated too much compared to the available space.

Attaching 3 screenshots to illustrate this.

Comment 1

[Patrick Brosset <:pbro>]

Attachment: computed-view-widest-2-columns.png

Comment 2

[Patrick Brosset <:pbro>]

Attachment: computed-view-2-lines.png

Comment 3

[Patrick Brosset <:pbro>]

Attachment: computed-view-narrowest-2-lines.png

Comment 4

[Emerson Veenstra [:emerson]]

I'll take this one

Comment 5

[vikneshwar]

Hai Patrick Brosset , 
   Can i work on this bug . This is my second bug . First one is https://bugzilla.mozilla.org/show_bug.cgi?id=957072 

Thank you :)

Comment 6

[Patrick Brosset <:pbro>]

Sorry, :emerson was first :)
Assigning the bug to you.

Let me know what kind of guidance you need for this bug. It's, I think, only a CSS bug.

Comment 7

[Emerson Veenstra [:emerson]]

Attachment: Patch for Bug 964263

Comment 8

[Patrick Brosset <:pbro>]

Comment on attachment 8373357 [details] [diff] [review]
Patch for Bug 964263

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

Thanks for the patch!
2 comments:

- can you explain why 100% wouldn't work? 95 seems a bit like a "magic number" to me and I wonder if it will work with all sorts of screen types and resolutions (having said that, the quick tests I did locally looked good!).
- the patch contains a change for the linux theme only. We have 3 theme directories inside /browser/themes (osx, windows, linux). You'll need to make the change too all 3 css files.

Thanks!

Comment 9

[Emerson Veenstra [:emerson]]

1. 100% pushed the triangle for expanding the property above the property title.
2. Oh, I didn't realise that. I will change that.

Comment 10

[Patrick Brosset <:pbro>]

(In reply to Emerson Veenstra [:emerson] from comment #9)
> 1. 100% pushed the triangle for expanding the property above the property
> title.
Ok got it, in that case could you try and use something like max-width: calc(100% - <insert-triangle-width-here>) ?

Comment 11

[Emerson Veenstra [:emerson]]

Attachment: bug964263.patch

I added width: calc(100%-12px) to the property title. 100% width worked fine for the property value.

Comment 12

[Patrick Brosset <:pbro>]

Comment on attachment 8373401 [details] [diff] [review]
bug964263.patch

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

Looking better!
See my comments below (they apply to all 3 themes).
Also, there's a media query further below in the file:

 @media (min-width: 400px) {
   .property-name {
     width: 200px;
   }
   .property-value {
     width: auto;
   }
 }

I think we should change the auto width to : width: calc(100% - 212px);
with a comment explaining that 212px comes from the arrow width + the 200px of the property-name.

::: browser/themes/linux/devtools/computedview.css
@@ +40,5 @@
>    vertical-align: middle;
>  }
>  
>  .property-name {
> +  width: calc(100%-12px);

- calc(100% - 12px) requires whitespaces around the operator, otherwise it'll be considered as an invalid property and won't be applied
- could you add a css comment line above this line to explain that the 12px come from the width of the arrow?

@@ +53,1 @@
>    max-width: 100%;

max-width can safely be removed since width is also 100%

Comment 13

[Emerson Veenstra [:emerson]]

Attachment: bug964263.patch v3

Did everything, tested it out, looks great.

Comment 14

[Patrick Brosset <:pbro>]

Comment on attachment 8374180 [details] [diff] [review]
bug964263.patch v3

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

Looking great!
Thanks for the patch.
Here is a try build for this patch, just to make sure, but tbh I don't expect any failures since this is only css change: https://tbpl.mozilla.org/?tree=Try&rev=d27211145809
Once this is green, let's land this.

Comment 15

[Patrick Brosset <:pbro>]

Try build is green, and patch is R+.
Fixed in fx-team incoming branch! https://hg.mozilla.org/integration/fx-team/rev/445dca4f26f0

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/445dca4f26f0