1497182 - Flexbox justify-content incorrectly drawn when using flex-direction: row-reverse

Status: VERIFIED FIXED

(DevTools :: Inspector, enhancement, P3)

Description

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: description.png

Luckily, because we draw the flex items after drawing the justify-content we display the white boxes on top of the erroneous content.

In the future we may add things that require justify content to be rendered correctly so we need to run the algorithm in reverse when using `flex-direction: row-reverse`.

This bug is not visible due to the way the flexbox highlighter is drawn so I will explain the issue with the current algorithm:

1. Open the attached description.
2. justify-content areas are the areas between shapes so in this image they are the areas highlighted in red.

Luckily, this is not visible because of the order we render the flex items but .

When using row (and not row reverse) we draw the images like this:
1. Draw a rectangle from 0 to the left hand of the first flex item.
2. Draw a rectangle from the right of the previous flex item to the left of the next flex item.
3. Repeat 2 until done.

Unfortunately we use the same algorithm for row reverse... this is what happens:
1. Draw a rectangle from 0 to the left hand of the first flex item.
   The first flex item is the rightmost one (#1) so we draw the pattern from 0 to the left hand of #1.
2. Draw a rectangle from the right of the previous flex item to the left of the next flex item.
   The previous flex item is the second to the right (#2) so we draw a rectangle from the right of #1 to the to the left of #2.
3. When repeated we end up drawing rectangles in this sequence:
  - Around #1 & #2.
  - Around #2 & #3.
  - Around #3 & #4.

Comment 1

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: Bug 1497182 - Flexbox justify-content incorrectly drawn when using flex-direction: row-reverse

Comment 2

[Daniel Holbert [:dholbert]]

Please also test this with "direction: rtl" (which adds another layer of right vs. left swapping).

Glancing at the patch right now, I wouldn't be surprised if this is broken under that as well.

Comment 3

[Daniel Holbert [:dholbert]]

(To test that, you'd add "direction: rtl" to the flex container [or any ancestor of the container, since 'direction' inherits by default])

Comment 4

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

(In reply to Daniel Holbert [:dholbert] from comment #2)
> Please also test this with "direction: rtl" (which adds another layer of
> right vs. left swapping).
> 
> Glancing at the patch right now, I wouldn't be surprised if this is broken
> under that as well.

Bug 1437631 Will add RTL support to the flexbox highlighter so this is not a concern until I implement it.

Comment 5

[Daniel Holbert [:dholbert]]

Thanks. This makes me think... maybe we should extend the flexbox C++ API to return an enum value that represents the physical direction of the main axis & the cross axis? (e.g. getMainAxisDirection and getCrossAxisDirection, which would a number from 0-4 where 0 means LTR, 1 means RTL, 2 means top-to-bottom, 3 means bottom-to-top)

We already have that information in flex layout, and it feels unnecessary to have to reimplement that logic in devtools JS.  Would that be helpful here?  I

Comment 6

[Patrick Brosset <:pbro>]

I think this would be super useful indeed. The more dump we can make devtools, the happier we are :)

Comment 7

[Patrick Brosset <:pbro>]

Obviously I meant *dumb*

Comment 8

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

Would you be able to create an example that very clearly demonstrates the problem visually? I tried playing around with different flex-direction and justify-content and couldn't really see a visual difference. If it is the case that there is no visual problems with the current algorithm, would it be worth adding the extra complexity of this code since this handling of the direction also happens in your RTL and presumably Writing Mode patches.

Comment 9

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

(In reply to Gabriel [:gl] (ΦωΦ) from comment #8)
> Would you be able to create an example that very clearly demonstrates the
> problem visually? I tried playing around with different flex-direction and
> justify-content and couldn't really see a visual difference. If it is the
> case that there is no visual problems with the current algorithm, would it
> be worth adding the extra complexity of this code since this handling of the
> direction also happens in your RTL and presumably Writing Mode patches.

I can roll this into the RTL patch if you would like but it was you that asked me to break this out into a new patch.

Comment 10

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

It was because I was looking at the RTL patch that made me look at this deeper and think if we needed all this extra complexity. I simply couldn't find a case where there was any visual error or difference to justify having all this code.

Comment 11

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: row-reverse issue.png

This attachment explains the issue quite clearly.

Comment 12

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

Can you also send the link to the test page?(In reply to Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe] from comment #11)
> Created attachment 9017580 [details]
> row-reverse issue.png
> 
> This attachment explains the issue quite clearly.

Can you also attach the test page?

Comment 13

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

We discussed this in a meeting and decided to experiment with drawing over the whole flex container instead to try and simplify the code.

Comment 14

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: Issue.png

Comment 15

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

You can easily see the issue by looking at attachment 9019066 [details] (attached)

STR:
1. Go to https://mikeratcliffe.github.io/BrowserTestFiles/flexbox.html
2. Open the flexbox inspector on the flex-container.

The overlay is displayed correctly.

3. Change flex-direction to row-reverse.

The flex container will be missing the white background from above and below the flex items in row 1.

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/aa01df6dc4db
Flexbox justify-content incorrectly drawn when using flex-direction: row-reverse r=gl

Comment 17

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/aa01df6dc4db

Comment 18

[Cristian Comorasu, QA [:ccomorasu], Releae Desktop QA - Please contact [e-mail]]

I reproduced this issue using 64.0a1(2018-10-08), on Windows 10 x64.
I can confirm this issue is fixed, I verified using Fx 65.0b10 on Windows 10 x64, macOS 10.13 and Ubuntu 16.04 LTS.
Cheers!