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

      No description provided.

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

Part 1:
- Adds the box-sizing, display, position and z-index properties in the box model
- Removed the total width and height label (OK'd by Helen)

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

Part 2:
- Move the computed view toolbar back to the top. Talked extensively with Helen about the placement of the computed view toolbar given these added layout properties. The decision reached was to move the toolbar back to the top.

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

Part 3: 
- Removes the outline to property name and values in the computed view. Right now if we click on the individual name and value there is a brief outline before the entire row is outlined/selected, this removes that brief outline.
- Remove the arrow beside the property values in the entire computed view to match with Helen's design (Also ok'd by Helen)

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

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

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

Comment on attachment 8775686 [details] [diff] [review]
Part 3: Removes the outline to property name and values and the arrow beside the property values in the computed view [1.0]

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

::: devtools/client/themes/computed.css
@@ +87,5 @@
>  .property-name {
>    overflow-x: hidden;
>    text-overflow: ellipsis;
>    white-space: nowrap;
> +  outline: 0 !important;

Why does this have to be important?  I don't see a focus outline when testing locally without the patch

@@ +103,5 @@
>  .property-value {
>    overflow-x: hidden;
>    text-overflow: ellipsis;
>    white-space: nowrap;
>    background-position: 2px center;

Looks like this background-position isn't needed anymore if there's no background image

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

Comment on attachment 8775684 [details] [diff] [review]
Part 1: Display the box-sizing, display, position and z-index properties in the box model [1.0]

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

::: devtools/client/inspector/layout/layout.js
@@ -612,4 @@
>        this._lastRequest = null;
>        let width = layout.width;
>        let height = layout.height;
> -      let newLabel = SHARED_L10N.getFormatStr("dimensions", width, height);

We should keep this localization around and use it instead of the hardcoded string here: https://dxr.mozilla.org/mozilla-central/source/devtools/client/inspector/layout/layout.js#674.

@@ +711,5 @@
> +    let layoutInfo = ["box-sizing", "display", "position", "z-index"];
> +
> +    this.layoutInfo.innerHTML = "";
> +
> +    for (let info of layoutInfo) {

This really feels like the elements here should be implemented in the computed view container instead.  Besides just duplicated markup / behavior, there's missing context menu items and keybindings since it's not a real PropertyView but just looks like one.

I'm not sure the goal here: should it "be" part of the computed view, or are these labels meant for something else?  

There's also a case where you get duplicated values with the computed view when having something like: data:text/html,<div style='box-sizing:border-box;'>hi</div>.

All in all, I'm not sure I have enough context on this new 'layout info' feature to give a good review.  Is there a design / spec you are working off of here?

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

Comment on attachment 8775707 [details] [diff] [review]
Part 6: Hide the layout view when filtering the computed view [1.0]

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

::: devtools/client/inspector/computed/computed.js
@@ +555,5 @@
>      this.searchClearButton.hidden = this.searchField.value.length === 0;
>      this._filterChangedTimeout = setTimeout(() => {
>        if (this.searchField.value.length > 0) {
>          this.searchField.setAttribute("filled", true);
> +        this.layoutView.wrapper.hidden = true;

I think instead of passing in the layoutView here, the computed view should emit an event(s) on the inspector, and the layout view should listen to that event and manage the hiding itself.  This fits in more closely with other events within the inspector (like computed-view-property-expanded) and keeps the two tools more separate

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

(In reply to Brian Grinstead [:bgrins] from comment #12)
> Comment on attachment 8775686 [details] [diff] [review]
> Part 3: Removes the outline to property name and values and the arrow beside
> the property values in the computed view [1.0]
> 
> Review of attachment 8775686 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: devtools/client/themes/computed.css
> @@ +87,5 @@
> >  .property-name {
> >    overflow-x: hidden;
> >    text-overflow: ellipsis;
> >    white-space: nowrap;
> > +  outline: 0 !important;
> 
> Why does this have to be important?  I don't see a focus outline when
> testing locally without the patch
> 

This outline is actually overwritten by the user agent. Without the important, clicking the property name or value will quickly flash an outline before https://dxr.mozilla.org/mozilla-central/source/devtools/client/inspector/computed/computed.js#985 will focus on the property-row. 

> @@ +103,5 @@
> >  .property-value {
> >    overflow-x: hidden;
> >    text-overflow: ellipsis;
> >    white-space: nowrap;
> >    background-position: 2px center;
> 
> Looks like this background-position isn't needed anymore if there's no
> background image

Removed.

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

Comment on attachment 8775686 [details] [diff] [review]
Part 3: Removes the outline to property name and values and the arrow beside the property values in the computed view [1.0]

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

Did you mean to upload the new version with the background position removed?  r+ with that change

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

Comment on attachment 8776076 [details] [diff] [review]
Part 6: Hide the layout view when filtering the computed view [2.0]

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

::: devtools/client/inspector/computed/test/browser_computed_search-filter_clear.js
@@ +54,4 @@
>    let propertyViews = computedView.propertyViews;
>    let searchField = computedView.searchField;
>    let searchClearButton = computedView.searchClearButton;
> +  let layoutWrapper = computedView.layoutView.wrapper;

This line should be deleted

::: devtools/client/inspector/layout/layout.js
@@ +221,5 @@
>      let header = this.doc.getElementById("layout-header");
>      header.addEventListener("dblclick", this.onToggleExpander);
>  
> +    this.onFilterComputedView = this.onFilterComputedView.bind(this);
> +    this.inspector.on("computed-view-filter-filled",

With this setup they should have the same event name and the layout view can bind onto just the one (since the arg to the event is controlling visibility).  Something like "computed-view-filter-changed".  Also, please add the event name to the list of other events in inspector-panel.js

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

https://hg.mozilla.org/integration/fx-team/rev/cb8fe696526cea65b8bbdcb6f62ae0a7c4d13145
Bug 1289995 - Implement the basic design spec of the box model in the computed view r=bgrins

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

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

	Cristian Comorasu, QA [:ccomorasu], Releae Desktop QA - Please contact mboldan@mozilla.com
	Comment 24 • 8 years ago

I can confirm the improvements have landed, I verified using Fx 50.1.0, build ID: 20161208153507, Fx 51.0b11, build ID: 20170103031119 and Fx 52.0a2, build ID: 20170105004018 and Fx53.0a1, build ID: 20170104030214, on Windows 10 x64, Mac OS X 10.11 and Ubuntu 14.04 LTS.

Cheers!