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.

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

[css-grid] Note implies losing an aspect-ratio when it shouldn't? #5713

Closed
bfgeek opened this issue Nov 10, 2020 · 38 comments
Closed

[css-grid] Note implies losing an aspect-ratio when it shouldn't? #5713

bfgeek opened this issue Nov 10, 2020 · 38 comments
Labels

Comments

@bfgeek
Copy link

bfgeek commented Nov 10, 2020

https://drafts.csswg.org/css-grid-1/#grid-item-sizing

There is a note which says "Note: This can distort the aspect ratio of the item, if it has one."

This implies that if there is 'stretch' in one dimension, it should distort the image. This note should probably say something along the lines of "Note: This can distort the aspect ratio of the item, if it has stretch in both axes." or something.

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

@bfgeek
Copy link
Author

bfgeek commented Nov 10, 2020

cc/ @fantasai @tabatkins

@Loirooriol
Copy link
Contributor

Not sure if I'm missing something, but your testcase only has stretch in a single axis, and the aspect ratio is distorted in all Chromium, WebKit and Firefox. So the current spec matches implementations.

@bfgeek
Copy link
Author

bfgeek commented Nov 10, 2020

@Loirooriol - I think what's happened is that folks took the note too literally (and normatively). FWIW this now doesn't match what happens with flex:
https://www.software.hixie.ch/utilities/js/live-dom-viewer/?saved=8676
(requires Chrome Canary, and recent version of Firefox).

@Loirooriol
Copy link
Contributor

It's not just the note, though. The spec text says

Use the inline size calculation rules for non-replaced boxes (defined in CSS 2 §10.3.3 Block-level, non-replaced elements in normal flow).

where CSS 2 §10.3.3 basically ignores aspect ratios. So it's not clear to me that the spec had a different intention.

Also see #5615

@Loirooriol
Copy link
Contributor

Regarding consistency with flex, I don't think your testcase is testing the proper thing. The flex item is effectively stretched in the cross axis, it's just that it's not stretched in the main axis because justify-self doesn't apply, so the aspect ratio is preserved.

The interesting testcase would be https://software.hixie.ch/utilities/js/live-dom-viewer/saved/8677

In latest Chromium with experimental flags, the grid item and the flex item are identical (aspect-ratio has no effect). But if I apply https://chromium-review.googlesource.com/c/chromium/src/+/2526928, they are different:

master vs patch

@fantasai
Copy link
Collaborator

@Loirooriol “Use the inline size calculation rules for non-replaced boxes” is supposed to apply to the sizing in the relevant axis, not in both axes, though.

I agree with @bfgeek, the behavior here seems quite wrong.

@bfgeek
Copy link
Author

bfgeek commented Nov 11, 2020

Implementations also seem to have bugs when determining track contributions, e.g.
https://www.software.hixie.ch/utilities/js/live-dom-viewer/?saved=8681
(currently an unstable layout in chrome).

I'm planning on doing some implementation work in this area - and will add some .tentative tests referring to this issue.

@Loirooriol
Copy link
Contributor

OK, so I think

  • If both axes have stretch alignment with auto sizes, both should be stretched, likely distorting the aspect ratio.
  • If one axis has stretch with auto size and the other has a definite size, the former should be stretched and the latter should use the definite size, likely distorting the aspect ratio. This matches flexbox and implementations.
  • If one axis has stretch with auto size and the other has non-stretch alignment with auto size, the former should be stretched. I'm fine with making the latter resolve to the stretched size converted through the aspect ratio, matching flexbox. But note it's not what implementations currently do.

@bfgeek
Copy link
Author

bfgeek commented Nov 11, 2020

The only concern I have is for the following case:

<div style="display: block; width: 200px; border: solid;">
  <div style="height: 100px; aspect-ratio: 1/1;"></div>
</div>

<div style="display: grid; width: 200px; border: solid;">
  <div style="height: 100px; aspect-ratio: 1/1;"></div>
</div>

Conceptually these cases are the same, both make 'auto' "stretch" in the inline-axis, and definite height in the block-axis, and an aspect-ratio.

@Loirooriol
Copy link
Contributor

@bfgeek In that case the elements have justify-self: normal, so I think

  • For the grid item, the width is "as for a block-level box" [1] and "as for a replaced element" [2]. That is, CSS 2 §10.3.4, which will respect the aspect ratio and the width will be 100px.
  • For the block box, justify-self: normal theoretically behaves as stretch [3], but I think CSS Align is wrong here, since replaced block elements are not stretched. I think that due to aspect ratio it should be sized as a replaced element, resulting in CSS 2 §10.3.4 again.

But if they had justify-self: stretch, then

  • Per [4], the grid item would explicitly use CSS 2 §10.3.3, resulting in a width of 200px, ignoring the aspect ratio.
  • Per [5], the block box would be sized "to the length necessary to make its outer size as close to filling the alignment container as possible", which I guess maps to CSS 2 §10.3.3 too.

@bfgeek
Copy link
Author

bfgeek commented Nov 12, 2020

For the block box, justify-self: normal theoretically behaves as stretch [3], but I think CSS Align is wrong here, since replaced block elements are not stretched.

It's correct for non-replaced elements. Which stretch be default. That line should probably read:

normal Behavior | Behaves as stretch except for replaced elements, tables, and some form controls which are start.
(or something).

But if they had justify-self: stretch

I'm not sure we should make a distinction between justify-self: stretch , and justify-self: normal.

@Loirooriol
Copy link
Contributor

I'm not sure we should make a distinction between justify-self: stretch, and justify-self: normal.

What's the point of having them as different values, then?

@Loirooriol
Copy link
Contributor

cc/ @BorisChiou

@bfgeek
Copy link
Author

bfgeek commented Nov 13, 2020

What's the point of having them as different values, then?

My answer would be - for the default behaviour switch between replaced and non-replaced elements.

So there are two clear options here.

  1. Define a "strong" stretch, and a "weak" stretch. E.g. if you have justify-self: stretch this is a "strong" stretch, justify-self: normal is a "weak" stretch. The priority list for this would be something like:
  • "weak" stretch alignment.
  • aspect-ratio transferred size.
  • "strong" stretch alignment.
  • definite size.
  1. Define an aspect-ratio transferred size as have higher priority than stretch alignment, e.g.
  • stretch alignment.
  • aspect-ratio transferred size.
  • definite size.

I prefer (2) as I somewhat view a aspect-ratio with a definite size in the opposite axis the same as, a definite size in the original axis.

@fantasai any thoughts?

@Loirooriol
Copy link
Contributor

But with (2), how would you treat an element with aspect-ratio but stretch alignment in both axes?

CSS Grid makes it clear that then the aspect ratio will probably be distorted, we have interoperability, and being able to opt out the ratio seems useful for authors. So if stretch has precedence over the aspect ratio when specified in both axis, it seems more consistent to me if it also takes precedence when specified in a single axis. That's why I prefer (1).

@bfgeek
Copy link
Author

bfgeek commented Nov 14, 2020

But with (2), how would you treat an element with aspect-ratio but stretch alignment in both axes?

Both "win" it gets distorted.

Here both the inline-axis, and block-axis are stretched. So the default axis "wins" resulting in the image being distorted. The "full" priority list for (2) would be:

  • intrinsic size.
  • transferred (by aspect-ratio) stretch alignment.
  • stretch alignment.
  • transferred (by aspect-ratio) definite size.
  • definite size.

chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Nov 14, 2020
This block-stretching logic as:
https://chromium-review.googlesource.com/c/chromium/src/+/2513771
... did for the inline-axis.

Previously we didn't have the concept of "stretching" the block-axis
instead relying on the parent layout algorithm to set a fixed
block-size.

This allow stretching (similar to the inline-dimension).

The failing tests (which I'll update in the test expectations) are due
to this specification issue:
w3c/csswg-drafts#5713

The changes in layout_replaced.cc are needed to transfer the stretched
size through the aspect-ratio.

Bug: 1045599
Change-Id: Ifdd8653d81a1ead84a0afbf5ebd31ba0b75dc8ee
@Loirooriol
Copy link
Contributor

IMO option (1) looks simpler this way:

  1. alignment:
    • if stretch, or normal behaving as stretch: stretch.
    • otherwise:
      1. intrinsic size.
      2. transferred (by aspect-ratio) stretch alignment.
      3. transferred (by aspect-ratio) definite size.
  2. definite size.

@bfgeek
Copy link
Author

bfgeek commented Nov 14, 2020

I don't think your definition of (1) satisfies this case:

<div style="display: block; width: 200px; border: solid;">
  <div style="height: 100px; aspect-ratio: 1/1;"></div>
</div>

As here justify-self: normal is stretch per implementations (and https://drafts.csswg.org/css-align/#justify-block). Although as discussed this definition is incomplete due to not saying that tables, replaced, and some form controls should be start

@Loirooriol
Copy link
Contributor

But in that case justify-self: normal shouldn't behave as stretch. https://drafts.csswg.org/css-align/#justify-block is wrong, as you say it should exclude replaced elements (and others), and elements with aspect-ratio are sized as replaced, so they should be excluded too. Then the width would be 100px, not stretched, matching Nightly and Canary with aspect-ratio enabled.

@bfgeek
Copy link
Author

bfgeek commented Nov 14, 2020

For more clarity, when a width/height is 'auto' there are (at the moment) only two options, either:
a) stretch
b) fit-content aka. shrink-to-fit

Currently replaced, and non-replaced elements in grid, and block layout behave the same way by default.

@bfgeek
Copy link
Author

bfgeek commented Nov 14, 2020

But in that case justify-self: normal shouldn't behave as stretch.

How does target end up at 200px wide if it isn't stretching?

<div style="display: block; width: 200px; border: solid;">
  <div id="target" style=""></div>
</div>

@bfgeek
Copy link
Author

bfgeek commented Nov 14, 2020

and elements with aspect-ratio are sized as replaced, so they should be excluded too.

Ok so this is where we disagree and is an interesting point, I believe that we've been working under the assumption that applying an aspect-ratio to a non-replaced element, doesn't make it replaced. As this has a different set of sizing constraints that would be bad for most things.

@bfgeek
Copy link
Author

bfgeek commented Nov 14, 2020

E.g. from: https://drafts.csswg.org/css-sizing-4/#aspect-ratio

"Note: Having a preferred aspect ratio does not make a box into a replaced element; layout rules specific to replaced elements do not generally apply to non-replaced boxes with a preferred aspect ratio. "

@Loirooriol
Copy link
Contributor

Yes, in that other case it doesn't have a preferred aspect ratio, it's not replaced, it's not a table, etc. Then normal behaves as stretch indeed.

applying an aspect-ratio to a non-replaced element, doesn't make it replaced

