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-images] Behaviour of SVG degenerate aspect-ratios #6286

Open
bfgeek opened this issue May 12, 2021 · 7 comments
Open

[css-images] Behaviour of SVG degenerate aspect-ratios #6286

bfgeek opened this issue May 12, 2021 · 7 comments

Comments

@bfgeek
Copy link

bfgeek commented May 12, 2021

This issue is mainly just so that we can discuss this SVGWG issue:
w3c/svgwg#848

There are a few tests in the css-grid testsuite asserting behaviour.

@bfgeek bfgeek added the Agenda+ label May 12, 2021
@css-meeting-bot
Copy link
Member

The CSS Working Group just discussed [css-images] Behaviour of SVG degenerate aspect-ratios.

The full IRC log of that discussion <dael> Topic: [css-images] Behaviour of SVG degenerate aspect-ratios
<lea> I will reiterate that using CSSColorValue as input is not something that needs to be explicitly added to a spec, as long as it serializes
<dael> github: https://github.com//issues/6286
<dael> iank_: Digging into replaced elements. Series of test cases in grid repo that is probing this and there's a subtle difference in impl
<dael> iank_: If you go to SVG issue it contins example in 1st comment. When embed SVG in image need to know svg intrinsic size and aspect-ratio
<dael> iank_: svg are unique in you can have one or the other or both. FF has intrinsic but no a-r.
<dael> iank_: Blink and Gecko has both
<dael> iank_: Specific exampl,e have width 0px, height 50px. Viewbox says a-r is 1-1
<dael> iank_: FF have intrinsic 0x50 which matches. a-r is different
<dael> iank_: FF does one interpretation where treats a-r as degenerate and null. Blink and WK go this is degenerate so fallback to viewbox
<TabAtkins> example: <img width=100px src="<svg viewBox='0 0 1 1' width='0' height='50px'>">
<dael> iank_: That's the crux of the issue
<TabAtkins> what is the <img>'s height?
<astearns> ack fantasai
<dael> fantasai: If in general case have width of 1px rather than 0 we will ignore viewbox and use a-r from width and height?
<dael> iank_: Correct
<dael> fantasai: b/c it's degenerate blink fallsback to viewbox
<dael> iank_: Correct. Written down in the css spec. Linked to SVG text but it's same text.
<fantasai> s/fallsback to viewbox/falls back to viewbox, and gecko continues to ignore viewbox/
<dael> iank_: I don't think when algo was written it considered that width and height could result in degenerate a-r
<dael> fantasai: Yeah, I don't htink we considered this
<dael> iank_: You can also spec width -50px and get same behavior
<dael> fantasai: I have no opinion of what we out to do here. Don't think it matters for authoring. Would be interested to know what Amelia thinks
<TabAtkins> I too have no real opinion on how we resolve this. Both behaviors seem reasonable.
<dael> iank_: Yeah, tagged Amelia on the SVG but haven't received a response
<dael> fantasai: Is there a reason to do one or the other?
<dael> iank_: Not particularly. Could argue blink/WK is slightly better in that we use an a-r when it's available. But really I don't expect authors to write this. Only reason I'm bringing it up is there's 6 test cases asserting a behavior, I think written by a Gecko engineer. Not clear those tests are right
<dael> fantasai: Yeah, should clarify
<dael> fantasai: As long as we can't think of a reason my inclination is let Amelia read and make a decision for us
<dael> fantasai: I can ping her
<dael> iank_: Anyone else with thoughts?
<dael> astearns: This is just what to do in this edge case and theres no real world changes we can think of that would result from this?
<dael> iank_: Correct. Not addressing b/c a bug. Going through grid test suite and noticed this edge case was unclear
<dael> astearns: Happy going with Amelia but also happy going with 2/3 of engines went this way so let's spec
<dael> astearns: Leave until Amelia comes?
<dael> fantasai: Or resolve to do whatever she says
<dael> astearns: Objects?
<dael> chrishtr: Question
<dael> chrishtr: Did we discuss if degenerate cases in non-svg handled same?
<dael> fantasai: 2 sourses of info for a-r. most image formats don't have that confusion. We have general degenerate case, but here is when hit in more primary source of information. Do we fall back to secondary?
<dael> iank_: Some precedent in a-r where if you specific degenerate it falls back to images a-r I believe
<dael> iank_: Is that right?
<dael> fantasai: Don't remember off top of head but I think we decided that to match SVG
<fantasai> "If the <ratio> is degenerate, the property instead behaves as auto."
<dael> iank_: With that small amount of precedent then Blink/WK behavior you can tie together. a-r behavior is if it's degenerate it falls back to next best thing
<dael> astearns: Did that answer question?
<dael> chrishtr: What I heard is it's much more uncommon because images are unlikely to have 0 height
<dael> fantasai: Not a real world case
<iank_> https://www.software.hixie.ch/utilities/js/live-dom-viewer/?saved=9292
<dael> chrishtr: b/c SVG is replaced leemnt it behaves different
<dael> iank_: Example ^ showing fallback for a-r property. I spec degenerate where it will use the images a-r
<dael> iank_: That's the fallback. If we want a-r to have this fallback being more consistent then blink/wk fallback makes more sense
<cbiesinger> I mean, for a while a-r: 1/0 was a parse error
<dael> astearns: Given that bit of consistency shall we resolve on Blink/WK and see if Amelia objects?
<cbiesinger> but I think that was changed for consistency with calc() that computes to zero
<fantasai> and then changed to 'auto' because that's what SVG does, IIRC ...
<dael> dholbert: I weakly lean against that. Feel like with a-r property if you're explicitly providing invalid it makes sense to not do anything. In SVG we've got a usable height and width and it makes sense that does establish a value to use
<dael> iank_: You can also spec -10px width and has same behavior. And you don't render at -10
<dael> dholbert: True. More that a-r is downstream. Don't feel too strongly. Feels a little weird adding a special fallback that no one will need for real content. I think it's a trivial change
<dael> fantasai: I think we should wait and hear back from Amelia. She might have SVG logic that leans one way

