Implement a flex item sizing highlighter that will illustrate flex basis/shrink/grow of a flex item.

You will see that I inlined the SVG using Data URIs because there is a security error otherwise by specifying the svg files. Part 0 isn't meant to be reviewed and only serves as a way to test the highlighter.

Comment on attachment 8995866 [details] Bug 1477614 - Part 1: Implement a Flex Item Highlighter that highlights a flex item's flex-basis, flex-grow and flex-shrink. https://reviewboard.mozilla.org/r/260196/#review267214 ::: devtools/server/actors/highlighters.css:230 (Diff revision 1) > +:-moz-native-anonymous .flex-item-sizing { > + background-repeat: round; > + position: absolute; > + transform-origin: 0 0; > +} > + > +:-moz-native-anonymous .flex-item-sizing.top { > + background-image: url('data:image/svg+xml;utf8,<svg width="16" height="16" viewBox="0 0 24 24" xmlns="http://www.w3.org/2000/svg"><path fill="rgb(148, 0, 255)" fill-opacity=".8" fill-rule="evenodd" clip-rule="evenodd" d="M12.707 7.293a1 1 0 0 0-1.414 0l-7 7a1 1 0 0 0 1.414 1.414L12 9.414l6.293 6.293a1 1 0 0 0 1.414-1.414l-7-7z"></path></svg>'); > +} > + > +:-moz-native-anonymous .flex-item-sizing.left { > + background-image: url('data:image/svg+xml;utf8,<svg width="16" height="16" viewBox="0 0 24 24" xmlns="http://www.w3.org/2000/svg"><path fill="rgb(148, 0, 255)" fill-opacity=".8" fill-rule="evenodd" clip-rule="evenodd" d="M15.707 4.293a1 1 0 0 1 0 1.414L9.414 12l6.293 6.293a1 1 0 1 1-1.414 1.414l-7-7a1 1 0 0 1 0-1.414l7-7a1 1 0 0 1 1.414 0z"></path></svg>'); > +} > + > +:-moz-native-anonymous .flex-item-sizing.bottom { > + background-image: url('data:image/svg+xml;utf8,<svg width="16" height="16" viewBox="0 0 24 24" xmlns="http://www.w3.org/2000/svg"><path fill="rgb(148, 0, 255)" fill-opacity=".8" fill-rule="evenodd" clip-rule="evenodd" d="M4.293 8.293a1 1 0 0 1 1.414 0L12 14.586l6.293-6.293a1 1 0 1 1 1.414 1.414l-7 7a1 1 0 0 1-1.414 0l-7-7a1 1 0 0 1 0-1.414z"></path></svg>'); > +} > + > +:-moz-native-anonymous .flex-item-sizing.right { > + background-image: url('data:image/svg+xml;utf8,<svg width="16" height="16" viewBox="0 0 24 24" xmlns="http://www.w3.org/2000/svg"><path fill="rgb(148, 0, 255)" fill-opacity=".8" fill-rule="evenodd" clip-rule="evenodd" d="M8.293 4.293a1 1 0 0 1 1.414 0l7 7a1 1 0 0 1 0 1.414l-7 7a1 1 0 0 1-1.414-1.414L14.586 12 8.293 5.707a1 1 0 0 1 0-1.414z"></path></svg>'); > +} I made a similar comment further down: I think renaming to `.flex-item-growth` would work better. Also, please add comments to the 4 top, left, bottom, right classes, so we can easily understand what these inline svgs are for. ::: devtools/server/actors/highlighters/flex-item.js:86 (Diff revision 1) > + const group = createSVGNode(this.win, { > + nodeType: "g", > + parent: svg, > + attributes: { > + class: "group", > + role: "presentation" > + }, > + prefix: this.ID_CLASS_PREFIX > + }); This element doesn't seem needed, unless I missed something. It contains only one child element, and is never accessed via JavaScript. So it looks like you can remove it. ::: devtools/server/actors/highlighters/flex-item.js:144 (Diff revision 1) > + _getPathCoordinates(p1, p2, p3, p4) { > + return "M" + p1.x + "," + p1.y + " " + > + "L" + p2.x + "," + p2.y + " " + > + "L" + p3.x + "," + p3.y + " " + > + "L" + p4.x + "," + p4.y + " " + > + "L" + p4.x + "," + p1.y; I think you meant: `"L" + p1.x + "," + p1.y;` But really, I think you don't need that part. I think you can close the path with the command `z`. Also, what about using a template string instead: ``` return `M${p1.x},${p1.y} L${p2.x},${p2.y} L${p3.x},${p3.y} L${p4.x},${p4.y} z`; ``` ::: devtools/server/actors/highlighters/flex-item.js:179 (Diff revision 1) > + return this.currentQuads.margin.length || > + this.currentQuads.border.length || > + this.currentQuads.padding.length || > + this.currentQuads.content.length; I don't think you need to check all 4 box types. Checking just one should be enough. If an element has no padding, then it's padding quads are the same as its content quads. Same for border and margin. So just checking the content quads is enough. ::: devtools/server/actors/highlighters/flex-item.js:246 (Diff revision 1) > + _updateFlexItem() { > + if (!this._nodeNeedsHighlighting()) { > + return false; > + } > + > + const flexItemData = getFlexItemData(this.options.flexData, this.currentNode); Can you explain why do you want the flexData to be passed in as an option, instead of the highlighter getting it by itself? I think it makes the highlighter a bit less easily reusable. ::: devtools/server/actors/highlighters/flex-item.js:273 (Diff revision 1) > + const basis = this.getElement("basis"); > + basis.setAttribute("d", ""); > + > + const { mainBaseSize } = flexItemData; > + const { p1, p2, p3, p4 } = this.currentQuads.content[0]; > + const isColumn = this.options.flexDirection.startsWith("column"); Same question for flexDirection. I think the highlighter should go and find this information itself, so it's not up to its callers to do it. ::: devtools/server/actors/highlighters/flex-item.js:279 (Diff revision 1) > + { x: p3.x, y: p2.y + mainBaseSize }, > + { x: p4.x, y: p1.y + mainBaseSize } > + ) > + : > + this._getPathCoordinates( > + p1, > + { x: p1.x + mainBaseSize, y: p2.y }, > + { x: p1.x + mainBaseSize, y: p3.y }, The problem here is that you're mixing final/transformed geometry coordinates (quads), with defined/untransformed sizes (mainBaseSize). So this will be incorrect as soon as you transform the element (or one of its ancestor) using CSS transform. So, this will take some maths to get right. Here's the theory: - p1-p2 form a straight line (even when transformed) along which the base size first point needs to be calculated. - You could simply calculate the slope/angle of this line, knowing p1, you would be able to calculate where the base point needs to be. - Note that you would also need to calculate the "final" base size, in transformed terms. You can do this because you know the total size in transformed and untransformed geometry (p1-p2 and mainVase+mainDeltaSize). I attempted a quick fix, still buggy, but it's a start: ``` _updateFlexBasis(flexItemData) { const basis = this.getElement("basis"); basis.setAttribute("d", ""); const { mainBaseSize, mainDeltaSize } = flexItemData; const totalUntransformedSize = mainBaseSize + mainDeltaSize; const { p1, p2, p3, p4 } = this.currentQuads.content[0]; const isColumn = this.options.flexDirection.startsWith("column"); let flexBasisPath; if (isColumn) { const baseTransformedSize = mainBaseSize * (p4.y - p1.y) / totalUntransformedSize; const leftLineSlope = (p4.y - p1.y) / (p4.x - p1.x); const rightLineSlope = (p3.y - p2.y) / (p3.x - p2.x); const leftLineAngle = Math.atan(leftLineSlope); const rightLineAngle = Math.atan(rightLineSlope); flexBasisPath = this._getPathCoordinates( p1, p2, { x: p2.x + baseTransformedSize * Math.cos(rightLineAngle), y: p2.y + baseTransformedSize * Math.sin(rightLineAngle) }, { x: p1.x + baseTransformedSize * Math.cos(leftLineAngle), y: p1.y + baseTransformedSize * Math.sin(leftLineAngle) }, ); } else { const baseTransformedSize = mainBaseSize * (p2.x - p1.x) / totalUntransformedSize; const topLineSlope = (p2.y - p1.y) / (p2.x - p1.x); const bottomLineSlope = (p4.y - p3.y) / (p4.x - p3.x); const topLineAngle = Math.atan(topLineSlope); const bottomLineAngle = Math.atan(bottomLineSlope); flexBasisPath = this._getPathCoordinates( p1, { x: p1.x + baseTransformedSize * Math.cos(topLineAngle), y: p1.y + baseTransformedSize * Math.sin(topLineAngle) }, { x: p4.x + baseTransformedSize * Math.cos(bottomLineAngle), y: p4.y + baseTransformedSize * Math.sin(bottomLineAngle) }, p4 ); } this.getElement("basis").setAttribute("d", flexBasisPath); } ``` ::: devtools/server/actors/highlighters/flex-item.js:300 (Diff revision 1) > + * current flex item. > + * > + * @param {Object} flexItemData > + * Object representation of the Flex item data object. > + */ > + _updateFlexSizing(flexItemData) { Why not use a SVG path here too? I find it a bit strange that we use one for the flexbasis, but a regular div for the flexsizing. Moreover, supporting css transformations won't be as easy with a div. I suggest migrating this to make use a path just like in the other function, and doing similar maths to support transformation right from the start (cause it's easy enough, unlike other highlighthers like css-grid where we had to add it later because of the complexity of it, here we just have to calculate positions of points on lines). The only problem with this is that the CSS repeating background-image won't work anymore. However you can instead use a `<pattern>` that you can reference from the `fill` property. Also, one minor point: can we rename this to `_updateFlexGrowth` instead? I think it is more self-explanatory this way.

Comment on attachment 8995867 [details] Bug 1477614 - Part 2: Display flex max-width/height clamping lock in the Flex Item Highlighter. https://reviewboard.mozilla.org/r/260198/#review267218 ::: devtools/server/actors/highlighters/flex-item.js:330 (Diff revision 1) > + const { bounds } = this.currentQuads.content[0]; > + const isColumn = this.options.flexDirection.startsWith("column"); > + > + if (isColumn) { > + left = bounds.left + (bounds.width / 2) - 8; > + top = bounds.bottom - 8; > + } else { > + left = bounds.right - 8; > + top = bounds.top + (bounds.height / 2) - 8; Instead of using the `bounds` property here, I think it might work better to use the points. I think the bounds define a rectangle around the element, and always a rectangle, even if transformation is involved. The points however follow the transformed shape, so using them to position the lock will lead to better results. Because the position of the lock isn't as important as drawing the flex-basis correctly, this could wait for a follow-up bug if you wanted to. Having said that, drawing the lock at the right position using the points would be exactly the same as drawing the flex-basis correctly (see my comment on the previous patch).

Capturing some discussions with Victoria about when should the flex item highlighter appear: > My thinking was that it would show up when 1. a container is toggled on and 2. the user selects a flex item > via the container accordion. This does seem convoluted and it seems risky to have special behavior based on > the toggle. Better behavior is probably 1. regardless of whether container is toggled, 2. the item overlay > shows in-page if the item accordion was navigated to via Layout. (Going along with previous assumption that > a user will want to mainly see the Container info when first selecting an element via markup view.)

This is a feature enhancement that we were hoping to capture in the Flexbox inspector milestone 2 scope, but as we're getting near our deadline for M2 and as this is separate enough, I think we should move it to later. If we do manage to land it in time for 65, fine, but if we don't, no big deal.