Closed Bug 1405319 Opened 7 years ago Closed 7 years ago

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

Categories

(Core :: Layout, defect, P3)

defect

Tracking

()

VERIFIED FIXED
mozilla58
Tracking Status
firefox57 --- fix-optional
firefox58 --- verified

People

(Reporter: rego, Assigned: MatsPalmgren_bugz)

Details

(Keywords: testcase)

Attachments

(5 files)

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.
Attached image Current output
Attached 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.
Status: ASSIGNED → RESOLVED
Closed: 7 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.

Attachment

General

Created:
Updated:
Size: