Closed Bug 1491235 Opened 6 years ago Closed 6 years ago

[css-contain] Size containment should not suppress baseline alignment (but layout containment should)

Categories

(Core :: Layout, defect, P3)

Product:
Core
Component:
Layout
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla64
Tracking Flags:
Tracking Status
firefox64 --- fixed

People

(Reporter: rego, Assigned: rego)

References

(
URL
)

Details

Attachments

(1 file, 2 obsolete files)

0001-Bug-1491235-css-contain-Make-contain-layout-not-cont.patch
(deleted), patch
dholbert
: review+
Details | Diff | Splinter Review
User Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/69.0.3497.92 Safari/537.36 Steps to reproduce: This is a test case to reproduce the issue: http://w3c-test.org/css/css-contain/contain-size-baseline-001.html The CSSWG when this was discussed was: http://w3c-test.org/css/css-contain/contain-size-baseline-001.html Apart from that WPT test, the test contain-size-inline-block-001.html from Firefox is checking the wrong thing.
Component: Untriaged → Layout
Product: Firefox → Core
The link to the CSSWG issue was wrong, it should be https://github.com/w3c/csswg-drafts/issues/2995 Note that apart from contain-size-inline-block-001.html some tests references would need to be updated, as baseline alignment is not suppresed anymore: * contain-size-fieldset-001-ref.html * contain-size-fieldset-002-ref.html
Blocks: css-contain-layout, css-contain-size
URL: https://github.com/w3c/csswg-drafts/i...
Status: UNCONFIRMED → NEW
Ever confirmed: true
Summary: [css-contain] Size containment does not suppresses baseline alignment → [css-contain] Size containment should not suppress baseline alignment (but layout containment should)
Priority: -- → P3
And another test that needs to get updated is: * contain-size-inline-flex-001.html
This is my first Gecko patch, but I wanted to update those tests. Please let me know if something is very wrong. O:-)
Attachment #9014718 - Flags: review?(dholbert)
Mentor: rego
Assignee: nobody → rego
QA Contact: svoisen
BTW, once test fails with my patch: http://w3c-test.org/css/css-contain/contain-size-flexbox-001.html But because the version in the repository is not up to date regarding WPT. If it gets updated before this lands, then it should pass again. :-)
Mentor: rego
QA Contact: svoisen
Comment on attachment 9014718 [details] [diff] [review] 0001-Bug-1491235-css-contain-Layout-and-size-containment-.patch Review of attachment 9014718 [details] [diff] [review]: ----------------------------------------------------------------- Thank you for filing & fixing doing this! (And congrats on your first Gecko patch :)) A few nits, at first glance: (1) Please update the commit message to more directly describe the change, rather than hinting at the overall topic. Maybe something like the following: Bug 1491235: Make 'contain:layout' (not of 'contain:size') suppress baseline measurements. (2) Please update the code-comment in contain-size-inline-flex-001.html (whose reference case you're fixing), like you did in some of the other tests here. Specifically, the comments here need updating: https://dxr.mozilla.org/mozilla-central/source/layout/reftests/w3c-css/submitted/contain/contain-size-inline-flex-001.html#27-32 (3) For the reftests that you're fixing here, please also fix our "authoritative" copy, which is (currently) the copy in "layout/reftests/w3c-css/submitted/contain". Simplest way to do this is probably to just run this, locally: > cp testing/web-platform/tests/css/vendor-imports/mozilla/mozilla-central-reftests/contain/*html layout/reftests/w3c-css/submitted/contain/ > hg revert layout/reftests/w3c-css/submitted/contain/contain-size-multicol-003* (the 'hg revert' is necessary because that tests has intentional differences between the two folders, unrelated to your changes, from -moz prefixing in our "private" layout/reftests copy.)
(In reply to Daniel Holbert [:dholbert] from comment #5) > (1) Please update the commit message to more directly describe the change, > rather than hinting at the overall topic. Maybe something like the following: > Bug 1491235: Make 'contain:layout' (not of 'contain:size') suppress baseline measurements. Sorry, stray word -- s/not of/not/. (I'd initially phrased my suggested-text as "...instead of...", and then swapped in "...not...", but didn't clean up after myself properly. :))
Comment on attachment 9014718 [details] [diff] [review] 0001-Bug-1491235-css-contain-Layout-and-size-containment-.patch Review of attachment 9014718 [details] [diff] [review]: ----------------------------------------------------------------- I think this is my only other feedback, in addition to what I posted in the previous comment. This is probably r+ with feedback addressed, but I'll mark it r- for now because I'd like to take a look at the final version before it lands. ::: testing/web-platform/tests/css/vendor-imports/mozilla/mozilla-central-reftests/contain/contain-size-fieldset-001-ref.html @@ +22,5 @@ > </head> > <body> > + <div class="container"> > + l<br>i > + </div> The changes/additions to this reference case feel fairly substantial. Given that this reftest isn't meant to test baseline-alignment[1], maybe it'd be better to revert these changes and simply add... vertical-align: top; ...to the .container{} style rule, in the testcase and reference case? That one-line tweak (on its own) makes the test pass before & after your code-changes, and it'd let us preserve the nice purity of the reference case containers *truly being empty*, which is quite nice for trivially exercising/demonstrating the spec's requirement that contain:size stuff "must be treated as having no contents". (https://drafts.csswg.org/css-contain/#containment-size) (The goal of this test is purely to exercise *sizing* (particularly intrinsic sizing) -- the only reason it's tripping up some baseline special cases is because it's got "display:inline-block". But we're only using that because it's a convenient way to trigger intrinsic sizing -- not because we're aiming to test inline layout. The next test, -002, is the one that's intending to test baseline alignment, so we do have coverage for that.) ::: testing/web-platform/tests/css/vendor-imports/mozilla/mozilla-central-reftests/contain/contain-size-fieldset-002.html @@ +2,4 @@ > <html> > <head> > <meta charset="utf-8"> > + <title>CSS Test: 'contain: size' on fieldset elements should cause them to be baseline-aligned regularly.</title> clarity nit: s/should cause them to be/shouldn't prevent them from being/ (contain:size isn't *causing* them to baseline align -- it's just not interfering anymore.)
Attachment #9014718 - Flags: review?(dholbert) → review-
Thank you very much for the thoroughly review. I believe I've applied all your suggested changes in the new version of the patch. Apart from that http://w3c-test.org/css/css-contain/contain-size-flexbox-001.html hasn't been updated yet, so I guess we have two options: 1) Import the changes on that test as part of this patch. 2) Mark the test as failure, until the changes are imported and it starts to pass. I don't know what's the preferred option or if you have any other idea about that.
Attachment #9014718 - Attachment is obsolete: true
Attachment #9015190 - Flags: review?(dholbert)
RE that outstanding issue with the test update: I don't 100% know the mechanics of our WPT synchronization, but I'd say: - The simplest option is option 1, i.e. directly fixing the test in mozilla-central, and then allowing those changes to propagate to WPT in our next sync (which I think would happen in the next ~week). - However: if you've already committed the test-fix upstream somewhere [i.e. if we're already going to receive a test update via some future sync from https://github.com/web-platform-tests/ ], then I'd lean towards option 2, to be on the safe side, because I'm not sure how well our synchronization logic handles the "identical change locally + upstream" scenario. (It probably Just Works, but I'm not confident enough to impose the sorting-that-out burden on jgraham in case it's painful.)
Besides that, the patch looks good! Holding off on r+ since it needs that last tweak (per comment 8 - 9) before it can land. I took the liberty of pushing this to our Try repository to see if it passes tests: https://treeherder.mozilla.org/#/jobs?repo=try&revision=09e57e89460c699b6133a24d4a2dd29faaf9f288 (I assume that the one test from comment 8 will be reported as an unexpected failure right now, but hopefully that'll be it besides maybe an unrelated known-intermittent issue or two.)
Attachment #9015190 - Flags: review?(dholbert)
Flags: needinfo?(rego)
The test was already fixed in WPT (September 28th) and it seems it has just being imported into Firefox now! So now all tests should be passing and we don't need to do anything else, just remove the .ini file as the modified test is now passing thanks to this patch (also the "contain: layout" version starts to pass with this patch).
Attachment #9015190 - Attachment is obsolete: true
Flags: needinfo?(rego)
Attachment #9015450 - Flags: review?(dholbert)
The (WebRender-only) oranges on that Try run look like a known intermittent, so I'll go ahead and land this. Thanks again for the patch, rego!
Attachment #9015450 - Flags: review?(dholbert) → review+
Pushed by dholbert@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/4e07d3243403 [css-contain] Make 'contain:layout' (not 'contain:size') suppress baseline measurements r=dholbert
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/13444 for changes under testing/web-platform/tests
Upstream web-platform-tests status checks passed, PR will merge once commit reaches central.
https://hg.mozilla.org/mozilla-central/rev/4e07d3243403
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox64: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla64
(In reply to Web Platform Test Sync Bot from comment #19) > Can't merge web-platform-tests PR due to failing upstream checks: > Github PR https://github.com/web-platform-tests/wpt/pull/13444 > * Taskcluster (pull_request) > (https://tools.taskcluster.net/task-group-inspector/#/ZlQuYJP0T8WINBWrBMxvvg) This comment seems misleading as the PR has been actually merged upstream: https://github.com/web-platform-tests/wpt/pull/13444
Yeah, it looks like everything worked out OK, but I'm curious what was going on in comment 19. (Notably, it seems quite busted for a bot to post "can't merge" [comment 19] after it already posted that it merged successfully [comment 18]...) jgraham, do you know what happened here?
Flags: needinfo?(james)
I think what's happening is that we aren't handling required status checks properly. So I think we find that the PR is mergable, merge it, and then subsequently get a failure which we report here. So the simplest thing to do is just to check if the PR already merged when handling status checks and bail if it did.
Flags: needinfo?(james)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: