1289995 - Implement the basic design spec of the box model in the computed view

Status: VERIFIED FIXED

(DevTools :: Inspector, defect, P1)

Comment 1

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

Attachment: Part 1: Display the box-sizing, display, position and z-index properties in the box model

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)

Comment 2

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

Attachment: Part 2: Move the computed view toolbar back to the top

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.

Comment 3

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

Attachment: Part 3: Removes the outline to property name and values and the arrow beside the property values in the computed view

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)

Comment 4

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

Attachment: Part 4: Use the compact form of the box model

Comment 5

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

Attachment: Part 1: Display the box-sizing, display, position and z-index properties in the box model [1.0]

Comment 6

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

Attachment: Part 2: Move the computed view toolbar back to the top [1.0]

Comment 7

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

Attachment: Part 3: Removes the outline to property name and values and the arrow beside the property values in the computed view [1.0]

Comment 8

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

Attachment: Part 4: Use the compact form of the box model [1.0]

Comment 9

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

Attachment: Part 5: Make sure computed view tests are querying the computed property view [1.0]

Comment 10

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

Attachment: Part 6: Hide the layout view when filtering the computed view [1.0]

Comment 11

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

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

Comment 12

[Brian Grinstead [:bgrins]]

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

Comment 13

[Brian Grinstead [:bgrins]]

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?

Comment 14

[Brian Grinstead [:bgrins]]

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

Comment 15

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

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

Comment 16

[Brian Grinstead [:bgrins]]

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

Comment 17

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

Attachment: Part 3: Removes the outline to property name and values and the arrow beside the property values in the computed view [2.0]

Comment 18

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

Attachment: Part 6: Hide the layout view when filtering the computed view [2.0]

Comment 19

[Brian Grinstead [:bgrins]]

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

Comment 20

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

Attachment: Part 6: Hide the layout view when filtering the computed view [3.0]

Comment 21

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

Attachment: 1289995.patch

Comment 22

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

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

Comment 23

[Carsten Book [:Tomcat]]

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

Comment 24

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

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!