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

Status: VERIFIED FIXED

(Core :: Layout, defect, P3)

Description

[Manuel Rego Casasnovas]

Attachment: test-firefox-issue.html

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.

Comment 1

[Manuel Rego Casasnovas]

Attachment: Current output

Comment 2

[Manuel Rego Casasnovas]

Attachment: Expected output

Comment 3

[Mats Palmgren (inactive)]

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...

Comment 4

[Mats Palmgren (inactive)]

Attachment: part 1 - Don't try to optimize re-positioning of Flexbox/Grid abs.pos. children since they are affected by CSS Align positioning too

Comment 5

[Mats Palmgren (inactive)]

Attachment: part 2 - Reftests.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=5a0d43ec0623e1e8c31fc2528d46ff6d193e1005

Comment 6

[Daniel Holbert [:dholbert]]

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

Comment 7

[Daniel Holbert [:dholbert]]

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

Comment 8

[Pulsebot]

Pushed by [e-mail]:
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.

Comment 9

[Mats Palmgren (inactive)]

Yes, they passed with position:relative in the reference files
so I enabled them on Android too and filed bug 1415756 to follow-up.

Comment 10

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/d441a4991106
https://hg.mozilla.org/mozilla-central/rev/64d402a152c1

Comment 11

[Mohammad Maruf Rahman]

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

Comment 12

[Tanvir Rahman]

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]

Comment 13

[Petruta Horea [:phorea], Desktop Test Engineering (back on Sep 5th)]

Marking as verified based on above comments.