Closed
Bug 1507663
Opened 6 years ago
Closed 6 years ago
contain-size-multicol-003.html needs an update to stop testing baseline (and related cleanup in other "contain" baseline-related tests)
Categories
(Core :: Layout, enhancement, P3)
Core
Layout
Tracking
()
RESOLVED
FIXED
mozilla65
Tracking | Status | |
---|---|---|
firefox65 | --- | fixed |
People
(Reporter: dholbert, Assigned: dholbert)
References
Details
Attachments
(6 files)
contain-size-multicol-003.html still expects "contain:size" to suppress baseline alignment, in its very first div.
That's now an incorrect expectation, as of bug 1491235. Apparently the tests still "passes" for now (maybe because multicol elements do their own baseline-suppression by accident?), but it stops passing as of bug 1491915.
We need to split the baseline-related chunk of this test out into a separate test that uses 'contain:layout' rather than 'contain:size'.
Assignee | ||
Comment 1•6 years ago
|
||
Here's the chunk of the test that needs splitting out:
https://searchfox.org/mozilla-central/rev/c0b26c40769a1e5607a1ae8be37fe64df64fc55e/layout/reftests/w3c-css/submitted/contain/contain-size-multicol-003.html#40-44
Assignee | ||
Updated•6 years ago
|
Flags: needinfo?(dholbert)
Comment 2•6 years ago
|
||
A quick search of "baseline alignment as if" shows the following chuck in contain-size-button-001.html. It might need to be updated, too, but I'm not sure.
https://searchfox.org/mozilla-central/rev/647b9eb1099e373e079e16f147f74f3d1d65eed3/layout/reftests/w3c-css/submitted/contain/contain-size-button-001.html#74-76
Also, there's a typo "basic" in <fieldset class="basc">. After fixing that, maybe we don't need the "fieldset" style.
https://searchfox.org/mozilla-central/rev/647b9eb1099e373e079e16f147f74f3d1d65eed3/layout/reftests/w3c-css/submitted/contain/contain-size-fieldset-002-ref.html#40
Assignee | ||
Comment 3•6 years ago
|
||
I filed bug 1508441 on the button issue from comment 2 - looks like our impl is broken (out of date) there.
Assignee | ||
Comment 4•6 years ago
|
||
Assignee | ||
Comment 5•6 years ago
|
||
Note that Firefox doesn't actually match this expectation yet, so I've added a
'fails' annotation to the manifest with the followup bug number.
Also, this patch makes several other improvements to this test:
- remove red background in testcase. This was making the testcase spuriously
fail in Chrome, because Chrome paints (at least) a 1px-tall background-area
on empty buttons, which meant a 1px-tall red area in the testcase vs. a
1px-tall gray area in the reference case.
- clear floats to prevent them from piling up awkwardly.
- use 'vertical-align:top' to turn off baseline alignment in parts of the test
where the testcase has text and the reference case does not (and where we're
not intentionally testing the baseline's influence on layout).
Depends on D12614
Assignee | ||
Comment 6•6 years ago
|
||
Depends on D12615
Assignee | ||
Comment 7•6 years ago
|
||
Note that we don't get this correct for form controls yet, so the -002 test is annotated as "fails" for now.
Depends on D12616
Assignee | ||
Comment 8•6 years ago
|
||
This class wasn't applied due to a typo, and it's unnecessary anyway -- there's
a separate 'fieldset {...}' CSS rule further down in the file that has the same
effect (hiding the border and the textual contents).
Depends on D12617
Assignee | ||
Comment 9•6 years ago
|
||
TYLin, I'm hoping you're up for reviewing here. :) (No big rush, sorry to post just before the holiday weekend -- this can definitely wait until next week.)
I'm not sure how much you've looked at containment, but it's not too complicated -- the main backstory here is that 'baseline' suppression was moved from 'contain:size' to 'contain:layout' (which we mostly implemented in bug 1491235), and we still have some work to do on updating tests (this bug!) and implementation (bug 1508441) to stay kept up with that spec-change.
Try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b1ea4a6a5c634f92d5195b035731a674baa2e0dc
Flags: needinfo?(dholbert)
Assignee | ||
Updated•6 years ago
|
Assignee: nobody → dholbert
Status: NEW → ASSIGNED
Summary: contain-size-multicol-003.html needs an update to stop testing baseline → contain-size-multicol-003.html needs an update to stop testing baseline (and related cleanup in other "contain" baseline-related tests)
Comment 10•6 years ago
|
||
Pushed by dholbert@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/2ca8fb2abe07
part 1: Uncomment/invert expectations in some reftests to now expect that contain:size *does not* interfere with baseline alignment. r=TYLin
https://hg.mozilla.org/integration/autoland/rev/1fe8baada237
part 2: Adjust reftest 'contain-size-button-001.html' to expect that contain:size *does not* suppress baseline alignment. r=TYLin
https://hg.mozilla.org/integration/autoland/rev/c20d49e40432
part 3: Update titles to remove stale references to baseline alignment, in two reftests that don't test baseline alignment. r=TYLin
https://hg.mozilla.org/integration/autoland/rev/c99b6f6c3bcf
part 4: Add dedicated reftests to verify that "contain:layout" suppresses baseline alignment. r=TYLin
https://hg.mozilla.org/integration/autoland/rev/a3755a4ba5aa
part 5: Remove stray/unused markup for "basic"/"basc" class in contain-size-fieldset-002-ref.html. r=TYLin
Comment 11•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/2ca8fb2abe07
https://hg.mozilla.org/mozilla-central/rev/1fe8baada237
https://hg.mozilla.org/mozilla-central/rev/c20d49e40432
https://hg.mozilla.org/mozilla-central/rev/c99b6f6c3bcf
https://hg.mozilla.org/mozilla-central/rev/a3755a4ba5aa
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla65
Comment 12•6 years ago
|
||
WPT import at https://github.com/web-platform-tests/wpt/pull/14264
Comment 13•6 years ago
|
||
The test was pointing to the wrong reference.
Dunno if it's fine to upload a patch here or if I should report a different bug.
Comment 14•6 years ago
|
||
(In reply to Manuel Rego Casasnovas from comment #13)
> Created attachment 9029358 [details] [diff] [review]
> 0001-Bug-1507663-Fix-reference-in-contain-layout-suppress.patch
>
> The test was pointing to the wrong reference.
>
> Dunno if it's fine to upload a patch here or if I should report a different
> bug.
I think usually we don't reopen bugs or land patches in FIXED bugs, so I'd file a new one blocking this one :)
You need to log in
before you can comment on or make changes to this bug.
Description
•