[css-contain] contain:size on button shouldn't suppress baseline alignment (but contain:layout should)

RESOLVED FIXED

Status

()

defect
P3
normal
RESOLVED FIXED
7 months ago
Last month

People

(Reporter: dholbert, Assigned: dholbert)

Tracking

(Blocks 2 bugs)

Trunk
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox65 disabled, firefox66 disabled, firefox67 disabled, firefox67.0.5 disabled, firefox68 fixed)

Details

Attachments

(2 attachments)

Assignee

Description

7 months ago
Posted file test.html
Looks like we missed a spot in bug 1491235 -- "contain:size" still seems to suppress baseline alignment on buttons, but it should not.

(but contain:layout should)

STR:
 1. Load attached testcase.

EXPECTED RESULTS:
 - The first two buttons should have the same vertical alignment (honoring the text's baseline).
 - The last 3 buttons should have the same vertical alignment (not honoring the text's baseline).

(Note: TYLin noticed that this might be a problem in bug 1507663 comment 2, via a test that still expects the old now-incorrect behavior.)
Assignee

Comment 1

7 months ago
"contain:layout" doesn't currently suppress baseline alignment on a few other form controls, too:
 - <input> type=button, submit, text, number, date, time
 - <textarea>

Let's fix that here as well. I'm adding a reftest in bug 1507663 which covers these & which I'll annotate as 'fails' with a reference to this bug.
Assignee

Comment 2

6 months ago
The new WPT test in bug 1514838, "css/css-contain/contain-layout-baseline-005.html", also exercises all of these & reveals a few more things where we aren't yet making "contain:layout" suppress baseline:
 - inline-grid
 - inline-table
Assignee

Comment 3

2 months ago

The original issue (for <button> and button-flavored widgets) turns out to be pretty trivial -- we just need to do s/IsContainSize/IsContainLayout/ here in the ascent-computation portion of nsHTMLButtonControlFrame::ReflowButtonContents():
https://searchfox.org/mozilla-central/rev/75294521381b331f821aad3d6b60636844080ee2/layout/forms/nsHTMLButtonControlFrame.cpp#314-320

Assignee: nobody → dholbert
Status: NEW → ASSIGNED
Assignee

Comment 5

2 months ago

(So far there's just "part 1", but I'll be posting a "part 2" here later on, to address the issues/test-failures noted in comment 1 and 2.)

Assignee

Comment 6

2 months ago

(In reply to Daniel Holbert [:dholbert] from comment #5)

(So far there's just "part 1", but I'll be posting a "part 2" here later on, to address the issues/test-failures noted in comment 1 and 2.)

Haven't gotten to this yet and I've got a bit of a review backlog now -- so I'm going to go ahead and land Part 1 now, and I'll post Part 2 in a few days, either here or in a followup bug. (Since we've got open test-failures with this bug number in an annotation-comment, I'll tentatively still plan on doing it in this bug.) (I think this is possible via Lando, though worst case I can just land Part 2 on Inbound.)

Assignee

Updated

2 months ago
Keywords: leave-open

Comment 7

2 months ago
Pushed by dholbert@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/7e12e84c8716
part 1: Adjust button reflow to suppress baseline for contain:layout, instead of for contain:size. r=TYLin
Assignee

Updated

Last month
Blocks: 1552287
Assignee

Comment 9

Last month

I spun off bug 1552287 for the remaining work here. Let's close this bug out, since the original testcase has been addressed.

Earlier releases aren't affected because containment is preffed off there, after the "early-beta" period (via the guard in bug 1532471).

Status: ASSIGNED → RESOLVED
Closed: Last month
Resolution: --- → FIXED

Comment 10

Last month
Pushed by dholbert@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/73c897cf5ec0
followup: adjust reftest failure annotation to point to an updated bug number instead of this closed bug. (no review, test-manifest-only, DONTBUILD)
You need to log in before you can comment on or make changes to this bug.