Helen V. Holmes (:helenvholmes) (:✨) Reporter
	Description • 9 years ago

The box model probably doesn't need its own panel. As its own panel, it contributes to devtools noise where I think there doesn't need to be. With five panels already enabled in the the inspector-rules area, I think it would be a good idea to fold the contents of the box model panel into the computed styles at the bottom.

	Patrick Brosset <:pbro>
	Comment 1 • 9 years ago

Long ago, I filed a lot bugs for potential improvements to the box model view: bug 1150496.
My intention was I thought the box model could become a lot better in explaining why a certain element appeared the way it did and where it did. And doing so required showing more information in and around the box model diagram.

I don't think moving the box model into the computed-style panel is incompatible with these improvements, so I'm not against this move. Plus, we do have too many tabs right now.

Maybe bgrins and gl have ideas about this.

	Brian Grinstead [:bgrins]
	Comment 2 • 9 years ago

(In reply to Helen V. Holmes (:helenvholmes) (:✨) from comment #0)
> The box model probably doesn't need its own panel. As its own panel, it
> contributes to devtools noise where I think there doesn't need to be. With
> five panels already enabled in the the inspector-rules area, I think it
> would be a good idea to fold the contents of the box model panel into the
> computed styles at the bottom.

Sure, that makes sense.  The computed panel already shows the computed value of these properties anyway.  The main downsides I can think of are that:
1) It will require some scrolling to get to (especially if 'browser styles' is enabled).  Although that's more accessible than clicking on a different tab.  The main annoyance would be if you are toggling between elements to compare their box model and you had to keep scrolling to the bottom each time.  That could be solved maybe by moving it to the top of the computed view, although that brings up issues with the filter box's positioning I think.
2) It might not as be discoverable since it's not surfaced in the top level UI

I'm all for reducing the number of tabs though.  Curious if anyone has thoughts on (1).  To further illustrate: I've had times where I know some element is really tall / causing overflow but don't know which element is so tall and which one is shorter and overflowing.  So I'll start at the bottom of the DOM tree and press 'up' continually while keeping an eye on the box model panel until it changes how I'm expecting.  If it were located at the bottom of the computed view, it will likely scroll out of view if one element has more computed styles listed than another.  In fact any movement at all would make it hard to keep an eye on the value I care about.

	Gabriel [:gl] (ΦωΦ) Assignee
	Comment 3 • 9 years ago

If we move it into the computed view, we should do what chrome is doing and move it to the top and shift the filter bar under the box model.

I also don't think it would be an idea to move the box model to also be under the rule view.

	Brian Grinstead [:bgrins]
	Comment 4 • 9 years ago

