Closed Bug 1497182 Opened 3 years ago Closed 3 years ago

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

Categories

(DevTools :: Inspector, enhancement, P3)

enhancement

Tracking

(firefox65 verified)

VERIFIED FIXED
Firefox 65
Tracking Status
firefox65 --- verified

People

(Reporter: miker, Assigned: miker)

References

(Blocks 1 open bug)

Details

(Whiteboard: [dt-q])

Attachments

(2 files, 2 obsolete files)

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.
Flags: needinfo?(mratcliffe)
(To test that, you'd add "direction: rtl" to the flex container [or any ancestor of the container, since 'direction' inherits by default])
(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.
Flags: needinfo?(mratcliffe)
No longer depends on: 1497174
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
I think this would be super useful indeed. The more dump we can make devtools, the happier we are :)
Obviously I meant *dumb*
Attachment #9015255 - Flags: review?(gl)
Attachment #9015255 - Flags: review?(gl)
Flags: needinfo?(mratcliffe)
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?
Flags: needinfo?(gl)
We discussed this in a meeting and decided to experiment with drawing over the whole flex container instead to try and simplify the code.
Attachment #9015230 - Attachment is obsolete: true
Attachment #9017580 - Attachment is obsolete: true
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.
Pushed by mratcliffe@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/aa01df6dc4db
Flexbox justify-content incorrectly drawn when using flex-direction: row-reverse r=gl
https://hg.mozilla.org/mozilla-central/rev/aa01df6dc4db
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 65
Flags: qe-verify+

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!

Status: RESOLVED → VERIFIED
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.