1422554 - Display flex-direction when highlighting flexbox

Status: NEW

(DevTools :: Inspector, enhancement, P3)

Description

[Darren Hobin [:hobindar]]

User Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/60.0.3112.78 Safari/537.36

Comment 1

[Darren Hobin [:hobindar]]

Attachment: Bug 1422554 - Display flex-direction when highlighting flexbox

Review commit: https://reviewboard.mozilla.org/r/204900/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/204900/

Comment 2

[Darren Hobin [:hobindar]]

Comment on attachment 8933966 [details]
Bug 1422554 - Display flex-direction when highlighting flexbox

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/204900/diff/1-2/

Comment 3

[Patrick Brosset <:pbro>]

Comment on attachment 8933966 [details]
Bug 1422554 - Display flex-direction when highlighting flexbox

https://reviewboard.mozilla.org/r/204900/#review211476

Good start. Thanks!
I have made a few comments below.
I also have a few concerns with the approach for the UI, but we can always experiment by landing this approach, testing with users and making changes, since this is all behind a pref. Anyway, here they are:
- if the container is big and touches the edge of the viewport, then the arrows aren't visible,
- if the container is very small, then the arrows may be bigger than itself, looking a bit weird,
- it may be confusing to know which arrow is which. One idea would be to only draw the main arrow. This is the main direction and flexbox is 1D only, not 2D, so it's the most important one to draw.

::: devtools/server/actors/highlighters/flexbox.js:197
(Diff revision 2)
>      if (isTopLevel) {
>        this.hide();
>      }
>    }
>  
> +  renderFlexAxis() {

In flexbox, axis are called "main" and "cross", so we should use this vocabulary here too.
Also, I would split the 2 arrows in 2 different functions to make it more clear.
These should be called `renderMainAxisArrow` and `renderCrossAxisArrow`. Then internally they could call another function like `renderAxisArrow` so the code is not duplicated, but at least it's easier to read the code.

::: devtools/server/actors/highlighters/flexbox.js:219
(Diff revision 2)
> +    let bigHeight = 1;
> +    let bigWidth = 1;
> +    let smallHeight = 0.75;
> +    let smallWidth = 0.5;

Same comment about using the correct vocabulary here: please use main instead of big and cross instead of small.
Also, these numbers are hard coded here, so they should be moved as constants at the top of the file, without self explanatory names.

::: devtools/server/actors/highlighters/flexbox.js:224
(Diff revision 2)
> +    let bigHeight = 1;
> +    let bigWidth = 1;
> +    let smallHeight = 0.75;
> +    let smallWidth = 0.5;
> +
> +    let computedStyle = this.currentNode.ownerGlobal.getComputedStyle(this.currentNode);

You should instead use the `getComputedStyle` from utils/markup just like we do in the css-transform.js highlighter. This way, it will also work with pseudo-elements.

::: devtools/server/actors/highlighters/flexbox.js:225
(Diff revision 2)
> +    let bigWidth = 1;
> +    let smallHeight = 0.75;
> +    let smallWidth = 0.5;
> +
> +    let computedStyle = this.currentNode.ownerGlobal.getComputedStyle(this.currentNode);
> +    let direction = computedStyle.getPropertyValue("flex-direction");

This made me realize that nowhere in this highlighter do we check that the node is actually a flex container. So this highlighter, as it is now, works with any node. I don't think it should. I think we should implement a simple `isFlexContainer` method, similar to the `isGrid` method in css-grid.js. 
This way, we can be sure that we're dealing with a flexbox container here and therefore that flex-direction makes sense.

::: devtools/server/actors/highlighters/flexbox.js:227
(Diff revision 2)
> +    let smallWidth = 0.5;
> +
> +    let computedStyle = this.currentNode.ownerGlobal.getComputedStyle(this.currentNode);
> +    let direction = computedStyle.getPropertyValue("flex-direction");
> +
> +    if (direction === "row") {

How about `row-reverse` and `column-reverse`?

::: devtools/server/actors/highlighters/utils/canvas.js:76
(Diff revision 2)
> +  let arrowMid = 40 * arrowHeight;
> +  let arrowTip = 80 * arrowHeight;
> +  let headWidth = 40 * arrowWidth;
> +  let tailWidth = 20 * arrowWidth;

Please move this numbers as `const` at the top of the file.

Comment 4

[Darren Hobin [:hobindar]]

Comment on attachment 8933966 [details]
Bug 1422554 - Display flex-direction when highlighting flexbox

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/204900/diff/2-3/

Comment 5

[Darren Hobin [:hobindar]]

Attachment: WIP design

Comment 6

[Darren Hobin [:hobindar]]

Since submitting my last patch, :gl sent me a new WIP design (attached). My revised patch reflects the new design in addition to making the changes requested by :pbro.

Comment 7

[Darren Hobin [:hobindar]]

Comment on attachment 8933966 [details]
Bug 1422554 - Display flex-direction when highlighting flexbox

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/204900/diff/3-4/