1435749 - Correctly render all flex items in the flexbox highlighter

Status: RESOLVED FIXED

(DevTools :: Inspector, enhancement, P3)

Description

[Patrick Brosset <:pbro>]

Attachment: all-items.html

The code for the flexbox highlighter makes an incorrect assumption that all flex items are the flexbox container's children elements.

See this line: https://searchfox.org/mozilla-central/rev/e06af9c36a73a27864302cd2f829e6200dee8541/devtools/server/actors/highlighters/flexbox.js#347

This can be incorrect for (at least) 3 cases:

- Text nodes: if there is an unwrapped text node in between 2 flex items, then it becomes an anonymous flex item too. It can't be styled, but it should still be highlighted. Right now, it isn't.

- Pseudo-elements: ::before and ::after pseudos of the flex container also become flex items. Right now the highlighter doesn't show them.

- display:contents children: if a child of a flex container uses display:contents then it isn't rendered, but it's children are. So they should be highlighted too.

This can be very easily fixed by using the node.getAsFlexContainer() chrome API which we already use anyway:

node.getAsFlexContainer()[0].getItems() returns an array of items in the first line.

I'll attach a test case that shows all of the cases that should be handled.

Comment 1

[Daniel Holbert [:dholbert]]

(In reply to Patrick Brosset <:pbro> from comment #0)
> The code for the flexbox highlighter makes an incorrect assumption that all
> flex items are the flexbox container's children elements.

(This assumption is incorrect in the reverse direction, too - some children will be rendered but are not flex items, particularly the abspos/fixedpos children.  I spun off bug 1435787 for that -- feel free to merge it into this issue if that's cleaner though.)

Comment 2

[Patrick Brosset <:pbro>]

Would using .getItems() fix everything at once here? If so, I'm guessing we can just keep one of the 2 bugs.

Comment 3

[Daniel Holbert [:dholbert]]

Ah right, I forgot we added that API. I'm pretty sure that should be trustable to return the actual items (including/excluding [grand]children as-appropriate). So I agree that'd fix everything at once -- I'll dupe my abspos bug over to this one.

Comment 5

[Daniel Holbert [:dholbert]]

Attachment: testcase 2: flex container with abs/fixedpos children (which shouldn't be highlighted as items)

Comment 6

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: Bug 1435749 - Get the correct flex items when rendering in the flexbox highlighter.

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

Comment 7

[Patrick Brosset <:pbro>]

Comment on attachment 8956606 [details]
Bug 1435749 - Get the correct flex items when rendering in the flexbox highlighter.

https://reviewboard.mozilla.org/r/225536/#review231528

Very happy to see this!
We'll need more changes throughout this file so justify-content is drawn correctly too, but it's a great start.

Comment 8

[Patrick Brosset <:pbro>]

Attachment: Bug 1435749 - Cut out justify-content areas using the correct flex items in the flexbox highlighter;

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

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5576eb6fee39
Get the correct flex items when rendering in the flexbox highlighter. r=pbro

Comment 10

[Gabriel Luong [:gl] (ΦωΦ)]

Comment on attachment 8956739 [details]
Bug 1435749 - Cut out justify-content areas using the correct flex items in the flexbox highlighter;

https://reviewboard.mozilla.org/r/225704/#review231770

Remove the leave-open before landing.

::: devtools/server/actors/highlighters/flexbox.js:498
(Diff revision 1)
>  
>      let { bounds } = this.currentQuads.content[0];
> -    let flexItems = this.currentNode.children;
>      let flexLines = this.currentNode.getAsFlexContainer().getLines();
>      let computedStyle = getComputedStyle(this.currentNode);
> -    let direction = computedStyle.getPropertyValue("flex-direction");
> +    let isColumn = computedStyle.getPropertyValue("flex-direction").startsWith("column");

I know we do this in other places so we should probably make this consistent by doing the same thing in every instance we need the flex direction.

::: devtools/server/actors/highlighters/flexbox.js:541
(Diff revision 1)
>          crossSize = Math.round(crossSize);
>          crossStart = Math.round(crossStart);
>  
> -        if (direction.startsWith("column") &&
> -            crossStart <= left &&
> -            left <= right &&
> +        if (isColumn) {
> +          clearRect(this.ctx, crossStart, top, crossSize + crossStart, bottom,
> +                    this.currentMatrix);

Only one indent to make this consistent in this file.

::: devtools/server/actors/highlighters/flexbox.js:544
(Diff revision 1)
> -        if (direction.startsWith("column") &&
> -            crossStart <= left &&
> -            left <= right &&
> -            right <= crossSize + crossStart) {
> -          // Remove the margin area for justify-content
> -          let marginTop = Math.round(parseFloat(
> +        if (isColumn) {
> +          clearRect(this.ctx, crossStart, top, crossSize + crossStart, bottom,
> +                    this.currentMatrix);
> +        } else {
> +          clearRect(this.ctx, left, crossStart, right, crossSize + crossStart,
> +                    this.currentMatrix);

Same as above.

Comment 11

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/5576eb6fee39

Comment 12

[Patrick Brosset <:pbro>]

Comment on attachment 8956739 [details]
Bug 1435749 - Cut out justify-content areas using the correct flex items in the flexbox highlighter;

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

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/71ecd11c2bf6
Cut out justify-content areas using the correct flex items in the flexbox highlighter; r=gl

Comment 14

[Cristian Brindusan [:cbrindusan]]

https://hg.mozilla.org/mozilla-central/rev/71ecd11c2bf6