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

VERIFIED FIXED in Firefox 50

Status

defect
P1
normal
VERIFIED FIXED
3 years ago
Last year

People

(Reporter: gl, Assigned: gl)

Tracking

(Depends on 1 bug, Blocks 1 bug)

unspecified
Firefox 50
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox50 verified, firefox51 verified, firefox52 verified, firefox53 verified)

Details

Attachments

(1 attachment, 13 obsolete attachments)

16.96 KB, patch
Details | Diff | Splinter Review
No description provided.
Assignee: nobody → gl
Status: NEW → ASSIGNED
Priority: -- → P1
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)
Attachment #8775455 - Flags: review?(jdescottes)
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.
Attachment #8775456 - Flags: review?(jdescottes)
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)
Attachment #8775457 - Flags: review?(jdescottes)
Attachment #8775458 - Flags: review?(jdescottes)
Attachment #8775455 - Attachment is obsolete: true
Attachment #8775455 - Flags: review?(jdescottes)
Attachment #8775684 - Flags: review?(bgrinstead)
Attachment #8775456 - Attachment is obsolete: true
Attachment #8775456 - Flags: review?(jdescottes)
Attachment #8775685 - Flags: review?(bgrinstead)
Attachment #8775687 - Flags: review?(bgrinstead)
Attachment #8775458 - Attachment is obsolete: true
Attachment #8775458 - Flags: review?(jdescottes)
Attachment #8775685 - Flags: review?(bgrinstead) → review+
Attachment #8775687 - Flags: review?(bgrinstead) → review+
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
Attachment #8775686 - Flags: review?(bgrinstead)
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?
Attachment #8775684 - Flags: review?(bgrinstead)
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
Attachment #8775707 - Flags: review?(bgrinstead)
Blocks: 1150496
(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.
Attachment #8775686 - Flags: review?(bgrinstead)
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
Attachment #8775686 - Flags: review?(bgrinstead) → review+
Attachment #8775691 - Flags: review?(bgrinstead)
Attachment #8775707 - Attachment is obsolete: true
Attachment #8776076 - Flags: review?(bgrinstead)
Attachment #8775684 - Attachment is obsolete: true
Attachment #8775691 - Attachment is obsolete: true
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
Attachment #8776076 - Attachment is obsolete: true
Attachment #8776076 - Flags: review?(bgrinstead)
Attachment #8776082 - Flags: review?(bgrinstead)
Attachment #8776082 - Flags: review?(bgrinstead) → review+
Summary: Display the box-sizing, display, position and z-index properties in the box model → Implement the basic design spec of the box model in the computed view
Posted patch 1289995.patchSplinter Review
Attachment #8775685 - Attachment is obsolete: true
Attachment #8775687 - Attachment is obsolete: true
Attachment #8776058 - Attachment is obsolete: true
Attachment #8776082 - Attachment is obsolete: true
Attachment #8776096 - Flags: review+
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
https://hg.mozilla.org/mozilla-central/rev/cb8fe696526c
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 50
Depends on: 1327247
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!
Status: RESOLVED → VERIFIED
QA Contact: cristian.comorasu
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.