Closed Bug 940275 Opened 11 years ago Closed 7 years ago

Show top/left/right/bottom properties in the layout view

Categories

(DevTools :: Inspector, defect, P3)

Product:
DevTools
Component:
Inspector
Platform:
x86
All
Type:
defect

Tracking

(firefox55 verified)

Status:
VERIFIED FIXED
Milestone:
Firefox 55
Tracking Flags:
Tracking Status
firefox55 --- verified

People

(Reporter: paul, Assigned: lockhart, Mentored)

References

(Blocks 1 open bug)

Details

(Keywords: dev-doc-needed, Whiteboard: [good first bug][lang=html][lang=js][not super super easy])

Attachments

(4 files, 4 obsolete files)

UI spec
9.05 KB, image/png
Details
Bug 940275 - Part 1: Add positioning properties to box model
59 bytes, text/x-review-board-request
gl
: review+
Details
Bug 940275 - Part 2: Unit tests for boxmodel position.
59 bytes, text/x-review-board-request
Details
940275-1.patch
15.42 KB, patch
Details | Diff | Splinter Review 
      No description provided.
Blocks: gaia-devtools
Whiteboard: [good-first-bug][lang=html][lang=js][mentor=paul][not super super easy]
hi Paul, I would like to be assigned to this bug. I have good knowledge on html language but is it possible to provide some information on where to start, as this is my first bug.
(In reply to taj from comment #1)
> hi Paul, I would like to be assigned to this bug. I have good knowledge on
> html language but is it possible to provide some information on where to
> start, as this is my first bug.

Hi Taj,

The layout view is the view you see in the inspector tabs on the right (right panel here: http://i.imgur.com/rZZDe92.png ).

Code is located here: https://mxr.mozilla.org/mozilla-central/source/browser/devtools/layoutview/

You can see a similar patch here: bug 939840, patch v1.1.

The idea is to reproduce more or less what Firebug does. Which means adding 4 more numbers around the margin box.

Let me know if you have any questions.
Hi Paul, the layout view shows the bottom value of rectangle as 20, top, right and left values are 0. Do you want me to find the actual value for 0 and leave the b0ttom value as 20.
The 20px bottom value you see in the screenshot is the margin-bottom, not the bottom property.
Take a look at what Firebug does (make sure you inspect a "positioned" element though, otherwise the top/bottom/left/right are not shown).
You don't need to copy what's in Firebug, as this is not perfect either, especially the css style.
Whiteboard: [good-first-bug][lang=html][lang=js][mentor=paul][not super super easy] → [good first bug][lang=html][lang=js][mentor=paul][not super super easy]
Hi Paul,  suppose any body haven't taken the bug to fix. I would like to work on this.  As I'm new to this , can you help me in some cases?
1) you mean add 4 numbers out side from the margin box?
2) code location which you gave has a folder called test. what is the use of it?
3) How to test only that part without debugging all the source code?
Flags: needinfo?(paul)
(In reply to Dulanja Wijethunga from comment #6)
> Hi Paul,  suppose any body haven't taken the bug to fix. I would like to
> work on this.  As I'm new to this , can you help me in some cases?

I think Taj is working on that (see comment 3).

> 1) you mean add 4 numbers out side from the margin box?

Yes.

> 2) code location which you gave has a folder called test. what is the use of it?

It's for the mochitest: https://developer.mozilla.org/en/docs/Mochitest

> 3) How to test only that part without debugging all the source code?

Not sure to understand this question.
Flags: needinfo?(paul)
Attached patch bug299431.patch (obsolete) — — Splinter Review 
Hi Paul.I have tried to fix this bug.If you feel something is missing please guide me.This is my first bug.Feeling excited
Attached patch Bug940275.patch (obsolete) — — Splinter Review 

        Attachment #8352966 -
        Attachment is obsolete: true

    
    

    
  