@astearns astearns removed the Agenda+ label May 19, 2021
@astearns
Copy link
Member

@AmeliaBR you were tagged in the SVG issue. We are waiting for your input on how to handle this case.

@bfgeek
Copy link
Author

bfgeek commented Jun 15, 2021

Ping @AmeliaBR @fantasai

@AmeliaBR
Copy link
Contributor

AmeliaBR commented Jun 16, 2021 via email

@bfgeek bfgeek added the Agenda+ label Jun 16, 2021
@bfgeek
Copy link
Author

bfgeek commented Jun 16, 2021

looking at both SVG as image and element

For this specific case its slightly important that we use an "SVG as an image", as a "width: 0" has meaning within CSS, which makes us "ignore" the aspect-ratio (consistently between implementations).

However we can look at the behaviour of "width: -1px" which does have significant implementation differences, consider:

<!DOCTYPE html>
<style>
img, svg {
  border: solid;
  background: lime;
}
</style>

<img style="width: 100px" src="
    data:image/svg+xml,<svg xmlns='http://www.w3.org/2000/svg' width='-1px' height='50px' viewBox='0 0 1 1'></svg>">

<svg style="" xmlns="http://www.w3.org/2000/svg" width="-1px" height="50px" viewBox="0 0 1 1"></svg>

<svg style="width: -1px; height: 50px;" xmlns="http://www.w3.org/2000/svg" viewBox="0 0 1 1"></svg>

OR

<!DOCTYPE html>
<style>
img, svg {
  border: solid;
  background: lime;
}
</style>

<img style="height: 100px" src="
    data:image/svg+xml,<svg xmlns='http://www.w3.org/2000/svg' width='-1px' height='50px' viewBox='0 0 1 1'></svg>">

