Closed Bug 704132 Opened 10 years ago Closed 10 years ago

Style Inspector's left column should auto-fit text contents

Categories

(DevTools :: Inspector, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 12

People

(Reporter: miker, Unassigned)

References

Details

(Whiteboard: [styleinspector])

Attachments

(1 file, 3 obsolete files)

The style inspector's left column should auto-fit it's contents. The contents of the right-column should wrap as on small screens values are not always visible.

Mihai created a patch for this in another bug but because of bug 460637 we could not use it. If the patch now works we will use it ... otherwise it would make a great test case for bug 460637.
Attached patch Flexible Column Widths (obsolete) — Splinter Review
Wow, that took longer than expected partly because of changes that we had made to the code, partly due to xul quirks and partly because I am sick and my brain is in standby mode this week.

I have updated the patch and removed a lot of cruft from the css files. I have also logged bug 705276 to separate content & document css.

This bug does raise the "invalid BC damage area" assertion which in turn logs hundreds of stack traces. This slows the test seriously enough to make it time out and fail so I will explain how to reproduce it in bug 460637 so that it can be fixed.

Joe, can you look over the css and make sure I am not unnecessarily breaking any Daoist rules?
Attachment #575860 - Attachment is obsolete: true
Attachment #576928 - Flags: review?(jwalker)
Comment on attachment 576928 [details] [diff] [review]
Flexible Column Widths

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

I can only give feedback right now, so f+ nor r+. I'm assuming that the padding:0's are either needed or will be removed.
Dao will need to take a look.

::: browser/devtools/styleinspector/CssHtmlTree.jsm
@@ +634,4 @@
>    {
>      let doc = this.tree.doc;
> +    this.element = doc.createElementNS(HTML_NS, "tr");
> +    this.element.setAttribute("class", this.propertyHeaderClassName);

Minor, not something I would demand you go an change everywhere, but classList rocks imho.

::: browser/themes/gnomestripe/devtools/csshtmltree.css
@@ +49,5 @@
>  }
>  
>  
>  .property-header {
> +  padding: 0;

Do we actually need this? Isn't it the default?
Attachment #576928 - Flags: feedback+
Michael, if you see still the assert once bug 460637 is fixed please file a bug and CC me.
(In reply to Joe Walker from comment #3)
> Comment on attachment 576928 [details] [diff] [review] [diff] [details] [review]
> Flexible Column Widths
> 
> Review of attachment 576928 [details] [diff] [review] [diff] [details] [review]:
> -----------------------------------------------------------------
> 
> I can only give feedback right now, so f+ nor r+. I'm assuming that the
> padding:0's are either needed or will be removed.
> Dao will need to take a look.
> 
> ::: browser/devtools/styleinspector/CssHtmlTree.jsm
> @@ +634,4 @@
> >    {
> >      let doc = this.tree.doc;
> > +    this.element = doc.createElementNS(HTML_NS, "tr");
> > +    this.element.setAttribute("class", this.propertyHeaderClassName);
>  
> Minor, not something I would demand you go an change everywhere, but
> classList rocks imho.
> 

I agree but this is how it has been done in this section of the code so I chose not to change it.
> ::: browser/themes/gnomestripe/devtools/csshtmltree.css
> @@ +49,5 @@
> >  }
> >  
> >  
> >  .property-header {
> > +  padding: 0;
> 
> Do we actually need this? Isn't it the default?

It is not the default and we do need it.

Thanks for the feedback.
Comment on attachment 576928 [details] [diff] [review]
Flexible Column Widths

Dão, can you review this as it is all style related?
Attachment #576928 - Flags: review?(dao)
Attached patch Flexible Column Widths (obsolete) — Splinter Review
Rebased due to failing windows test
Attachment #576928 - Attachment is obsolete: true
Attachment #576928 - Flags: review?(dao)
Attachment #577590 - Flags: review?(dao)
Whiteboard: [styleinspector] → [styleinspector][has-patch]
Comment on attachment 577590 [details] [diff] [review]
Flexible Column Widths

This seems to regress the expander styling, i.e. they expanders are cut off again... Tested on Ubuntu 11.10.
Attachment #577590 - Flags: review?(dao) → review-
Duplicate of this bug: 697398
(In reply to Dão Gottwald [:dao] from comment #8)
> Comment on attachment 577590 [details] [diff] [review]
> Flexible Column Widths
> 
> This seems to regress the expander styling, i.e. they expanders are cut off
> again... Tested on Ubuntu 11.10.

Fixed
Attachment #577590 - Attachment is obsolete: true
Attachment #582800 - Flags: review?(dao)
Attachment #582800 - Flags: review?(dao) → review+
Whiteboard: [styleinspector][has-patch] → [styleinspector][land-in-fx-team]
Attachment #582800 - Flags: review+
https://hg.mozilla.org/integration/fx-team/rev/35d39a331bb8
Whiteboard: [styleinspector][land-in-fx-team] → [styleinspector][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/35d39a331bb8
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Whiteboard: [styleinspector][fixed-in-fx-team] → [styleinspector]
Target Milestone: --- → Firefox 12
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.