Closed Bug 1507663 Opened 1 year ago Closed 1 year 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)

enhancement

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'.
Flags: needinfo?(dholbert)
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
I filed bug 1508441 on the button issue from comment 2 - looks like our impl is broken (out of date) there.
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
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
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
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: 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)
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
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.
(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 :)
Depends on: 1511963
You need to log in before you can comment on or make changes to this bug.