1292051 - Show box model properties under the box model

Status: VERIFIED FIXED

(DevTools :: Inspector, enhancement, P2)

Description

[[e-mail]]

Attachment: 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

Comment 1

[[e-mail]]

Attachment: 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?

Comment 2

[Helen V. Holmes (:helenvholmes) (:✨)]

(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.

Comment 3

[Helen V. Holmes (:helenvholmes) (:✨)]

Here's a small Codepen demo with the interaction: http://codepen.io/helenvholmes/pen/BzqKKP

Comment 4

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: 1289995-1.patch

Hi Fred, I have actually worked on this in Bug 1289995 originally.

Comment 5

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: 1289995-5.patch

Comment 6

[Brian Grinstead [:bgrins]]

(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

Comment 7

[Julian Descottes [:jdescottes]]

Inspector bug triage, filter on CLIMBING SHOES.

Comment 8

[[e-mail]]

Attachment: updated properties spec

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

Comment 9

[[e-mail]]

Gabriel, are you still working on this issue? 
We'd like move Box-model related work forward and this seems a blocker issue.

Comment 10

[Gabriel Luong [: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.

Comment 11

[Gabriel Luong [:gl] (ΦωΦ)]

Got the okay from Helen on moving forward with bgrin's suggestion

Comment 12

[[e-mail]]

-------------------------------------------
► 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

Comment 13

[[e-mail]]

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 :)

Comment 14

[[e-mail]]

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).

Comment 15

[Helen V. Holmes (:helenvholmes) (:✨)]

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.

Comment 16

[Brian Grinstead [:bgrins]]

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.

Comment 17

[Brian Grinstead [:bgrins]]

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)?

Comment 18

[Patrick Brosset <:pbro>]

- 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.

Comment 19

[Helen V. Holmes (:helenvholmes) (:✨)]

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.

Comment 20

[Brian Grinstead [:bgrins]]

(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

Comment 21

[Stanford Lockhart [:lockhart]]

Attachment: bug1292051-1.0.0-part1.patch

Comment 22

[Gabriel Luong [: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.

Comment 23

[Stanford Lockhart [:lockhart]]

Attachment: 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.

Comment 24

[Stanford Lockhart [:lockhart]]

Attachment: bug1292051-v3-part1.patch

eslint fixes

Comment 25

[Gabriel Luong [: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.

Comment 26

[Stanford Lockhart [:lockhart]]

Attachment: bug1292051-part1-v4.patch

Comment 27

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: 1292051.patch

Comment 28

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/429163a5fdc7
Show box model properties under the box model. r=gl

Comment 29

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/429163a5fdc7

Comment 30

[Gabriel Luong [:gl] (ΦωΦ)]

For QE, this only appears in the box model in the layout panel. This layout panel requires devtools.layoutview.enabled to be enabled.

Comment 31

[Stanford Lockhart [:lockhart]]

Attachment: Bug 1292051 - Part 2: Add unit tests for properties under the box model.

Review commit: https://reviewboard.mozilla.org/r/116268/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/116268/

Comment 32

[Stanford Lockhart [:lockhart]]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/116268/diff/1-2/

Comment 33

[Stanford Lockhart [:lockhart]]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/116268/diff/2-3/

Comment 34

[Cristian Comorasu, QA [:ccomorasu], Releae Desktop QA - Please contact [e-mail]]

Thank you Gabriel for the clarification.

Comment 35

[Gabriel Luong [:gl] (ΦωΦ)]

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

Comment 36

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2538e16328c7
Part 2: Add unit tests for properties under the box model. r=gl

Comment 37

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

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.

Comment 38

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: 1292051-unit-test.patch

Comment 39

[Gabriel Luong [:gl] (ΦωΦ)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=bc723e6699ffb0fed01821dd9f4b5d709274dfe8

Comment 40

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: 1292051-part-2

Comment 41

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ce9c47088a2e
Part 2: Add unit tests for properties under the box model. r=gl

Comment 42

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/ce9c47088a2e

Comment 43

[Cristian Comorasu, QA [:ccomorasu], Releae Desktop QA - Please contact [e-mail]]

I can confirm this issue is fixed. I verified using Fx55.0b3 using Windows 10 x64, macOS X 10.12.5 and Ubuntu 14.04LTS x64.

Cheers!