<svg style="" xmlns="http://www.w3.org/2000/svg" width="-1px" height="50px" viewBox="0 0 1 1"></svg>

<svg style="width: -1px; height: 50px;" xmlns="http://www.w3.org/2000/svg" viewBox="0 0 1 1"></svg>

Here Blink's behaviour (post M93 - we've fixed lots of bugs in this area) and Safari is "treat -1px as invalid, and fallback to the viewBox aspect-ratio".
Gecko's behaviour is "treat -1px as 0px when defined as an SVG attribute however in CSS ignore -1px and fallback to the aspect-ratio".

I somewhat prefer Blink/Safari behaviour of consistently treating "-1px as invalid here".

This however is only about a "clearly invalid" value of -1px. 0px is valid for intrinsic sizing purposes and the question if it should fallback to the viewBox aspect-ratio still remains.

<!DOCTYPE html>
<style>
img, svg {
  border: solid;
  background: lime;
}
</style>

<img style="width: 100px" src="
    data:image/svg+xml,<svg xmlns='http://www.w3.org/2000/svg' width='0' height='50px' viewBox='0 0 1 1'></svg>">

<svg style="" xmlns="http://www.w3.org/2000/svg" width="0" height="50px" viewBox="0 0 1 1"></svg>

<svg style="width: ; height: 50px;" xmlns="http://www.w3.org/2000/svg" viewBox="0 0 1 1"></svg>

Here the intrinsic-size is clearly 0x50. But does the aspect-ratio fallback to the viewBox to be 1/1 or not?
This somewhat depends on how "-1px" is handled.

@AmeliaBR
Copy link
Contributor

AmeliaBR commented Jun 16, 2021 via email

@css-meeting-bot
Copy link
Member

The CSS Working Group just discussed SVG degenerate aspect ratios, and agreed to the following:

  • RESOLVED: degenerate aspect ratios derived from SVG width/height attributes fall back to viewbox aspect ratio (whether due to negative values or zero values)
The full IRC log of that discussion <fantasai> Topic: SVG degenerate aspect ratios
<fantasai> github: https://github.com//issues/6286
<fantasai> iank_: We were going to ask Amelia, but she had to defer
<fantasai> iank_: So it's up to us
<fantasai> iank_: I posted a comment describing the various behaviors
<fantasai> iank_: I still slightly prefer the WebKit/Blink behavior
<fantasai> iank_: ...
<fantasai> iank_: Reminder that this is just about interop on an edge case for which we have testcases in WPT, not something that impacts web devs
<fantasai> Rossen: can you summarize?
<fantasai> iank_: With negative widths, Blink/WebKit will treat as invalid whereas Firefox will treat as zero
<fantasai> iank_: Blink/WebKit will use the viewbox aspect ratio as a fallback
<fantasai> iank_: whereas Firefox will have no aspect ratio
<fantasai> iank_: If there is a negative width/height specified, it gets clamped to zero and then continue
<fantasai> iank_: Firefox's behavior from there was to treat aspect-ratio as null
<fantasai> iank_: Whereas when we have a negative width/height, we treat as invalid
<fantasai> iank_: So we use fallback aspect ratio
<fantasai> iank_: Separate issue of when we have explicit width of zero and height of 50
<fantasai> iank_: Blink/WebKit will fallback to viewbox of 1:1
<fantasai> iank_: and Firefox will behave as if there is no aspect ratio
<fantasai> Note: spec currently says “If the <ratio> is degenerate, the property instead behaves as auto.”
<fantasai> Rossen: so your preference is to fall back from degenerate width/height on SVG to the viewbox aspect ratio
<fantasai> iank_: Right. This mirrors how the aspect-ratio property itself falls back from degenerate ratio value to intrinsic ratio of image
<fantasai> dholbert: Are you proposing just for negative case, or also for zero case?
<emeyer> fantasai: So the proposal is to fall back to the intrinsic aspect ratio
<emeyer> fantasi: In all degenerate cases
<fantasai> s/intrinsic/viewbox/
<fantasai> dholbert: OK, would not object to that
<emeyer> s/fantasi/fantasai/
<dholbert> iank_, got it, thanks
<fantasai> fantasai: So the proposal is that in the same way that the aspect-ratio property's explicit degenerate aspect ratios fall back to the intrinsic aspect ratio of the image, the proposal is that a degenerate aspect ratio from an SVG derived from its width/height attributes also falls back: to its viewbox ratio
<fantasai> RESOLVED: degenerate aspect ratios derived from SVG width/height attributes fall back to viewbox aspect ratio (whether due to negative values or zero values)