Doesn't make it completely replaced, but https://drafts.csswg.org/css-sizing-4/#aspect-ratio-automatic says

When a box has a preferred aspect ratio, its automatic sizes are calculated the same as for a replaced element with an intrinsic aspect ratio and no intrinsic dimension in that axis

@bfgeek
Copy link
Author

bfgeek commented Nov 14, 2020

So if we change how we interpret the justify-self: normal rule to not stretch if things have an aspect-ratio, this will mean that:

<div style="width: 100px; border: solid;">
  <div style="aspect-ratio: 1/1; background: green;">

...will result in a 0x0 div which I don't think is desired.

@Loirooriol
Copy link
Contributor

I think in that case the stretching is not because of justify-self: normal. It's just that the fit-content size, according to https://drafts.csswg.org/css-sizing/#intrinsic-sizes,

For boxes with a preferred aspect ratio, but no intrinsic size:
If the available space is definite in the inline axis, use the stretch fit into that size for the inline size and calculate the block size using the aspect ratio.

Browsers already do this for

<div style="display: grid; width: 100px; border: solid;">
  <svg viewBox="0 0 1 1" style="background: green; justify-self: start; align-self: start">

which has start alignment instead of normal, this shows the stretching is not due to normal behaving as stretch.

@bfgeek
Copy link
Author

bfgeek commented Nov 15, 2020

My example has an intrinsic size, it's just [0, 0]. SVGs are special as they can have no intrinsic size as you describe.

chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Jan 27, 2021
As discussed in w3c/csswg-drafts#5713

Change-Id: I90c8bd9c5516d11cae6798d7d7cb6c679e166994
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Jan 27, 2021
As discussed in w3c/csswg-drafts#5713

Change-Id: I90c8bd9c5516d11cae6798d7d7cb6c679e166994
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Jan 28, 2021
As discussed in w3c/csswg-drafts#5713

Change-Id: I90c8bd9c5516d11cae6798d7d7cb6c679e166994
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Jan 28, 2021
As discussed in w3c/csswg-drafts#5713

Change-Id: I90c8bd9c5516d11cae6798d7d7cb6c679e166994
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Jan 28, 2021
As discussed in w3c/csswg-drafts#5713

