Add Enhanced BOX MODEL Viewer, showing All Data at once + bold style for significant data (value!=0)

RESOLVED FIXED

Status

Other Applications
DOM Inspector
--
enhancement
RESOLVED FIXED
7 years ago
6 years ago

People

(Reporter: Javier "Darth", Assigned: Javier "Darth")

Tracking

Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(4 attachments, 16 obsolete attachments)

9.54 KB, application/zip
Details
2.14 KB, text/html
Details
16.17 KB, image/png
Details
18.60 KB, patch
crussell
: review+
Details | Diff | Splinter Review
(Assignee)

Description

7 years ago
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2.12) Gecko/20101026 BTRS25104 Firefox/3.6.12 ( )
Build Identifier: Version 2.0.8

Add new viewer: an Enhanced Box Model one that shows all properties at the same time (see attached screenshot).
It also shows in bold font the properties whose value != 0

Also have an optional bugfix (included in separate patch), validating the presence of style object (details in comment)

Reproducible: Always
(Assignee)

Updated

7 years ago
Version: unspecified → Trunk
(Assignee)

Comment 1

7 years ago
Created attachment 493630 [details]
Screenshot of Enhanced Box-Model Viewer.
(Assignee)

Comment 2

7 years ago
Created attachment 493631 [details]
Patch files & Actual Files to review or try this. (for Domi 2.0.8)

From included README.TXT:
In these 3 patch files are the new Enhanced Box Model Viewer:
  1st) Shows the new copy of the 2 original BoxModel files (.js & .xul)
  2nd) The CORE of this enhancement!.
  3rd) An extra patch with a bugfix in BoxModel's validation code. (described in a comment)

Included also the ACTUAL FILES that you can put in your copy of DOM INSPECTOR 2.0.8 to try this enhancement.
(Assignee)

Comment 3

7 years ago
Created attachment 493632 [details] [diff] [review]
1/2 Patch with proposed Enhancement.

1/2 - Patch file showing the copy of boxModel.js & .xul to new files: boxModelAll.js & xul
(Assignee)

Comment 4

7 years ago
Created attachment 493633 [details] [diff] [review]
2/2 Patch with proposed Enhancement.

2/2 - Patch with actual changes for this enhancement.  Diff done against RELEASED Domi 2.0.8
(Assignee)

Comment 5

7 years ago
Created attachment 493649 [details] [diff] [review]
1/2 Patch for 'trunk' build, with proposed Enhancement

Patch: copied boxModel.js & .xul to new files: boxModelAll.js & xul
Attachment #493632 - Attachment is obsolete: true
(Assignee)

Comment 6

7 years ago
Created attachment 493650 [details] [diff] [review]
2/2 Patch for 'trunk' build, with proposed Enhancement

Patch: added New viewer "Box Model - View All".
Attachment #493633 - Attachment is obsolete: true
(Assignee)

Updated

7 years ago
Attachment #493650 - Attachment description: 2/2 Patch for 'TRUNK' build, with proposed Enhancement → 2/2 Patch for 'trunk' build, with proposed Enhancement
(Assignee)

Updated

7 years ago
Attachment #493631 - Attachment description: Patch files & Actual Files to review or try this enhancement. → Patch files & Actual Files to review or try this. (for Domi 2.0.8)
(Assignee)

Comment 7

7 years ago
Also, I've created an Enhanced Dom-Inspector-addon to try this and other enhancements available here:
   https://addons.mozilla.org/firefox/addon/254571/
(Forum/Support: http://tiny.cc/domi)
I think it might actually make sense to just get rid of the viewers this duplicates, and have it all in one pane.  Thoughts, crussel?
(In reply to comment #8)
> I think it might actually make sense to just get rid of the viewers this
> duplicates, and have it all in one pane.  Thoughts, crussel?

Yep.
(In reply to comment #9)
> Yep.
You want to do the reviews on this bug?  I likely won't be able to look at it for at least two weeks.
I might be able to look at it this afternoon.
Assignee: nobody → redlibrepy
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
(Assignee)

Comment 12

7 years ago
Created attachment 493756 [details] [diff] [review]
2/2 Patch for 'trunk' build, with proposed Enhancement

Patch: added New viewer "Box Model - View All".  *cleaned* version
Attachment #493650 - Attachment is obsolete: true
(Assignee)

Comment 13

7 years ago
I've uploaded a cleaned version of the 2nd (core) patch.

Thank you for your interest in this!
Comment on attachment 493756 [details] [diff] [review]
2/2 Patch for 'trunk' build, with proposed Enhancement

Oh, man.  I didn't realize how l10n-unfriendly the boxModel viewer is.

First, like comment 8 says, you should just change boxModel instead of creating boxModelAll.  I also feel like this patch leaves a lot of evidence around of code that was influenced by the fact that originally (i.e., the way the DOM Inspector currently works), only one metric type was visible at a time.  Including...

>    updateStatGroup: function()
>    {
> -    var ml = document.getElementById("mlStats");
> -    this.showStatGroup(ml.value);
> +    this.showStatGroup();
>    },

This is now just a needless level of indirection.  Get rid of updateStateGroup, and rename showStatGroup to something like "showStats".

> +  groupType: 'p',
...
> +      this.groupType= 'p';
>        this.showPositionStats();
> @@ -133,3 +134,4 @@
>        this.showPositionStats();
> -    } else if (aGroup == "dimension") {
> +	  
> +      this.groupType= 'd';
>        this.showDimensionStats();
> @@ -135,3 +137,4 @@
>        this.showDimensionStats();
> -    } else if (aGroup == "margin") {
> +
> +      this.groupType= 'm';
>        this.showMarginStats();
> @@ -137,3 +140,4 @@
>        this.showMarginStats();
> -    } else if (aGroup == "border") {
> +
> +      this.groupType= 'b';
>        this.showBorderStats();
> @@ -139,3 +143,4 @@
>        this.showBorderStats();
> -    } else if (aGroup == "padding") {
> +
> +      this.groupType= 'g';

groupType is begging to be a parameter.  You can change showStatistic to take an aGroupType argument.

Now that all the values are being displayed all at once, there's no reason to be even setting the labels programmatically anymore, e.g., "x", "y", "screen x", "screen y" (much less using hardcoded strings in the source instead of stringbundle properties...).  Ugh.  If you don't mind, please factor out the aSide argument in showStatistic and create corresponding entities in boxModel.dtd.

E.g., afterward, the showStatistic calls in showPositionStats would become:

    let groupType = "position";
    if ("boxObject" in this.mSubject) { // xul elements
      var bx = this.mSubject.boxObject;
      this.showStatistic(groupType, 1, 1, bx.x);
      this.showStatistic(groupType, 1, 2, bx.y);
      this.showStatistic(groupType, 2, 1, bx.screenX);
      this.showStatistic(groupType, 2, 2, bx.screenY);

and the XUL for the position group would look like:

          <label value="&xCoord.label;"/>
          <label id="positionR1C1Value"/>
          <label value="&screenXCoord.label;"/>
          <label id="positionR1C2Value"/>
...
          <label value="&yCoord.label;"/>
          <label id="positionR2C1Value"/>
          <label value="&screenYCoord.label;"/>
          <label id="positionR2C2Value"/>

Or similar.  All of the "C3" labels can be removed, since they're not even used.

> +    var bData= aSize.indexOf("px")>=0? aSize.indexOf("0px")!=0: aSize!="0"; //  detects i.e: '10px'

bData should be something more descriptive, like "nonzero".  Also, operators like equality operators, assignment, and the ternary conditional (? … : …) should have a space on both sides.  Lines shouldn't be longer than 80 columns.  (I usually break after 78 columns.)

> +groupbox    caption > label{
> +	font-weight: bold;
> +}
> +
> +label.somedata{
> +	font-weight: bold;

One space after groupbox and no tabs, please.  Also, could you change "somedata" to "nonzero"?

> +	color: #0000B0;
> +}

You shouldn't set this unless you're willing to come up with some mechanism that selects an appropriate color in the case of collisions (or other visually similar values) with the background color.  For example, you need to consider the case where the user's theme (either from the application or the system theme) already specifies the background color to be #0000B0 or some other color on which it would be very hard to read.

Making the value bold should be enough.

Also, in the future, when you create a patch, you should ask for a reviewer <https://developer.mozilla.org/en/Hacking_Mozilla#Getting_Reviews>.  You can request review from me in the review field.  Thanks for working on this.
Attachment #493756 - Flags: review-
Attachment #493649 - Flags: review-
(Assignee)

Comment 15

7 years ago
Created attachment 493949 [details] [diff] [review]
Second patch with requested changes.

- Done all requests except moving Titles to XUL+DTD (comments follow).
Attachment #493649 - Attachment is obsolete: true
Attachment #493756 - Attachment is obsolete: true
(Assignee)

Comment 16

7 years ago
/*
- Border data take pretty much horizontal space in bold style. 
  I have 3 ideas to improve this: (can be combined also)
1) .value-column {
     margin-right: 2em; // lower the 30px here
   } 
2) Use titles like "b-bottom" instead of "border-bottom", showed in screenshot.
3) Style it like this:  *3px* solid rgb(255,255,255)
   there is: making bold only the "3px".
   (Can be done ie. putting 2 joined <label>s in XUL there, and a little adjust
   for the code)


- (Added "class" attrib to <groupbox>es to identify them for styling purposes.
  Ie. maybe someone would want to go for a different style for border data)
*/

/*
- Instead of moving the title to XUL+DTD for these titles: ("screen y"),
*/
  this.showStatistic(groupType, 1, 1, "screen y", bx.width);
/*
  how about using a stringbundle like below?

  Also, The XUL/DTD option will put me under a somewhat more tight schedule.
  (There is also a Box-Model bugfix + other DomI proposal that I want to submit)
  Please let me know if that is a priority for you and I will look forward to do it.
*/
  Bundle: {
    screenYCoord: this.getStringTitle("screenYCoord.label", "screen y");
	...
  }
  // I think It might be easier to read/maintain this code having the 
  // Title (varname) and Value together here, instead of Title in XUL+DTD.
  this.showStatistic(groupType, 1, 1, this.Bundle.screenYCoord, bx.width);
  ...

  getStringTitle: function(propName, defTitle)
  {
    var title = stringbundle.getString("viewers.boxModel."+propName));
    // We can use fallback value here if propName not defined yet in this locale.
    return title ? title : defTitle; 
  }
  //<stringbundle id="stringbundle" src="chrome://inspector/locale/inspector.properties"/>

/*
- There are 2 lines that now have few more than 78 columns. 
  Should I break them? (Will be pretty the same if we use stringbundle)
*/
      this.showStatistic(groupType, 1, 1, "box width" , this.mSubject.offsetWidth);
      this.showStatistic(groupType, 1, 2, "box height", this.mSubject.offsetHeight);
      this.showStatistic(groupType, 2, 1, "content width" , "");
      this.showStatistic(groupType, 2, 2, "content height", "");

/*
  Please let me know if I can be of any further help.
  Thanks!
*/
(Assignee)

Updated

7 years ago
Attachment #493949 - Flags: review?(Sevenspade)
Comment on attachment 493949 [details] [diff] [review]
Second patch with requested changes.


(In reply to comment #16)
> /*
> - Border data take pretty much horizontal space in bold style. 

You're right, but it currently takes up too much space now, even without bold.

>   I have 3 ideas to improve this: (can be combined also)
> 1) .value-column {
>      margin-right: 2em; // lower the 30px here
>    } 

I agree that it would be better to use 2em here, instead of pixels, since ems are relative, but there's no guarantee that 2em is less than 30px.  On my system, for example, it is, but the difference is negligible; if I bump my settings up to 12 for the default application font, 2em is actually *wider* than 30px.  If you'd go ahead and change it, that would be great.

> 2) Use titles like "b-bottom" instead of "border-bottom", showed in screenshot.

That won't do.

> 3) Style it like this:  *3px* solid rgb(255,255,255)
>    there is: making bold only the "3px".
>    (Can be done ie. putting 2 joined <label>s in XUL there, and a little adjust
>    for the code)

That still only gains negligible space.

It'd be better to just put each part on its own line for all groups, so it looks like:

Margin
  margin-top:    0px
  margin-right:  0px
  margin-bottom: 0px
  margin-left:   0px

Border
  border-top:    0px none rgb(0,0,0)
  border-right:  0px none rgb(0,0,0)
  border-bottom: 0px none rgb(0,0,0)
  border-left:   0px none rgb(0,0,0)

et cetera.

I'm not entirely sure that the border style and color should be represented here anyway, but it's probably best to try not to make a decision on it in this bug.

> - (Added "class" attrib to <groupbox>es to identify them for styling purposes.
>   Ie. maybe someone would want to go for a different style for border data)

There's no need for that; it's not used in the DOM Inspector skin CSS.  If you have a theme where you're actually trying to style these, feel free to add ids, though.

> /*
> - Instead of moving the title to XUL+DTD for these titles: ("screen y"),
> */
>   this.showStatistic(groupType, 1, 1, "screen y", bx.width);
> /*
>   how about using a stringbundle like below?

Stringbundles are unnecessary here.  If it can be done declaratively without awkwardness, it should be.

>   Also, The XUL/DTD option will put me under a somewhat more tight schedule.

That's okay.  That's not within the scope of this bug.  If you don't want to take care of it in this bug, I'll do it in bug XXX.

> /*
> - There are 2 lines that now have few more than 78 columns. 
>   Should I break them? (Will be pretty the same if we use stringbundle)
> */

Any lines you touch should have appropriate line breaks for length.  You don't have to change any lines you aren't touching.  (But you can if you want to.  If you are going to, please do that in a separate patch that comes before any real changes to the code.)

>       this.showStatistic(groupType, 1, 1, "box width" ,
> this.mSubject.offsetWidth);
>       this.showStatistic(groupType, 1, 2, "box height",
> this.mSubject.offsetHeight);
>       this.showStatistic(groupType, 2, 1, "content width" , "");
>       this.showStatistic(groupType, 2, 2, "content height", "");
> 
> /*
>   Please let me know if I can be of any further help.
>   Thanks!
> */

Comment on attachment 493949 [details] [diff] [review]
> +  showStatistic: function(groupType, aCol, aRow, aSide, aSize)

Depending on whether you decide to go ahead and put the group data in a single column in this bug, aCol and aRow (and this method?) might become obselete.

> +    var label = document.getElementById(groupType+"R"+aRow+"C"+aCol+"Label");
> +    var val   = document.getElementById(groupType+"R"+aRow+"C"+aCol+"Value");

Spaces around operators, please.

> +    aSize = "" + aSize;

aSize.toString() should be fine.

> +    var nonzero = aSize.indexOf("px") >= 0 ? aSize.indexOf("0px") != 0 : 
> +                                             aSize != "0";

There's a space at the end of the first line that needs to be removed.

Nit: elsewhere in DOM Inspector code, ternary conditionals get broken like:

condition ?
  then-clause :
  fallback;

You could also write it as:

var str = aSize.toString();
var nonzero = !!(str.indexOf("px") < 0 ? aSize : str.indexOf("0px"));

> +      this.showStatistic(groupType, 1, 1, "box width" , bx.width);
> +      this.showStatistic(groupType, 1, 2, "box height", bx.height);
> +      this.showStatistic(groupType, 2, 1, "content width" , "");
> +      this.showStatistic(groupType, 2, 2, "content height", "");

If you're going to align these, please put the extra spaces after the comma.

> +    var groupType= aName;
> +    this.showStatistic(groupType, 1, 1, aName+"-top"   , aData[0]);
> +    this.showStatistic(groupType, 2, 1, aName+"-right" , aData[1]);
> +    this.showStatistic(groupType, 1, 2, aName+"-bottom", aData[2]);
> +    this.showStatistic(groupType, 2, 2, aName+"-left"  , aData[3]);

Spaces here, too.

> +groupbox caption > label {
> +  font-weight: bold;
> +}

This rule is completely unneeded, actually.
Attachment #493949 - Flags: review?(Sevenspade) → review-
(In reply to comment #17)
> If you don't want to take care of it in this bug, I'll do it in bug XXX.

Bug 615799
Pfft.  Bug 615779.
(Assignee)

Comment 20

7 years ago
> It'd be better to just put each part on its own line for all groups, so it
> looks like:
> 
> Margin
>  margin-top:    0px
>  margin-right:  0px
>  margin-bottom: 0px
>  margin-left:   0px
> 
> Border
>  border-top:    0px none rgb(0,0,0)
>  border-right:  0px none rgb(0,0,0)
>  border-bottom: 0px none rgb(0,0,0)
>  border-left:   0px none rgb(0,0,0)
> 
> et cetera.

Do you mean getting rid of <groupbox>es?  Without <groupbox> is the only working way I found (Not sure If I understand you correctly). I tried to mix <rows> and <groupbox> but failed. How should I do it?
(Or maybe, Ie., How about setting a min-width for columns, so everything but borders can be aligned)

> I'm not entirely sure that the border style and color should be represented
> here anyway, but it's probably best to try not to make a decision on it in 
> this bug.

I agree. Sorry If I wasn't clear about the styling thing before. My bad English. :-)

> > +  showStatistic: function(groupType, aCol, aRow, aSide, aSize)
> 
> Depending on whether you decide to go ahead and put the group data in a single
> column in this bug, aCol and aRow (and this method?) might become obsolete.

I’m not sure if I understand it. Do you mean something like this?

      this.showStatistic(groupType + "_x", "x", bx.x);
      this.showStatistic(groupType + "_y", "y", bx.y);
      ....
      this.showStatistic(groupType + "_top",   aName+"-top",    aData[0]);
      this.showStatistic(groupType + "_right", aName+"-right" , aData[1]);

      <label id="position_x_Value"/>
      ....
      <label id="margin_top_Value"/>


> > +    aSize = "" + aSize;
> 
> aSize.toString() should be fine.

No problem. 
I use the ["" + aSize] approach to be always on the safe side against any (future) incoming null/undefined values. (That can happen in a BoxModel extended version I'm trying here, that includes more css properties)

Thanks!
(In reply to comment #20)
> > It'd be better to just put each part on its own line for all groups, so it
> > looks like:
> > 
> > Margin
> >  margin-top:    0px
> >  margin-right:  0px
> >  margin-bottom: 0px
> >  margin-left:   0px
> > 
> > Border
> >  border-top:    0px none rgb(0,0,0)
> >  border-right:  0px none rgb(0,0,0)
> >  border-bottom: 0px none rgb(0,0,0)
> >  border-left:   0px none rgb(0,0,0)
> > 
> > et cetera.
> 
> Do you mean getting rid of <groupbox>es?

No.  Right now they're in a 2 by 2 grid.  I mean to take it out of a 2 by 2 layout and just put them in a single column, one after the other.

> > > +  showStatistic: function(groupType, aCol, aRow, aSide, aSize)
> > 
> > Depending on whether you decide to go ahead and put the group data in a single
> > column in this bug, aCol and aRow (and this method?) might become obsolete.
> 
> I’m not sure if I understand it.

Maybe my comments above make it easier to understand what I was referring to now.  If they're no longer in a 2 by 2 layout, aCol and aRow are irrelevant arguments.
(In reply to comment #20)
> > aSize.toString() should be fine.
> 
> No problem. 
> I use the ["" + aSize] approach to be always on the safe side against any
> (future) incoming null/undefined values.

In general, just use early returns for those types of values.
If you don't mind, could you also remove the import of util.dtd from boxModel.xul (and appropriately renumber dtd3 to dtd2)?
Blocks: 537994
Comment on attachment 493949 [details] [diff] [review]
> +          <label id="dimensionR1C2Label"/>
> +          <label id="dimensionR1C2Value"/>
...
> +          <label id="dimensionR2C2Label"/>
> +          <label id="dimensionR2C2Value"/>

Hey look.  You can delete these, too.  content width and content height seem to have never been anything.
(Assignee)

Comment 25

7 years ago
> > > Margin
> > >  margin-top:    0px
> > >  margin-right:  0px
> > >  margin-bottom: 0px
> > >  margin-left:   0px
> > > 
> > > Border
> > >  border-top:    0px none rgb(0,0,0)
> > >  border-right:  0px none rgb(0,0,0)
> > >  border-bottom: 0px none rgb(0,0,0)
> > >  border-left:   0px none rgb(0,0,0)
> 
> No.  Right now they're in a 2 by 2 grid.  I mean to take it out of a 2 by 2
> layout and just put them in a single column, one after the other.

Now I got it right. :-) I'm preparing it. 

> 
> > > > +  showStatistic: function(groupType, aCol, aRow, aSide, aSize)
> > > 
> > > Depending on whether you decide to go ahead and put the group data in a 
> > > single column in this bug, aCol and aRow (and this method?) might become obsolete.
> > 
> > I’m not sure if I understand it.
> 
> Maybe my comments above make it easier to understand what I was referring to
> now.  If they're no longer in a 2 by 2 layout, aCol and aRow are irrelevant
> arguments.

That's right. I'm going to prepare a version with only one numeric parameter instead of Row & Col. Is that ok with You?
(Assignee)

Comment 26

7 years ago
(In reply to comment #23)
> If you don't mind, could you also remove the import of util.dtd from
> boxModel.xul (and appropriately renumber dtd3 to dtd2)?

Consider it done.
(In reply to comment #25)
> I'm going to prepare a version with only one numeric parameter
> instead of Row & Col. Is that ok with You?

I guess.  I'd actually eventually rather see something like 

this.showStatistic(this.mBoxWidthValueLabel, bx.width);
this.showStatistic(this.mBoxHeightValueLabel, bx.height);

Where mBoxWidthValueValueLabel refers to the correct label, after it has been set during viewer initialization by using getElementById, but I guess it can wait for the l10n corrections.
(Assignee)

Comment 28

7 years ago
Created attachment 495670 [details]
New BoxModel layout concept notes with screenshots.html

I prepared some notes with screenshots for you to evaluate. 
It about the new XUL layout, the 'All data in one column', and other variants that came up to my head :-)
I opted for this html file in order to show screenshots with notes alongside them. Please feel free to advice how shoud I present/upload that stuff here. (Ie. should I've uploaded all those screenshots here?)
Thank you!
(Assignee)

Comment 29

7 years ago
Btw I thought also, that the new layout showed there can be optional, controlled by a user-preference-entry.
(In reply to comment #28)
> Created attachment 495670 [details]
> New BoxModel layout concept notes with screenshots.html
> 
> I prepared some notes with screenshots for you to evaluate. 
> It about the new XUL layout, the 'All data in one column'

Yes.  The first example is exactly what I had in mind.

> Please feel free to advice how shoud I present/upload that stuff here.
> (Ie. should I've uploaded all those screenshots here?)

In general, yes, for longevity.  You can just add notes in the comment accompanying the attachment.

The other variants look like they'd fall under bug 255940.  From that bug, it seems like the main reason it never got off the ground is that there wasn't a good solution that made it accessible to screen readers, amongst some other problems with the patches.
(Assignee)

Comment 31

7 years ago
/*
Very well. 
However (only sharing a thought here), when I am looking at real data, I found that It could be easier to catch the element's properties with the original layout, with the top&bottom and left&right values together:
> Margin
>  margin-top:    0px    margin-right:  0px
>  margin-bottom: 0px    margin-left:   0px

This layout looks nice to me too, but those values (those across 'vertical' and 'horizontal' axis) are alternated here and made me more difficult to catch quickly what side have what value, but It could be only me :)
> Margin
>  margin-top:    0px
>  margin-right:  0px
>  margin-bottom: 0px
>  margin-left:   0px


I've moved the getElementById() operations at Initialization time, and I've stored the references in fields inside their own 'groups' shown below: (By using those groups, I am able to simplify a lot the code at initialization and in showSideStats()... )
*/

  groupPosition:  {  groupType: "position"  },
  groupDimension: {  groupType: "dimension" },
  groupMargin:    {  groupType: "margin"    },
  groupBorder:    {  groupType: "border"    },
  groupPadding:   {  groupType: "padding"   },


  showMarginStats: function()
  {
    ...
    this.showSideStats(this.groupMargin, data);
  },
  showBorderStats: function()
  {
    ...
    this.showSideStats(this.groupBorder, data);
  },
  showPaddingStats: function()
  {
    ...
    this.showSideStats(this.groupPadding, data);
  },

/*
If this is ok with you, I'll prepare the next patch.
  
On a side note, having played quite a bit with the xul file, I've already done the L10n thing.
*/
(In reply to comment #31)
> If this is ok with you, I'll prepare the next patch.

Sounds great.
Blocks: 615779
(Assignee)

Comment 33

7 years ago
Created attachment 496992 [details]
Screenshot with proposed patch applied.
(Assignee)

Comment 34

7 years ago
Created attachment 496994 [details] [diff] [review]
Third patch as showed in last screenshot. Includes L10n
Attachment #493949 - Attachment is obsolete: true
Attachment #496994 - Flags: review?(Sevenspade)
Comment on attachment 496994 [details] [diff] [review]
Third patch as showed in last screenshot. Includes L10n

>@@ -128,4 +169,4 @@ 
>   },
>   
>-  showStatGroup: function(aGroup)
>+  showStatistic: function(aGroup, aField,  aSize)
>   {
>@@ -131,24 +172,14 @@ 

Please use a higher number for diff context in the future.  -U 8 is pretty standard <https://developer.mozilla.org/index.php?title=en/Creating_a_patch>.

>+  showStatistic: function(aGroup, aField,  aSize)
...
>+    if( aSize === null || aSize === undefined ){
>+        aSize = "";
>+    }
>+    aGroup[aField].setAttribute("value", aSize);
>+
>+    var str = aSize.toString();
>+    var nonzero = !!(str.indexOf("px") < 0 ? aSize : str.indexOf("0px"));
>+
>+    aGroup[aField].setAttribute("class", nonzero ? "nonzero" : "");
>   },

It doesn't make any sense for aGroup and aField to be two different parameters here.

|showStatistic: function BMVr_ShowStatistic(aFieldLabel, aSize)| would be much better here for a couple of reasons:

i.  It's more obvious to see what's going on with the function calls than
    |this.showStatistic(aGroup, "x", bx.x)|.  (See comment 27.)
ii. There's more than one type of (easily-preventable) extra cost in
    |function(aFoo, aBar) { … aFoo[aBar] … }|

To continue in a similar manner:
>+    this.initGroup(this.groupPosition,  this.posFields);
>+    this.initGroup(this.groupDimension, this.dimFields);
>+    this.initGroup(this.groupMargin,  this.boxSide);
>+    this.initGroup(this.groupBorder,  this.boxSide);
>+    this.initGroup(this.groupPadding, this.boxSide);
...
>+  groupPosition:  {  groupType: "position"  },
>+  groupDimension: {  groupType: "dimension" },
>+  groupMargin:    {  groupType: "margin"    },
>+  groupBorder:    {  groupType: "border"    },
>+  groupPadding:   {  groupType: "padding"   },
>+
>+  dimFields: [ "width", "height" ],  // "content_width, content_height"
>+  posFields: [ "x",     "y",     "screenX", "screenY" ],
>+  boxSide:   [ "top",   "right", "bottom",  "left"    ],
>+
>+  initGroup: function(aGroup, aFields)
>+  {
>+    for( var ix = 0; ix < aFields.length; ix ++ ){
>+      var field = aFields[ix];
>+      aGroup[field]= this.getXulLabel(aGroup.groupType + "_" + field + "_");
>+    }
>+  },
>+
>+  getXulLabel: function(sId)
>+  {
>+    sId += "Value";
>+    var elem= document.getElementById(sId);
>+    if( ! elem ){
>+        throw "No element found. id="+sId; // early catch for any missing elem.
>+    }
>+    return elem;
>+  },

This really isn't necessary.  If you simply do

this.mPositionX = document.getElementById("position_x_Value");
this.mPositionY = document.getElementById("position_y_Value");
this.mPositionScreenX = document.getElementById("position_screenX_Value");

et cetera (for all values), by my count, it would be 18 lines, versus 33.  There are a couple of other benefits to this approach over the one in the patch:

i.  It is immediately obvious what is happening.  With initGroup/getXulLabel, 
    it's a bit obfuscated, and as a reader you have to work out what's going
    on.
ii. With initGroup/getXulLabel, there's a lot of extra computation, with what
    seems like no benefit, e.g., allowing the design of other parts of the
    code to be more generalized.

The only other parts of the code where the approach above plays a role is in showSideStats, since it uses boxSide:

>+  showSideStats: function(aGroup, aData)
...
>+    for( var ix = 0; ix <= 3; ix ++ ){
>+      this.showStatistic(aGroup, this.boxSide[ix],    aData[ix]);
>+    }
>   },

This can be remedied by just making making boxSide local to the function.  (Alternatively, showMarginStats, showBorderStats, and showPaddingStats as well as readMarginStyle, readBorderStyle, and readPaddingStyle could seriously be reworked to make use of a common BOXSIDES array and a more generalized approach between them, if you wanted to do some more refactoring.  You don't have to, though, and I'd suggest against doing it in this bug even if you really did want to.)

Regarding
>+    for( var ix = 0; ix <= 3; ix ++ ){

The spaces should go on the outside of the parentheses, i.e., before the open parethensis, not after it, and after the close parenthesis, not before it.  Please use i instead of ix in your loops, and |< 4| instead of |<= 3|...

Nit: Also, |let| in the initializer instead of |var|.

>+    this.showPositionStats();
>+
>+    this.showDimensionStats();
>+
>+    this.showMarginStats();
>+
>+    this.showBorderStats();
>+
>+    this.showPaddingStats();

Nit: Please collapse these lines to be single-spaced.

+      this.showStatistic(aGroup, "screenX", ""); //hide title
+      this.showStatistic(aGroup, "screenY", ""); //hide title

I'm not sure "title" is the right word here. Space before "hide".

>+      // "content width, height"

Why are these here?

>+  <groupbox id="boxposition">

Inter-caps for this id, i.e., boxPosition, please.  Similar comments apply for the others.

>+          <label id="position_x_Label" value="&xCoord.label;" class="title"/>
>+          <label id="position_x_Value"/>

Same as above: positionXValue.  Why do the first ones (the *Labels) even have ids?  (The *Values should probably be readonly, plain textboxes, actually, instead of labels.)

>+          <label id="margin_top_Label"    value="&margin.top.label;" class="title"/>

If these ids stay, don't forget to add breaks for line length.  And one space between attributes should be enough.

>+++ a/resources/locale/en-US/viewers/boxModel.dtd	

You'll need to remove the other locales from the Makefile.

>+<!ENTITY margin.top.label    "margin-top">
>+<!ENTITY margin.right.label  "margin-right">
>+<!ENTITY margin.bottom.label "margin-bottom">
>+<!ENTITY margin.left.label   "margin-left">
>+
>+<!ENTITY border.top.label    "border-top">
>+<!ENTITY border.right.label  "border-right">
>+<!ENTITY border.bottom.label "border-bottom">
>+<!ENTITY border.left.label   "border-left">
>+
>+<!ENTITY padding.top.label    "padding-top">
>+<!ENTITY padding.right.label  "padding-right">
>+<!ENTITY padding.bottom.label "padding-bottom">
>+<!ENTITY padding.left.label   "padding-left">

These should just be in the XUL.  They are atoms defined in the CSS spec, and don't vary by locale.

>+          <label id="padding_left_Label"   value="&padding.left.label;" class="title"/>
...
>+groupbox:not(#boxborder) label:not(.title) {
>+  text-align: right;
>+}

This is a really roundabout way of saying "I want the values to be right-aligned (except for the border info).".  If you change these all to be textboxes as mentioned above, it would make the selector more straightforward.  (I also wouldn't worry about excluding the border info.  It's very likely that border-left, et al will get changed to border-left-width, et cetera in a future bug.)

>+column.value-column {
>+  min-width: 4em;

.value-column should be enough.  This also only (theoretically) gives you enough space for two digits—two for the actual numerica value and two characters for "px" in the fields that include it.  Please bump it up to 5em.

You'll also need to make sure the viewer is scrollable now.  You can accomplish this by putting all of the groupboxes into a vbox, and putting that into a flexible box with |overflow: auto;|.

>+column:not(.value-column) {
>+  min-width: 8em;
>+}

I'm not sure what you're going for here.  It looks like you might be trying to make sure that the values line up across groupboxes.  If that's the point, there's a better way to do it: set all the value columns to be flexible.  That should work if you use it in conjunction with the approach that I described above to get scrolling working.
Attachment #496994 - Flags: review?(Sevenspade) → review-
(Assignee)

Comment 36

7 years ago
Created attachment 497190 [details]
Screenshot showing some test alignment settings (see comment)


> >+groupbox:not(#boxborder) label:not(.title) {
> >+  text-align: right;
> >+}
> 
> ... 
> (I also wouldn't worry about excluding the border info.  It's very likely that
> border-left, et al will get changed to border-left-width, et cetera in a 
> future bug.)

I tested that (see attachment), but I think the border data could be displayed with major misalignment like in this case. (At least meanwhile 'border-left-width', et al isn't implemented)
What should I do?


> You'll also need to make sure the viewer is scrollable now. You can accomplish
> this by putting all of the groupboxes into a vbox, and putting that into a
> flexible box with |overflow: auto;|.
> 
> >+column:not(.value-column) {
> >+  min-width: 8em;
> >+}
> 
> I'm not sure what you're going for here.  It looks like you might be trying to
> make sure that the values line up across groupboxes.  If that's the point,
> there's a better way to do it: set all the value columns to be flexible.  That
> should work if you use it in conjunction with the approach that I described
> above to get scrolling working.

Okp, the attached screenshot shows that implementation too, including the already-working scrolling thing (If I understand all of It correctly). But I find that the values lays too far of their respective titles this way. Please advice.
(Assignee)

Comment 37

7 years ago
(In reply to comment #35)
> Comment on attachment 496994 [details] [diff] [review]
> Third patch as showed in last screenshot. Includes L10n
> 
> ...
> To continue in a similar manner:
> >+  this.initGroup(this.groupPosition,  this.posFields);
> >+  ...
> >+  groupPosition:  {  groupType: "position"  },
> >+  ...
> >+
> >+  dimFields: [ "width", "height" ],  // "content_width, content_height"
> >+  posFields: [ "x",     "y",     "screenX", "screenY" ],
> >+  boxSide:   [ "top",   "right", "bottom",  "left"    ],
> >+
> >+  initGroup: function(aGroup, aFields)
> >+  ...
> >+  getXulLabel: function(sId)
> >+  ...
> 
> This really isn't necessary.  If you simply do
> 
> this.mPositionX = document.getElementById("position_x_Value");
> this.mPositionY = document.getElementById("position_y_Value");
> this.mPositionScreenX = document.getElementById("position_screenX_Value");

Okp, If you don't mind, please allow me to do it like this: (using the groups)

  this.groupPosition.x =       document.getElementById("positionXValue");
  this.groupPosition.screenX = document.getElementById("positionScreenXValue");
  ...
  this.groupMargin.top =       document.getElementById("marginTopValue");
  ...
  
so, the original code will suffer fewer alterations, like here: (See comment 31.)

    this.showSideStats(this.groupMargin, data);
    ...
    this.showSideStats(this.groupBorder, data);
    ...
    this.showSideStats(this.groupPadding, data);

And the showStatistic() calls can be like this:

    this.showStatistic(aGroup.screenX, bx.screenX);
    this.showStatistic(aGroup.screenY, bx.screenY);

    this.showStatistic(aGroup.top,    aData[0]);
    this.showStatistic(aGroup.right,  aData[1]);
    ...
	
In next comment I explain why I coded it like that, with some examples of how I extend this code to do enhancements I foresee in order to reproduce some original BoxModel 'fine' behavior and other future things You've already mentioned.


> (Alternatively, showMarginStats, showBorderStats, and showPaddingStats as well
> as readMarginStyle, readBorderStyle, and readPaddingStyle could seriously be
> reworked to make use of a common BOXSIDES array and a more generalized approach
> between them, if you wanted to do some more refactoring.  You don't have to,
> though, and I'd suggest against doing it in this bug even if you really did
> want to.)

I agree. I've thought about that already too.
 
> +      this.showStatistic(aGroup, "screenX", ""); //hide title
> +      this.showStatistic(aGroup, "screenY", ""); //hide title
> 
> I'm not sure "title" is the right word here. Space before "hide".

Okp, should I use 'label'?
(I used 'title' wherever relevant, in order for me not getting confused between text referring to 'title'-labels visible to the user and other <label> elements. Inspired in Firefox 4 'titlebar' element too :-)
(Assignee)

Comment 38

7 years ago
** Everybody: This is a OFFTOPIC-sort-of-Comment: about my coding style connected to future enhancements I foresee **


> To continue in a similar manner:
> >+  this.initGroup(this.groupPosition,  this.posFields);
> >+  ...
> >+
> >+  initGroup: function(aGroup, aFields)
> >+  {...}
> >+  getXulLabel: function(sId)
> >+  {...}
> 
> This really isn't necessary.  If you simply do
> 
> this.mPositionX = document.getElementById("position_x_Value");
> this.mPositionY = document.getElementById("position_y_Value");
> this.mPositionScreenX = document.getElementById("position_screenX_Value");
> 
> et cetera (for all values), by my count, it would be 18 lines, versus 33. 
> There are a couple of other benefits to this approach over the one in the
> patch:
> 
> i.  It is immediately obvious what is happening.  With initGroup/getXulLabel, 
>     it's a bit obfuscated, and as a reader you have to work out what's going
>     on.
> ii. With initGroup/getXulLabel, there's a lot of extra computation, with what
>     seems like no benefit, e.g., allowing the design of other parts of the
>     code to be more generalized.

I write code like that because I'm thinking (pretty always) about easy-of-maintenance, extensibility and code centralization.  Ie. I 'like' writing a centralized unique function with only one getElementById() line; so I can easily alter/maintain the code or add enhancements (*). That's my humble way of seeing it :-)

(Also, You are right about code being more obfuscated, perhaps there is a better way I couldn't find -when using my coding style-. I see the extra computation too, but I found it being executed only at initialization time.)

(*) In this particular module, I am foreseeing some future incoming changes: We *might* have to be able to access some 'Titles' <label>s (again) Ie. to restore a feature of the original BoxModel: hiding the 'screenX' & 'screenY' labels for html elements. (that's the reason I leaved the '//hide title' there too) And more future titles/values manipulation might add some refinements to all data showed to the user. Of course It can be done by other means also. Only my 2 cents. :-)

If It helps for making myself clearer, here is an example of my (quickly written) code where I have the border-data splitted into separate <label>s (It can be useful to implement the 'border-left-width' option you mentioned):
(Showed also with sample code for getting the *Label elements)

  initGroup: function(aGroup, aFields, isTitle)
  {
    if( ! isTitle ){
      aGroup.titles = {}
      aGroup.titles.groupType = aGroup.groupType;
      this.initGroup(aGroup.titles, aFields, true);
    }
    for( var ix = 0; ix < aFields.length; ix ++ ){
      var field = aFields[ix];
      if( aGroup.groupType == "border" && ! isTitle ){ // NOT titles!
	    // better suffixes here: like '(border)_width', '_style', '_color'
        aGroup[field + "_1"]= this.getXulLabel(aGroup.groupType +
                                               "_" + field + "_1_", isTitle);
        aGroup[field + "_2"]= this.getXulLabel(aGroup.groupType +
                                               "_" + field + "_2_", isTitle);
      }
      aGroup[field]=          this.getXulLabel(aGroup.groupType +
                                               "_" + field + "_",   isTitle);
    }
  },
  getXulLabel: function(sId, isTitle)
  {
    sId += isTitle ? "Label" : "Value";
    var elem= document.getElementById(sId);
	...
    return elem;
  },
  ...
  showStatistic: function(aGroup, aField,  aSize)
  {
    if( aGroup.groupType == "border" ){
      if( aSize.length == 3 ){
        aGroup[aField + "_1"].setAttribute("value", aSize[1]);
        aGroup[aField + "_2"].setAttribute("value", aSize[2]);

        aSize= aSize[0];
      }else{
        throw "aSize.length != 3";
      }
    }
	...
  },
  

> >+  showStatistic: function(aGroup, aField,  aSize)
> ...
> >+    aGroup[aField].setAttribute("value", aSize);
> ...
> >+    aGroup[aField].setAttribute("class", nonzero ? "nonzero" : "");
> >   },
> 
> It doesn't make any sense for aGroup and aField to be two different parameters
> here.

That's right! I used them that way because of the way I was using them in the example right-above. But I see now It can be easily done without the 2 parameters, thanks.


> >+        <label id="position_x_Label" value="&xCoord.label;" class="title"/>
> >+        <label id="position_x_Value"/>
> 
> ...  Why do the first ones (the *Labels) even have ids? 

Due to the reasons explained above (be able to retrieve *Label elements), and also for having the chance to do fine-styling of them in themes/addons.
(Like the rest, these are only my humble point of view. Please feel free to advice on all this)

I hope I didn't take much more of your time with this.
Thanks a lot!
(In reply to comment #38)
> I write code like that because I'm thinking (pretty always) about
> easy-of-maintenance
...
> If It helps for making myself clearer, here is an example

I'm not really sure what that code is supposed to do (beyond what attachment 496994 [details] [diff] [review] does).  Can you explain again what you're trying to accomplish with it, and why it's better accomplished in that way rather than simply using getElementById?
(In reply to comment #36)
> > I'm not sure what you're going for here.  It looks like you might be trying to
> > make sure that the values line up across groupboxes.  If that's the point,
> > there's a better way to do it: set all the value columns to be flexible.  That
> > should work if you use it in conjunction with the approach that I described
> > above to get scrolling working.
> 
> Okp, the attached screenshot shows that implementation too, including the
> already-working scrolling thing (If I understand all of It correctly).

I can't see from attachment 497190 [details] that scrolling works, just to let you know.  To be clear, what I'm referring to is when there's not enough vertical space to show all the groupboxes, it should be possible to scroll up and down to see the rest, rather than the groupboxes at the bottom being clipped.

> But I
> find that the values lays too far of their respective titles this way. Please
> advice.

It's fine to not force the boxes' ends to align.  I just thought that's what you were shooting for, given the CSS you wrote.  If that wasn't your intention, there's still the issue of resolving what the following lines from attachment 496994 [details] [diff] [review] are supposed to be for:

>+column:not(.value-column) {
>+  min-width: 8em;
>+}

(In reply to comment #37)
> > +      this.showStatistic(aGroup, "screenX", ""); //hide title
> > +      this.showStatistic(aGroup, "screenY", ""); //hide title
> > 
> > I'm not sure "title" is the right word here. Space before "hide".
> 
> Okp, should I use 'label'?
> (I used 'title' wherever relevant, in order for me not getting confused between
> text referring to 'title'-labels visible to the user and other <label>
> elements.

"label" should be fine.  The values shouldn't be in labels, anyway; they should be in textboxes with readonly="true" class="plain".

> Inspired in Firefox 4 'titlebar' element too :-)

I don't know what that means.
(Assignee)

Comment 41

7 years ago
Created attachment 497219 [details]
Example screenshot of my original alignment intention.

(In reply to comment #40)
> (In reply to comment #36)
> > > I'm not sure what you're going for here.  It looks like you might be trying to
> > > make sure that the values line up across groupboxes.  If that's the point,
> > > there's a better way to do it: set all the value columns to be flexible.  That
> > > should work if you use it in conjunction with the approach that I described
> > > above to get scrolling working.
> > 
> > Okp, the attached screenshot shows that implementation too, including the
> > already-working scrolling thing (If I understand all of It correctly).
> 
> I can't see from attachment 497190 [details] that scrolling works, just to let you know. 
> To be clear, what I'm referring to is when there's not enough vertical space to
> show all the groupboxes, it should be possible to scroll up and down to see the
> rest, rather than the groupboxes at the bottom being clipped.

Yes, that's the scrolling that I've implemented (but didn't show in the previous screenshot). 
(I mentioned the scrolling before only because you talked about it working in conjunction to the flexible value columns. Please disregard that, I think next paragraph can make clear my original intention)

> > But I
> > find that the values lays too far of their respective titles this way. 
> > Please advice.
> 
> It's fine to not force the boxes' ends to align.  I just thought that's what
> you were shooting for, given the CSS you wrote.  If that wasn't your intention,
> there's still the issue of resolving what the following lines from attachment
> 496994 [details] are supposed to be for:
> 
> >+column:not(.value-column) {
> >+  min-width: 8em;
> >+}

Sorry I didn't made it clear before. Yes, my intention with the |min-width: 8em;| plus |min-width: 4em;| was to align the values across groupboxes (well, to have a high chance of getting them aligned most of the time really).
That's what I'm showing now in this attachment. I'm including visible borders in this example hoping to make more clear my intention. The right half of the screenshot reflects the misalignment I get when I remove the |min-width: 8em;| rule.

Rules applied to left half of this example:
column.value-column {
  min-width: 4em;
  border: 1px solid green;
}
column:not(.value-column) {
  min-width: 8em;
  border: 1px solid blue;
}

(ps: I'll do also the requested change from 4em to 5em. In general, I do agree with all subjects I haven't answered)


> > Inspired in Firefox 4 'titlebar' element too :-)
> 
> I don't know what that means.

I meant the 'titlebar' name given to a new outstanding element in Fx4 pushed ahead my idea of speaking about 'titles' when referring to label texts here.
(In reply to comment #41)
> > there's still the issue of resolving what the following lines from attachment
> > 496994 [details] are supposed to be for:
> > 
> > >+column:not(.value-column) {
> > >+  min-width: 8em;
> > >+}
> 
> Sorry I didn't made it clear before. Yes, my intention with the |min-width:
> 8em;| plus |min-width: 4em;| was to align the values across groupboxes (well,
> to have a high chance of getting them aligned most of the time really).

Except that doesn't actually work.  In order to guarantee that the specified width can accommodate the text, you need to use 1 em per character, I believe.  At that point, you're better off actually aligning them using natural widths—at least, that's the case for my system.  I suspect it would be even more pronounced if on your system.

Comment 43

7 years ago
(In reply to comment #42)
> Except that doesn't actually work.  In order to guarantee that the specified
> width can accommodate the text, you need to use 1 em per character, I believe. 

Note that - confusingly - em is a height unit, not a width unit. 1em is defined in CSS to be the same as the *line height*, while we have ch as a width unit, defined as 1ch being the *average character width* of the used font. We don't have any unit that guarantees all characters fit, but using ch units, you're more likely to catch it somewhat safely than using em units.
(Assignee)

Comment 44

7 years ago
(In reply to comment #43 - Robert Kaiser)
> (In reply to comment #42)
> > Except that doesn't actually work.  In order to guarantee that the specified
> > width can accommodate the text, you need to use 1 em per character, I believe. 
> 
> Note that - confusingly - em is a height unit, not a width unit. 1em is defined
> in CSS to be the same as the *line height*, while we have ch as a width unit,
> defined as 1ch being the *average character width* of the used font. We don't
> have any unit that guarantees all characters fit, but using ch units, you're
> more likely to catch it somewhat safely than using em units.

Thank you Robert. Yes, ch units (https://developer.mozilla.org/en/CSS/length) worked consistently in my system. I can use them if We continue to go this way.


(In reply to comment #42 - Colby Russell)
> > Sorry I didn't made it clear before. Yes, my intention with the |min-width:
> > 8em;| plus |min-width: 4em;| was to align the values across groupboxes (well,
> > to have a high chance of getting them aligned most of the time really).
> 
> Except that doesn't actually work.  In order to guarantee that the specified
> width can accommodate the text, you need to use 1 em per character, I believe. 
> At that point, you're better off actually aligning them using natural widths—at
> least, that's the case for my system.  I suspect it would be even more
> pronounced if on your system.

Ok, no prob. 
I'm not sure what 'natural widths' means here (my poor English maybe). 
What would you suggest to do here?
(In reply to comment #44)
> I'm not sure what 'natural widths' means here (my poor English maybe).

I was referring to the natural widths of the boxes based on their contents, without explicitly setting widths.

> What would you suggest to do here?

Find a cross-platform, cross-locale way to do alignment or lose the rule.  Comment 35 contains a suggestion for one approach.
(Assignee)

Comment 46

7 years ago
(In reply to comment #45)
> > What would you suggest to do here?
> 
> Find a cross-platform, cross-locale way to do alignment or lose the rule. 
> Comment 35 contains a suggestion for one approach.

No prob.
Well, I can't find a cross-locale way to do that. (Not without a XUL refactoring, using only one grid and removing the groupboxes).
About Comment 35 approach: I've modified already this rule (bumped to 5em) (1): |.value-column{min-width: 5em;}|. Also I've tried setting all the value columns to be flexible and that renders like left-half of screenshot in attachment 497190 [details] .
So, If I'm understanding all well up to here, I can simply drop this rule |column:not(.value-column){min-width: 8em;}|

(1) If this rule stays |.value-column{min-width: 5em;}| I found better to change it to use 'ch' units |.value-column{min-width: 8ch;}|, if you're ok with that.


(I'm also preparing an answer to Comment 39)
(In reply to comment #46)
> Well, I can't find a cross-locale way to do that.
...
> Also I've tried setting all the value columns
> to be flexible and that renders like left-half of screenshot in attachment
> 497190 [details] .

I'm confused.  Is it working or not?

> So, If I'm understanding all well up to here, I can simply drop this rule
> |column:not(.value-column){min-width: 8em;}|
> 
> (1) If this rule stays |.value-column{min-width: 5em;}| I found better to
> change it to use 'ch' units

Yeah, that's good.

> |.value-column{min-width: 8ch;}|

Why the change to 8?
(Assignee)

Comment 48

7 years ago
Created attachment 499982 [details]
Screenshot with last proposed changes. (comments follow)
Attachment #493630 - Attachment is obsolete: true
Attachment #496992 - Attachment is obsolete: true
Attachment #497190 - Attachment is obsolete: true
Attachment #497219 - Attachment is obsolete: true
(Assignee)

Comment 49

7 years ago
(In reply to comment #47)
> (In reply to comment #46)
> > Well, I can't find a cross-locale way to do that.
> ...
> > Also I've tried setting all the value columns
> > to be flexible and that renders like left-half of screenshot in attachment
> > 497190 [details] .
> 
> I'm confused.  Is it working or not?

I think not. The flexible value-columns leaves the actual data column located too far from theirs respective labels (comment #36).

> > So, If I'm understanding all well up to here, I can simply drop this rule
> > |column:not(.value-column){min-width: 8em;}|
> > 
> > (1) If this rule stays |.value-column{min-width: 5em;}| I found better to
> > change it to use 'ch' units
> 
> Yeah, that's good.
> 
> > |.value-column{min-width: 8ch;}|
> 
> Why the change to 8?

In my system, 8ch is the same width as 5em, but the 8ch value seems consistent for me, because that value is 1ch more that the exact width needed for the *bold-styled* '1000' value (see attachment 499982 [details]).  (7ch would be 7x the width of the -non bold- '0' char, and that is very similar to the width of that 4 -bold- digits mentioned)

But, as I see now, we can drop this second rule also |.value-column{min-width: 8ch;}|, because those 2 rules were only an attempt to align values across groups.
So, without those, boxmodel will look like new attachment 499982 [details].
(Assignee)

Comment 50

7 years ago
(In reply to comment #39)
> (In reply to comment #38)
> > I write code like that because I'm thinking (pretty always) about
> > easy-of-maintenance
> ...
> > If It helps for making myself clearer, here is an example
> 
> I'm not really sure what that code is supposed to do (beyond what attachment
> 496994 [details] does).  Can you explain again what you're trying to accomplish with it,
> and why it's better accomplished in that way rather than simply using
> getElementById?


> (In comment #38 ...)
> If It helps for making myself clearer, here is an example of my (quickly
> written) code where I have the border-data splitted into separate <label>s (It
> can be useful to implement the 'border-left-width' option you mentioned):

This example code writes each border-data into these 3 separate <label> elements:
    <label id="border_top_Value"/>     « border-width
    <label id="border_top_1_Value"/>   « border-style
    <label id="border_top_2_Value"/>   « border-color
    ...

And here is the complete code. Here readBorderStyle() returns an array with the 3 properties, and then the array is processed by showStatistic().
(please disregard showStatistic's separate parameters 'aGroup' & 'aField', they can be reworked easily.)

> (In comment #38 ...)
> (Showed also with sample code for getting the *Label elements)
> 
>   initGroup: function(aGroup, aFields, isTitle)
>   {
>     if( ! isTitle ){
>       aGroup.titles = {}
>       aGroup.titles.groupType = aGroup.groupType;
>       this.initGroup(aGroup.titles, aFields, true);
>     }
>     for( var ix = 0; ix < aFields.length; ix ++ ){
>       var field = aFields[ix];
>       if( aGroup.groupType == "border" && ! isTitle ){ // NOT titles!
>         // better suffixes here: like '(border)_width', '_style', '_color'
>         aGroup[field + "_1"]= this.getXulLabel(aGroup.groupType +
>                                                "_" + field + "_1_", isTitle);
>         aGroup[field + "_2"]= this.getXulLabel(aGroup.groupType +
>                                                "_" + field + "_2_", isTitle);
>       }
>       aGroup[field]=          this.getXulLabel(aGroup.groupType +
>                                                "_" + field + "_",   isTitle);
>     }
>   },
>   getXulLabel: function(sId, isTitle)
>   {
>     sId += isTitle ? "Label" : "Value";
>     var elem= document.getElementById(sId);
>     ...
>     return elem;
>   },
>   ...
>   showStatistic: function(aGroup, aField,  aSize)
>   {
>     if( aGroup.groupType == "border" ){
>       if( aSize.length == 3 ){
>         aGroup[aField + "_1"].setAttribute("value", aSize[1]);
>         aGroup[aField + "_2"].setAttribute("value", aSize[2]);
> 
>         aSize= aSize[0];
>       }else{
>         throw "aSize.length != 3";
>       }
>     }
>     ...
>   },

  readBorderStyle: function(aStyle, aSide)
  {
    var style = aStyle.getPropertyCSSValue("border-"+aSide+"-style").cssText;
    if (!style || !style.length) {
      return ["none", "", ""];
    } else {
      return [aStyle.getPropertyCSSValue("border-"+aSide+"-width").cssText + "", 
              style + "",
              aStyle.getPropertyCSSValue("border-"+aSide+"-color").cssText];
    }
  }

Well, this (working) example code was my attempt to show my 'centralized-code' point of view. Ie, the 'centralized' code at initGroup() function (& other parts) lets me do & test changes like this quickly as ideas come to my mind :-)

Lets make clear that I'm not pretending to include these changes in this bug (unless you want to).  I'll be happy to include some of this in a follow-up or another bug if you found this useful.
(Assignee)

Comment 51

7 years ago
> > (In comment #37)
> > This really isn't necessary.  If you simply do
> > 
> > this.mPositionX = document.getElementById("position_x_Value");
> > this.mPositionY = document.getElementById("position_y_Value");
> > this.mPositionScreenX = document.getElementById("position_screenX_Value");
> 
> Okp, If you don't mind, please allow me to do it like this: (using the groups)
> 
>   this.groupPosition.x =       document.getElementById("positionXValue");
>   this.groupPosition.screenX = document.getElementById("positionScreenXValue");
>   ...
>   this.groupMargin.top =       document.getElementById("marginTopValue");
>   ...
> so, the original code will suffer fewer alterations ...

Back on main subject, please confirm If you are ok with the approach above and I'll upload the patch.
Thanks!
(In reply to comment #51)
> Back on main subject, please confirm If you are ok with the approach above and
> I'll upload the patch.

I can't really say until I've seen the patch.
(Assignee)

Comment 53

7 years ago
Created attachment 500125 [details]
Screenshot with fourth patch applied.
Attachment #499982 - Attachment is obsolete: true
(Assignee)

Comment 54

7 years ago
Created attachment 500126 [details] [diff] [review]
Fourth proposed patch, using textbox.


> textbox.plain {
>   margin-bottom: 2px !important;
> }

Make this property the same as label's (avoids misalignment)

> groupbox:not(#boxBorder) column.value-column {
>   max-width: 10ch;
> }

I find textbox's default size too wide. Textbox's size attr. leaves bold-styled boxes wider than the others introducing misalignment. 
I wonder if there some way to make a textbox grow dynamically depending on content, like label does.
Attachment #496994 - Attachment is obsolete: true
Attachment #500126 - Flags: review?(Sevenspade)
Comment on attachment 500126 [details] [diff] [review]
Fourth proposed patch, using textbox.


+    if( aSize === null || aSize === undefined ){

I'm pretty sure this will do the same thing as |aSize == null|.  Also, the space goes before the open parenthesis and after the close parenthesis.

+    var aGroup= this.groupPosition;

Space before the assignment operator.  Also, aGroup needs a better name here; it's not the name of argument, but it looks like one.  Similar comments apply to the aGroup assignments in the other show* functions.

+      this.showStatistic(aGroup.screenX, ""); // hide label
+      this.showStatistic(aGroup.screenY, ""); // hide label

This doesn't actually hide anything.  Please remove the comment or add the code to do hiding/unhiding. (~6 new lines of code, plus a couple of ids added to the appropriate rows.)

+              <label id="positionScreenXLabel" value="&screenXCoord.label;"
+                                                               class="title"/>

Please line up attributes after linebreaks starting with the first attribute on the previous line.  I.e., put class below id.

+textbox.plain {
+  margin-bottom: 2px !important;
+}

This isn't cross-platform.  (It doesn't align correctly on OS X, for example.)  Use align="baseline" for the grid row to get the contents to align.

+#enclosingBox {
+  -moz-box-flex: 1;
+  overflow: auto;
+}

Use flex="1" in the XUL instead of here.

(In reply to comment #54)
> > groupbox:not(#boxBorder) column.value-column {
> >   max-width: 10ch;
> > }
> 
> I find textbox's default size too wide. Textbox's size attr. leaves bold-styled
> boxes wider than the others introducing misalignment. 
> I wonder if there some way to make a textbox grow dynamically depending on
> content, like label does.

Yeah, the approach from comment 35 only works if you want to align the values across all groupboxes.

You could write a function that iterates through the textboxes in a group, then sets the size attribute on each to the number of chars displayed in the textbox with the longest value for that group:

function BMVr_NormalizeTextboxSize(aGroup) { … }

Then call that in the tail of showPositionStats, showDimensionStats, etc.
Attachment #500126 - Flags: review?(Sevenspade) → review-
(Assignee)

Comment 56

7 years ago
Created attachment 500554 [details] [diff] [review]
Fifth proposed patch.


(In reply to comment #55)
> Comment on attachment 500126 [details] [diff] [review]
> (In reply to comment #54)
> You could write a function that iterates through the textboxes in a group, then
> sets the size attribute on each to the number of chars displayed in the textbox
> with the longest value for that group:

I've found (now) that simply setting each textbox size attr. works!
Attachment #500126 - Attachment is obsolete: true
Attachment #500554 - Flags: review?(Sevenspade)
(In reply to comment #55)
> I'm pretty sure this will do the same thing as |aSize == null|.  Also, the
> space goes before the open parenthesis and after the close parenthesis.
better still: if (!aSize) {
(In reply to comment #57)
> (In reply to comment #55)
> > I'm pretty sure this will do the same thing as |aSize == null|.  Also, the
> > space goes before the open parenthesis and after the close parenthesis.
> better still: if (!aSize) {

That won't work for when aSize is 0.
(In reply to comment #58)
> That won't work for when aSize is 0.
D'oh!  That's what I get for doing a drive-by :)
Comment on attachment 500554 [details] [diff] [review]
Fifth proposed patch.

-  showStatGroup: function(aGroup)
+  showStatistic: function(anElement,  aSize)

aElement, not anElement.  (The "a" is for "argument"; it's not an article <http://en.wikipedia.org/wiki/Article_%28grammar%29>.)

+  showElement: function(anElement,  bShow)
   {
…
+    anElement.setAttribute("hidden", bShow ? "false" : "true");
   },

This method is superfluous; there's already a method that hides elements: the |hidden| setter <http://mxr.mozilla.org/mozilla-central/source/content/xul/content/src/nsXULElement.cpp#1922>.

+    var groupBox = this.groupPosition;

This isn't a groupbox element.  Call it |group| or something else, but not groupBox.
   
+      this.showStatistic(groupBox.x, this.mSubject.offsetLeft);
+      this.showStatistic(groupBox.y, this.mSubject.offsetTop);
+      this.showStatistic(groupBox.screenX, "");
+      this.showStatistic(groupBox.screenY, "");
+      this.showElement(groupBox.screenXLabel, false);
+      this.showElement(groupBox.screenYLabel, false);

Please hide/show the two rows instead of just the labels so there's no blank space after the x and y rows.  (There's also no need to set screenX and screenY to the empty string after this change.)

+<!ENTITY dimension.width.label  "box width">
+<!ENTITY dimension.height.label "box height">

Nit: name these something like dimensionWidth.label or boxWidth.label.

+groupbox#boxBorder column.value-column {
+  -moz-box-flex: 1;
+}

Use flex="1" in the XUL.
(Assignee)

Comment 61

7 years ago
Created attachment 500635 [details] [diff] [review]
Sixth proposed patch.


Updated patch as of comment #60
Attachment #500554 - Attachment is obsolete: true
Attachment #500635 - Flags: review?(Sevenspade)
Attachment #500554 - Flags: review?(Sevenspade)
(Assignee)

Comment 62

7 years ago
Created attachment 500648 [details] [diff] [review]
Proposed patch (6.1)


Updated patch as of comment #60 (+ whitespace adjustment from patch 6)
Attachment #500635 - Attachment is obsolete: true
Attachment #500648 - Flags: review?(Sevenspade)
Attachment #500635 - Flags: review?(Sevenspade)
Comment on attachment 500648 [details] [diff] [review]
Proposed patch (6.1)

There are 9 instances in this patch of spaces appearing at the end of the line.  Please remove those.

(In reply to comment #17)
> > +    var nonzero = aSize.indexOf("px") >= 0 ? aSize.indexOf("0px") != 0 : 
> > +                                             aSize != "0";> You could also write it as:
> 
> var str = aSize.toString();
> var nonzero = !!(str.indexOf("px") < 0 ? aSize : str.indexOf("0px"));

Even better:

var nonzero = aSize != 0 && str.indexOf("0px"));

>+    aElement.setAttribute("value", aSize);
>+
>+    var str = aSize.toString();
>+    var nonzero = !!(str.indexOf("px") < 0 ? aSize : str.indexOf("0px"));

Since we're explicitly calling toString and saving the result, to prevent two string conversions of aSize, please reorder this like:

var str = aSize.toString();
aElement.setAttribute("value", aSize);
var nonzero = aSize != 0 && str.indexOf("0px"));
Attachment #500648 - Flags: review?(Sevenspade) → review+
Sorry.  That should be:

var str = aSize.toString();
aElement.setAttribute("value", str);
var nonzero = aSize != 0 && str.indexOf("0px"));
(Assignee)

Comment 65

7 years ago
Created attachment 500721 [details] [diff] [review]
Proposed patch (6.5)


Updated patch as of comment #63
Attachment #500648 - Attachment is obsolete: true
Attachment #500721 - Flags: review?(Sevenspade)
Attachment #500721 - Flags: review?(Sevenspade) → review+

Comment 66

7 years ago
Javier, do you need help checking this patch in or do you have commit access yourself?
Keywords: checkin-needed
(Assignee)

Comment 67

7 years ago
Yes, I'd need help checking this in, because I don't have commit access.
Thank you.
You can add checkin-needed to a bug when you need someone to check in for you <https://developer.mozilla.org/En/Developer_Guide/How_to_Submit_a_Patch#Committing_the_Patch>.

http://hg.mozilla.org/dom-inspector/rev/4163ab44e89b
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
Blocks: 592530
Keywords: checkin-needed
(Assignee)

Comment 69

7 years ago
Yes, Phil did that already.

Thank you for everything!
(In reply to comment #69)
> Yes, Phil did that already.
> 
> Thank you for everything!
No, thank *you* for the patch!
Duplicate of this bug: 109482
You need to log in before you can comment on or make changes to this bug.