Comment on attachment 8945010 [details] Bug 1432599 - Part 2: Display the current flex container element in the flexbox panel and allow for toggling of the flexbox highlighter. https://reviewboard.mozilla.org/r/215200/#review220756 Static analysis found 1 defect in this patch. - 1 defect found by mozlint You can run this analysis locally with: - `./mach lint check path/to/file` (Python/Javascript/wpt) If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx ::: devtools/client/inspector/flexbox/flexbox.js:179 (Diff revision 1) > + // If the FlexboxFront didn't yet have access to the NodeFront for its container, > + // then get it from the walker. This happens when the walker hasn't yet seen this > + // particular DOM Node in the tree yet, or when we are connected to an older server. > + if (!nodeFront) { > + try { > + nodeFront = await this.walker.getNodeFromActor(flexbox.actorID, Error: 'flexbox' is not defined. [eslint: no-undef]

Comment on attachment 8945010 [details] Bug 1432599 - Part 2: Display the current flex container element in the flexbox panel and allow for toggling of the flexbox highlighter. https://reviewboard.mozilla.org/r/215200/#review220778 This looks pretty good, thank you Gabriel for laying the foundation to this new tool! I made a few comments, most importants are about code duplication and compatibility. I also tested this locally, works great. There's one use case that we need to know what to do with: when an element is both an item and a container, for now, we display it as a container in the flex tool. Should we show both with an option to choose? I think this needs to be discussed more, so not in this bug, but I wanted to raise it. Also, I think we need some indication of whether what you currently have selected is an container or an item. Right now, if you select an item, it will display its container with the checkbox, which is good, but I wish it would say something like: "Flexbox item selected, here's the container:", or something better than this. Again, I think this and the previous topic are all connected to a larger UX discussion about the tool, so I won't block the review on these 2, but we need to start discussing. ::: devtools/client/inspector/flexbox/components/Flexbox.js:45 (Diff revision 2) > - } > + }, > + dom.div( > + { > + className: "flexbox-content" > + }, > + FlexboxList({ Why use a list here? I think this only ever displays one container, right? ::: devtools/client/inspector/flexbox/components/FlexboxItem.js:34 (Diff revision 2) > + /** > + * 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. > + * > + * @params {NodeFront} nodeFront > + * The NodeFront for which we want to create a grip-like object. > + * @returns {Object} a grip-like object that can be used with Reps. > + */ > + translateNodeFrontToGrip(nodeFront) { I think we now have this in at least 2 or 3 places in the code. We need to do either one of these 2 things: - fix issue 92 - or move this code to a helper somewhere This is straight up code duplication, and we should stop doing this. ::: devtools/client/inspector/flexbox/components/FlexboxList.js:1 (Diff revision 2) > +/* This Source Code Form is subject to the terms of the Mozilla Public Not sure that this component is even needed. We only displau one item for now. And judging by the survey results, I don't think we'll change this in the near future. ::: devtools/client/inspector/flexbox/flexbox.js:68 (Diff revision 2) > + > + /** > + * Returns true if the layout panel is visible, and false otherwise. > + */ > + isPanelVisible() { > + return this.inspector && this.inspector.toolbox && this.inspector.sidebar && Shouldn't this also take into account the fact that the accordion may be closed? ::: devtools/client/inspector/flexbox/flexbox.js:88 (Diff revision 2) > + */ > + onHighlighterChange(event, nodeFront) { > + let highlighted = event === "flexbox-highlighter-shown"; > + > + // Only tell the store that the highlighter changed if it did change. > + // If we're still highlighting the same node, with the same color, no need to force nit: this comment mentions color here as being something we check, but we don't. So this comment should be updated. ::: devtools/client/inspector/flexbox/flexbox.js:130 (Diff revision 2) > + this.lastHighlighterNode = node; > + this.lastHighlighterState = node !== this.highlighters.flexboxHighlighterShown; > + > + this.highlighters.toggleFlexboxHighlighter(node); > + this.store.dispatch(updateFlexboxHighlighted(node, > + node !== this.highlighters.flexboxHighlighterShown)); You could use `this.lastHighlighterState` here instead of doing this comparison again. ::: devtools/client/inspector/flexbox/flexbox.js:134 (Diff revision 2) > + * Handler for "new-root" event fired by the inspector and "new-node-front" event fired > + * by the inspector selection. Updates the flexbox panel if it is visible. If we also take into account if the accordion is closed, then this should also handle accordion open events (if they even exist). ::: devtools/client/inspector/flexbox/flexbox.js:189 (Diff revision 2) > + return; > + } > + } > + > + this.store.dispatch(updateFlexbox({ > + id: flexboxFront.actorID, I wonder if we shouldn't call this property `actorID` instead of just `id`. At least it's very clear this way what it means. `id` is very generic and could be anything. ::: devtools/client/inspector/flexbox/utils/l10n.js:1 (Diff revision 2) > +/* This Source Code Form is subject to the terms of the Mozilla Public > + * License, v. 2.0. If a copy of the MPL was not distributed with this > + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ > + > +"use strict"; > + > +const { LocalizationHelper } = require("devtools/shared/l10n"); > +const L10N = new LocalizationHelper("devtools/client/locales/layout.properties"); > + > +module.exports = { > + getStr: (...args) => L10N.getStr(...args), > + getFormatStr: (...args) => L10N.getFormatStr(...args), > + getFormatStrWithNumbers: (...args) => L10N.getFormatStrWithNumbers(...args), > + numberWithDecimals: (...args) => L10N.numberWithDecimals(...args), > +}; This is the same content as grid/utils/l10n.js, and pointing to the same properties file. Can you please find a way to reuse the same module instead of introducing a new one? ::: devtools/server/actors/layout.js:142 (Diff revision 2) > }, > > /** > - * Returns an array of FlexboxActor objects for all the flexbox containers found by > - * iterating below the given rootNode. > + * Returns the flex container found by iterating on the given node. The current > + * node can be a flex container or flex item. If it is a flex item, returns the parent > + * flex container. Otherwise, return null if the current or parent node is a flex typo: if the current or parent *is not* a flex container ::: devtools/server/actors/layout.js:167 (Diff revision 2) > - } > + let flexboxActor = null; > > - for (let {document} of this.tabActor.windows) { > - flexboxes = [...flexboxes, ...this.getFlexbox(document.documentElement)]; > + // Check if the current node is a flex container. > + if (this.isFlexContainer(treeWalker.currentNode)) { > + flexboxActor = new FlexboxActor(this, treeWalker.currentNode); > + } else if (this.isFlexContainer(treeWalker.parentNode())) { This won't work in the following case: ```html <div style="display:flex"> <div style="display:contents"> <div>this is the item</div> </div> </div> ``` Indeed, the `display:contents` node is style the parentNode of the item, but it's not the flexbox container. However its own parent is. So we need something a bit smarter here, that gives us the actual flexbox container given any flex item. No matter how many un-rendered ancestors are in between. ::: devtools/shared/specs/layout.js:25 (Diff revision 2) > > const layoutSpec = generateActorSpec({ > typeName: "layout", > > methods: { > - getAllFlexbox: { > + getCurrentFlexbox: { So, there's a minor but still existing backward compatibility problem here. The `getAllFlexbox` method landed in 59. And this new method that replaces it is going to land in 60. So, if someone connects with a FF60 front-end, to a FF59 server, then the tool will fail with an error. This may happen when debugging Firefox for Android on a device. We still need to pay attention to backward compatibility. Sometimes we do this by choosing one method or the other depending on which method is available on the server. In your case, I think it's different, we should detect if `getCurrentFlexbox` exists, and if it doesn't, just disable the whole flexbox panel entirely. You should do this somewhere in the `FlexboxInspector` class, by using the `actorHasMethod` function, and if it returns false, then silently exit or something. I guess in this case it would be fine just leaving the panel empty, but at least not crashing with an error.

Comment on attachment 8945010 [details] Bug 1432599 - Part 2: Display the current flex container element in the flexbox panel and allow for toggling of the flexbox highlighter. https://reviewboard.mozilla.org/r/215200/#review220778 > Shouldn't this also take into account the fact that the accordion may be closed? Yes, but let's fixed that in a separate bug. We currently listen for a "select" event from the inspector sidebar to know when to update the flexbox/grid panel. We would need to add an onToggle method that is passed to the accordion to know when to update the panel otherwise. > If we also take into account if the accordion is closed, then this should also handle accordion open events (if they even exist). Let's move this to a separate bug so we can handle accordion toggle events. > So, there's a minor but still existing backward compatibility problem here. The `getAllFlexbox` method landed in 59. And this new method that replaces it is going to land in 60. > So, if someone connects with a FF60 front-end, to a FF59 server, then the tool will fail with an error. > > This may happen when debugging Firefox for Android on a device. We still need to pay attention to backward compatibility. > > Sometimes we do this by choosing one method or the other depending on which method is available on the server. In your case, I think it's different, we should detect if `getCurrentFlexbox` exists, and if it doesn't, just disable the whole flexbox panel entirely. > > You should do this somewhere in the `FlexboxInspector` class, by using the `actorHasMethod` function, and if it returns false, then silently exit or something. I guess in this case it would be fine just leaving the panel empty, but at least not crashing with an error. We never actually landed any code previously that used getAllFlexbox and the Flexbox pane is still pref'd off and the FlexboxInspector class is actually empty and un-initialized until now. I am wondering if we still need to do this, but I am guessing no as my guess.

(In reply to Gabriel [:gl] (ΦωΦ) from comment #5) > > So, there's a minor but still existing backward compatibility problem here. The `getAllFlexbox` method landed in 59. And this new method that replaces it is going to land in 60. > > So, if someone connects with a FF60 front-end, to a FF59 server, then the tool will fail with an error. > > > > This may happen when debugging Firefox for Android on a device. We still need to pay attention to backward compatibility. > > > > Sometimes we do this by choosing one method or the other depending on which method is available on the server. In your case, I think it's different, we should detect if `getCurrentFlexbox` exists, and if it doesn't, just disable the whole flexbox panel entirely. > > > > You should do this somewhere in the `FlexboxInspector` class, by using the `actorHasMethod` function, and if it returns false, then silently exit or something. I guess in this case it would be fine just leaving the panel empty, but at least not crashing with an error. > > We never actually landed any code previously that used getAllFlexbox and the > Flexbox pane is still pref'd off and the FlexboxInspector class is actually > empty and un-initialized until now. I am wondering if we still need to do > this, but I am guessing no as my guess. Yes we do. The old method exists in the server on 59. So, if we land this, and have this new UI available in 60, and if you connect it to a FF59 server, then it will break. So I think you still need to test for the existence of the method.

Comment on attachment 8945010 [details] Bug 1432599 - Part 2: Display the current flex container element in the flexbox panel and allow for toggling of the flexbox highlighter. https://reviewboard.mozilla.org/r/215200/#review223612

Comment on attachment 8948306 [details] Bug 1432599 - Part 2: Display the current flex container element in the flexbox panel and allow for toggling of the flexbox highlighter. https://reviewboard.mozilla.org/r/217788/#review223614 Thanks Gabe. Looks good. I do have a few other comments, but I'll R+ this. Please address these comments before landing. ::: devtools/client/inspector/flexbox/components/FlexboxItem.js:32 (Diff revision 1) > + this.onFlexboxCheckboxClick = this.onFlexboxCheckboxClick.bind(this); > + this.onFlexboxInspectIconClick = this.onFlexboxInspectIconClick.bind(this); I thought React didn't need this and autobound stuff? Can you try without these 2 lines and see if it still works? ::: devtools/client/inspector/flexbox/flexbox.js:113 (Diff revision 1) > + const hasGetCurrentFlexbox = await this.inspector.target.actorHasMethod("layout", > + "getCurrentFlexbox"); > + if (!hasGetCurrentFlexbox) { > + return; > + } No need to do this over and over again. Once you're connected to a target, the list of supported methods won't change. You should move this to async init and be done with it. ::: devtools/client/inspector/flexbox/flexbox.js:140 (Diff revision 1) > + if (flexbox.actorID == flexboxFront.actorID) { > + return; > + } > + > + // Update the flexbox panel with the new flexbox front contents. > + this.update(flexboxFront); Wait, why do we want to update here? If I remember correctly, we do this for the grid inspector because we use it to update the grid overlay widget. But in flex, we don't really need to update anything on reflow, do we? ::: devtools/client/inspector/flexbox/flexbox.js:163 (Diff revision 1) > + > + this.update(); > + } > + > + /** > + * Handler for a change in the input checkboxes in the FlexboxList component. nit: The FlexboxList component does not exist anymore. Please update this comment. ::: devtools/client/inspector/flexbox/flexbox.js:205 (Diff revision 1) > + try { > + const hasGetCurrentFlexbox = await this.inspector.target.actorHasMethod("layout", > + "getCurrentFlexbox"); > + if (!hasGetCurrentFlexbox) { > + return; > + } Same comment here, this should be detected once only when the panel is initialized, that's it. ::: devtools/server/actors/layout.js:186 (Diff revision 1) > - return this.getFlexbox(rootNode.rawNode); > + > + switch (displayType) { > + case "inline-flex": > + case "flex": > + return new FlexboxActor(this, currentNode); > + case "contents": It's probably safer just using `default` instead of `case` here. ::: devtools/server/actors/layout.js:188 (Diff revision 1) > + continue; > - } > + } > > - for (let {document} of this.tabActor.windows) { > + break; This continue, and break after the switch feels a bit weird to me. I wonder if we should not use a switch at all: ``` while ((currentNode = treeWalker.parentNode())) { if (!currentNode) { break; } displayType = this.walker.getNode(currentNode).displayType; if (displayType === "inline-flex" || displayType === "flex") { return new FlexboxActor(this, currentNode); } } ```

Pushed by gabriel.luong@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/de4276013d2f Part 1: Refactor translateNodeFrontToGrip function to inspector shared utils. r=pbro

Comment on attachment 8948306 [details] Bug 1432599 - Part 2: Display the current flex container element in the flexbox panel and allow for toggling of the flexbox highlighter. https://reviewboard.mozilla.org/r/217788/#review223614 > I thought React didn't need this and autobound stuff? Can you try without these 2 lines and see if it still works? Yea, I have tried it, and the binding is needed. > Wait, why do we want to update here? > If I remember correctly, we do this for the grid inspector because we use it to update the grid overlay widget. But in flex, we don't really need to update anything on reflow, do we? On a reflow, it is possible that a selected element can suddenly morph into a flexbox layout because of media queries. I don't know why that would happen, but it's possible. Otherwise, we would need to update with the flexboxFront which would contain the updated flex item fragments in the future hence the TODO. > It's probably safer just using `default` instead of `case` here. We can't use default here because we actually want to continue in this while loop only for display: contents and walk up the tree to see if it is a flex container. By default, we only show a flexbox checkbox because the current select item is either a flex container or flex item. > This continue, and break after the switch feels a bit weird to me. I wonder if we should not use a switch at all: > > ``` > while ((currentNode = treeWalker.parentNode())) { > if (!currentNode) { > break; > } > > displayType = this.walker.getNode(currentNode).displayType; > if (displayType === "inline-flex" || displayType === "flex") { > return new FlexboxActor(this, currentNode); > } > } > ``` Like the comment above, we want to only walk up the tree to see if the current selected item is a flex item and therefore it's parent is a flex container. Otherwise, if it is display: contents we keep walking up to check if it is a flex container. We need the break here because we only want to continue in this while loop only if the selected element is display: contents. Otherwise, we know this is not a flex item and its parent isn't a flex container.

@pbro: Checking in with you based on my comments from your last review.

(In reply to Gabriel [:gl] (ΦωΦ) from comment #14) > > I thought React didn't need this and autobound stuff? Can you try without these 2 lines and see if it still works? > Yea, I have tried it, and the binding is needed. Fine. I don't know React enough anyway. > > Wait, why do we want to update here? > > If I remember correctly, we do this for the grid inspector because we use it to update the grid overlay widget. But in flex, we don't really need to update anything on reflow, do we? > > On a reflow, it is possible that a selected element can suddenly morph into > a flexbox layout because of media queries. I don't know why that would > happen, but it's possible. You're right, thanks. > > It's probably safer just using `default` instead of `case` here. > > We can't use default here because we actually want to continue in this while > loop only for display: contents and walk up the tree to see if it is a flex > container. By default, we only show a flexbox checkbox because the current > select item is either a flex container or flex item. > > > This continue, and break after the switch feels a bit weird to me. I wonder if we should not use a switch at all: > > > > ``` > > while ((currentNode = treeWalker.parentNode())) { > > if (!currentNode) { > > break; > > } > > > > displayType = this.walker.getNode(currentNode).displayType; > > if (displayType === "inline-flex" || displayType === "flex") { > > return new FlexboxActor(this, currentNode); > > } > > } > > ``` > > Like the comment above, we want to only walk up the tree to see if the > current selected item is a flex item and therefore it's parent is a flex > container. Otherwise, if it is display: contents we keep walking up to check > if it is a flex container. > > We need the break here because we only want to continue in this while loop > only if the selected element is display: contents. Otherwise, we know this > is not a flex item and its parent isn't a flex container. Oh right, that makes sense. Sorry I missed to see that point. We definitely don't want to continue walking if the element is not a flex item.

Pushed by gabriel.luong@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/a4ae1745e9d4 Part 2: Display the current flex container element in the flexbox panel and allow for toggling of the flexbox highlighter. r=pbro

Backout by aciure@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/91be73741295 Backed out changeset a4ae1745e9d4 for mochitest devtools failures, a=backout on a CLOSED TREE

Backed out changeset a4ae1745e9d4 (bug 1432599) for mochitest devtools failures, a=backout on a CLOSED TREE Push that caused the failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=a4ae1745e9d41f3da39ac23075430c657c569ccf&filter-classifiedState=unclassified&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception recent failure log https://treeherder.mozilla.org/logviewer.html#?job_id=160942893&repo=mozilla-inbound&lineNumber=10029 backout:https://hg.mozilla.org/integration/mozilla-inbound/rev/91be737412952e421f27fd0d7a90edd6bb95f0c5

Pushed by gabriel.luong@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/b3eb902f01f4 Part 2: Display the current flex container element in the flexbox panel and allow for toggling of the flexbox highlighter. r=pbro