Closed Bug 1338298 Opened 9 years ago Closed 9 years ago

Use the Node Reps for the grid container label rendering

Categories

(DevTools :: Inspector, defect, P3)

Product:
DevTools
Component:
Inspector
Type:
defect

Tracking

(firefox54 fixed)

Status:
RESOLVED FIXED
Milestone:
Firefox 54
Tracking Flags:
Tracking Status
firefox54 --- fixed

People

(Reporter: gl, Assigned: jdescottes)

References

(Blocks 1 open bug)

Details

Attachments

(3 files, 1 obsolete file)

Bug 1338298 - part1: use node reps to display grid containers in layout view;
59 bytes, text/x-review-board-request
gl
: review+
Details
Bug 1338298 - part2: highlight node on mouseover in grid listing;
59 bytes, text/x-review-board-request
gl
: review+
Details
Bug 1338298 - part3: add open inspector link in grid listing;
59 bytes, text/x-review-board-request
gl
: review+
Details
This is block on the fact that the Reps currently cannot accept a NodeFront. We should use the element node Reps to render the label we have for the grid container listing, so we can also use the element highlighter to highlighter the actual element with the grid container.
Priority: -- → P3
Assignee: nobody → jdescottes
Status: NEW → ASSIGNED
Comment on attachment 8839523 [details] Bug 1338298 - part1: use node reps to display grid containers in layout view; https://reviewboard.mozilla.org/r/114142/#review115844 ::: devtools/client/inspector/layout/components/GridItem.js:70 (Diff revision 2) > onToggleGridHighlighter(grid.nodeFront); > }, > > - render() { > - let { grid } = this.props; > - let { nodeFront } = grid; > + /** > + * While waiting for a reps fix in https://github.com/devtools-html/reps/issues/92, > + * translate nodeFront to a grip-like object that can be used with an ElementNode rep. Add @params, @returns JSDoc ::: devtools/client/inspector/layout/components/GridItem.js:115 (Diff revision 2) > onChange: this.onGridCheckboxClick, > } > ), > - gridName > + Rep({ > + object: this.translateNodeFrontToGrip(nodeFront), > + defaultRep: ElementNode, Order defaultRep before object
Attachment #8839523 - Flags: review?(gl) → review+
The last commit added here is purely for testing purposes. It fixes a reps issue that prevents using the "inspect" icon, but the real fix has been implemented on Github (https://github.com/devtools-html/reps/pull/96) and will come with the next reps bundle update (before the end of the week).
Created Bug 1341635 to track the next update of the reps bundle. Blocking this one on it.
Depends on: 1341635
Comment on attachment 8839885 [details] Bug 1338298 - part2: highlight node on mouseover in grid listing; https://reviewboard.mozilla.org/r/114390/#review116266
Attachment #8839885 - Flags: review?(gl) → review+
Comment on attachment 8839885 [details] Bug 1338298 - part2: highlight node on mouseover in grid listing; https://reviewboard.mozilla.org/r/114390/#review116270 ::: devtools/client/inspector/layout/layout.js:220 (Diff revision 3) > /** > - * Shows the box-model highlighter on the currently selected element. > + * Shows the box-model highlighter on the element corresponding to the provided > + * NodeFront or on the current selection. > * > + * @param {NodeFront} nodeFront > + * The node to highlight. Will default to current selection if missing Add a period at the end.
Comment on attachment 8839885 [details] Bug 1338298 - part2: highlight node on mouseover in grid listing; https://reviewboard.mozilla.org/r/114390/#review116274 ::: devtools/client/inspector/layout/components/GridItem.js:122 (Diff revision 3) > ), > Rep({ > defaultRep: ElementNode, > object: this.translateNodeFrontToGrip(nodeFront), > + onDOMNodeMouseOut: () => { > + this.props.onHideBoxModelHighlighter(); We should destructure onHideBoxModelHighlighter and onShowBoxModelHighlighter at line 100 and simply avoid calling this.props.X
Comment on attachment 8839886 [details] Bug 1338298 - part3: add open inspector link in grid listing; https://reviewboard.mozilla.org/r/114392/#review116276 I wasn't able to test this locally, but the code otherwise looks fine. I imagine this would be doing a select node, but I am not quite sure if this is what we want without playing around with it locally. ::: devtools/client/inspector/layout/components/GridItem.js:128 (Diff revision 3) > this.props.onHideBoxModelHighlighter(); > }, > onDOMNodeMouseOver: () => { > this.props.onShowBoxModelHighlighter({ nodeFront }); > }, > + onInspectIconClick: () => { Something about this declaration is giving me problems locally. The code looks fine, but just the string onInspectIconClick is giving me this failure: DOMException [SyntaxError: "An invalid or illegal string was specified" code: 12 nsresult: 0x8053000c location: resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/shared/vendor/react-dev.js:20706] ::: devtools/client/inspector/layout/layout.js:110 (Diff revision 3) > getSwatchColorPickerTooltip: () => { > return this.swatchColorPickerTooltip; > }, > > /** > + * Set the inspector selection. Add an empty line to separate the JSDoc comment and @params.
Attachment #8839886 - Flags: review?(gl)
(In reply to Gabriel Luong [:gl][1 biz day review guarantee] (ΦωΦ) from comment #20) > Comment on attachment 8839886 [details] > Bug 1338298 - part3: add open inspector link in grid listing; > > https://reviewboard.mozilla.org/r/114392/#review116276 > > I wasn't able to test this locally, but the code otherwise looks fine. I > imagine this would be doing a select node, but I am not quite sure if this > is what we want without playing around with it locally. > > ::: devtools/client/inspector/layout/components/GridItem.js:128 > (Diff revision 3) > > this.props.onHideBoxModelHighlighter(); > > }, > > onDOMNodeMouseOver: () => { > > this.props.onShowBoxModelHighlighter({ nodeFront }); > > }, > > + onInspectIconClick: () => { > > Something about this declaration is giving me problems locally. The code > looks fine, but just the string onInspectIconClick is giving me this failure: > > DOMException [SyntaxError: "An invalid or illegal string was specified" > code: 12 > nsresult: 0x8053000c > location: resource://gre/modules/commonjs/toolkit/loader.js -> > resource://devtools/client/shared/vendor/react-dev.js:20706] > > ::: devtools/client/inspector/layout/layout.js:110 > (Diff revision 3) > > getSwatchColorPickerTooltip: () => { > > return this.swatchColorPickerTooltip; > > }, > > > > /** > > + * Set the inspector selection. > > Add an empty line to separate the JSDoc comment and @params. To clarify the DOMException I am getting prevents any of the grid inspector rendering. So, I see a blank grid accordion.
Comment on attachment 8839885 [details] Bug 1338298 - part2: highlight node on mouseover in grid listing; https://reviewboard.mozilla.org/r/114390/#review116282 ::: devtools/client/inspector/layout/components/GridItem.js:118 (Diff revision 3) > value: grid.id, > checked: grid.highlighted, > onChange: this.onGridCheckboxClick, > } > ), > Rep({ This should actually be indented like the dom.input above: Rep( { ... } )
Comment on attachment 8839886 [details] Bug 1338298 - part3: add open inspector link in grid listing; https://reviewboard.mozilla.org/r/114392/#review116298 Thought about it some more, and it is what we want. I am still unsure about that DOMException if that is just me not having the right Reps(?) locally. r+ assuming everything works.
Attachment #8839886 - Flags: review+
Regarding the DOMException: did you import the last patch ? As I said, there is an issue with Reps right now (cf comment 8, comment 16). Normally with the "fix reps svg" patch you should be able to test this. Let me know if it's not the case.
Rebased on the latest inbound, addressed review comments. We still need to wait for Bug 1341635 to merge with inbound (only in autoland atm)
Attachment #8839887 - Attachment is obsolete: true
Pushed by gabriel.luong@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/48fdd165bf07 part1: use node reps to display grid containers in layout view;r=gl https://hg.mozilla.org/integration/mozilla-inbound/rev/43b2341cd314 part2: highlight node on mouseover in grid listing;r=gl https://hg.mozilla.org/integration/mozilla-inbound/rev/e3e72862322d part3: add open inspector link in grid listing;r=gl
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: