Closed
Bug 1289995
Opened 8 years ago
Closed 8 years ago
Implement the basic design spec of the box model in the computed view
Categories
(DevTools :: Inspector, defect, P1)
DevTools
Inspector
Tracking
(firefox50 verified, firefox51 verified, firefox52 verified, firefox53 verified)
VERIFIED
FIXED
Firefox 50
People
(Reporter: gl, Assigned: gl)
References
(Depends on 1 open bug, Blocks 1 open bug)
Details
Attachments
(1 file, 13 obsolete files)
(deleted),
patch
|
gl
:
review+
|
Details | Diff | Splinter Review |
No description provided.
Assignee | ||
Updated•8 years ago
|
Assignee: nobody → gl
Status: NEW → ASSIGNED
Priority: -- → P1
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)
Attachment #8775455 -
Flags: review?(jdescottes)
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.
Attachment #8775456 -
Flags: review?(jdescottes)
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)
Attachment #8775457 -
Flags: review?(jdescottes)
Assignee | ||
Comment 5•8 years ago
|
||
Attachment #8775455 -
Attachment is obsolete: true
Attachment #8775455 -
Flags: review?(jdescottes)
Attachment #8775684 -
Flags: review?(bgrinstead)
Assignee | ||
Comment 6•8 years ago
|
||
Attachment #8775456 -
Attachment is obsolete: true
Attachment #8775456 -
Flags: review?(jdescottes)
Attachment #8775685 -
Flags: review?(bgrinstead)
Assignee | ||
Comment 7•8 years ago
|
||
Attachment #8775457 -
Attachment is obsolete: true
Attachment #8775457 -
Flags: review?(jdescottes)
Attachment #8775686 -
Flags: review?(bgrinstead)
Assignee | ||
Updated•8 years ago
|
Attachment #8775458 -
Attachment is obsolete: true
Attachment #8775458 -
Flags: review?(jdescottes)
Assignee | ||
Comment 11•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=765286f8241b
Updated•8 years ago
|
Attachment #8775685 -
Flags: review?(bgrinstead) → review+
Updated•8 years ago
|
Attachment #8775687 -
Flags: review?(bgrinstead) → review+
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
Attachment #8775686 -
Flags: review?(bgrinstead)
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?
Attachment #8775684 -
Flags: review?(bgrinstead)
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
Attachment #8775707 -
Flags: review?(bgrinstead)
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.
Assignee | ||
Updated•8 years ago
|
Attachment #8775686 -
Flags: review?(bgrinstead)
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
Attachment #8775686 -
Flags: review?(bgrinstead) → review+
Assignee | ||
Updated•8 years ago
|
Attachment #8775691 -
Flags: review?(bgrinstead)
Assignee | ||
Comment 17•8 years ago
|
||
Attachment #8775686 -
Attachment is obsolete: true
Attachment #8776058 -
Flags: review+
Assignee | ||
Comment 18•8 years ago
|
||
Attachment #8775707 -
Attachment is obsolete: true
Attachment #8776076 -
Flags: review?(bgrinstead)
Assignee | ||
Updated•8 years ago
|
Attachment #8775684 -
Attachment is obsolete: true
Assignee | ||
Updated•8 years ago
|
Attachment #8775691 -
Attachment is obsolete: true
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
Assignee | ||
Comment 20•8 years ago
|
||
Attachment #8776076 -
Attachment is obsolete: true
Attachment #8776076 -
Flags: review?(bgrinstead)
Attachment #8776082 -
Flags: review?(bgrinstead)
Updated•8 years ago
|
Attachment #8776082 -
Flags: review?(bgrinstead) → review+
Assignee | ||
Updated•8 years ago
|
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
Assignee | ||
Comment 21•8 years ago
|
||
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+
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
Comment 23•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/cb8fe696526c
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox50:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 50
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!
Status: RESOLVED → VERIFIED
status-firefox52:
--- → verified
status-firefox53:
--- → verified
QA Contact: cristian.comorasu
Updated•8 years ago
|
status-firefox51:
--- → verified
Updated•6 years ago
|
Product: Firefox → DevTools
Updated•4 years ago
|
Component: Inspector: Computed → Inspector
You need to log in
before you can comment on or make changes to this bug.
Description
•