moz-wptsync-bot pushed a commit to web-platform-tests/wpt that referenced this issue Jun 24, 2021
…ze calculation of SVG outer frames.

Based on w3c/csswg-drafts#6286, we fall back to the
viewport rect if
1. width/height is a degenerate ratio, or
2. width or height is a negative number.

Differential Revision: https://phabricator.services.mozilla.com/D118666

bugzilla-url: https://bugzilla.mozilla.org/show_bug.cgi?id=1717877
gecko-commit: bed7b4b9ff74229a6be65b2fd069f59162231583
gecko-reviewers: emilio
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this issue Jun 25, 2021
…r intrinsic size calculation of SVG outer frames. r=emilio

Based on w3c/csswg-drafts#6286, we fall back to the
viewport rect if
1. width/height is a degenerate ratio, or
2. width or height is a negative number.

Differential Revision: https://phabricator.services.mozilla.com/D118666
moz-wptsync-bot pushed a commit to web-platform-tests/wpt that referenced this issue Jun 25, 2021
…ze calculation of SVG outer frames.

Based on w3c/csswg-drafts#6286, we fall back to the
viewport rect if
1. width/height is a degenerate ratio, or
2. width or height is a negative number.

Differential Revision: https://phabricator.services.mozilla.com/D118666

bugzilla-url: https://bugzilla.mozilla.org/show_bug.cgi?id=1717877
gecko-commit: bed7b4b9ff74229a6be65b2fd069f59162231583
gecko-reviewers: emilio
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
…r intrinsic size calculation of SVG outer frames. r=emilio

Based on w3c/csswg-drafts#6286, we fall back to the
viewport rect if
1. width/height is a degenerate ratio, or
2. width or height is a negative number.

Differential Revision: https://phabricator.services.mozilla.com/D118666
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 26, 2021
…width/height when defined to compute the logical height/width

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

Source/WebCore:

As discussed at w3c/csswg-drafts#6286 (comment), degenerate
aspect ratios derived from SVG width/height attributes fall back to viewbox aspect ratio
(whether due to negative values or zero values).

When computing the logical height/width using an intrinsic aspect ratio, RenderReplaced uses the
overridingLogicalWidth/overridingLogicalHeight whenever defined as long as the flex or
grid item has an intrinsic size. For an SVG graphic though, it's common to have an intrinsic aspect
ratio but not to have an intrinsic width or height. For this special case, we still should use
overridingLogicalWidth/overridingLogicalHeight for logical height/width calculations.

Reviewed by Javier Fernandez.

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

LayoutTests:

Reviewed by Javier Fernandez.

Update TestExpectations as 4 failed grid WPT tests are now passing.

* TestExpectations:


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

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

Source/WebCore:

As discussed at w3c/csswg-drafts#6286 (comment), degenerate
aspect ratios derived from SVG width/height attributes fall back to viewbox aspect ratio
(whether due to negative values or zero values).

When computing the logical height/width using an intrinsic aspect ratio, RenderReplaced uses the
overridingLogicalWidth/overridingLogicalHeight whenever defined as long as the flex or
grid item has an intrinsic size. For an SVG graphic though, it's common to have an intrinsic aspect
ratio but not to have an intrinsic width or height. For this special case, we still should use
overridingLogicalWidth/overridingLogicalHeight for logical height/width calculations.

Reviewed by Javier Fernandez.

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