Change-Id: I90c8bd9c5516d11cae6798d7d7cb6c679e166994
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2651651
Commit-Queue: Christian Biesinger <cbiesinger@chromium.org>
Reviewed-by: Oriol Brufau <obrufau@igalia.com>
Reviewed-by: Javier Fernandez <jfernandez@igalia.com>
Reviewed-by: Ian Kilpatrick <ikilpatrick@chromium.org>
Auto-Submit: Christian Biesinger <cbiesinger@chromium.org>
Cr-Commit-Position: refs/heads/master@{#848107}
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Jan 28, 2021
As discussed in w3c/csswg-drafts#5713

Change-Id: I90c8bd9c5516d11cae6798d7d7cb6c679e166994
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2651651
Commit-Queue: Christian Biesinger <cbiesinger@chromium.org>
Reviewed-by: Oriol Brufau <obrufau@igalia.com>
Reviewed-by: Javier Fernandez <jfernandez@igalia.com>
Reviewed-by: Ian Kilpatrick <ikilpatrick@chromium.org>
Auto-Submit: Christian Biesinger <cbiesinger@chromium.org>
Cr-Commit-Position: refs/heads/master@{#848107}
pull bot pushed a commit to Yannic/chromium that referenced this issue Jan 28, 2021
As discussed in w3c/csswg-drafts#5713

Change-Id: I90c8bd9c5516d11cae6798d7d7cb6c679e166994
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2651651
Commit-Queue: Christian Biesinger <cbiesinger@chromium.org>
Reviewed-by: Oriol Brufau <obrufau@igalia.com>
Reviewed-by: Javier Fernandez <jfernandez@igalia.com>
Reviewed-by: Ian Kilpatrick <ikilpatrick@chromium.org>
Auto-Submit: Christian Biesinger <cbiesinger@chromium.org>
Cr-Commit-Position: refs/heads/master@{#848107}
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this issue Feb 2, 2021
…element in both axes, a=testonly

Automatic update from web-platform-tests
[css-grid] Correctly stretch a replaced element in both axes

As discussed in w3c/csswg-drafts#5713

Change-Id: I90c8bd9c5516d11cae6798d7d7cb6c679e166994
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2651651
Commit-Queue: Christian Biesinger <cbiesinger@chromium.org>
Reviewed-by: Oriol Brufau <obrufau@igalia.com>
Reviewed-by: Javier Fernandez <jfernandez@igalia.com>
Reviewed-by: Ian Kilpatrick <ikilpatrick@chromium.org>
Auto-Submit: Christian Biesinger <cbiesinger@chromium.org>
Cr-Commit-Position: refs/heads/master@{#848107}

--

wpt-commits: 199d0dd07e5d5a599da70d19a702943202e3dadf
wpt-pr: 27347
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified-and-comments-removed that referenced this issue Feb 3, 2021
…element in both axes, a=testonly

Automatic update from web-platform-tests
[css-grid] Correctly stretch a replaced element in both axes

As discussed in w3c/csswg-drafts#5713

Change-Id: I90c8bd9c5516d11cae6798d7d7cb6c679e166994
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2651651
Commit-Queue: Christian Biesinger <cbiesingerchromium.org>
Reviewed-by: Oriol Brufau <obrufauigalia.com>
Reviewed-by: Javier Fernandez <jfernandezigalia.com>
Reviewed-by: Ian Kilpatrick <ikilpatrickchromium.org>
Auto-Submit: Christian Biesinger <cbiesingerchromium.org>
Cr-Commit-Position: refs/heads/master{#848107}

--

wpt-commits: 199d0dd07e5d5a599da70d19a702943202e3dadf
wpt-pr: 27347

UltraBlame original commit: 60d4ee523bbdc52e07b2643083a8dd139164a59a
gecko-dev-updater pushed a commit to marco-c/gecko-dev-comments-removed that referenced this issue Feb 3, 2021
…element in both axes, a=testonly

Automatic update from web-platform-tests
[css-grid] Correctly stretch a replaced element in both axes

As discussed in w3c/csswg-drafts#5713

Change-Id: I90c8bd9c5516d11cae6798d7d7cb6c679e166994
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2651651
Commit-Queue: Christian Biesinger <cbiesingerchromium.org>
Reviewed-by: Oriol Brufau <obrufauigalia.com>
Reviewed-by: Javier Fernandez <jfernandezigalia.com>
Reviewed-by: Ian Kilpatrick <ikilpatrickchromium.org>
Auto-Submit: Christian Biesinger <cbiesingerchromium.org>
Cr-Commit-Position: refs/heads/master{#848107}

--

wpt-commits: 199d0dd07e5d5a599da70d19a702943202e3dadf
wpt-pr: 27347

UltraBlame original commit: 60d4ee523bbdc52e07b2643083a8dd139164a59a
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified that referenced this issue Feb 3, 2021
…element in both axes, a=testonly

Automatic update from web-platform-tests
[css-grid] Correctly stretch a replaced element in both axes

As discussed in w3c/csswg-drafts#5713

Change-Id: I90c8bd9c5516d11cae6798d7d7cb6c679e166994
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2651651
Commit-Queue: Christian Biesinger <cbiesingerchromium.org>
Reviewed-by: Oriol Brufau <obrufauigalia.com>
Reviewed-by: Javier Fernandez <jfernandezigalia.com>
Reviewed-by: Ian Kilpatrick <ikilpatrickchromium.org>
Auto-Submit: Christian Biesinger <cbiesingerchromium.org>
Cr-Commit-Position: refs/heads/master{#848107}

--

wpt-commits: 199d0dd07e5d5a599da70d19a702943202e3dadf
wpt-pr: 27347

UltraBlame original commit: 60d4ee523bbdc52e07b2643083a8dd139164a59a
jamienicol pushed a commit to jamienicol/gecko that referenced this issue Feb 4, 2021
…element in both axes, a=testonly

Automatic update from web-platform-tests
[css-grid] Correctly stretch a replaced element in both axes

As discussed in w3c/csswg-drafts#5713

Change-Id: I90c8bd9c5516d11cae6798d7d7cb6c679e166994
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2651651
Commit-Queue: Christian Biesinger <cbiesinger@chromium.org>
Reviewed-by: Oriol Brufau <obrufau@igalia.com>
Reviewed-by: Javier Fernandez <jfernandez@igalia.com>
Reviewed-by: Ian Kilpatrick <ikilpatrick@chromium.org>
Auto-Submit: Christian Biesinger <cbiesinger@chromium.org>
Cr-Commit-Position: refs/heads/master@{#848107}

--

wpt-commits: 199d0dd07e5d5a599da70d19a702943202e3dadf
wpt-pr: 27347
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Jul 2, 2021
Renaming scheme got lost, however basically:

grid-item-no-aspect-ratio-stretch-4.html -> grid-item-aspect-ratio-stretch-1.html
grid-item-no-aspect-ratio-stretch-5.html -> grid-item-aspect-ratio-stretch-2.html
grid-item-no-aspect-ratio-stretch-6.html -> grid-item-aspect-ratio-stretch-3.html
grid-item-no-aspect-ratio-stretch-7.html -> grid-item-aspect-ratio-stretch-4.html

These tests all had a viewBox defining a valid aspect-ratio. Due to:
w3c/csswg-drafts#6286 (comment)
These tests *should* have an aspect-ratio, and when stretched in one
dimension, should reflect to the other dimension (if unconstrained). See:
w3c/csswg-drafts#5713 (comment)

The below two tests basically just got renamed:
grid-item-no-aspect-ratio-stretch-8.html -> grid-item-no-aspect-ratio-stretch-4.html
grid-item-no-aspect-ratio-stretch-9.html -> grid-item-no-aspect-ratio-stretch-5.html
grid-item-no-aspect-ratio-stretch-10.html -> grid-item-no-aspect-ratio-stretch-6.html

But tests updated to correctly assert that the natural size would still
be respected.

To all these test-cases I also added "grid-template: 100% / 100%;" as
there is further complexity when inside an auto row/column which is
tested elsewhere.
(Transferred minimum size for replaced elements with an aspect-ratio).

Bug: 1114013
Change-Id: I062f67e291cc62fa63a53370595780dae16abf3b
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Jul 2, 2021
Renaming scheme got lost, however basically:

grid-item-no-aspect-ratio-stretch-4.html -> grid-item-aspect-ratio-stretch-1.html
grid-item-no-aspect-ratio-stretch-5.html -> grid-item-aspect-ratio-stretch-2.html
grid-item-no-aspect-ratio-stretch-6.html -> grid-item-aspect-ratio-stretch-3.html
grid-item-no-aspect-ratio-stretch-7.html -> grid-item-aspect-ratio-stretch-4.html

These tests all had a viewBox defining a valid aspect-ratio. Due to:
w3c/csswg-drafts#6286 (comment)
These tests *should* have an aspect-ratio, and when stretched in one
dimension, should reflect to the other dimension (if unconstrained). See:
w3c/csswg-drafts#5713 (comment)

The below two tests basically just got renamed:
grid-item-no-aspect-ratio-stretch-8.html -> grid-item-no-aspect-ratio-stretch-4.html
grid-item-no-aspect-ratio-stretch-9.html -> grid-item-no-aspect-ratio-stretch-5.html
grid-item-no-aspect-ratio-stretch-10.html -> grid-item-no-aspect-ratio-stretch-6.html

But tests updated to correctly assert that the natural size would still
be respected.

To all these test-cases I also added "grid-template: 100% / 100%;" as
there is further complexity when inside an auto row/column which is
tested elsewhere.
(Transferred minimum size for replaced elements with an aspect-ratio).

Bug: 1114013
Change-Id: I062f67e291cc62fa63a53370595780dae16abf3b
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Jul 2, 2021
Renaming scheme got lost, however basically:

grid-item-no-aspect-ratio-stretch-4.html -> grid-item-aspect-ratio-stretch-1.html
grid-item-no-aspect-ratio-stretch-5.html -> grid-item-aspect-ratio-stretch-2.html
grid-item-no-aspect-ratio-stretch-6.html -> grid-item-aspect-ratio-stretch-3.html
grid-item-no-aspect-ratio-stretch-7.html -> grid-item-aspect-ratio-stretch-4.html

These tests all had a viewBox defining a valid aspect-ratio. Due to:
w3c/csswg-drafts#6286 (comment)
These tests *should* have an aspect-ratio, and when stretched in one
dimension, should reflect to the other dimension (if unconstrained). See:
w3c/csswg-drafts#5713 (comment)

The below two tests basically just got renamed:
grid-item-no-aspect-ratio-stretch-8.html -> grid-item-no-aspect-ratio-stretch-4.html
grid-item-no-aspect-ratio-stretch-9.html -> grid-item-no-aspect-ratio-stretch-5.html
grid-item-no-aspect-ratio-stretch-10.html -> grid-item-no-aspect-ratio-stretch-6.html

But tests updated to correctly assert that the natural size would still
be respected.

To all these test-cases I also added "grid-template: 100% / 100%;" as
there is further complexity when inside an auto row/column which is
tested elsewhere.
(Transferred minimum size for replaced elements with an aspect-ratio).

Bug: 1114013
Change-Id: I062f67e291cc62fa63a53370595780dae16abf3b
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3003564
Reviewed-by: David Grogan <dgrogan@chromium.org>
Reviewed-by: Kurt Catti-Schmidt <kschmi@microsoft.com>
Commit-Queue: Ian Kilpatrick <ikilpatrick@chromium.org>
Cr-Commit-Position: refs/heads/master@{#898351}
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Jul 2, 2021
Renaming scheme got lost, however basically:

grid-item-no-aspect-ratio-stretch-4.html -> grid-item-aspect-ratio-stretch-1.html
grid-item-no-aspect-ratio-stretch-5.html -> grid-item-aspect-ratio-stretch-2.html
grid-item-no-aspect-ratio-stretch-6.html -> grid-item-aspect-ratio-stretch-3.html
grid-item-no-aspect-ratio-stretch-7.html -> grid-item-aspect-ratio-stretch-4.html

These tests all had a viewBox defining a valid aspect-ratio. Due to:
w3c/csswg-drafts#6286 (comment)
These tests *should* have an aspect-ratio, and when stretched in one
dimension, should reflect to the other dimension (if unconstrained). See:
w3c/csswg-drafts#5713 (comment)

The below two tests basically just got renamed:
grid-item-no-aspect-ratio-stretch-8.html -> grid-item-no-aspect-ratio-stretch-4.html
grid-item-no-aspect-ratio-stretch-9.html -> grid-item-no-aspect-ratio-stretch-5.html
grid-item-no-aspect-ratio-stretch-10.html -> grid-item-no-aspect-ratio-stretch-6.html

But tests updated to correctly assert that the natural size would still
be respected.

To all these test-cases I also added "grid-template: 100% / 100%;" as
there is further complexity when inside an auto row/column which is
tested elsewhere.
(Transferred minimum size for replaced elements with an aspect-ratio).

Bug: 1114013
Change-Id: I062f67e291cc62fa63a53370595780dae16abf3b
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3003564
Reviewed-by: David Grogan <dgrogan@chromium.org>
Reviewed-by: Kurt Catti-Schmidt <kschmi@microsoft.com>
Commit-Queue: Ian Kilpatrick <ikilpatrick@chromium.org>
Cr-Commit-Position: refs/heads/master@{#898351}
blueboxd pushed a commit to blueboxd/chromium-legacy that referenced this issue Jul 3, 2021
Renaming scheme got lost, however basically:

grid-item-no-aspect-ratio-stretch-4.html -> grid-item-aspect-ratio-stretch-1.html
grid-item-no-aspect-ratio-stretch-5.html -> grid-item-aspect-ratio-stretch-2.html
grid-item-no-aspect-ratio-stretch-6.html -> grid-item-aspect-ratio-stretch-3.html
grid-item-no-aspect-ratio-stretch-7.html -> grid-item-aspect-ratio-stretch-4.html

These tests all had a viewBox defining a valid aspect-ratio. Due to:
w3c/csswg-drafts#6286 (comment)
These tests *should* have an aspect-ratio, and when stretched in one
dimension, should reflect to the other dimension (if unconstrained). See:
w3c/csswg-drafts#5713 (comment)

The below two tests basically just got renamed:
grid-item-no-aspect-ratio-stretch-8.html -> grid-item-no-aspect-ratio-stretch-4.html
grid-item-no-aspect-ratio-stretch-9.html -> grid-item-no-aspect-ratio-stretch-5.html
grid-item-no-aspect-ratio-stretch-10.html -> grid-item-no-aspect-ratio-stretch-6.html


But tests updated to correctly assert that the natural size would still
be respected.

To all these test-cases I also added "grid-template: 100% / 100%;" as
there is further complexity when inside an auto row/column which is
tested elsewhere.
(Transferred minimum size for replaced elements with an aspect-ratio).

Bug: 1114013
Change-Id: I062f67e291cc62fa63a53370595780dae16abf3b
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3003564
Reviewed-by: David Grogan <dgrogan@chromium.org>
Reviewed-by: Kurt Catti-Schmidt <kschmi@microsoft.com>
Commit-Queue: Ian Kilpatrick <ikilpatrick@chromium.org>
Cr-Commit-Position: refs/heads/master@{#898351}
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this issue Jul 3, 2021
…sts., a=testonly

Automatic update from web-platform-tests
[wpt] Fix grid-item (no) aspect-ratio tests.

Renaming scheme got lost, however basically:

grid-item-no-aspect-ratio-stretch-4.html -> grid-item-aspect-ratio-stretch-1.html
grid-item-no-aspect-ratio-stretch-5.html -> grid-item-aspect-ratio-stretch-2.html
grid-item-no-aspect-ratio-stretch-6.html -> grid-item-aspect-ratio-stretch-3.html
grid-item-no-aspect-ratio-stretch-7.html -> grid-item-aspect-ratio-stretch-4.html

These tests all had a viewBox defining a valid aspect-ratio. Due to:
w3c/csswg-drafts#6286 (comment)
These tests *should* have an aspect-ratio, and when stretched in one
dimension, should reflect to the other dimension (if unconstrained). See:
w3c/csswg-drafts#5713 (comment)

The below two tests basically just got renamed:
grid-item-no-aspect-ratio-stretch-8.html -> grid-item-no-aspect-ratio-stretch-4.html
grid-item-no-aspect-ratio-stretch-9.html -> grid-item-no-aspect-ratio-stretch-5.html
grid-item-no-aspect-ratio-stretch-10.html -> grid-item-no-aspect-ratio-stretch-6.html

But tests updated to correctly assert that the natural size would still
be respected.

To all these test-cases I also added "grid-template: 100% / 100%;" as
there is further complexity when inside an auto row/column which is
tested elsewhere.
(Transferred minimum size for replaced elements with an aspect-ratio).

Bug: 1114013
Change-Id: I062f67e291cc62fa63a53370595780dae16abf3b
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3003564
Reviewed-by: David Grogan <dgrogan@chromium.org>
Reviewed-by: Kurt Catti-Schmidt <kschmi@microsoft.com>
Commit-Queue: Ian Kilpatrick <ikilpatrick@chromium.org>
Cr-Commit-Position: refs/heads/master@{#898351}

--

wpt-commits: ec4e2eecc6a5bd2f21c81dbe03862c2cdf17cf8c
wpt-pr: 29567
jamienicol pushed a commit to jamienicol/gecko that referenced this issue Jul 16, 2021
…sts., a=testonly

Automatic update from web-platform-tests
[wpt] Fix grid-item (no) aspect-ratio tests.

Renaming scheme got lost, however basically:

grid-item-no-aspect-ratio-stretch-4.html -> grid-item-aspect-ratio-stretch-1.html
grid-item-no-aspect-ratio-stretch-5.html -> grid-item-aspect-ratio-stretch-2.html
grid-item-no-aspect-ratio-stretch-6.html -> grid-item-aspect-ratio-stretch-3.html
grid-item-no-aspect-ratio-stretch-7.html -> grid-item-aspect-ratio-stretch-4.html

These tests all had a viewBox defining a valid aspect-ratio. Due to:
w3c/csswg-drafts#6286 (comment)
These tests *should* have an aspect-ratio, and when stretched in one
dimension, should reflect to the other dimension (if unconstrained). See:
w3c/csswg-drafts#5713 (comment)

The below two tests basically just got renamed:
grid-item-no-aspect-ratio-stretch-8.html -> grid-item-no-aspect-ratio-stretch-4.html
grid-item-no-aspect-ratio-stretch-9.html -> grid-item-no-aspect-ratio-stretch-5.html
grid-item-no-aspect-ratio-stretch-10.html -> grid-item-no-aspect-ratio-stretch-6.html

But tests updated to correctly assert that the natural size would still
be respected.

To all these test-cases I also added "grid-template: 100% / 100%;" as
there is further complexity when inside an auto row/column which is
tested elsewhere.
(Transferred minimum size for replaced elements with an aspect-ratio).

Bug: 1114013
Change-Id: I062f67e291cc62fa63a53370595780dae16abf3b
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3003564
Reviewed-by: David Grogan <dgrogan@chromium.org>
Reviewed-by: Kurt Catti-Schmidt <kschmi@microsoft.com>
Commit-Queue: Ian Kilpatrick <ikilpatrick@chromium.org>
Cr-Commit-Position: refs/heads/master@{#898351}

--

wpt-commits: ec4e2eecc6a5bd2f21c81dbe03862c2cdf17cf8c
wpt-pr: 29567
webkit-commit-queue pushed a commit to WebKit/WebKit that referenced this issue Jul 19, 2021
…lignments are applied to both axes

https://bugs.webkit.org/show_bug.cgi?id=227573

Reviewed by Javier Fernandez.

Source/WebCore:

As discussed in w3c/csswg-drafts#5713, for the replaced element as a grid item,
when both axes have stretch alignments applied and there is no auto margin(s) presented, the aspect ratio
should be ignored if there is any.

Part of this patch is an import of Chromium CL at
https://chromium-review.googlesource.com/c/chromium/src/+/2651651

* rendering/RenderBox.cpp:
(WebCore::RenderBox::hasStretchedLogicalHeight const):
(WebCore::RenderBox::shouldComputeLogicalWidthFromAspectRatio const):
* rendering/RenderBox.h:

LayoutTests:

Two grid WPT tests are now passing.

* TestExpectations:


Canonical link: https://commits.webkit.org/239764@main
git-svn-id: https://svn.webkit.org/repository/webkit/trunk@280022 268f45cc-cd09-0410-ab3c-d52691b4dbfc
webkit-commit-queue pushed a commit to WebKit/WebKit that referenced this issue Jul 19, 2021
…t when defined to compute the logical width

https://bugs.webkit.org/show_bug.cgi?id=227984

Reviewed by Javier Fernandez.
LayoutTests/imported/w3c:

* web-platform-tests/css/css-grid/alignment/grid-alignment-implies-size-change-011-expected.txt:
* web-platform-tests/css/css-grid/alignment/grid-alignment-implies-size-change-012-expected.txt:
* web-platform-tests/css/css-grid/alignment/grid-alignment-implies-size-change-013-expected.txt:
* web-platform-tests/css/css-grid/alignment/grid-alignment-implies-size-change-014-expected.txt:
* web-platform-tests/css/css-grid/alignment/grid-alignment-implies-size-change-017-expected.txt:
* web-platform-tests/css/css-grid/alignment/grid-alignment-implies-size-change-018-expected.txt:

Source/WebCore:

As discussed in w3c/csswg-drafts#5713, for images as grid items, when
stretch alignment is only applied in one axis we should respect aspect-ratio on the other.

When computing the logical width using an intrinsic aspect ratio, RenderReplaced should use the
overridingLogicalHeight whenever defined just as how it does for flex items. This change is to
replace the use of intrinsic (non-stretched) logical height in current code with the overridingLogicalHeight.

This allows us to pass an additional of 9 grid WPT tests.

* rendering/RenderReplaced.cpp:
(WebCore::RenderReplaced::computeReplacedLogicalWidth const):

LayoutTests:

9 grid WPT tests are now passing.

* TestExpectations:


Canonical link: https://commits.webkit.org/239766@main
git-svn-id: https://svn.webkit.org/repository/webkit/trunk@280024 268f45cc-cd09-0410-ab3c-d52691b4dbfc
philn pushed a commit to philn/old-webkit that referenced this issue Jul 21, 2021
…lignments are applied to both axes

https://bugs.webkit.org/show_bug.cgi?id=227573

Reviewed by Javier Fernandez.

Source/WebCore:

As discussed in w3c/csswg-drafts#5713, for the replaced element as a grid item,
when both axes have stretch alignments applied and there is no auto margin(s) presented, the aspect ratio
should be ignored if there is any.

Part of this patch is an import of Chromium CL at
https://chromium-review.googlesource.com/c/chromium/src/+/2651651

* rendering/RenderBox.cpp:
(WebCore::RenderBox::hasStretchedLogicalHeight const):
(WebCore::RenderBox::shouldComputeLogicalWidthFromAspectRatio const):
* rendering/RenderBox.h:

LayoutTests:

Two grid WPT tests are now passing.

* TestExpectations:


git-svn-id: http://svn.webkit.org/repository/webkit/trunk@280022 268f45cc-cd09-0410-ab3c-d52691b4dbfc
philn pushed a commit to philn/old-webkit that referenced this issue Jul 21, 2021
…t when defined to compute the logical width

https://bugs.webkit.org/show_bug.cgi?id=227984

Reviewed by Javier Fernandez.
LayoutTests/imported/w3c:

* web-platform-tests/css/css-grid/alignment/grid-alignment-implies-size-change-011-expected.txt:
* web-platform-tests/css/css-grid/alignment/grid-alignment-implies-size-change-012-expected.txt:
* web-platform-tests/css/css-grid/alignment/grid-alignment-implies-size-change-013-expected.txt:
* web-platform-tests/css/css-grid/alignment/grid-alignment-implies-size-change-014-expected.txt:
* web-platform-tests/css/css-grid/alignment/grid-alignment-implies-size-change-017-expected.txt:
* web-platform-tests/css/css-grid/alignment/grid-alignment-implies-size-change-018-expected.txt:

Source/WebCore:

As discussed in w3c/csswg-drafts#5713, for images as grid items, when
stretch alignment is only applied in one axis we should respect aspect-ratio on the other.

When computing the logical width using an intrinsic aspect ratio, RenderReplaced should use the
overridingLogicalHeight whenever defined just as how it does for flex items. This change is to
replace the use of intrinsic (non-stretched) logical height in current code with the overridingLogicalHeight.

This allows us to pass an additional of 9 grid WPT tests.

* rendering/RenderReplaced.cpp:
(WebCore::RenderReplaced::computeReplacedLogicalWidth const):

LayoutTests:

9 grid WPT tests are now passing.

* TestExpectations:


git-svn-id: http://svn.webkit.org/repository/webkit/trunk@280024 268f45cc-cd09-0410-ab3c-d52691b4dbfc
mjfroman pushed a commit to mjfroman/moz-libwebrtc-third-party that referenced this issue Oct 14, 2022
This block-stretching logic as:
https://chromium-review.googlesource.com/c/chromium/src/+/2513771
... did for the inline-axis.

Previously we didn't have the concept of "stretching" the block-axis
instead relying on the parent layout algorithm to set a fixed
block-size.

This allow stretching (similar to the inline-dimension).

The failing tests (which I'll update in the test expectations) are due
to this specification issue:
w3c/csswg-drafts#5713

The changes in layout_replaced.cc are needed to transfer the stretched
size through the aspect-ratio.

Bug: 1045599
Change-Id: Ifdd8653d81a1ead84a0afbf5ebd31ba0b75dc8ee
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2523743
Commit-Queue: Ian Kilpatrick <ikilpatrick@chromium.org>
Reviewed-by: Kurt Catti-Schmidt <kschmi@microsoft.com>
Reviewed-by: David Grogan <dgrogan@chromium.org>
Reviewed-by: Morten Stenshorne <mstensho@chromium.org>
Cr-Commit-Position: refs/heads/master@{#827854}
GitOrigin-RevId: eaff997ccac677831d0a3c72e638ddf6528276d7
mjfroman pushed a commit to mjfroman/moz-libwebrtc-third-party that referenced this issue Oct 14, 2022
From: w3c/csswg-drafts#5713

Basically these tests stretch an image in one axis, and previously
tested that the image should lose its aspect-ratio.
When stretching in one axis, and when there is no size constraint in the
other axis it should respect the aspect-ratio.

Change-Id: Ic1374f50b4d561b3c38b94a4f60724f950e6513e
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2611567
Reviewed-by: Christian Biesinger <cbiesinger@chromium.org>
Commit-Queue: Ian Kilpatrick <ikilpatrick@chromium.org>
Cr-Commit-Position: refs/heads/master@{#841117}
GitOrigin-RevId: 0eb684e1e380c65380150f4ba2de14cc0bbef5ac
mjfroman pushed a commit to mjfroman/moz-libwebrtc-third-party that referenced this issue Oct 14, 2022
As discussed in w3c/csswg-drafts#5713

Change-Id: I90c8bd9c5516d11cae6798d7d7cb6c679e166994
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2651651
Commit-Queue: Christian Biesinger <cbiesinger@chromium.org>
Reviewed-by: Oriol Brufau <obrufau@igalia.com>
Reviewed-by: Javier Fernandez <jfernandez@igalia.com>
Reviewed-by: Ian Kilpatrick <ikilpatrick@chromium.org>
Auto-Submit: Christian Biesinger <cbiesinger@chromium.org>
Cr-Commit-Position: refs/heads/master@{#848107}
GitOrigin-RevId: 2ba07b2385593e296bb69271b4c27402a6d929c3
mjfroman pushed a commit to mjfroman/moz-libwebrtc-third-party that referenced this issue Oct 14, 2022
Renaming scheme got lost, however basically:

grid-item-no-aspect-ratio-stretch-4.html -> grid-item-aspect-ratio-stretch-1.html
grid-item-no-aspect-ratio-stretch-5.html -> grid-item-aspect-ratio-stretch-2.html
grid-item-no-aspect-ratio-stretch-6.html -> grid-item-aspect-ratio-stretch-3.html
grid-item-no-aspect-ratio-stretch-7.html -> grid-item-aspect-ratio-stretch-4.html

These tests all had a viewBox defining a valid aspect-ratio. Due to:
w3c/csswg-drafts#6286 (comment)
These tests *should* have an aspect-ratio, and when stretched in one
dimension, should reflect to the other dimension (if unconstrained). See:
w3c/csswg-drafts#5713 (comment)

The below two tests basically just got renamed:
grid-item-no-aspect-ratio-stretch-8.html -> grid-item-no-aspect-ratio-stretch-4.html
grid-item-no-aspect-ratio-stretch-9.html -> grid-item-no-aspect-ratio-stretch-5.html
grid-item-no-aspect-ratio-stretch-10.html -> grid-item-no-aspect-ratio-stretch-6.html

But tests updated to correctly assert that the natural size would still
be respected.

To all these test-cases I also added "grid-template: 100% / 100%;" as
there is further complexity when inside an auto row/column which is
tested elsewhere.
(Transferred minimum size for replaced elements with an aspect-ratio).

Bug: 1114013
Change-Id: I062f67e291cc62fa63a53370595780dae16abf3b
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3003564
Reviewed-by: David Grogan <dgrogan@chromium.org>
Reviewed-by: Kurt Catti-Schmidt <kschmi@microsoft.com>
Commit-Queue: Ian Kilpatrick <ikilpatrick@chromium.org>
Cr-Commit-Position: refs/heads/master@{#898351}
NOKEYCHECK=True
GitOrigin-RevId: fa1b4b99cae7291ebb66327c43bdebfaad763044
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants