Closed Bug 964263 Opened 6 years ago Closed 6 years ago

Unused blankspace in css computed-view when resizing the sidebar

Categories

(DevTools :: Inspector, defect)

defect
Not set

Tracking

(Not tracked)

VERIFIED FIXED
Firefox 30

People

(Reporter: pbro, Assigned: eveenstra26)

Details

(Whiteboard: [good first bug][lang=css][mentor=pbrosset])

Attachments

(4 files, 2 obsolete files)

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.
Whiteboard: [good first bug][lang=css][mentor=pbrosset]
I'll take this one
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 :)
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.
Assignee: nobody → eveenstra26
Attached patch Patch for Bug 964263 (obsolete) — Splinter Review
Attachment #8373357 - Flags: review?(pbrosset)
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!
Attachment #8373357 - Flags: review?(pbrosset)
1. 100% pushed the triangle for expanding the property above the property title.
2. Oh, I didn't realise that. I will change that.
(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>) ?
Attached patch bug964263.patch (obsolete) — Splinter Review
I added width: calc(100%-12px) to the property title. 100% width worked fine for the property value.
Attachment #8373357 - Attachment is obsolete: true
Attachment #8373401 - Flags: review?(pbrosset)
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%
Attachment #8373401 - Flags: review?(pbrosset)
Did everything, tested it out, looks great.
Attachment #8373401 - Attachment is obsolete: true
Attachment #8374180 - Flags: review?(pbrosset)
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.
Attachment #8374180 - Flags: review?(pbrosset) → review+
Try build is green, and patch is R+.
Fixed in fx-team incoming branch! https://hg.mozilla.org/integration/fx-team/rev/445dca4f26f0
Whiteboard: [good first bug][lang=css][mentor=pbrosset] → [good first bug][lang=css][mentor=pbrosset][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/445dca4f26f0
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [good first bug][lang=css][mentor=pbrosset][fixed-in-fx-team] → [good first bug][lang=css][mentor=pbrosset]
Target Milestone: --- → Firefox 30
Status: RESOLVED → VERIFIED
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.