Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Are css/css-contain/contain-layout-baseline-005.html css/css-contain/contain-layout-button-001.html correct? #45889

Open
bfgeek opened this issue Apr 24, 2024 · 9 comments
Open

Are css/css-contain/contain-layout-baseline-005.html css/css-contain/contain-layout-button-001.html correct? #45889

bfgeek opened this issue Apr 24, 2024 · 9 comments
Labels
css-contain

Comments

@bfgeek
Copy link
Contributor

bfgeek commented Apr 24, 2024

I'm currently investigating making buttons block-layout by default in Blink.

As part of this work I'm fixing how we synthesize baselines for various form elements in an inline-context.
(Note: this only applies to inline baselines, in all other contexts (flex/grid align-items: baseline) we synthesize based off the border-box edges).

Broadly speaking everything synthesizes off the margin-box edges except:

  • <input type=radio>/<inpput type=checkbox> - with effective appearance they synthesize off the border-box edges, and with appearance:none of the margin-box edges.
  • <input type=range> in Blink/Webkit synthesizes off the border-box edge, Firefox margin-box.
  • <button>/<input type=button>/<input type=submit>/<input type=reset> synthesizes off the content-box edge. Testcase: https://www.software.hixie.ch/utilities/js/live-dom-viewer/?saved=12646 (Blink isn't correct here but we'll fix that).

When containment is applied, we all seem to be synthesizing button like things off the margin-box edge.

I think this is incorrect per-spec:

https://drafts.csswg.org/css-contain-1/#containment-layout

For the purpose of the vertical-align property, or any other property whose effects need to relate the position of the layout containment box's baseline to something other than its descendants, the containment box is treated as having no baseline.

(To me) This reads as if:

baseline <button></button> <button style="contain:layout"></button>

Should be aligned the same way (e.g. the context-box edge(s)) as we are just treating them as having no baseline, and are instead synthesizing it.

@frivoal @dholbert @emilio Thoughts?

cc/ @mrego As you wrote (at least) one of the tests :P.

Ian
@bfgeek
Copy link
Contributor Author

bfgeek commented Apr 24, 2024

https://wpt.live/css/css-contain/contain-layout-baseline-005.html
https://wpt.live/css/css-contain/contain-layout-button-001.html

@bfgeek
Copy link
Contributor Author

bfgeek commented Apr 24, 2024

https://chromium-review.googlesource.com/c/chromium/src/+/5479489 - if anyone is curious about the baseline patch.

@dholbert
Copy link
Contributor

I'm not yet entirely sure what makes the most sense here, but one other situation that's worth comparing is what happens when these elements have an orthogonal writing-mode. In Firefox, that makes us synthesize a baseline using their border-edge, it seems.

Same WM as parent: https://www.software.hixie.ch/utilities/js/live-dom-viewer/saved/12648
Same WM as parent, plus contain:layout: https://www.software.hixie.ch/utilities/js/live-dom-viewer/saved/12649
(Firefox uses the bottom content-edge of in the first case, vs. bottom margin-edge in the second case, as IanK noted)

Orthogonal WM to parent: https://www.software.hixie.ch/utilities/js/live-dom-viewer/saved/12647
Orthogonal WM to parent, plus contain:layout: https://www.software.hixie.ch/utilities/js/live-dom-viewer/saved/12650
(Firefox renders these two^ identically, using the bottom border-edge to synthesize the baseline)

@dholbert
Copy link
Contributor

Chrome (v126 dev) seems to synthesize a baseline from the bottom margin-edge, for all of the cases in my previous comment, except for the first one where it uses the content-box bottom for the button and input type="color" vs. the margin-box for the others (I think that inconsistency is the thing @bfgeek mentioned as being not correct)

@bfgeek
Copy link
Contributor Author

bfgeek commented Apr 24, 2024

See also whatwg/html#5065

I'm not yet entirely sure what makes the most sense here, but one other situation that's worth comparing is what happens when these elements have an orthogonal writing-mode. In Firefox, that makes us synthesize a baseline using their border-edge, it seems.

