Last Comment Bug 704132 - Style Inspector's left column should auto-fit text contents
: Style Inspector's left column should auto-fit text contents
Status: RESOLVED FIXED
[styleinspector]
:
Product: Firefox
Classification: Client Software
Component: Developer Tools: Inspector (show other bugs)
: unspecified
: All All
: -- normal (vote)
: Firefox 12
Assigned To: Nobody; OK to take it and work on it
:
:
Mentors:
: 697398 (view as bug list)
Depends on: 460637 702861
Blocks: 589375
  Show dependency treegraph
 
Reported: 2011-11-21 07:22 PST by Michael Ratcliffe [:miker] [:mratcliffe]
Modified: 2012-01-03 05:47 PST (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Flexible Column Widths (bitrotted) (8.53 KB, patch)
2011-11-21 07:25 PST, Michael Ratcliffe [:miker] [:mratcliffe]
no flags Details | Diff | Splinter Review
Flexible Column Widths (18.16 KB, patch)
2011-11-25 07:01 PST, Michael Ratcliffe [:miker] [:mratcliffe]
jwalker: feedback+
Details | Diff | Splinter Review
Flexible Column Widths (18.09 KB, patch)
2011-11-29 06:14 PST, Michael Ratcliffe [:miker] [:mratcliffe]
dao+bmo: review-
Details | Diff | Splinter Review
Flexible Column Widths (18.20 KB, patch)
2011-12-19 05:54 PST, Michael Ratcliffe [:miker] [:mratcliffe]
dao+bmo: review+
paul: review+
Details | Diff | Splinter Review

Description Michael Ratcliffe [:miker] [:mratcliffe] 2011-11-21 07:22:48 PST
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.
Comment 1 Michael Ratcliffe [:miker] [:mratcliffe] 2011-11-21 07:25:00 PST
Created attachment 575860 [details] [diff] [review]
Flexible Column Widths (bitrotted)
Comment 2 Michael Ratcliffe [:miker] [:mratcliffe] 2011-11-25 07:01:45 PST
Created attachment 576928 [details] [diff] [review]
Flexible Column Widths

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?
Comment 3 Joe Walker [:jwalker] (needinfo me or ping on irc) 2011-11-25 11:48:25 PST
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?
Comment 4 Bernd 2011-11-26 13:10:42 PST
Michael, if you see still the assert once bug 460637 is fixed please file a bug and CC me.
Comment 5 Michael Ratcliffe [:miker] [:mratcliffe] 2011-11-27 10:27:29 PST
(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 6 Michael Ratcliffe [:miker] [:mratcliffe] 2011-11-27 10:29:09 PST
Comment on attachment 576928 [details] [diff] [review]
Flexible Column Widths

Dão, can you review this as it is all style related?
Comment 7 Michael Ratcliffe [:miker] [:mratcliffe] 2011-11-29 06:14:06 PST
Created attachment 577590 [details] [diff] [review]
Flexible Column Widths

Rebased due to failing windows test
Comment 8 Dão Gottwald [:dao] 2011-12-04 12:01:32 PST
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.
Comment 9 Michael Ratcliffe [:miker] [:mratcliffe] 2011-12-13 03:49:49 PST
*** Bug 697398 has been marked as a duplicate of this bug. ***
Comment 10 Michael Ratcliffe [:miker] [:mratcliffe] 2011-12-19 05:54:02 PST
Created attachment 582800 [details] [diff] [review]
Flexible Column Widths

(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
Comment 12 Tim Taubert [:ttaubert] 2012-01-03 05:47:49 PST
https://hg.mozilla.org/mozilla-central/rev/35d39a331bb8

Note You need to log in before you can comment on or make changes to this bug.