(In reply to nil.15111993 from comment #8)
> Created attachment 8352966 [details] [diff] [review]
> bug299431.patch
> 
> Hi Paul.I have tried to fix this bug.If you feel something is missing please
> guide me.This is my first bug.Feeling excited

Are you sure that this is the right patch ?

    
    

    
  
(In reply to Girish Sharma [:Optimizer] from comment #10)
> (In reply to nil.15111993 from comment #8)
> > Created attachment 8352966 [details] [diff] [review]
> > bug299431.patch
> > 
> > Hi Paul.I have tried to fix this bug.If you feel something is missing please
> > guide me.This is my first bug.Feeling excited
> 
> Are you sure that this is the right patch ?
No.It not the patch for this by mistake i attached it.
I have attached another attachment Bug940275.patch this is what i want to submit

    
  

        Attachment #8352971 -
        Flags: feedback?(paul)

    
  

        Attachment #8352971 -
        Flags: review?(paul)

    
    

    
  
Comment on attachment 8352971 [details] [diff] [review]
Bug940275.patch

Hi Nilesh, sorry for the late reply (holidays and stuff…).

This patch shows the top/bottom/right/left CSS values (http://i.imgur.com/I30MdCU.png). But if the position is static, these values don't mean anything. Do we want to show the computed values? Do we want to show the used values? Should we use the getBoxQuads? Also, that makes the view messy. Shouldn't we put that somewhere else?

Mike, Julien, what do you guys think?

        Attachment #8352971 -
        Flags: review?(paul)

        Attachment #8352971 -
        Flags: feedback?(paul)
Flags: needinfo?(mratcliffe)
Flags: needinfo?(felash)

    
    

    
  
Here are the things that can be hard to grasp when developing CSS:

* which values we have for top/left/right/bottom (don't display it when it has no meaning)
* when absolutely positioning, which element is the positioning context?
* know where an element is in the page (so that means: computed values, but for every positioning layout, not only for static layout, )
Flags: needinfo?(felash)

    
    

    
  
(sorry, I pressed the button to soon).

3rd point: so it's the position relatively to the viewport

To me, this bug would be fine with the first point only, and we can file other bugs for the 2 other points.

    
    

    
  
(In reply to Paul Rouget [:paul] from comment #12)
> Comment on attachment 8352971 [details] [diff] [review]
> Bug940275.patch
> 
> Hi Nilesh, sorry for the late reply (holidays and stuff…).
> 
> This patch shows the top/bottom/right/left CSS values
> (http://i.imgur.com/I30MdCU.png). But if the position is static, these
> values don't mean anything. Do we want to show the computed values? Do we
> want to show the used values? Should we use the getBoxQuads? Also, that
> makes the view messy. Shouldn't we put that somewhere else?
> 
> Mike, Julien, what do you guys think?

We have had to make the border box the same style as the rest for the box model highlighter (we needed a physical area for mouseover to move the guides around the border when hovered).

With this change and the one I just mentioned the layout view looks even more cluttered so we should probably avoid putting this stuff in the layout view. Not sure what information would be most useful or where we could put it.
Flags: needinfo?(mratcliffe)

    
    

    
  
Hey, I'd like to work on this bug, but what's the point right now? CSS values, boundingClientRect or what? We can make it customizable by letting user to switch between them, too.

    
    

    
  
Hi Paul Rouget

I would want to help fix this bug, can you assign it to me please. I will be attempting to fix the bug with a team, thank you.

    
    

    
  
Hi Paul,

I have started to work on this bug and thank you for providing me with some information, however it appears that there are 3 files, view.js, view.css, and html file. Do I need to work on both view.js and view.css in order to fix the bug or working on view.js only will help in fixing the bug.

thank you

    
    

    
  
Hi Paul,

I have made a patch for this bug, which I have uploaded and was wondering if you could check it for me.

thank you
Flags: needinfo?(paul)
Mentor: paul
Whiteboard: [good first bug][lang=html][lang=js][mentor=paul][not super super easy] → [good first bug][lang=html][lang=js][not super super easy]

    
  

        Attachment #8352971 -
        Flags: feedback?(mratcliffe)
Flags: needinfo?(paul)

    
    

    
  
(In reply to taj from comment #19)
> Hi Paul,
> 
> I have made a patch for this bug, which I have uploaded and was wondering if
> you could check it for me.
> 
> thank you

I will take over as mentor as Paul is working on another project at the moment.

The patch is awesome but it looks like it is on an older version of Firefox. Can you rebase the patch so that it can be applied to fx-team? To clone that repo use:
hg clone http://hg.mozilla.org/integration/fx-team

When you have done this attach the patch and ask me for review.
Mentor: paul → mratcliffe

    
    

    
  
Hi Michael,I would like to be get assigned to this bug.I just went though the previous comments and proposals.I would like to work on this.Hope this will be going to my first bugfix.

    
    

    
  
(In reply to Anjana S from comment #21)
> Hi Michael,I would like to be get assigned to this bug.I just went though
> the previous comments and proposals.I would like to work on this.Hope this
> will be going to my first bugfix.

Of course, ask on #devtools if you have any questions.
Assignee: nobody → anjanasasindran123

    
    

    
  
Thank you for assigning me to this bug.It will be helpful if you could provide me some basic info on this Bug , so i could work on . Thank you :)

    
    

    
  
Hi,
 Refering the previous comments ,I guess we have to work on /mozilla-central/browser/devtools/layoutview$r .But in that I found these files -moz.build ,  view.css  view.js  view.xhtml and one directory named as test.So can you please tell me on which file I should work on.Please correct me if I'm wrong.

I didn't saw you in #devtools,forgive me if I'm not suppose ask these type of questions here.

Thank you:)

    
    

    
  
<p class="border top"><span data-box="border" class="editable" tooltip="border-top"></span></p>
      <p class="border right"><span data-box="border" class="editable" tooltip="border-right"></span></p>
      <p class="border bottom"><span data-box="border" class="editable" tooltip="border-bottom"></span></p>
      <p class="border left"><span data-box="border" class="editable" tooltip="border-left"></span></p>

      <p class="margin top"><span data-box="margin" class="editable" tooltip="margin-top"></span></p>
      <p class="margin right"><span data-box="margin" class="editable" tooltip="margin-right"></span></p>
      <p class="margin bottom"><span data-box="margin" class="editable" tooltip="margin-bottom"></span></p>
      <p class="margin left"><span data-box="margin" class="editable" tooltip="margin-left"></span></p>

      <p class="padding top"><span data-box="padding" class="editable" tooltip="padding-top"></span></p>
      <p class="padding right"><span data-box="padding" class="editable" tooltip="padding-right"></span></p>
      <p class="padding bottom"><span data-box="padding" class="editable" tooltip="padding-bottom"></span></p>
      <p class="padding left"><span data-box="padding" class="editable" tooltip="padding-left"></span></p>
this is a code snippet which I found in /mozilla-central/browser/devtools/layoutview$r/view.xhtml
Is this is area which I'm suppose to work?
Please reply me :)
Thank you.

    
    

    
  
Can i work on this bug ?

    
    

    
  
(In reply to Shekhar Prasad Rajak from comment #26)
> Can i work on this bug ?

It is assigned to Anjana at the moment... Anjana, are you still working on this?
Flags: needinfo?(anjanasasindran123)

    
    

    
  
Okay, seems like this is free for you to work on.
Assignee: anjanasasindran123 → shekharstudy
Flags: needinfo?(anjanasasindran123)
Blocks: 1150496

    
    

    
  
Unassigning for now.
Assignee: shekharstudy → nobody

    
    

    
  
I'd like to take this bug. I've written the code to show positions, and will provide a patch in a few hours. In the meantime, I've uploaded this preview: https://youtu.be/u9WBbqscLgE

During development, I found 1 issue I need clarification about, and 1 other bug:
1) When element has auto margin, computed style's getPropertyValue("right") returns the actual distance, even if "right" is specified in the css. This is different than how chrome behaves, and I'm not sure what the spec says about this case. I guess it's implemented in CssLogic.

This can be seen in the video, which has this url:
data:text/html,<style>div { position: absolute; top: 42px; right: 43px; bottom: 44px; left: 45px; height: 100.111px; width: 100px; border: 10px solid black; padding: 20px; margin: 30px auto;}</style><div></div>

2) When element has margin auto then editing the value in the layout view and pressing "Enter", the UI of layout view returns to "auto" even though the element did get the proper edited value. I guess this is a client-side bug of layout view. I can see how to fix that as well.

    
    

    
  
After going through bugs 1150496 and 1123851 I now see the bigger picture. Do you think it's still worth to add the simple functionality of top/right/bottom/left boxes as a first step?

    
    

    
  


  
Please review this patch. It contains the fix along with the necessary tests.

    
    

    
  
(In reply to Amit Zur from comment #31)
> After going through bugs 1150496 and 1123851 I now see the bigger picture.
> Do you think it's still worth to add the simple functionality of
> top/right/bottom/left boxes as a first step?
Yes, adding the top/right/bottom/left data in the box model view is still worth, in fact it completely fits in the broader picture described in bug 1150496 I think.

    
    

    
  
(In reply to Patrick Brosset [:pbrosset] [:patrick] [:pbro] from comment #33)
> (In reply to Amit Zur from comment #31)
> > After going through bugs 1150496 and 1123851 I now see the bigger picture.
> > Do you think it's still worth to add the simple functionality of
> > top/right/bottom/left boxes as a first step?
> Yes, adding the top/right/bottom/left data in the box model view is still
> worth, in fact it completely fits in the broader picture described in bug
> 1150496 I think.
So do you think the proposed solution in the screencast I've shared [0] and in the patch I attached [1] resolves this issue?

[0] https://youtu.be/u9WBbqscLgE
[1] attachment 8625175 [details] [diff] [review]

    
    

    
  
Any response to my comments would be highly appreciated

    
    

    
  
Amit, we were all at a workweek all last week and during last week-end everybody was traveling. Therefore please understand some delay in the answers. Devtools developers usually answer real quick, just let them come back to work :)

    
    

    
  
Thanks,
Your work is also highly appreciated :)

        Attachment #8352971 -
        Attachment is obsolete: true
Flags: needinfo?(pbrosset)

    
    

    
  
Comment on attachment 8625175 [details] [diff] [review]
patch for adding position values in box model layout view

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

Sorry for the delay Amit. Mike is the mentor on this bug, so you should flag him for review (I'll do it), but I'll do a quick fly-by review now.

Some general feedback:
- The code has changed since you did this patch unfortunately, so it doesn't apply anymore on the latest fx-team. Can you rebase your patch?
- I see some instances of inconsistent coding style, we started to use ESLint to avoid these, can you take a look at this: https://wiki.mozilla.org/DevTools/CodingStandards and check your changes against our ESLint rules?
- I think the layout needs some love, the left/right properties, when visible, are sort of too close to the edge of the panel and the top property is too close to the size and position of the element (in the header part)
- These 4 new numbers have no legend, the margin, border and padding regions do have one though, so it may be confusing for people to see these numbers with no context. Maybe we could just add another rectangle around the margin region, with a transparent background, that contains these positions. This way we would be able to add a legend for it, just like we do for the other.
- Finally, and most importantly, right now the panel shows the "computed" values for top/right/bottom/left, which means that even if the element was only positioned with "top:0;left:0;" then all 4 coordinates will be displayed. I think this is a bit misleading. I don't think we should show them. Making it possible to set these unset values by clicking on the field is still desirable I think, but it should be possible for users to determine if these values are set by looking at the UI. Right now it feels like all 4 values are always set.
- Oh, one more thing, if you hover over values in the layout view (like margin-top for example), a tooltip is shown with the selector, stylesheet, line number where this value is defined. This doesn't seem to work with the new position values.

::: browser/devtools/layoutview/view.js
@@ +170,5 @@
>        position: {selector: "#element-position",
>                   property: "position",
>                   value: undefined},
> +      top: {selector: ".position.top > span",
> +                    property: "top",

Please align the object properties vertically, like the other ones in this.map?

@@ +427,5 @@
>          return null;
>        }
>  
> +      let shouldShowPosition = /^(absolute|relative|fixed)$/.test(layout.position),
> +          positionRE = /^(top|right|bottom|left)$/;

Please move both of these regexps as const variables at the top of this file, next to the other ones?

        Attachment #8625175 -
        Flags: review?(mratcliffe)

        Attachment #8625175 -
        Flags: feedback+
Assignee: nobody → sendwithchibo
Flags: needinfo?(mratcliffe)

    
    

    
  
Comment on attachment 8625175 [details] [diff] [review]
patch for adding position values in box model layout view

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

Your patch is looking pretty good. Can you address Patrick's comments and then I will be happy to review the code for you?

        Attachment #8625175 -
        Flags: review?(mratcliffe)

    
    

    
  
Thanks for the thorough fly-by, Patrick. I'll address your comments and attach a new patch.

(In reply to Patrick Brosset [:pbrosset] [:pbro] from comment #38)
> - These 4 new numbers have no legend, the margin, border and padding regions
> do have one though, so it may be confusing for people to see these numbers
> with no context. Maybe we could just add another rectangle around the margin
> region, with a transparent background, that contains these positions. This
> way we would be able to add a legend for it, just like we do for the other.
If there's a box with a legend, then for static positioned elements should this box still appear? Should it disappear but still take up its space so that the layout won't jump when navigating between elements in the inspector?

> - Finally, and most importantly, right now the panel shows the "computed"
> values for top/right/bottom/left, which means that even if the element was
> only positioned with "top:0;left:0;" then all 4 coordinates will be
> displayed. I think this is a bit misleading. I don't think we should show
> them. Making it possible to set these unset values by clicking on the field
> is still desirable I think, but it should be possible for users to determine
> if these values are set by looking at the UI. Right now it feels like all 4
> values are always set.
I agree. But then there's a difference from border/margin/padding.
From PageStyleActor's getLayout I'm getting only the computed values. I'd need another API then, to get only the set values. Can you point me in the right direction?

    
    

    
  
(In reply to Amit Zur from comment #40)
> > - Finally, and most importantly, right now the panel shows the "computed"
> > values for top/right/bottom/left, which means that even if the element was
> > only positioned with "top:0;left:0;" then all 4 coordinates will be
> > displayed. I think this is a bit misleading. I don't think we should show
> > them. Making it possible to set these unset values by clicking on the field
> > is still desirable I think, but it should be possible for users to determine
> > if these values are set by looking at the UI. Right now it feels like all 4
> > values are always set.
> I agree. But then there's a difference from border/margin/padding.
> From PageStyleActor's getLayout I'm getting only the computed values. I'd
> need another API then, to get only the set values. Can you point me in the
> right direction?
Flagging Mike for an answer on this.
Flags: needinfo?(mratcliffe)

    
    

    
  
You should use:
document.getElementById("nodeId").style.getAuthoredPropertyValue("top");

The problem is that getAuthoredPropertyValue method is marked as Chrome only so you need to add it to the node actor (toolkit/devtools/server/actors/inspector.js). I would suggest adding it after get computedStyle(). The method should look something like this:

```
  getAuthoredPropertyValue: method(function(property) {
    return this.rawNode.style.getAuthoredPropertyValue(property);
  }, {
    request: {
      property: Arg(0, "string")
    },
    response: {
      value: RetVal("string")
    }
  }),
```

You can then access this via the node front:
```
let node = this.inspector.selection.nodeFront;
node.getAuthoredPropertyValue("top").then(value => {
  // Set the top value to whatever it is set to (could be "")
});
```
Flags: needinfo?(mratcliffe)

    
    

    
  
Fantastic! Do you think there should be some indication that values are authored and not computed? And since unauthored values should still be editable, should they be presented in a different way, say half transparent? With what value?

    
    

    
  
(In reply to Amit Zur from comment #43)
> Fantastic! Do you think there should be some indication that values are
> authored and not computed?

Since computed styles are a mixture of computed and calculated values many properties have some kind of custom value e.g. should font-family show all the values set or only the used value... maybe it should be the used font? The point is that computed style is a strange beast so I don't think we need a special indicator.

> And since unauthored values should still be
> editable, should they be presented in a different way, say half transparent?
> With what value?

Half transparent would give us accessibility issues for the partially sighted but how about a squiggly underline and a tooltip?

    
    

    
  
Oh, what value? unset values should be "unset"

    
    

    
  
I'm bumping into a few walls - 
1) How should static positioned elements be shown? Here are two alternatives:
https://youtu.be/9CoM2H0e3oM - keep place for the position rectangle
https://youtu.be/d9MajrndA74 - don't show position rectangle (don't mind the bug of the wrong offset in the layout)

2) getAuthoredPropertyValue keeps returning the computed value. I'm using the snippet from comment #42, and use it like so in styles.js:
```
for (let prop of [
      "top",
      "right",
      "bottom",
      "left"
    ]) {
      dump("p: " + prop + ": " + style.getAuthoredPropertyValue(prop) + "\n");
      layout[prop] = style.getAuthoredPropertyValue(prop);
    }
```
Could you verify?

3) Not a wall, but I couldn't see why PageStyleActor's getLayout method has these lines: 
```
for (let i in this.map) {
      let property = this.map[i].property;
      this.map[i].value = parseFloat(style.getPropertyValue(property));
    }
```

Mike, thanks for your help
Flags: needinfo?(mratcliffe)

    
    

    
  
(In reply to Michael Ratcliffe [:miker] [:mratcliffe] from comment #45)
> Oh, what value? unset values should be "unset"

I think auto is better since it's supported on all browsers.

    
    

    
  
(In reply to Amit Zur from comment #46)
> I'm bumping into a few walls - 
> 1) How should static positioned elements be shown? Here are two alternatives:
> https://youtu.be/9CoM2H0e3oM - keep place for the position rectangle
> https://youtu.be/d9MajrndA74 - don't show position rectangle (don't mind the
> bug of the wrong offset in the layout)
> 

The second looks best to me.

> 2) getAuthoredPropertyValue keeps returning the computed value. I'm using
> the snippet from comment #42, and use it like so in styles.js:
> ```
> for (let prop of [
>       "top",
>       "right",
>       "bottom",
>       "left"
>     ]) {
>       dump("p: " + prop + ": " + style.getAuthoredPropertyValue(prop) +
> "\n");
>       layout[prop] = style.getAuthoredPropertyValue(prop);
>     }
> ```
> Could you verify?
> 

Yes, I logged bug 958118 about this a while back. I assume that you are obtaining the style object using getComputedStyle? When you do that then you will get the computed properties. You need to use:
this.rawNode.style.getAuthoredPropertyValue(property)

> 3) Not a wall, but I couldn't see why PageStyleActor's getLayout method has
> these lines: 
> ```
> for (let i in this.map) {
>       let property = this.map[i].property;
>       this.map[i].value = parseFloat(style.getPropertyValue(property));
>     }
> ```
> 

Considering that this.map should be undefined here you should be safe to remove it.

> Mike, thanks for your help

It is a pleasure.
Flags: needinfo?(mratcliffe)

    
    

    
  


  
1) CSS - I made some major changes because everything was hard coded with numbers, and I wanted to hide the position box for static positions. So I introduced some variables that should also make the css file cleaner. Part of the change was because I wanted to use calc() for line-height, but that doesn't work and there's a bug 594933 on it. 
From a responsive design point - there are 2 breakpoints, one at 228px and one at 278px for cases with/without the position box.
I would love feedback on these changes.

2) PageStyleActor::getLayout - I'm taking the authoredPropertyValue from node.rawNode, but I'm getting an exception whenever there's a property that wasn't authored. Do you know why?

[Exception... "Failure"  nsresult: "0x80004005 (NS_ERROR_FAILURE)"  location: "JS frame :: resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/server/actors/styles.js :: PageStyleActor<.getLayout< :: line 829"  data: no]

3) Using getAuthoredPropertyValue returns "calc(...)" for cases where calc() was used. That string then shows up in the layout view. Is that a desired effect?

4) If one of top/right/bottom/left is not authored, I return "-" in getLayout. I thought that using "auto" is misleading since it's not consistent with "auto" margin, which is a real computed value.

5) I found that there's some glitches with the InplaceEditor, but I could figure out why. I press "Return" to submit the changes, but then the UI is reset and I have to reselect this element in the inspector for the layout to refresh.

        Attachment #8631623 -
        Flags: review?(mratcliffe)

    
    

    
  
Comment on attachment 8631623 [details] [diff] [review]
add position box for non-static positioned elements

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

(In reply to Amit Zur from comment #49)
> Created attachment 8631623 [details] [diff] [review]
> add position box for non-static positioned elements
> 
> 1) CSS - I made some major changes because everything was hard coded with
> numbers, and I wanted to hide the position box for static positions. So I
> introduced some variables that should also make the css file cleaner. Part
> of the change was because I wanted to use calc() for line-height, but that
> doesn't work and there's a bug 594933 on it. 
> From a responsive design point - there are 2 breakpoints, one at 228px and
> one at 278px for cases with/without the position box.
> I would love feedback on these changes.
> 

They look fine to me.

> 2) PageStyleActor::getLayout - I'm taking the authoredPropertyValue from
> node.rawNode, but I'm getting an exception whenever there's a property that
> wasn't authored. Do you know why?
> 
> [Exception... "Failure"  nsresult: "0x80004005 (NS_ERROR_FAILURE)" 
> location: "JS frame :: resource://gre/modules/commonjs/toolkit/loader.js ->
> resource://gre/modules/devtools/server/actors/styles.js ::
> PageStyleActor<.getLayout< :: line 829"  data: no]
> 

You are running into http://mxr.mozilla.org/mozilla-central/source/layout/style/nsDOMCSSDeclaration.cpp#208 ... it tries to get the authored value but if there isn't one it throws NS_ERROR_FAILURE. This is by design so anywhere that you use getAuthoredPropertyValue() it should be wrapped in a try / catch block.

> 3) Using getAuthoredPropertyValue returns "calc(...)" for cases where calc()
> was used. That string then shows up in the layout view. Is that a desired
> effect?
> 

I suppose so, if that is the property value then I guess that is what we should show... especially if the field is editable.

> 4) If one of top/right/bottom/left is not authored, I return "-" in
> getLayout. I thought that using "auto" is misleading since it's not
> consistent with "auto" margin, which is a real computed value.
> 

That makes sense.

> 5) I found that there's some glitches with the InplaceEditor, but I could
> figure out why. I press "Return" to submit the changes, but then the UI is
> reset and I have to reselect this element in the inspector for the layout to
> refresh.

The inplace editor applies changes when the focus changes. Try watching what happens to focus when the field changes.

::: browser/themes/shared/devtools/layoutview.css
@@ +54,3 @@
>  @media (max-height: 228px) {
> +  #main { 
> +    --box-size: 18px; 

NIT: Trailing spaces on these two lines.

@@ +282,4 @@
>    color: var(--theme-highlight-blue);
>  }
>  
> +#main.no-position #position { 

NIT: Trailing space

::: toolkit/devtools/server/actors/inspector.js
@@ +341,5 @@
>      return CssLogic.getComputedStyle(this.rawNode);
>    },
>  
> +  getAuthoredPropertyValue: method(function(property) {
> +    return this.rawNode.style.getAuthoredPropertyValue(property);

This should be in a try / catch block as getAuthoredPropertyValue() throws an exception when there is no authored value.

You should return "" on failure.

::: toolkit/devtools/server/actors/styles.js
@@ +826,5 @@
> +    ]) {
> +      // TODO: there shouldn't be a try/catch here, but right now there is an exception if there isn't an authored property value
> +      try {
> +        layout[prop] = style.getAuthoredPropertyValue(prop) || "-";
> +      } catch (ex)  { 

NIT: Trailing space

@@ +828,5 @@
> +      try {
> +        layout[prop] = style.getAuthoredPropertyValue(prop) || "-";
> +      } catch (ex)  { 
> +        layout[prop] = "-";
> +        dump(ex + "\n");

We should use console.error(ex) here instead of dump.

        Attachment #8631623 -
        Flags: review?(mratcliffe)

    
    

    
  
I'm still working on this. Currently there are 2 things I'm trying to go about:
1) In pageStyleActor, I can't get the values from node.rawNode.style.getAuthoredPropertyValue, since they might be defined in a stylesheet. Since I'm not reading it from the computed value, I would need to find the actual css rule that gave the element its property value. This sounds cumbersome. Either there should be some API to get that value, or I should use the computed value. WDYT?

BTW, in the product I'm working on I actually wrote this function, like this:

const domUtils = Cc["@mozilla.org/inspector/dom-utils;1"].getService(Ci.inIDOMUtils);
var rules = domUtils.getCSSStyleRules(elem), 
    ret = [];
for (var i = 0, ii = rules.Count(); i < ii; i++) {
    ret.push(rules.GetElementAt(i));
}
return ret;


2) Inplace Editor - still need to investigate why blur is resetting the other values, and other UI glitches occur.
Flags: needinfo?(mratcliffe)

    
    

    
  
Sorry, the code I wrote is only part of the function. This is the method for getting the "authored property value":

function getAuthoredPropertyValue(elem, prop) {
    var authoredStyles = getAuthoredStyles(elem), currValue;
    for (var j = authoredStyles.length - 1, jj = 0; j >= jj; j--) {
        currValue = authoredStyles[j].getPropertyValue(prop);
        if (currValue) {
            break;
        }
    }
    return currValue;
}

function getAuthoredStyles(elem) {
  var rules = domUtils.getCSSStyleRules(elem), 
    ret = [];
  for (var i = 0, ii = rules.Count(); i < ii; i++) {
    ret.push(rules.GetElementAt(i));
  }
  return ret;
}

    
    

    
  
(In reply to Amit Zur from comment #51)
> I'm still working on this. Currently there are 2 things I'm trying to go
> about:
> 1) In pageStyleActor, I can't get the values from
> node.rawNode.style.getAuthoredPropertyValue, since they might be defined in
> a stylesheet. Since I'm not reading it from the computed value, I would need
> to find the actual css rule that gave the element its property value. This
> sounds cumbersome. Either there should be some API to get that value, or I
> should use the computed value. WDYT?
> 

That API is domUtils.getCSSStyleRules(elem) but it is more complex than it first appears as we need to obey the option to display user agent rules or not among a whole host of other things. We deal with all of that in CssLogic.

You should use getMatchedSelectors(around line 438 & 954) instead. If there are no matched selectors simply use the authored value.
Flags: needinfo?(mratcliffe)

    
    

    
  
I assume I should use 'user' in the third parameter to getMatchedSelectors, right?
Any idea how I can execute actor methods from the browser toolbox's console?
I tried something like this:
layoutview.inspector.pageStyle.getMatchedSelectors(node, "color", "user").then(ret => { console.log(ret); })

but that didn't work out well.

    
    

    
  
(In reply to Amit Zur from comment #54)
> I assume I should use 'user' in the third parameter to getMatchedSelectors,
> right?

We really should be using the constants instead but whoever wrote this method probably didn't realize that they are available:
CssLogic.FILTER.USER
CssLogic.FILTER.UA

So blah.getMatchedSelectors(node, "color", { filter: CssLogic.FILTER.USER })

> Any idea how I can execute actor methods from the browser toolbox's console?
> I tried something like this:
> layoutview.inspector.pageStyle.getMatchedSelectors(node, "color",
> "user").then(ret => { console.log(ret); })
> 
> but that didn't work out well.

I am surprised that didn't work... maybe because the console is sandboxed.

I suppose you could try this so that you can at least see any error messages:
layoutview.inspector.pageStyle.getMatchedSelectors(node, "color", { filter: CssLogic.FILTER.USER }).then(console.log, console.error);

    
    

    
  
Thanks...
More functional questions arise as I'm getting the user defined styles. I'm still not confident that going with the user-written style is the natural solution. Since different units can be used, we must show them. This leads to a situation that when 'px' is used, we must show 'px' - which is inconsistent with the other box properties that omit the 'px'.
Here's a short video that shows the current implementation (sorry for the low res):
http://youtu.be/1bSikWLN0_o

    
    

    
  
(In reply to Amit Zur from comment #56)
> Thanks...
> More functional questions arise as I'm getting the user defined styles. I'm
> still not confident that going with the user-written style is the natural
> solution. Since different units can be used, we must show them. This leads
> to a situation that when 'px' is used, we must show 'px' - which is
> inconsistent with the other box properties that omit the 'px'.
> Here's a short video that shows the current implementation (sorry for the
> low res):
> http://youtu.be/1bSikWLN0_o

You are right... now that we are using authored styles the unit type becomes way more important.

All of the box model fields are editable but we display the computed value. This means that if you type 10pt in e.g. a border dimension and press enter it displays e.g. 240 which is not usually what the user wants.

I think we should display the values for all regions (top, right, bottom, left, margin, border, padding) like this:
if (has authored value) {
  display authored value along with units
} else if (top, right, bottom or left) {
  display auto
} else {
  display computed value + "px"
}

We should also rotate the text for the left and right values so that the box can retain it's dimensions... in layoutview.css:
.margin.right,
.border.right,
.padding.right {
  transform(90deg);
}

.margin.left,
.border.left,
.padding.left {
  transform(-90deg);
}

    
    

    
  
Let me just chime in on this. I know the conversation has gone on for quite some time already, and that you've provided several patches, which are much appreciated, but as you said, I'm also not confident of the solution so far.

The box model view so far always shows computed values (except when you click to edit, but that's a different discussion).
This means that if a margin isn't set with CSS, the box model view will show 0 (note that Chrome devtools also shows computed values, but only for those properties that are set, for unset properties, it shows "-").

So, if we want to be consistent with this, we should always show top, right, bottom and left for all positioned elements, whether or not these properties are set in CSS.
This would be by far the easiest solution in terms of coding involved.

Showing computed values in this view feels natural because this is mostly an "output" view, something that gives you information about an element after it's had its layout calculated by the browser. It's a tool to understand how a set of CSS rules (which you see in the rule view) get turned into a box part of the page. Some of the future plans for this view include showing the positioning context element, the z-index and stacking context element, the box-sizing region, etc... All of which would be "output" information too.
Basically, I think we should consider the box model view exactly like the computed view, except that it only shows layout-related properties, and it does so in a visual way.

Now, knowing which css rule got to define a given top/left/margin/border/whatever computed value is great (and we already have tooltips for this), and being able to edit the authored value in place is even better, but the primary thing the box model view should show is the browser computed values.

So, I'd vote for showing computed top/right/bottom/left property values (only for positioned elements) only.

In terms of UI, I like the fact that the position box is drawn around the margin, like other boxes, and that it's only visible for positioned elements. One suggestion though: I *think* it might look even better if the position box still took place even when hidden so that navigating from one unpositioned node to a positioned node doesn't make the box model view flicker too much (as you said in comment 40).

    
    

    
  
Email from Shivam Agarwal:
"Hey can u help me with this bug ? i am a newcomer and i want to contribute to the community."

@Amit: Sorry that our lack of planning seems to have put you off working on this bug... are you okay with Shivam taking a look?

    
    

    
  
Hi Michael,

I'd like to proceed with this bug, as I've invested a lot of time in both getting familiar with code as well as cracking some design aspects of it. In my attempts, some issues have surfaced, which led to Patrick's last remarks (comment 58).
Seems like we are closing in on a final solution, but we need to see that we cover all loose ends. I'd appreciate it if we can hop on a call to finalize the solution, and I'll proceed with creating a patch.

    
    

    
  
@Amit: Is there something that Patrick hasn't covered that you need to discuss?
Flags: needinfo?(sendwithchibo)

    
    

    
  
There is a discrepancy between comment 38 and comment 58. For unset properties is may be misleading and uninformative. E.g. top:0;left:0; showing right and bottom doesnt add any value. This may be the reason that chrome shows "-". 
Should we show values only for set properties?
Flags: needinfo?(sendwithchibo)

    
    

    
  
You are right... showing 0 for top, right, bottom and left is wrong so we should use "-" when they are unset. For margin, padding etc. we should show 0 when they are unset.

    
    

    
  
Ok, so to summarize:
1) Only show computed values
2) Don't show units (since computed values are in 'px' for all the properties)
3) For position properties only, show "-" if they are unset
4) Position box should be shown only for positioned elements, but should take place even when not shown.

Please confirm if this is the desired behavior. 

P.S. I must say I tend to agree with what chrome did, showing "-" for unset properties also for padding, margin and border. In the adaptive UI aspect, they also center the view within the panel, so there IS a slight movement of the boxes when moving between positioned and unpositioned elements.

    
    

    
  
Looks like Amit's request in comment 64 stayed unanswered. So, ni'ing Mike.

(In reply to Amit Zur from comment #64)
> Ok, so to summarize:
> 1) Only show computed values
> 2) Don't show units (since computed values are in 'px' for all the
> properties)
> 3) For position properties only, show "-" if they are unset
> 4) Position box should be shown only for positioned elements, but should
> take place even when not shown.
> 
> Please confirm if this is the desired behavior.

I didn't read the whole discussion, though I believe it's not necessary to display the position properties at all when they are not applicable.

> P.S. I must say I tend to agree with what chrome did, showing "-" for unset
> properties also for padding, margin and border.

This might make sense in regard of differenciating between 'not set' and 'set to zero'.

> In the adaptive UI aspect,
> they also center the view within the panel, so there IS a slight movement of
> the boxes when moving between positioned and unpositioned elements.

This can easily be avoided by always reserving the space for the position properties. (But only display them for positioned elements, as said above.)

Sebastian
Flags: needinfo?(mratcliffe)

    
    

    
  