(Ignoring <input type=color> for reasons) My current patch will synthesize based off the content edges in all of the above cases. (Which seems reasonable?)

@bfgeek
Copy link
Contributor Author

bfgeek commented Apr 24, 2024

cc/ @zcorpan

@dholbert
Copy link
Contributor

dholbert commented Apr 25, 2024 •
edited
Loading

At a high level, I think the following makes sense based on current specs:

I'm not entirely sure if how-that-baseline-is-synthesized is well-defined for buttons in an inline context, but it seems worth ensuring it's well-defined & consistent.

@bfgeek bfgeek mentioned this issue Apr 26, 2024
Open
@bfgeek
Copy link
Contributor Author

bfgeek commented Apr 26, 2024

Yeah - that's the same logic I followed as well - as such I believe that:

<button></button> <button style="contain:layout"></button>

https://www.software.hixie.ch/utilities/js/live-dom-viewer/?saved=12662

Should be aligned (ignoring the issue of how to synthesize baselines for button elements). No browser does this today (likely because of the tests).

I'm going to remove the <button> from one of the tests, and mark the other test as .tentative

chromium-wpt-export-bot pushed a commit that referenced this issue Apr 26, 2024
@bfgeek @chromium-wpt-export-bot
[layout] Fix synthesis of inline baselines.
e3a47d2 
Previously for <button>, <input type=button> (and friends), and
<input type=range> we'd explicitly produce a baseline at appropriate
"end" edge.

This wasn't strictly correct instead we should be synthesizing them
within LogicalBoxFragment::BaselineMetrics.

There are three different ways to synthesize these baseline, off the
margin-box (the default), content-box (buttons), and border-box (range
and checkbox/radio with effective appearance).

Instead of checking all these conditions, this patch introduces an
additional field to the computed style to tell BaselineMetrics which
edge the synthesize off.

This patch makes changes two WPT tests which (I believe) to be
incorrect see:
#45889

The TL;DR is that:
<button></button> <button style="contain:layout"></button>

Currently have different baselines when they shouldn't.

The image rebaselines are ensuring that we synthesize the baseline
for range controls consistently. Previously we'd synthesize them off
the border-box if they were in the same writing-mode, and off the
margin-box if in a different writing-mode.

Bug: 40937312
Change-Id: I28eedfbce58ead9d5e3119ae3b164c559fb9267c
Merged
aarongable pushed a commit to chromium/chromium that referenced this issue Apr 29, 2024
@bfgeek
[layout] Fix synthesis of inline baselines.
a19e491 
Previously for <button>, <input type=button> (and friends), and
<input type=range> we'd explicitly produce a baseline at appropriate
"end" edge.

This wasn't strictly correct instead we should be synthesizing them
within LogicalBoxFragment::BaselineMetrics.

There are three different ways to synthesize these baseline, off the
margin-box (the default), content-box (buttons), and border-box (range
and checkbox/radio with effective appearance).

Instead of checking all these conditions, this patch introduces an
additional field to the computed style to tell BaselineMetrics which
edge the synthesize off.

This patch changes two WPT tests which I believe were incorrect, see:
web-platform-tests/wpt#45889

The TL;DR is that:
<button></button> <button style="contain:layout"></button>

Currently have different baselines when they shouldn't.

The image rebaselines are ensuring that we synthesize the baseline
for range controls consistently. Previously we'd synthesize them off
the border-box if they were in the same writing-mode, and off the
margin-box if in a different writing-mode.

Bug: 40937312
Change-Id: I28eedfbce58ead9d5e3119ae3b164c559fb9267c
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5479489
Reviewed-by: Morten Stenshorne <mstensho@chromium.org>
Commit-Queue: Ian Kilpatrick <ikilpatrick@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1293898}
chromium-wpt-export-bot pushed a commit that referenced this issue Apr 30, 2024
@bfgeek @chromium-wpt-export-bot
[layout] Fix synthesis of inline baselines.
cd2ffe8 
Previously for <button>, <input type=button> (and friends), and
<input type=range> we'd explicitly produce a baseline at appropriate
"end" edge.

