The default bug view has changed. See this FAQ.

Show box model properties under the box model

RESOLVED FIXED in Firefox 55

Status

()

Firefox
Developer Tools: Computed Styles Inspector
P2
enhancement
RESOLVED FIXED
8 months ago
4 hours ago

People

(Reporter: gasolin, Assigned: lockhart)

Tracking

(Depends on: 2 bugs, Blocks: 1 bug)

Trunk
Firefox 55
Points:
---
Dependency tree / graph
Bug Flags:
qe-verify +

Firefox Tracking Flags

(firefox55 fixed)

Details

Attachments

(5 attachments, 8 obsolete attachments)

(Reporter)

Description

8 months ago
Created attachment 8777696 [details]
UI spec

from UI spec in bug 1150496 , we'd like have a list below model view, which shows properties that are set or relevant
(Reporter)

Updated

8 months ago
Depends on: 1150498
(Reporter)

Updated

8 months ago
Depends on: 1150499
(Reporter)

Comment 1

8 months ago
Created attachment 8777731 [details]
current box model

In current box model it shows the total layout-element-size and the current position property at the bottom

Where the computed layout-element-size could be shown in new UI, and how it looks like?
Flags: needinfo?(hholmes)
(In reply to Fred Lin [:gasolin] from comment #1)
> Created attachment 8777731 [details]
> current box model
> 
> In current box model it shows the total layout-element-size and the current
> position property at the bottom
> 
> Where the computed layout-element-size could be shown in new UI, and how it
> looks like?

I think the best plan of action is to not change anything fundamental to the way the current Computed area looks so that people can still find layout properties that are computed the same way they always have.

Fred has a good point, though—what is more helpful to see underneath the Box Model related to positioning? User-set authored styles, or computed styles? We'd need to default to one for certain, but I think a nice enhancement would be being able to toggle on click:

https://cl.ly/3q1Q0M2D3Q2T

Brian, for your ni?: underneath the box model area in the UI spec attachment, would it be better to default to authored styles, or computed styles? I'm leaning toward authored but would like to hear your thoughts.
Flags: needinfo?(hholmes) → needinfo?(bgrinstead)
Here's a small Codepen demo with the interaction: http://codepen.io/helenvholmes/pen/BzqKKP
Created attachment 8777855 [details] [diff] [review]
1289995-1.patch

Hi Fred, I have actually worked on this in Bug 1289995 originally.
Created attachment 8777856 [details] [diff] [review]
1289995-5.patch

Updated

8 months ago
Assignee: nobody → gl
(In reply to Helen V. Holmes (:helenvholmes) (:✨)(pls ni?) from comment #2)
> (In reply to Fred Lin [:gasolin] from comment #1)
> > Created attachment 8777731 [details]
> > current box model
> > 
> > In current box model it shows the total layout-element-size and the current
> > position property at the bottom
> > 
> > Where the computed layout-element-size could be shown in new UI, and how it
> > looks like?
> 
> I think the best plan of action is to not change anything fundamental to the
> way the current Computed area looks so that people can still find layout
> properties that are computed the same way they always have.
> 
> Fred has a good point, though—what is more helpful to see underneath the Box
> Model related to positioning? User-set authored styles, or computed styles?
> We'd need to default to one for certain, but I think a nice enhancement
> would be being able to toggle on click:
> 
> https://cl.ly/3q1Q0M2D3Q2T
> 
> Brian, for your ni?: underneath the box model area in the UI spec
> attachment, would it be better to default to authored styles, or computed
> styles? I'm leaning toward authored but would like to hear your thoughts.

Since it's in the computed view I'd default it to computed styles.  I'm not sure I understand exactly what is meant by 'authored' styles in this context.  Is that about showing a list of all applied styles with source links, sorted by specificity like the computed view does when you expand a property? If so, I was thinking we should leave that to the main part of the computed UI which is meant for that kind of listing.

Also one general idea about the UI - I'd be interested to see what it'd look like if these could be in a single row or two to save vertical space, and to make it feel more like it's part of the box model area and not the computed view.  So something like:

-------------------------------------------
|                                         |
|                                         |
|               100x100                   |
|                                         |
|                                         |
-------------------------------------------

content-box | block | left | static | 0

-------------------------------------------
► font-family:                        Arial
Flags: needinfo?(bgrinstead)
(Reporter)

Updated

8 months ago
Blocks: 1150496
Inspector bug triage, filter on CLIMBING SHOES.
Severity: normal → enhancement
Priority: -- → P2
(Reporter)

Comment 8

7 months ago
Created attachment 8783470 [details]
updated properties spec

I found the new properties spec from helen's spec collection https://projects.invisionapp.com/share/9G5R8XCYZ#/screens/179720590
(Reporter)

Comment 9

7 months ago
Gabriel, are you still working on this issue? 
We'd like move Box-model related work forward and this seems a blocker issue.
Flags: needinfo?(gl)
(In reply to Fred Lin [:gasolin] from comment #9)
> Gabriel, are you still working on this issue? 
> We'd like move Box-model related work forward and this seems a blocker issue.

Hi Fred, which box-model related work would this block currently? Helen is away this entire week for UX analysis, so, I was unable to confirm whether or not we should move on with the design proposed:

-------------------------------------------
|                                         |
|                                         |
|               100x100                   |
|                                         |
|                                         |
-------------------------------------------

content-box | block | left | static | 0

The above work can probably be finished in about a day from me. Pinging bgrins as well if we should just move forward from here.
Status: NEW → ASSIGNED
Flags: needinfo?(gl)
Flags: needinfo?(gasolin)
Flags: needinfo?(bgrinstead)
Got the okay from Helen on moving forward with bgrin's suggestion
Flags: needinfo?(gasolin)
Flags: needinfo?(bgrinstead)
(Reporter)

Comment 12

7 months ago
-------------------------------------------
► font-family:                        Arial

I mean the computed-like part, which is a basis to display attributes like z-index, box-sizing, and related hyper link functions
(Reporter)

Comment 13

7 months ago
for hyper link functions, I mean the special link right of z-index and position properties (in spec)

in bug 1297108 there's also a plan to convert box-model view to react, so we'd like make sure we didn't conflict any others work when we kick start :)
(Reporter)

Comment 14

7 months ago
Current we have 

-------------------------------------------
|                                         |
|                                         |
|               100x100                   |
|                                         |
|                                         |
-------------------------------------------
 100x100                          relative

The right corner is position property, The left corner computed width/hight will contain `content + padding/border/margin width`, which is pretty useful. 


The proposal

-------------------------------------------
|                                         |
|                                         |
|               100x100                   |
|                                         |
|                                         |
-------------------------------------------

content-box | block | left | static | 0


Seems removed that property. I think when a web developer see the panel at first time, its hard to know what does those properties mean with this compact view. And its the reason why we neglect the left corner computed width/hight because its not intuitive to find the difference between computed width/hight and the content width/hight (if target element's border/margin does not specified).
Brian, do you have an opinion on comment #14?

Personally I think it might work just fine, but it's hard to say exactly without seeing it in practice.
Flags: needinfo?(bgrinstead)
My 2 cents: I'd be OK without showing the full dimensions in the bottom-left - the same can be seen also by highlighting the node.  Going to forward to Patrick though for a decision.
Flags: needinfo?(bgrinstead) → needinfo?(pbrosset)
I guess we should also consider the distinction between the core box model UI and where it is shown.  For instance, do these additional property values only make sense to show in the upcoming Layout panel but not when it's embedded in Computed View (or vice versa)?
- About authored vs. computed:

We should always present computed values in the computed-view sidebar tab. People come to this tab for this reason: seeing how the browser *understood* the value for a given property, or which default value it gave it. That's what computed values are useful for.
If people want to see what CSS *they* authored, then they should go to the rule-view.
Say they've typed 'position: bsolute' (note the typo), then the rule-view will show them exactly that, plus a warning sign saying that the value isn't valid, and the computed-view will show them what the browser did decide to choose instead. We should keep it this way.

- About which information we show next to the box-model:

I had originally given a few rough ideas in bug 1150496, and that's what Helen used in her UI spec.
I don't think we've really settled on a final list though, and we should do this here.

I believe it's useful for users to have a place where they can see everything that can affect the position and size of an element.

In the rule-view, you see all the rules as they were authored, so position or box-sizing may be there somewhere, but hard to find, require scrolling or searching.
Same for the computed-view, we show all styles, and they are not grouped logically, but only alphabetically sorted.

Which is why if we summarize the most important ones next to the box-model it'd be useful.
Here's the list I can think of:

  - position: relative, absolute, fixed or sticky.
    This is the obvious one, we already show it, and should continue to do so.
    As a nice addition, absolutely positioned elements are "contained" in their offset parents.
    Which is why it might be nice to also add a link to the offset parent. But more on this later.

  - box-sizing: content-box or border-box.
    This is a very important one, because it changes the way width/height are applied.
    In fact, the dimension we normally show in the bottom-left corner can be explained by this.
    So I believe it would still be important to show these dimensions.

  - width/height: what I said before.

  - top, left, bottom, right: for relative/absolute/sticky/fixed elements this is also important.
    But I don't think this should be displayed in the list below the box-model.
    Instead something like bug 940275 should be done.

  - display: block, inline, inline-block, grid, flex, table, ...
    This might be a nice one to add to, because margins and dimensions don't always apply the same depending on the type of display.

  - z-index: I had originally thought we could show this one too, but I'm not sure anymore.
    It doesn't influence the position or size of an element, only its stacking order.

  - float: left/right/offset-start/offset-end
    This definitely influences the position of an element, so it'd be good to have it there too.
    Additionally, floats are "contained" in Block formatting contexts, which might be also nice to link to like we do for offset parents.

==> So, let's settle for this list: position, box-sizing, width/height, display, float.

Now, if we do show these things on one line only, in a compact view like:
100x200, absolute, border-box, block, left
it might be hard for people (esp. beginners) to know what each of these values mean.

I see several solutions, I think Helen should decide:

  - The compact view (one line) is fine and useful, we go for it.
  - We go instead for a list of 'name:value', each on one line.
  - We don't show this information in the box-model that's in the computed-view, but keep it only for later in the new Layout panel.

When I originally filed bug 1150496, the box-model was still in its own tab, and it wasn't a problem to add all of these properties there.
Now that it's in the same tab as the computed-view, the more properties we add next to the box-model, the more that will push down the computed-view listing.

- About offset-parent and block formatting contexts.

Just to clarify a little bit these concepts: some CSS properties only have meaning when the element is within an other element. For example, position:absolute;top:100px;left:100px; will apply relatively to the closest "positioned" ancestor. If, for instance, in your list of parents, you have one of them that has position:relative; then the element is positioned relatively to this parent.

Same for floats and z-indexes. They apply within some sort of a context parent.

These notions are often unknown by beginners. It's hard to understand why a floated element sticks out of its container for example, that sort of stuff.
So, that's why I thought it'd be useful to show, for float and position, what is the element that forms the context for these properties next to their values.
Something like:

position    absolute    <div.parent>

Where <div.parent> would be something you can hover to highlight the element in the page (maybe a tooltip appears with a reason why it's a context), and you can click to select it in the inspector. Which is exactly how we show DOM elements in the console, or in the animation inspector, so it would be a nice pattern that people already know.

I don't think we can do this in the compact box-model version inside the computed-view though, this would have to be only in the Layout view.
Flags: needinfo?(pbrosset)
Or, option 4: Because we intend to show this expanded, more useful view in the Layout panel, and because all the information below it is duplicated in the computed panel, I think it's safe to remove this properties in the Computed panel. We'll show them in the Layout panel. This way we preserve a useful, clear Computed panel, but still have the ability to surface more useful information to our users.
(In reply to Patrick Brosset <:pbro> from comment #18)
> These notions are often unknown by beginners. It's hard to understand why a
> floated element sticks out of its container for example, that sort of stuff.
> So, that's why I thought it'd be useful to show, for float and position,
> what is the element that forms the context for these properties next to
> their values.
> Something like:
> 
> position    absolute    <div.parent>
> 
> Where <div.parent> would be something you can hover to highlight the element
> in the page (maybe a tooltip appears with a reason why it's a context), and
> you can click to select it in the inspector. Which is exactly how we show
> DOM elements in the console, or in the animation inspector, so it would be a
> nice pattern that people already know.

That's a really good idea - even if you know how it works already finding the next positioned parent is tedious so if we can surface that it would be helpful.  Would need to send the correct actor ID down to the client along with each NodeFront

Updated

4 months ago
Assignee: gl → nobody
Status: ASSIGNED → NEW
status-firefox52: --- → affected
status-firefox53: --- → affected
QA Contact: cristian.comorasu

Updated

2 months ago
Assignee: nobody → lockhart
Status: NEW → ASSIGNED
(Assignee)

Comment 21

2 months ago
Created attachment 8832245 [details] [diff] [review]
bug1292051-1.0.0-part1.patch
Attachment #8832245 - Flags: review?(gl)
Comment on attachment 8832245 [details] [diff] [review]
bug1292051-1.0.0-part1.patch

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

::: devtools/client/inspector/layout/components/BoxModel.js
@@ +46,5 @@
>        }),
>        BoxModelInfo({
>          boxModel,
> +      }),
> +      BoxModelProperties({

These new BoxModelProperties should only be shown in the new Layout panel in the future. We intend on keeping the Box Model in both the Computed view and Layout view, but the difference is that the Computed view will have a compact view. To make this possible, we need to introduce a prop such as showCompactMode(?), and check for !showCompactMode before rendering the BoxModelProperties.

@@ +47,5 @@
>        BoxModelInfo({
>          boxModel,
> +      }),
> +      BoxModelProperties({
> +        boxModel

s/boxModel/boxModel, 

We add the trailing comma to make it easier for the next person who might need to add another prop.

::: devtools/client/inspector/layout/components/BoxModelProperties.js
@@ +6,5 @@
> +
> +const {addons, createClass, createFactory, DOM: dom, PropTypes} =
> +    require("devtools/client/shared/vendor/react");
> +
> +const { LocalizationHelper } = require("devtools/shared/l10n");

Move this above line 7. I like to sort the requires() alphabetically according to their require path. Otherwise, separate out the requires into different groups if their require path has nothing in common.

@@ +23,5 @@
> +  propTypes: {
> +    boxModel: PropTypes.shape(Types.boxModel).isRequired,
> +  },
> +
> +  mixins: [addons.PureRenderMixin],

s/[addons.PureRenderMixin]/[ addons.PureRenderMixin ]

@@ +26,5 @@
> +
> +  mixins: [addons.PureRenderMixin],
> +
> +  onToggleExpander: function () {
> +    let isOpen = this.expander.hasAttribute("open");

Rather than storing the state using these attributes, we should store the 'expanded' state in the react component state. First by initializing the state using getInitialState(). Checkout the Accordion.js for an example

@@ +38,5 @@
> +    }
> +  },
> +
> +  render() {
> +    let {boxModel} = this.props;

Add spaces to separate the destructuring {} -> { boxModel }

@@ +39,5 @@
> +  },
> +
> +  render() {
> +    let {boxModel} = this.props;
> +    let {layout} = boxModel;

Same as above

@@ +44,5 @@
> +
> +    let layoutInfo = ["box-sizing", "display", "float",
> +                      "line-height", "position", "z-index"];
> +
> +    const properties = layoutInfo.map(function (info) {

You can use an arrow function inside of the map(). We also don't necessarily need to initialize this properties constant either since we can do this inplace on line 89, but I will leave it up to you on whether or not you prefer initializing the constant or not since it doesn't hurt readability either.

@@ +58,5 @@
> +      },
> +      dom.div(
> +        {
> +          className: "boxmodel-properties-header",
> +          onDoubleClick: this.onToggleExpander

I think onClick should be sufficient here.

@@ +64,5 @@
> +        dom.div(
> +          {
> +            className: "boxmodel-properties-expander expander theme-twisty expandable",
> +            onClick: this.onToggleExpander,
> +            open: " ",

We should be able to remove this attribute with component states.

@@ +81,5 @@
> +      ),
> +      dom.div(
> +        {
> +          className: "boxmodel-properties-wrapper",
> +          ref: (ref) => {

Typically you can pass in a name to ref. Such as ref: "wrapper" and reference it later using this.refs.wrapper. I hope we can avoid using refs with component states.

::: devtools/client/inspector/layout/components/ComputedProperty.js
@@ +4,5 @@
> +
> +"use strict";
> +
> +const {addons, createClass, DOM: dom, PropTypes} =
> +    require("devtools/client/shared/vendor/react");

Should only have 2 spaces for the indent at the front.

@@ +15,5 @@
> +    name: PropTypes.string.isRequired,
> +    value: PropTypes.string.isRequired
> +  },
> +
> +  mixins: [addons.PureRenderMixin],

s/[addons.PureRenderMixin]/[ addons.PureRenderMixin ]

@@ +18,5 @@
> +
> +  mixins: [addons.PureRenderMixin],
> +
> +  render() {
> +    const {name, value} = this.props;

s/{name, value}/{ name, value }

@@ +19,5 @@
> +  mixins: [addons.PureRenderMixin],
> +
> +  render() {
> +    const {name, value} = this.props;
> +    const onFocus = () => {

Move the onFocus handler out of render() into its own function definition onFocus() { ... } inside of the React component class.

::: devtools/client/locales/en-US/boxmodel.properties
@@ +35,5 @@
>  # tooltip that appears when hovering over the button that allows users to edit the
>  # position of an element in the page.
>  boxmodel.geometryButton.tooltip=Edit position
> +
> +boxmodel.propertiesLabel=Box Model Properties

We need to add a localization note.

::: devtools/client/themes/boxmodel.css
@@ +249,5 @@
>    display: flex;
>    align-items: center;
>  }
> +
> +/* Box Model Properties: contains properties like box-sizing and z-index */

Let's just say:
contains a list of relevant box model properties

::: devtools/server/actors/styles.js
@@ +786,5 @@
>        "z-index",
>        "box-sizing",
> +      "display",
> +      "float",
> +      "line-height"

I really like this addition of displaying the line-height.
Attachment #8832245 - Flags: review?(gl)
(Assignee)

Comment 23

2 months ago
Created attachment 8833449 [details] [diff] [review]
bug1292051-v2-part1.patch

I left the require statements as-is, they seemed to be in alphabetical order already.

I also left in the "open" attribute on the expander, since it is used in the css rules for determining which arrow to display.  The logic for it uses state though.
Attachment #8832245 - Attachment is obsolete: true
Attachment #8833449 - Flags: review?(gl)
(Assignee)

Comment 24

2 months ago
Created attachment 8833792 [details] [diff] [review]
bug1292051-v3-part1.patch

eslint fixes
Attachment #8833449 - Attachment is obsolete: true
Attachment #8833449 - Flags: review?(gl)
Attachment #8833792 - Flags: review?(gl)
Comment on attachment 8833792 [details] [diff] [review]
bug1292051-v3-part1.patch

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

Changes look good. I think it will be ready to land once the comments are addressed. Thanks for the hard work Alex!

::: devtools/client/inspector/layout/components/App.js
@@ +33,5 @@
>      onShowBoxModelHighlighter: PropTypes.func.isRequired,
>      onToggleGridHighlighter: PropTypes.func.isRequired,
>      onToggleShowGridLineNumbers: PropTypes.func.isRequired,
>      onToggleShowInfiniteLines: PropTypes.func.isRequired,
> +    showCompactMode: PropTypes.bool.isRequired,

I realize showBoxModelProperties might be a better name for clarity. Can we rename all showCompactMode to showBoxModelProperties. I personally like to bunch up all the handler functions at the bottom of the prop list. So, these handler functions would probably be the only exception to my typical alphabetical orderliness. That said can you move showBoxModelProperties to before onShowBoxModelEditor.

Also, we won't be passing in showCompactMode at this level. I would imagine we would specify showBoxModelProperties when we render BoxModel({ showBoxModelProperties: true, ... }) inside of render(). So, we can remove this as well.

::: devtools/client/inspector/layout/components/BoxModel.js
@@ +56,5 @@
>        }),
>        BoxModelInfo({
>          boxModel,
> +      }),
> +      propertiesComponent,

We can probably do this instead:

showBoxModelProperties ? 
BoxModelProperties({ ... })
: 
null

::: devtools/client/inspector/layout/components/BoxModelProperties.js
@@ +3,5 @@
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +"use strict";
> +
> +const {addons, createClass, createFactory, DOM: dom, PropTypes} =

Add the a space between the opening and closing braces { addons, ..., PropTypes }

@@ +34,5 @@
> +
> +  onToggleExpander: function () {
> +    let isOpen = this.state.isOpen;
> +    this.setState({
> +      isOpen: !isOpen,

We can remove the declaration of isOpen by simply replacing !isOpen with !this.state.isOpen

@@ +57,5 @@
> +      },
> +      dom.div(
> +        {
> +          className: "boxmodel-properties-header",
> +          onClick: this.onToggleExpander,

I have been mistaken before, but the header should indeed be onDoubleClick

@@ +61,5 @@
> +          onClick: this.onToggleExpander,
> +        },
> +        dom.div(
> +          {
> +            className: "boxmodel-properties-expander expander theme-twisty expandable",

You can remove the expander and expandable classes because we aren't toggling the visibility of this expander arrow compare to the computed view.

@@ +62,5 @@
> +        },
> +        dom.div(
> +          {
> +            className: "boxmodel-properties-expander expander theme-twisty expandable",
> +            onClick: this.onToggleExpander,

Put onClick below open.

@@ +77,5 @@
> +      ),
> +      dom.div(
> +        {
> +          className: "boxmodel-properties-wrapper",
> +          hidden: !this.state.isOpen,

We need a tabindex: 0 here to make the properties tabbable and focusable. I haven't really done enough in this area for the new box model to make it easier for keyboard navigation but will be fixing this soon. Adding tabindex: 0 might still not make it work properly until that prior work is done.

::: devtools/client/inspector/layout/components/ComputedProperty.js
@@ +3,5 @@
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +"use strict";
> +
> +const {addons, createClass, DOM: dom, PropTypes} =

Add the a space between the opening and closing braces { addons, ..., PropTypes }

@@ +28,5 @@
> +    return dom.div(
> +      {
> +        className: "property-view",
> +        tabindex: "0",
> +        ref: (container) => {

Let's use:

ref: "container" and this.refs.container in onFocus()

::: devtools/client/inspector/layout/layout.js
@@ +79,5 @@
>      let app = App({
>        /**
> +       * Hides components when in compact mode, like the properties under the box model
> +       */
> +      showCompactMode: false,

Rename to showBoxModelProperties.
Attachment #8833792 - Flags: review?(gl) → review+
(Assignee)

Comment 26

2 months ago
Created attachment 8834167 [details] [diff] [review]
bug1292051-part1-v4.patch
Attachment #8833792 - Attachment is obsolete: true
Attachment #8834167 - Flags: review+
Created attachment 8835021 [details] [diff] [review]
1292051.patch
Attachment #8777855 - Attachment is obsolete: true
Attachment #8777856 - Attachment is obsolete: true
Attachment #8834167 - Attachment is obsolete: true
Attachment #8835021 - Flags: review+

Updated

a month ago
Keywords: leave-open

Updated

a month ago
Summary: Show properties that are set or relevant → Show box model properties under the box model

Comment 28

a month ago
Pushed by gabriel.luong@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/429163a5fdc7
Show box model properties under the box model. r=gl

Comment 29

a month ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/429163a5fdc7

Updated

a month ago
Attachment #8835021 - Flags: checkin+
For QE, this only appears in the box model in the layout panel. This layout panel requires devtools.layoutview.enabled to be enabled.
Flags: qe-verify+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Thank you Gabriel for the clarification.
status-firefox54: --- → affected

Comment 35

a day ago
mozreview-review
Comment on attachment 8842400 [details]
Bug 1292051 - Part 2: Add unit tests for properties under the box model.

https://reviewboard.mozilla.org/r/116268/#review124778
Attachment #8842400 - Flags: review?(gl) → review+

Comment 36

a day ago
Pushed by gabriel.luong@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2538e16328c7
Part 2: Add unit tests for properties under the box model. r=gl
status-firefox51: affected → ---
status-firefox52: affected → ---
status-firefox53: affected → ---
status-firefox54: affected → ---
Keywords: leave-open
Sorry, this had to backed out for leaks in devtools/client/inspector/boxmodel/test/browser_boxmodel.js:

https://hg.mozilla.org/integration/mozilla-inbound/rev/9ee82a5b65220e5b412880ba95dd27166efe0531

Push with failures: https://treeherder.mozilla.org/logviewer.html#?job_id=85539164&repo=mozilla-inbound
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=85544067&repo=mozilla-inbound

00:43:28    ERROR - TEST-UNEXPECTED-FAIL | devtools/client/inspector/boxmodel/test/browser_boxmodel.js | leaked 4 window(s) until shutdown [url = about:blank]
00:43:28    ERROR - TEST-UNEXPECTED-FAIL | devtools/client/inspector/boxmodel/test/browser_boxmodel.js | leaked 1 window(s) until shutdown [url = chrome://devtools/content/inspector/inspector.xhtml]
00:43:28    ERROR - TEST-UNEXPECTED-FAIL | devtools/client/inspector/boxmodel/test/browser_boxmodel.js | leaked 1 window(s) until shutdown [url = about:devtools-toolbox]

Please fix the issue and submit an updated patch.
Flags: needinfo?(lockhart)
Created attachment 8850028 [details] [diff] [review]
1292051-unit-test.patch
Attachment #8842400 - Attachment is obsolete: true
Attachment #8850028 - Attachment description: 1292051.patch → 1292051-unit-test.patch
Attachment #8850028 - Attachment filename: 1292051.patch → 1292051-unit-test.patch
https://treeherder.mozilla.org/#/jobs?repo=try&revision=bc723e6699ffb0fed01821dd9f4b5d709274dfe8

Updated

23 hours ago
Flags: needinfo?(lockhart)
Created attachment 8850242 [details] [diff] [review]
1292051-part-2
Attachment #8850028 - Attachment is obsolete: true
Attachment #8850242 - Flags: review+

Comment 41

17 hours ago
Pushed by gabriel.luong@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ce9c47088a2e
Part 2: Add unit tests for properties under the box model. r=gl

Comment 42

4 hours ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/ce9c47088a2e
Status: ASSIGNED → RESOLVED
Last Resolved: 4 hours ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
You need to log in before you can comment on or make changes to this bug.