(In reply to Gabriel Luong [:gl] (ΦωΦ) from comment #3)
> If we move it into the computed view, we should do what chrome is doing and
> move it to the top and shift the filter bar under the box model.

That would change the convention we have for filter bars being pinned to the top as the content behind it scrolls (like in rule view and font inspector).  Also UI-wise it might be a little tricky given the controls are in a secondary toolbar that would just be floating around.  But yeah, moving it to the top would give it a fixed position which would resolve my concern.

Maybe it could be below the filter bar but somehow collapsible so that people could hide it if they didn't care about it?

	Helen V. Holmes (:helenvholmes) (:✨) Reporter
	Comment 5 • 9 years ago

(In reply to Brian Grinstead [:bgrins] from comment #4)
> (In reply to Gabriel Luong [:gl] (ΦωΦ) from comment #3)
> > If we move it into the computed view, we should do what chrome is doing and
> > move it to the top and shift the filter bar under the box model.
> 
> That would change the convention we have for filter bars being pinned to the
> top as the content behind it scrolls (like in rule view and font inspector).
> Also UI-wise it might be a little tricky given the controls are in a
> secondary toolbar that would just be floating around.  But yeah, moving it
> to the top would give it a fixed position which would resolve my concern.
I agree, I like this idea.

> Maybe it could be below the filter bar but somehow collapsible so that
> people could hide it if they didn't care about it?
I'm not totally certain I understand the concern re: the filter toolbar. Brian, could you elaborate?

	Brian Grinstead [:bgrins]
	Comment 6 • 9 years ago

(In reply to Helen V. Holmes (:helenvholmes) (:✨) from comment #5)
> > Maybe it could be below the filter bar but somehow collapsible so that
> > people could hide it if they didn't care about it?
> I'm not totally certain I understand the concern re: the filter toolbar.
> Brian, could you elaborate?

I mean that the filter bar lives inside a little sub-toolbar that is pinned directly below the tabs.  If we were to move it below the box model somehow, then (a) it might visually look weird to have that toolbar floating in between the box model thing and the rest of the rows.  And (b) the filter would scroll out of view instead of being pinned to the top, which is different behavior than our current filtering in rule view / font view.  Does that make sense?

	Sebastian Zartner [:sebo]
	Comment 7 • 9 years ago

How about making the box model fixed at the top and make its display togglable like the pseudo-classes panel?
The only issue I can see with this is that it's taking vertical space from the list of computed values.

Sebastian

	Helen V. Holmes (:helenvholmes) (:✨) Reporter
	Comment 8 • 9 years ago

(In reply to Brian Grinstead [:bgrins] from comment #6)
> (In reply to Helen V. Holmes (:helenvholmes) (:✨) from comment #5)
> > > Maybe it could be below the filter bar but somehow collapsible so that
> > > people could hide it if they didn't care about it?
> > I'm not totally certain I understand the concern re: the filter toolbar.
> > Brian, could you elaborate?
> 
> I mean that the filter bar lives inside a little sub-toolbar that is pinned
> directly below the tabs.  If we were to move it below the box model somehow,
> then (a) it might visually look weird to have that toolbar floating in
> between the box model thing and the rest of the rows.  And (b) the filter
> would scroll out of view instead of being pinned to the top, which is
> different behavior than our current filtering in rule view / font view. 
> Does that make sense?

Ah, I see. I agree that I don't think we should be moving the filter bar around, I think that should remain pinned. 

I like Sebastian's thoughts in Comment 7; would that make you feel about issue 1 that you mentioned in comment 2?

	Patrick Brosset <:pbro>
	Comment 9 • 9 years ago

Filter on CLIMBING SHOES

	Gabriel [:gl] (ΦωΦ) Assignee
	Comment 10 • 8 years ago

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

	Gabriel [:gl] (ΦωΦ) Assignee
	Comment 11 • 8 years ago

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

	Gabriel [:gl] (ΦωΦ) Assignee
	Comment 12 • 8 years ago

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

	Brian Grinstead [:bgrins]
	Comment 14 • 8 years ago

Comment on attachment 8766366 [details] [diff] [review]
1247729.patch [2.0]

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

Sending review to Julian

	Gabriel [:gl] (ΦωΦ) Assignee
	Comment 15 • 8 years ago

Notes about the patch:
- Added tabindex to the layout view editors and container
- Added tabindex to the computed view property container
- Needed to scrollIntoView in some of the computed view tests because focus/click doesn't work when it is not in view

	Julian Descottes [:jdescottes]
	Comment 16 • 8 years ago

Comment on attachment 8766366 [details] [diff] [review]
1247729.patch [2.0]

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

Looks good overall, thanks! Keeping the review flag to discuss a few open questions.

Some additional comments:
- still found two references to computedview.view : devtools/client/inspector/test/head.js::selectComputedView & devtools/client/inspector/shared/test/browser_styleinspector_refresh_when_active.js
- when we select a node that has no box model (such as a comment node) the layoutView is not updated and retains its previous values. It should be emptied or hidden I suppose. This is not a regression, we already had the issue before, but I feel like this patch makes the layout view much more visible to our users so this should either be addressed here or in a follow-up.
- comment #7 mentioned adding a way to toggle the layout view, I assume we are moving this to a follow up?
- can you ask a UI review from Helen?

::: devtools/client/inspector/inspector.xul
@@ -62,5 @@
>               label="&fontInspectorTitle;"
>               crop="end"
>               hidden="true"/>
> -        <tab id="sidebar-tab-layoutview"
> -             label="&layoutViewTitle;"

layoutViewTitle is not used anywhere else and can be removed from devtools/client/locales/en-US/layoutview.dtd

@@ +105,5 @@
>                        label="&browserStylesLabel;"/>
>            </html:div>
>  
> +          <html:div id="computedview-container">
> +            <html:div id="layout-wrapper" tabindex="0">

I think you can get rid of the layout-wrapper, it was only used to fill the height of the panel when the layout view had a dedicated panel.

::: devtools/client/inspector/layout/layout.js
@@ +554,3 @@
>      if (isActive) {
>        this.trackReflows();
> +      this.update();

Not sure about calling update() here. Looking at onNewNode, we are calling setActive() and then update(), so it's weird to have setActive() use update() as well.

In the init method, we are still listening to "layoutview-selected" which will not be fired. I think if you listen to "computedview-selected" instead, we can remove the call to this.update() here.

@@ +560,5 @@
>    },
>  
>    /**
>     * Compute the dimensions of the node and update the values in
> +   * the linspector.xul document.

nit: s/linspector/inspector

::: devtools/client/shared/test/browser_telemetry_sidebar.js
@@ +27,5 @@
>    info("Testing sidebar");
>  
>    let inspector = toolbox.getCurrentPanel();
>    let sidebarTools = ["ruleview", "computedview", "fontinspector",
> +                      "animationinspector"];

Should we also remove the telemetry entries for the layoutview in telemetry.js and Histograms.json ?

::: devtools/client/themes/layout.css
@@ +17,5 @@
>    /* "Contain" the absolutely positioned #layout-main element */
>    position: relative;
>  }
>  
> +@media (max-height: 250px) {

Now that the layout view has to share its panel with the computed view, I feel like the compact version is a better choice most of the time. What about increasing the max-height here ?

When the toolbox is docked to the side, this media query becomes irrelevant. Maybe the compact version should always be used when docked to the side. Also applicable when the toolbox width is lower than 700px.

	Julian Descottes [:jdescottes]
	Comment 17 • 8 years ago

Adding devdoc-needed. MDN pages such as https://developer.mozilla.org/en-US/docs/Tools/Page_Inspector/UI_Tour#Box_Model_view will have to be updated to reflect the new UI.

	Helen V. Holmes (:helenvholmes) (:✨) Reporter
	Comment 18 • 8 years ago

Comment on attachment 8766366 [details] [diff] [review]
1247729.patch [2.0]

Took a gander—

I think this will land more easily if we add the arrow for the entire Box model area so it can be collapsed: http://cl.ly/3d44471S2H3K

Let's add a "Box Model" label at the top with the collapsible arrow next to it. (This is the direction the designs are moving anyway: http://cl.ly/1n280L3W0r3o)

It seems like the width declaration for #propertyContainer is wrong. Seems like the horizontal scroll is getting chopped off at an odd point in the container. You can still scroll all the way to the right and read all the properties so it's not major, but it seems like it's been introduced by your patch when I compare to release Nightly.

Lastly, the spacing around the top and bottom of the box model area feel a little large. I think that if you cut the white space between the width/height/static declaration and the top, the width/height/static declaration and the box model, and the box model to the next section each respectively in half that area won't feel so large.

	Gabriel [:gl] (ΦωΦ) Assignee
	Comment 19 • 8 years ago

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

	Gabriel [:gl] (ΦωΦ) Assignee
	Comment 20 • 8 years ago

@jdescottes, this should fix most of your feedback. I am gonna defer the layout view size until I have more of a formal decision/spec from an ui-review with Helen.

	Gabriel [:gl] (ΦωΦ) Assignee
	Comment 22 • 8 years ago

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

	Julian Descottes [:jdescottes]
	Comment 23 • 8 years ago

Comment on attachment 8769026 [details] [diff] [review]
1247729.patch [4.0]

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

Looks good! Let's do a last round of review after getting the ui-review from Helen and addressing my comments below.

FWIW, I think the toggling is a great improvement compared to the previous version!

::: devtools/client/inspector/layout/layout.js
@@ +189,5 @@
>    this.inspector = inspector;
> +  this.doc = document;
> +  this.wrapper = this.doc.getElementById("layout-wrapper");
> +  this.container = this.doc.getElementById("layout-container");
> +  this.expander = this.doc.getElementById("layout-expander");

make sure wrapper, container and expander are set to null in destroy().

@@ +210,5 @@
>      this.onSidebarSelect = this.onSidebarSelect.bind(this);
>      this.inspector.sidebar.on("select", this.onSidebarSelect);
>  
> +    this.onToggleExpander = this.onToggleExpander.bind(this);
> +    this.expander.addEventListener("click", this.onToggleExpander);

remove event listener in destroy()

@@ +456,5 @@
>    },
>  
>    onSidebarSelect: function (e, sidebar) {
> +    this.setActive(sidebar === "computedview");
> +    return this.update();

Is this.update() really needed ? When we select the computedview, onNewNode is already called and should trigger the update, right?

@@ +483,5 @@
> +        !this.inspector.selection.isElementNode()) {
> +      this.wrapper.hidden = true;
> +      return promise.resolve(undefined);
> +    }
> +

suggestion: maybe move this logic to update(), since we already handle this.wrapper.hidden = false there ? 

IMO:
- if view is hidden -> do not update
- if view is visible but node is invalid -> hide wrapper and do not update
- if view is visible and node is valid -> show wrapper and update

Feel free to keep as is if you prefer!

::: devtools/client/themes/layout.css
@@ +16,5 @@
> +}
> +
> +@media (max-height: 250px) {
> +  #layout-wrapper {
> +    height: 190px;

Collapsing is not working when this media query applies.

If you resize the toolbox to have height < 250px and collapse the box-model view, this rule still applies and the container does not look collapsed.

	Helen V. Holmes (:helenvholmes) (:✨) Reporter
	Comment 24 • 8 years ago

Comment on attachment 8769026 [details] [diff] [review]
1247729.patch [4.0]

Overall looking pretty good! A few small comments:

- Currently there's a thin line around the Box Model area: https://cl.ly/0A1k0u3j3Q2j I think this can be removed for now. Eventually we'll have it but it will have additional functionality attached to it, so until that's implemented I think we only need the small gray dotted line separating the box model from the computed values.
- I notice that the width/height and position declarations are gone. Was that intentional? I think it would be best to leave them in if it was—future designs will display those values as did the past implementation, so I think the transition will be smoother if we leave them in.

	Gabriel [:gl] (ΦωΦ) Assignee
	Comment 25 • 8 years ago

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

	Helen V. Holmes (:helenvholmes) (:✨) Reporter
	Comment 29 • 8 years ago

Comment on attachment 8769759 [details] [diff] [review]
1247729.patch [7.0]

Looking good! One small last thing:

Can we align the box-model arrow with the other arrows so everything is vertically aligned? https://cl.ly/043n1t330615

	Gabriel [:gl] (ΦωΦ) Assignee
	Comment 31 • 8 years ago

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

	Julian Descottes [:jdescottes]
	Comment 32 • 8 years ago

Comment on attachment 8769979 [details] [diff] [review]
1247729.patch [8.0]

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

Implementation in layout.js looks good now, just have two more layout/ux issues I spotted while testing (sorry for not seeing those before :/)

First, the #layout-geometry-editor button used to be next to the position label. Now it is displayed next to the
"Box Model" header. Just want to make sure this is intentional?

Second, see my comment in computed.css

::: devtools/client/themes/computed.css
@@ +31,3 @@
>  #propertyContainer {
>    -moz-user-select: text;
>    overflow: auto;

There is a scrollbar issue when a property value forces the property container to overflow. For instance a long url for a background image.

e.g: about:home on a local build, and inspect the #brandLogo element. 

You should see a horizontal scrollbar (on #computedview-container). But if you scroll to the bottom with the vertical scrollbar, there is a second horizontal scrollbar (on #property-container). 

FWIW, the first horizontal scrollbar is caused by the margin-right label "auto" which is rotated and has a line-height of 132px. This already occurs in the standalone layout-view, so this is not a regression by itself, just more annoying now that the layout-view is embedded in the computed view.

Even without considering the first buggy horizontal scrollbar, the user should not have to scroll down to the bottom of the list in order to be able to scroll horizontally.

I think we should remove the overflow auto here, so that the overflow is entirely driven by #computedview-container. It means that the box-model can be scrolled out, but I think this is an acceptable compromise (unless you have a better solution of course!)

	Gabriel [:gl] (ΦωΦ) Assignee
	Comment 33 • 8 years ago

(In reply to Julian Descottes [:jdescottes] from comment #32)
> First, the #layout-geometry-editor button used to be next to the position
> label. Now it is displayed next to the
> "Box Model" header. Just want to make sure this is intentional?

@Helen, Need info, halp!

	Julian Descottes [:jdescottes]
	Comment 35 • 8 years ago

(In reply to Julian Descottes [:jdescottes] from comment #32)
> Comment on attachment 8769979 [details] [diff] [review]
> 
> First, the #layout-geometry-editor button used to be next to the position
> label. Now it is displayed next to the
> "Box Model" header. Just want to make sure this is intentional?
> 

Just as a reference, you can see the edit position "crosshair" icon displayed next to Box Model in the screenshot attached here.

	Helen V. Holmes (:helenvholmes) (:✨) Reporter
	Comment 36 • 8 years ago

(In reply to Gabriel Luong [:gl] (ΦωΦ) from comment #33)
> (In reply to Julian Descottes [:jdescottes] from comment #32)
> > First, the #layout-geometry-editor button used to be next to the position
> > label. Now it is displayed next to the
> > "Box Model" header. Just want to make sure this is intentional?
> 
> @Helen, Need info, halp!

I have it positioned there in my designs, so it was intentional from me. Julian, other than noticing that it had changed, is there any cause for concern putting it there that worries you? If so, we can move it back.

	Julian Descottes [:jdescottes]
	Comment 37 • 8 years ago

(In reply to Helen V. Holmes (:helenvholmes) (:✨)(pls ni?) from comment #36)
> (In reply to Gabriel Luong [:gl] (ΦωΦ) from comment #33)
> > (In reply to Julian Descottes [:jdescottes] from comment #32)
> > > First, the #layout-geometry-editor button used to be next to the position
> > > label. Now it is displayed next to the
> > > "Box Model" header. Just want to make sure this is intentional?
> > 
> > @Helen, Need info, halp!
> 
> I have it positioned there in my designs, so it was intentional from me.
> Julian, other than noticing that it had changed, is there any cause for
> concern putting it there that worries you? If so, we can move it back.

Hadn't noticed it in the mockups that's all, I'm fine with the new position. Since it's hidden for most elements (which is another problem) I thought we might have missed it.

	Helen V. Holmes (:helenvholmes) (:✨) Reporter
	Comment 38 • 8 years ago

(In reply to Julian Descottes [:jdescottes] from comment #37)
> Hadn't noticed it in the mockups that's all, I'm fine with the new position.
> Since it's hidden for most elements (which is another problem) I thought we
> might have missed it.

Ahhh, okay, cool! I attached some designs for some enhancements to the box model in bug 1150496 and it seems like Gabe has been working on them simultaneously, which is why a few things have popped up in this migration bug that perhaps weren't expected.

Sounds like you don't have to worry about this point, Gabe.

	Gabriel [:gl] (ΦωΦ) Assignee
	Comment 40 • 8 years ago

Comment on attachment 8770416 [details] [diff] [review]
1247729.patch [9.0]

@yzen: I added some tabindex/tabbable focus to the box model around the individual margin, padding and border editors. Can you comment if this is desirable?

	Julian Descottes [:jdescottes]
	Comment 41 • 8 years ago

Comment on attachment 8770416 [details] [diff] [review]
1247729.patch [9.0]

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

Thanks for addressing all my comments!
Long urls now have an ellipsis which is not ideal, but users can toggle the property and see the full image-url so I think this is ok.

	Yura Zenevich [:yzen] (please use "needinfo")
	Comment 42 • 8 years ago

(In reply to Gabriel Luong [:gl] (ΦωΦ) from comment #40)
> Comment on attachment 8770416 [details] [diff] [review]
> 1247729.patch [9.0]
> 
> @yzen: I added some tabindex/tabbable focus to the box model around the
> individual margin, padding and border editors. Can you comment if this is
> desirable?

Hi Gabriel, we actually have this bug for box model a11y - bug 1243045. nancy is currently working on it as we had devised a keyboard accessibility plan for it. Would it be alright to land keyboard and other a11y improvements as part of that patch?

	Pulsebot
	Comment 43 • 8 years ago

Pushed by gabriel.luong@gmail.com:
https://hg.mozilla.org/integration/fx-team/rev/a097b61f5810
Move the box model panel into the computed styles panel r=jdescottes

	Gabriel [:gl] (ΦωΦ) Assignee
	Comment 44 • 8 years ago

https://hg.mozilla.org/integration/fx-team/rev/a097b61f58102776941bd3fa0023092ac3f27a3f
Bug 1247729 - Move the box model panel into the computed styles panel r=jdescottes

	Gabriel [:gl] (ΦωΦ) Assignee
	Comment 45 • 8 years ago

(In reply to Yura Zenevich [:yzen] from comment #42)
> (In reply to Gabriel Luong [:gl] (ΦωΦ) from comment #40)
> > Comment on attachment 8770416 [details] [diff] [review]
> > 1247729.patch [9.0]
> > 
> > @yzen: I added some tabindex/tabbable focus to the box model around the
> > individual margin, padding and border editors. Can you comment if this is
> > desirable?
> 
> Hi Gabriel, we actually have this bug for box model a11y - bug 1243045.
> nancy is currently working on it as we had devised a keyboard accessibility
> plan for it. Would it be alright to land keyboard and other a11y
> improvements as part of that patch?

Yes, this would be fine. I have removed the tabindex I have originally added.

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 46 • 8 years ago

I had to back this out in https://hg.mozilla.org/integration/fx-team/rev/34a128489d88 for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=10522159&repo=fx-team

	Gabriel [:gl] (ΦωΦ) Assignee
	Comment 47 • 8 years ago

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

	Gabriel [:gl] (ΦωΦ) Assignee
	Comment 48 • 8 years ago

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

	Gabriel [:gl] (ΦωΦ) Assignee
	Comment 49 • 8 years ago

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

	Gabriel [:gl] (ΦωΦ) Assignee
	Comment 50 • 8 years ago

https://hg.mozilla.org/integration/fx-team/rev/48d51227912b8ec3600758d59a995521e2fbea1c
Bug 1247729 - Move the box model panel into the computed styles panel r=jdescottes

	Carsten Book [:Tomcat]
	Comment 51 • 8 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/48d51227912b

	Tanvir Rahman
	Comment 53 • 8 years ago

Verified the new feature on latest Aurora 50.0a2

Build ID 	20160824004001
User Agent 	Mozilla/5.0 (Windows NT 10.0; WOW64; rv:50.0) Gecko/20100101 Firefox/50.0
[bugday-20160824]

	Will Bamberg [:wbamberg]
	Comment 54 • 8 years ago

I've updated the pages at:

https://developer.mozilla.org/en-US/docs/Tools/Page_Inspector/How_to/Examine_and_edit_the_box_model
and
https://developer.mozilla.org/en-US/docs/Tools/Page_Inspector/UI_Tour#Computed_view

to describe the new arrangement. I'm going to mark this one dev-doc-complete, but please let me know if you think anything else is needed.

	Sebastian Zartner [:sebo]
	Comment 55 • 8 years ago

And I've added a note about this change to https://developer.mozilla.org/en-US/Firefox/Releases/50.

Sebastian