LayoutTests:

Reviewed by Javier Fernandez.

Update TestExpectations as 4 failed grid WPT tests are now passing.

* TestExpectations:


Canonical link: https://commits.webkit.org/239948@main
git-svn-id: https://svn.webkit.org/repository/webkit/trunk@280290 268f45cc-cd09-0410-ab3c-d52691b4dbfc
philn pushed a commit to philn/old-webkit that referenced this issue Aug 1, 2021
…width/height when defined to compute the logical height/width

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

Source/WebCore:

As discussed at w3c/csswg-drafts#6286 (comment), degenerate
aspect ratios derived from SVG width/height attributes fall back to viewbox aspect ratio
(whether due to negative values or zero values).

When computing the logical height/width using an intrinsic aspect ratio, RenderReplaced uses the
overridingLogicalWidth/overridingLogicalHeight whenever defined as long as the flex or
grid item has an intrinsic size. For an SVG graphic though, it's common to have an intrinsic aspect
ratio but not to have an intrinsic width or height. For this special case, we still should use
overridingLogicalWidth/overridingLogicalHeight for logical height/width calculations.

Reviewed by Javier Fernandez.

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

LayoutTests:

Reviewed by Javier Fernandez.

Update TestExpectations as 4 failed grid WPT tests are now passing.

* TestExpectations:


git-svn-id: http://svn.webkit.org/repository/webkit/trunk@280290 268f45cc-cd09-0410-ab3c-d52691b4dbfc
webkit-commit-queue pushed a commit to WebKit/WebKit that referenced this issue Nov 21, 2021
…width/height when defined to compute the logical height/width

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

Reviewed by Javier Fernandez.

Source/WebCore:

This is a reland of r280290. It's got reverted because another patch r280078 caused crash and had been
reverted. r280290 is a dependent patch for r280078.

As discussed at w3c/csswg-drafts#6286 (comment), degenerate
aspect ratios derived from SVG width/height attributes fall back to viewbox aspect ratio
(whether due to negative values or zero values).

When computing the logical height/width using an intrinsic aspect ratio, RenderReplaced uses the
overridingLogicalWidth/overridingLogicalHeight whenever defined as long as the flex or
grid item has an intrinsic size. For an SVG graphic though, it's common to have an intrinsic aspect
ratio but not to have an intrinsic width or height. For this special case, we still should use
overridingLogicalWidth/overridingLogicalHeight for logical height/width calculations.

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

LayoutTests:

* TestExpectations: unskip two tests that are now passing.



Canonical link: https://commits.webkit.org/244487@main
git-svn-id: https://svn.webkit.org/repository/webkit/trunk@286100 268f45cc-cd09-0410-ab3c-d52691b4dbfc
annulen pushed a commit to qtwebkit/qtwebkit that referenced this issue Nov 22, 2021
…width/height when defined to compute the logical height/width

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

Reviewed by Javier Fernandez.

Source/WebCore:

This is a reland of r280290. It's got reverted because another patch r280078 caused crash and had been
reverted. r280290 is a dependent patch for r280078.

As discussed at w3c/csswg-drafts#6286 (comment), degenerate
aspect ratios derived from SVG width/height attributes fall back to viewbox aspect ratio
(whether due to negative values or zero values).

When computing the logical height/width using an intrinsic aspect ratio, RenderReplaced uses the
overridingLogicalWidth/overridingLogicalHeight whenever defined as long as the flex or
grid item has an intrinsic size. For an SVG graphic though, it's common to have an intrinsic aspect
ratio but not to have an intrinsic width or height. For this special case, we still should use
overridingLogicalWidth/overridingLogicalHeight for logical height/width calculations.

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

LayoutTests:

* TestExpectations: unskip two tests that are now passing.


git-svn-id: http://svn.webkit.org/repository/webkit/trunk@286100 268f45cc-cd09-0410-ab3c-d52691b4dbfc
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
Projects
None yet
Development

No branches or pull requests

5 participants