This wasn't strictly correct instead we should be synthesizing them
within LogicalBoxFragment::BaselineMetrics.

There are three different ways to synthesize these baseline, off the
margin-box (the default), content-box (buttons), and border-box (range
and checkbox/radio with effective appearance).

Instead of checking all these conditions, this patch introduces an
additional field to the computed style to tell BaselineMetrics which
edge the synthesize off.

This patch changes two WPT tests which I believe were incorrect, see:
#45889

The TL;DR is that:
<button></button> <button style="contain:layout"></button>

Currently have different baselines when they shouldn't.

The image rebaselines are ensuring that we synthesize the baseline
for range controls consistently. Previously we'd synthesize them off
the border-box if they were in the same writing-mode, and off the
margin-box if in a different writing-mode.

Bug: 40937312
Change-Id: I28eedfbce58ead9d5e3119ae3b164c559fb9267c
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5479489
Reviewed-by: Morten Stenshorne <mstensho@chromium.org>
Commit-Queue: Ian Kilpatrick <ikilpatrick@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1293898}
chromium-wpt-export-bot pushed a commit that referenced this issue Apr 30, 2024
@bfgeek @chromium-wpt-export-bot
[layout] Fix synthesis of inline baselines.
b312464 
Previously for <button>, <input type=button> (and friends), and
<input type=range> we'd explicitly produce a baseline at appropriate
"end" edge.

This wasn't strictly correct instead we should be synthesizing them
within LogicalBoxFragment::BaselineMetrics.

There are three different ways to synthesize these baseline, off the
margin-box (the default), content-box (buttons), and border-box (range
and checkbox/radio with effective appearance).

Instead of checking all these conditions, this patch introduces an
additional field to the computed style to tell BaselineMetrics which
edge the synthesize off.

This patch changes two WPT tests which I believe were incorrect, see:
#45889

The TL;DR is that:
<button></button> <button style="contain:layout"></button>

Currently have different baselines when they shouldn't.

The image rebaselines are ensuring that we synthesize the baseline
for range controls consistently. Previously we'd synthesize them off
the border-box if they were in the same writing-mode, and off the
margin-box if in a different writing-mode.

Bug: 40937312
Change-Id: I28eedfbce58ead9d5e3119ae3b164c559fb9267c
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5479489
Reviewed-by: Morten Stenshorne <mstensho@chromium.org>
Commit-Queue: Ian Kilpatrick <ikilpatrick@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1293898}
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this issue May 2, 2024
@bfgeek @moz-wptsync-bot
Bug 1893769 [wpt PR 45936] - [layout] Fix synthesis of inline baselin…
fbbc6dc 
…es., a=testonly

Automatic update from web-platform-tests
[layout] Fix synthesis of inline baselines.

Previously for <button>, <input type=button> (and friends), and
<input type=range> we'd explicitly produce a baseline at appropriate
"end" edge.

This wasn't strictly correct instead we should be synthesizing them
within LogicalBoxFragment::BaselineMetrics.

There are three different ways to synthesize these baseline, off the
margin-box (the default), content-box (buttons), and border-box (range
and checkbox/radio with effective appearance).

Instead of checking all these conditions, this patch introduces an
additional field to the computed style to tell BaselineMetrics which
edge the synthesize off.

This patch changes two WPT tests which I believe were incorrect, see:
web-platform-tests/wpt#45889

The TL;DR is that:
<button></button> <button style="contain:layout"></button>

Currently have different baselines when they shouldn't.

The image rebaselines are ensuring that we synthesize the baseline
for range controls consistently. Previously we'd synthesize them off
the border-box if they were in the same writing-mode, and off the
margin-box if in a different writing-mode.

Bug: 40937312
Change-Id: I28eedfbce58ead9d5e3119ae3b164c559fb9267c
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5479489
Reviewed-by: Morten Stenshorne <mstensho@chromium.org>
Commit-Queue: Ian Kilpatrick <ikilpatrick@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1293898}

--

wpt-commits: b312464e2203190809de7c34055a51b7643ba6c2
wpt-pr: 45936
ErichDonGubler pushed a commit to erichdongubler-mozilla/firefox that referenced this issue May 2, 2024
@bfgeek @moz-wptsync-bot
Bug 1893769 [wpt PR 45936] - [layout] Fix synthesis of inline baselin…
6e210d6 
…es., a=testonly

Automatic update from web-platform-tests
[layout] Fix synthesis of inline baselines.

Previously for <button>, <input type=button> (and friends), and
<input type=range> we'd explicitly produce a baseline at appropriate
"end" edge.

This wasn't strictly correct instead we should be synthesizing them
within LogicalBoxFragment::BaselineMetrics.

There are three different ways to synthesize these baseline, off the
margin-box (the default), content-box (buttons), and border-box (range
and checkbox/radio with effective appearance).

Instead of checking all these conditions, this patch introduces an
additional field to the computed style to tell BaselineMetrics which
edge the synthesize off.

This patch changes two WPT tests which I believe were incorrect, see:
web-platform-tests/wpt#45889

The TL;DR is that:
<button></button> <button style="contain:layout"></button>

Currently have different baselines when they shouldn't.

The image rebaselines are ensuring that we synthesize the baseline
for range controls consistently. Previously we'd synthesize them off
the border-box if they were in the same writing-mode, and off the
margin-box if in a different writing-mode.

Bug: 40937312
Change-Id: I28eedfbce58ead9d5e3119ae3b164c559fb9267c
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5479489
Reviewed-by: Morten Stenshorne <mstensho@chromium.org>
Commit-Queue: Ian Kilpatrick <ikilpatrick@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1293898}

--

wpt-commits: b312464e2203190809de7c34055a51b7643ba6c2
wpt-pr: 45936
@frivoal
Copy link
Contributor

frivoal commented May 30, 2024

I completely agree with #45889 (comment)

I probably agree with #45889 (comment), the nuance being that I don't know which bit of spec says that a button without containment also has no baseline (or alternatively, says that it does have one that happens to coincide with how it gets synthesized if it didn't).

i3roly pushed a commit to i3roly/firefox-dynasty that referenced this issue Jun 1, 2024
@bfgeek @moz-wptsync-bot
Bug 1893769 [wpt PR 45936] - [layout] Fix synthesis of inline baselin…
ad8b446 
…es., a=testonly

Automatic update from web-platform-tests
[layout] Fix synthesis of inline baselines.

Previously for <button>, <input type=button> (and friends), and
<input type=range> we'd explicitly produce a baseline at appropriate
"end" edge.

This wasn't strictly correct instead we should be synthesizing them
within LogicalBoxFragment::BaselineMetrics.

There are three different ways to synthesize these baseline, off the
margin-box (the default), content-box (buttons), and border-box (range
and checkbox/radio with effective appearance).

Instead of checking all these conditions, this patch introduces an
additional field to the computed style to tell BaselineMetrics which
edge the synthesize off.

This patch changes two WPT tests which I believe were incorrect, see:
web-platform-tests/wpt#45889

The TL;DR is that:
<button></button> <button style="contain:layout"></button>

Currently have different baselines when they shouldn't.

The image rebaselines are ensuring that we synthesize the baseline
for range controls consistently. Previously we'd synthesize them off
the border-box if they were in the same writing-mode, and off the
margin-box if in a different writing-mode.

Bug: 40937312
Change-Id: I28eedfbce58ead9d5e3119ae3b164c559fb9267c
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5479489
Reviewed-by: Morten Stenshorne <mstensho@chromium.org>
Commit-Queue: Ian Kilpatrick <ikilpatrick@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1293898}

--

wpt-commits: b312464e2203190809de7c34055a51b7643ba6c2
wpt-pr: 45936
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
css-contain
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@frivoal @dholbert @bfgeek