[css-grid][css-flexbox] Positioned aligned item lose alignment if offest property (in the other axis) is modified

VERIFIED FIXED in Firefox 58

Status

()

defect
P3
normal
VERIFIED FIXED
2 years ago
2 years ago

People

(Reporter: rego, Assigned: mats)

Tracking

({testcase})

Trunk
mozilla58
Points:
---

Firefox Tracking Flags

(firefox57 fix-optional, firefox58 verified)

Details

Attachments

(5 attachments)

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

Steps to reproduce:


Open the attached test case.


Actual results:


The positioned item is no longer vertically aligned.


Expected results:


The positioned item should be still vertically aligned.
Posted image Current output
Posted image Expected output
Component: Untriaged → Layout
Product: Firefox → Core
Flags: needinfo?(mats)
Priority: -- → P3
Thanks for the report.  It's definitely a bug.  I suspect it also affects Flexbox.

I think the problem is that our RecomputePosition optimizations
is unaware that CSS Align can also affect the position.

I'll fix it by disabling that optimization in this bug for now...
Assignee: nobody → mats
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Flags: needinfo?(mats)
Keywords: testcase
OS: Unspecified → All
Hardware: Unspecified → All
Summary: [css-grid] Positined aligned item lose alignment if offest property (in the othre axis) is modified → [css-grid][css-flexbox] Positioned aligned item lose alignment if offest property (in the other axis) is modified
Comment on attachment 8925397 [details] [diff] [review]
part 1 - Don't try to optimize re-positioning of Flexbox/Grid abs.pos. children since they are affected by CSS Align positioning too

Review of attachment 8925397 [details] [diff] [review]:
-----------------------------------------------------------------

r=me
Attachment #8925397 - Flags: review?(dholbert) → review+
Comment on attachment 8925398 [details] [diff] [review]
part 2 - Reftests.

Review of attachment 8925398 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/reftests/flexbox/reftest.list
@@ +19,5 @@
>  # background size in test vs. ref
>  fuzzy-if(cocoaWidget,1,2) random-if(winWidget||gtkWidget) skip-if(Android) == flexbox-align-self-baseline-horiz-3.xhtml  flexbox-align-self-baseline-horiz-3-ref.xhtml # XXXdholbert investigate the random-if. The skip-if(Android) is because checkbox/radio appearance:none doesn't work as expected.
>  == flexbox-align-self-baseline-horiz-4.xhtml flexbox-align-self-baseline-horiz-4-ref.xhtml
> +skip-if(Android) == flexbox-item-align-self-dynamic-pos-001.html flexbox-item-align-self-dynamic-pos-001-ref.html # fails on Android for unknown reasons: bug XXX
> +skip-if(Android) == flexbox-item-align-self-dynamic-pos-002.html flexbox-item-align-self-dynamic-pos-002-ref.html # fails on Android for unknown reasons: bug XXX

Hmm -- looking at the snapshot from this reftest-failure, it looks like the *reference case* is rendering incorrectly. That's surprising.  IIRC, we reflow reftests a different number of times on Android (I can't remember if it's an extra time or one fewer as compared to desktop), and I'll bet this is tickling some other abspos incremental-reflow bug.

Assuming the problem is just with Android & the reference case -- maybe you can avoid the problem (and simplify your reftest slightly) by making the reference cases' flex items "position:relative" rather than "position:absolute"?  That'll let us completely avoid the abspos codepath, and it shouldn't change the rendering at all (the "left: 20px" positioning will still have the same effect).

We should still file a bug on this failure (I'm happy to do so & try to investigate), and maybe even land an additional reftest-testcase spawned from the current reference, for good measure (marked as skip-if(Android)).  But I think it'd be nice to have the main reftests here running on Android, if we can fix the reference case to be predictable.

[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=c320736a9c2c2471f1b4b38dd490512929b6684f&filter-searchStr=android%20R9
Pushed by mpalmgren@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d441a4991106
part 1 - Don't try to optimize re-positioning of Flexbox/Grid abs.pos. children since they are affected by CSS Align positioning too.  r=dholbert
https://hg.mozilla.org/integration/mozilla-inbound/rev/64d402a152c1
part 2 - Reftests.
Yes, they passed with position:relative in the reference files
so I enabled them on Android too and filed bug 1415756 to follow-up.
https://hg.mozilla.org/mozilla-central/rev/d441a4991106
https://hg.mozilla.org/mozilla-central/rev/64d402a152c1
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
QA Whiteboard: [good first verify]
I have reproduced this bug with Nightly 58.0a1 (2017-10-03) on Windows 10, 64 Bit!

This bug's fix is verified with Latest Beta!

Build ID  : 20171207170405
User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:58.0) Gecko/20100101 Firefox/58.0
QA Whiteboard: [good first verify] → [good first verify] [testday-20171208]
I have reproduced this bug with Nightly 58.0a1 (2017-10-03) on ubuntu 16.04

The bug's fix is now verified on Latest Beta 58.0b10

Build ID   20171207170405
User Agent Mozilla/5.0 (X11; Linux x86_64; rv:58.0) Gecko/20100101 Firefox/58.0

[testday-20171208]
Marking as verified based on above comments.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.