(In reply to Amit Zur from comment #64)
> Ok, so to summarize:
> 1) Only show computed values
> 2) Don't show units (since computed values are in 'px' for all the
> properties)
> 3) For position properties only, show "-" if they are unset
> 4) Position box should be shown only for positioned elements, but should
> take place even when not shown.
> 
> Please confirm if this is the desired behavior. 
> 
> P.S. I must say I tend to agree with what chrome did, showing "-" for unset
> properties also for padding, margin and border. In the adaptive UI aspect,
> they also center the view within the panel, so there IS a slight movement of
> the boxes when moving between positioned and unpositioned elements.

This is exactly how it should be done... also see Sebastian's comments.
Flags: needinfo?(mratcliffe)
Priority: -- → P3

    
    

    
  
[good first bug] whiteboard -> keyword mass change
Keywords: good-first-bug

    
    

    
  

      
      
      
      

        Attached image
          
        UI spec
        — 
      

    


  
Do we want to implement the related UI spec in this issue?
Flags: needinfo?(pbrosset)

    
    

    
  
(In reply to Fred Lin [:gasolin] from comment #68)
> Created attachment 8783906 [details]
> UI spec
> 
> Do we want to implement the related UI spec in this issue?
Probably best filing a new one, this one is 3 years old, and has received a lot of discussion.
Also, the current UI spec [1] is pretty different from what we first envisaged here. So a new bug would be better.

[1] https://projects.invisionapp.com/d/main#/console/5476602/179720590/preview
Flags: needinfo?(pbrosset)

    
    

    
  
Invision link requires log in, could you attach a shared link?

Also, do you folks feel this is a good first bug? I think it is technically, but because of all the discussion around it and the visual impact it has, I sense it's getting a lot of spotlight that requires communication that normally fresh contributors like myself can't handle.

    
    

    
  
(In reply to Amit Zur from comment #70)
> Invision link requires log in, could you attach a shared link?
You should be able to access this one: https://projects.invisionapp.com/share/9G5R8XCYZ#/screens/179720590

    
    

    
      
  

    
    

    
  
I think it's a very nice design. I would like to emphasize how important it is to surface as much info on the context and decision made by the layout engine, e.g. z-index, stacking context.
The only reason this is not common knowledge among devs is that it's not accessible via any browser's devtools.

I loved the ideas conveyed in the design around inline-blocks. I think the best thing though would be to focus on flexbox. That would be both a competitive advantage but also and mainly a huge benefit to devs. There are a lot of common cases in layouts which are mixed flexbox and normal flow (e.g. both width and flex-basis) where surfacing the engine's final decision is super valuable to the user.
QA Contact: developer.tools → cristian.comorasu
Assignee: sendwithchibo → nobody

    
    

    
  
Hi Amir, 

I am unassigning you from the bug since there wasn't been any activity for awhile. I have a student who is interested in implementing this in the new box model that is written in react/redux. If you wanted to continue working on this, please let me know.

    
    

    
  
Hi,
I am new to this and this is my first bugfix. I just went through the previous comments and the new design proposal. I want to know if this is a good choice as my first bug. Also, I wanted to know if the same files need to be accessed as mentioned in the initial comments of the bug.

Thanks,
Ankita

    
    

    
  
(In reply to Ankita Tanwar from comment #75)
> Hi,
> I am new to this and this is my first bugfix. I just went through the
> previous comments and the new design proposal. I want to know if this is a
> good choice as my first bug. Also, I wanted to know if the same files need
> to be accessed as mentioned in the initial comments of the bug.
> 
> Thanks,
> Ankita

Sorry Ankita, I actually have an UCOSP student starte working on this.
Assignee: nobody → lockhart

    
  

        Attachment #8838179 -
        Flags: feedback?(gl)

    
    

    
  
Comment on attachment 8838179 [details]
Bug 940275 - Part 1: Add positioning properties to box model

PM'd you feedback

        Attachment #8838179 -
        Flags: feedback?(gl)

        Attachment #8625175 -
        Attachment is obsolete: true

        Attachment #8631623 -
        Attachment is obsolete: true

    
    

    
      
  

    
    

    
  
A patch for this bug has been submitted into MozReview.

Patrick, this patch uses computed values for all four directions, regardless of which ones are set.  This seems to be what the thread eventually decided on, but I just want to check that using the computed values for positioning is what we want.
Flags: needinfo?(pbrosset)

    
    

    
  
(In reply to Stanford Lockhart [:lockhart] from comment #80)
> A patch for this bug has been submitted into MozReview.
> 
> Patrick, this patch uses computed values for all four directions, regardless
> of which ones are set.  This seems to be what the thread eventually decided
> on, but I just want to check that using the computed values for positioning
> is what we want.
Yes it is. I just re-read this bug to remember the discussion we had. I think comment 58 and comment 64 are the best summary of the approach to take here.
Flags: needinfo?(pbrosset)

    
    

    
  
Comment on attachment 8838179 [details]
Bug 940275 - Part 1: Add positioning properties to box model

https://reviewboard.mozilla.org/r/113150/#review117990

Still has some problems if you switch to the dark theme. I would like to see the position-left/right editable fields to be positioned a bit closer to the line. I would add like 1 or 2px padding-left to the position-top/bottom.

::: devtools/client/inspector/boxmodel/components/BoxModelMain.js:88
(Diff revision 2)
>      return value;
>    },
>  
> +  getPositionValue(property) {
> +    let { layout } = this.props.boxModel;
> +    if (layout.position === "static") {

Add a new line before the if statement.

::: devtools/client/inspector/boxmodel/components/BoxModelMain.js:112
(Diff revision 2)
>    render() {
>      let { boxModel, onShowBoxModelEditor } = this.props;
>      let { layout } = boxModel;
>      let { height, width } = layout;
>  
> +    let displayPosition = layout.position != "static";

Move the position declarations under padding. I realize this was kinda alphabetically ordered so we might as well stick to it.

::: devtools/client/inspector/boxmodel/components/BoxModelMain.js:112
(Diff revision 2)
>    render() {
>      let { boxModel, onShowBoxModelEditor } = this.props;
>      let { layout } = boxModel;
>      let { height, width } = layout;
>  
> +    let displayPosition = layout.position != "static";

This should be:
layout.position && layout.position != "static"

::: devtools/client/inspector/boxmodel/components/BoxModelMain.js:142
(Diff revision 2)
>        {
>          className: "boxmodel-main",
>          onMouseOver: this.onHighlightMouseOver,
>          onMouseOut: this.props.onHideBoxModelHighlighter,
>        },
> +      displayPosition ? dom.span(

Format this such as:
displayPosition ?
  dom.span(
    {
      ...
    }
  )
  :
  null,

::: devtools/client/inspector/boxmodel/components/BoxModelMain.js:205
(Diff revision 2)
>  <span class="indent">>></span>            })
>  <span class="indent">>></span>          )
>  <span class="indent">>></span>        )
> +        )
>        ),
> +      displayPosition ? BoxModelEditable({

Same as above

::: devtools/client/inspector/boxmodel/components/BoxModelMain.js:212
(Diff revision 2)
> +        direction: "top",
> +        property: "position-top",
> +        textContent: positionTop,
> +        onShowBoxModelEditor,
> +      }) : null,
> +      displayPosition ? BoxModelEditable({

Same as above

::: devtools/client/inspector/boxmodel/components/BoxModelMain.js:219
(Diff revision 2)
> +        direction: "right",
> +        property: "position-right",
> +        textContent: positionRight,
> +        onShowBoxModelEditor,
> +      }) : null,
> +      displayPosition ? BoxModelEditable({

Same as above

::: devtools/client/inspector/boxmodel/components/BoxModelMain.js:226
(Diff revision 2)
> +        direction: "bottom",
> +        property: "position-bottom",
> +        textContent: positionBottom,
> +        onShowBoxModelEditor,
> +      }) : null,
> +      displayPosition ? BoxModelEditable({

Same as above

        Attachment #8838179 -
        Flags: review?(gl)

    
    

    
      
  


  

    
    

    
      
  

        Attachment #8843740 -
        Flags: review?(gl)
Status: NEW → ASSIGNED

    
    

    
  
Comment on attachment 8838179 [details]
Bug 940275 - Part 1: Add positioning properties to box model

https://reviewboard.mozilla.org/r/113150/#review119302

We also have some significant layout with the firebug theme that needs to be addressed.

::: devtools/client/themes/boxmodel.css:252
(Diff revisions 2 - 3)
> +  padding-left: 1px;
>  }
>  
>  .boxmodel-position.boxmodel-right,
>  .boxmodel-position.boxmodel-left {
> -  line-height: 30px;
> +  line-height: 25px;

Let's try a line-height of 15px.

        Attachment #8838179 -
        Flags: review?(gl)

    
    

    
  
Comment on attachment 8838179 [details]
Bug 940275 - Part 1: Add positioning properties to box model

https://reviewboard.mozilla.org/r/113150/#review119312

::: devtools/client/locales/en-US/boxmodel.properties:20
(Diff revision 3)
>  # displayed as a label.
>  boxmodel.title=Box Model
>  
> +# LOCALIZATION NOTE (boxmodel.position) This refers to the position in the box model and
> +# might be displayed as a label or as a tooltip.
> +boxmodel.position=position

"position" is a bit confusing, since to me it refers to the position property.

I would just omit the label, or show some more useful information.

    
    

    
  
(In reply to Tim Nguyen :ntim from comment #86)
> Comment on attachment 8838179 [details]
> Bug 940275 - Part 1: Add positioning properties to box model
> 
> https://reviewboard.mozilla.org/r/113150/#review119312
> 
> ::: devtools/client/locales/en-US/boxmodel.properties:20
> (Diff revision 3)
> >  # displayed as a label.
> >  boxmodel.title=Box Model
> >  
> > +# LOCALIZATION NOTE (boxmodel.position) This refers to the position in the box model and
> > +# might be displayed as a label or as a tooltip.
> > +boxmodel.position=position
> 
> "position" is a bit confusing, since to me it refers to the position
> property.
> 

I have to strongly disagree with this since we already imply top/left/bottom/right properties for each of the margin, border, and padding properties in the box model. Position would be no different.

> I would just omit the label, or show some more useful information.

Omitting the legend would probably cause more confusion. I would welcome alternative proposals backed with UX.

    
    

    
  
(In reply to Gabriel Luong [:gl][1 biz day review guarantee] (ΦωΦ) from comment #87)
> (In reply to Tim Nguyen :ntim from comment #86)
> > Comment on attachment 8838179 [details]
> > Bug 940275 - Part 1: Add positioning properties to box model
> > 
> > https://reviewboard.mozilla.org/r/113150/#review119312
> > 
> > ::: devtools/client/locales/en-US/boxmodel.properties:20
> > (Diff revision 3)
> > >  # displayed as a label.
> > >  boxmodel.title=Box Model
> > >  
> > > +# LOCALIZATION NOTE (boxmodel.position) This refers to the position in the box model and
> > > +# might be displayed as a label or as a tooltip.
> > > +boxmodel.position=position
> > 
> > "position" is a bit confusing, since to me it refers to the position
> > property.
> > 
> 
> I have to strongly disagree with this since we already imply
> top/left/bottom/right properties for each of the margin, border, and padding
> properties in the box model. Position would be no different.
...except the property names aren't position-{top/bottom/left/right}. While they are for border/padding/margin.

> > I would just omit the label, or show some more useful information.
> 
> Omitting the legend would probably cause more confusion. I would welcome
> alternative proposals backed with UX.

position itself is too broad, i.e a top offset of 100px from the parent element is different from a top offset of 100px from the viewport. I would display "position relative to: \n <element>", where <element> is either:

if the value of the position property of the current element is:
- relative, the element it's relative to is itself
- absolute, the element would be the first ancestor that has "position: relative", if not found, that just corresponds to the root element
- fixed, the element would be the viewport
- sticky <- this needs more careful treatment, I don't know the exact rules

But I really think "position" has not much meaning... the important thing is what those offsets are relative to.

    
    

    
  
Also, I would be careful with how the current code handles offset-{inline/block}-{start/end}.

    
    

    
  
> if the value of the position property of the current element is:
> - relative, the element it's relative to is itself
> - absolute, the element would be the first ancestor that has "position:
> relative", if not found, that just corresponds to the root element
> - fixed, the element would be the viewport
> - sticky <- this needs more careful treatment, I don't know the exact rules
> 
> But I really think "position" has not much meaning... the important thing is
> what those offsets are relative to.

Just heads up, we do have this kind of information in the geometry highlighter:

https://dxr.mozilla.org/mozilla-central/source/devtools/server/actors/highlighters/geometry-editor.js#94

I suspect we can re-use it here to display the offset parent.

    
    

    
  
We have talked about adding the parent context in a separate bug immediately following this one. This was originally spec'd out by Helen. The original goal of this bug was to be chrome parity.

In regards to the positioning label, we are gonna change it to "position: <value>". That said we will be adding the parent context in the top right corner that you can see in the better box model designs. I suspect this will come in part 3 or a follow up bug.

    
    

    
      
  

    
    

    
      
  

    
    

    
  
Comment on attachment 8838179 [details]
Bug 940275 - Part 1: Add positioning properties to box model

https://reviewboard.mozilla.org/r/113150/#review119630

::: devtools/client/inspector/boxmodel/components/BoxModelInfo.js
(Diff revision 4)
>        dom.span(
>          {
>            className: "boxmodel-element-size",
>          },
>          SHARED_L10N.getFormatStr("dimensions", width, height)
> -      ),

We want to keep this for now.

        Attachment #8838179 -
        Flags: review?(gl)

    
    

    
      
  

    
    

    
      
  

    
    

    
  
Comment on attachment 8838179 [details]
Bug 940275 - Part 1: Add positioning properties to box model

https://reviewboard.mozilla.org/r/113150/#review121502

So, I was mostly reviewing the CSS for a long time. We have 2 major issues on top of the changes I have marked:
- If you resize the sidebar to be smaller, the position-left/right will move upward.
- The box model also kinda overlaps with eachother, which was the not the case before in the computed view, but should also be fixed for the layout view.

::: devtools/client/themes/boxmodel.css:238
(Diff revision 5)
> +  color: var(--theme-highlight-purple);
> +}
> +
> +.boxmodel-position.boxmodel-top,
> +.boxmodel-position.boxmodel-bottom {
> +  width: 35px;

This looks better using the width: 21px that is defined elsewhere. We should do that by adding boxmodel-top/bottom into that rule

::: devtools/client/themes/boxmodel.css:239
(Diff revision 5)
> +}
> +
> +.boxmodel-position.boxmodel-top,
> +.boxmodel-position.boxmodel-bottom {
> +  width: 35px;
> +  margin: 0 calc(50% - 1px);

I suspect we can also use left values instead of this margib as well.

::: devtools/client/themes/boxmodel.css:248
(Diff revision 5)
> +
> +.boxmodel-position.boxmodel-right,
> +.boxmodel-position.boxmodel-left {
> +  line-height: 15px;
> +  width: 30px;
> +  margin: calc(11% - 1px) 0;

Should probably be using top values instead of margin.

::: devtools/client/themes/boxmodel.css:281
(Diff revision 5)
>  .boxmodel-legend[data-box="margin"] {
>    color: var(--theme-highlight-blue);
>  }
>  
> +.boxmodel-legend[data-box="position"] {
> +  color: var(--theme-highlight-blue);

We should use the same color as the position field colors. var(--theme-highlight-purple)

::: devtools/client/themes/boxmodel.css:282
(Diff revision 5)
>    color: var(--theme-highlight-blue);
>  }
>  
> +.boxmodel-legend[data-box="position"] {
> +  color: var(--theme-highlight-blue);
> +  margin: -20px 0;

Lets move this further to the left so it's at the left most edge of where position-left line might start, and have the margin-top kinda be align with the position top field value. 

Something like margin: -18px -9px;

        Attachment #8838179 -
        Flags: review?(gl)

    
    

    
      
  

    
    

    
      
  

    
    

    
  
Comment on attachment 8838179 [details]
Bug 940275 - Part 1: Add positioning properties to box model

https://reviewboard.mozilla.org/r/113150/#review122496

I actually fixed a lot of the CSS styles for you. So, no worries on fixing the issues just wanted to point out some of the changes needed.

::: devtools/client/themes/boxmodel.css:13
(Diff revision 6)
>  
>  .boxmodel-container {
>    /* The view will grow bigger as the window gets resized, until 400px */
>    max-width: 400px;
>    margin: 0px auto;
>    padding: 0;

Added an overflow: auto; here.

::: devtools/client/themes/boxmodel.css:41
(Diff revision 6)
> -     visible */
> -  background-color: white;
>    color: var(--theme-selection-color);
>    /* Make sure there is some space between the window's edges and the regions */
>    margin: 14px 14px 4px 14px;
>    width: calc(100% - 2 * 14px);

Added a min-width: 240px

::: devtools/client/themes/boxmodel.css:45
(Diff revision 6)
>    margin: 14px 14px 4px 14px;
>    width: calc(100% - 2 * 14px);
>  }
>  
> +.boxmodel-box {
> +  margin: 0 25px;

This can simply be margin: 25px and remove the 25px top/bottom margin on boxmodel-margins

::: devtools/client/themes/boxmodel.css:80
(Diff revision 6)
>  
>  /* Regions colors */
>  
>  .boxmodel-margins {
>    border-color: #edff64;
> +  margin: 25px 0;

We can remove this and just make margin: 25px at boxmodel-box.

::: devtools/client/themes/boxmodel.css:241
(Diff revision 6)
> +}
> +
> +.boxmodel-position.boxmodel-top,
> +.boxmodel-position.boxmodel-bottom {
> +  border-left: 1px solid var(--theme-highlight-purple);
> +  left: 50%;

Seems like left: 49.5% makes itthe border line more centered.

::: devtools/client/themes/boxmodel.css:278
(Diff revision 6)
>    margin: 2px 6px;
>    z-index: 1;
>  }
>  
>  .boxmodel-legend[data-box="margin"] {
> -  color: var(--theme-highlight-blue);
> +  color: var(--theme-highlight-purple);

Should stick to being blue

::: devtools/client/themes/boxmodel.css:286
(Diff revision 6)
> +.boxmodel-legend[data-box="position"] {
> +  color: var(--theme-highlight-purple);
> +  margin: -18px -9px;
> +}
> +
> +.boxmodel-legend .boxmodel-element-position {

This doesn't seem to be used anywhere.

        Attachment #8838179 -
        Flags: review?(gl) → review+

    
    

    
  
Pushed by gabriel.luong@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9ed72c2f16d8
Part 1: Add positioning properties to box model r=gl
Flags: qe-verify+
Keywords: leave-open

    
    

    
  
(In reply to Gabriel [:gl] (ΦωΦ) from comment #100)
> ::: devtools/client/themes/boxmodel.css:241
> (Diff revision 6)
> > +}
> > +
> > +.boxmodel-position.boxmodel-top,
> > +.boxmodel-position.boxmodel-bottom {
> > +  border-left: 1px solid var(--theme-highlight-purple);
> > +  left: 50%;
> 
> Seems like left: 49.5% makes itthe border line more centered.

calc(50% - 1px) probably makes more sense.

    
    

    
  
(In reply to Tim Nguyen :ntim from comment #103)
> (In reply to Gabriel [:gl] (ΦωΦ) from comment #100)
> > ::: devtools/client/themes/boxmodel.css:241
> > (Diff revision 6)
> > > +}
> > > +
> > > +.boxmodel-position.boxmodel-top,
> > > +.boxmodel-position.boxmodel-bottom {
> > > +  border-left: 1px solid var(--theme-highlight-purple);
> > > +  left: 50%;
> > 
> > Seems like left: 49.5% makes itthe border line more centered.
> 
> calc(50% - 1px) probably makes more sense.

That sounds good. Can you file a follow up for this fix, thanks.

    
  
Depends on: 1347619
No longer depends on: 1347619

    
    

    
  
Comment on attachment 8843740 [details]
Bug 940275 - Part 2: Unit tests for boxmodel position.

https://reviewboard.mozilla.org/r/117314/#review125598

Clearing review since we talked about this during the meeting

        Attachment #8843740 -
        Flags: review?(gl)
Status: ASSIGNED → RESOLVED
Closed: 7 years ago

          status-firefox55:
          --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55

    
    

    
  
I have reproduced this bug with Firefox nightly 28.0a1 (2013-11-19) on Windows 10, 64 Bit.

The Bug's fix is now verified on latest nightly 55.0a1.

Build ID 	20170404030204
User Agent 	Mozilla/5.0 (Windows NT 10.0; WOW64; rv:55.0) Gecko/20100101 Firefox/55.0

[bugday-20170405]

    
    

    
  
Thanks Md.Majedul isalm for verifying this issue. 

I confirm that the issue is no longer reproducible on Firefox (2017-04-06), under Mac OS X 10.12.1, or under Ubuntu 16.04x64.
I'm marking this issue Verified-Fixed.
Status: RESOLVED → VERIFIED

          status-firefox55:
          fixed → verified
Flags: qe-verify+
Keywords: good-first-bug

    
  
Product: Firefox → DevTools

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






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: