1362999 - intrinsic width calculations in abspos element with implicitly definite height doesn't take percent-height intrinsic-ratio descendants into account

Status: NEW

(Core :: Layout, defect, P3)

Description

[yisibl]

Attachment: expected.png

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_12_4) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/58.0.3029.96 Safari/537.36

Steps to reproduce:

1. Open http://jsbin.com/jemupowobo/edit?output
2. Checked 'Resize me' button


Actual results:

The img is not fully displayed.


Expected results:

See: expected.png

At present, only use this hack fixed `height: calc(100% - 50px - 500px)`

Chrome has been fixed in: https://codereview.chromium.org/2828453002/

But Chrome has two other similar problems:

https://bugs.chromium.org/p/chromium/issues/detail?id=719441
https://bugs.chromium.org/p/chromium/issues/detail?id=719452

Comment 1

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

jsbin seems to be down now, so I can't view the testcase.

Comment 2

[yisibl]

(In reply to David Baron :dbaron: ⌚️UTC-7 from comment #1)
> jsbin seems to be down now, so I can't view the testcase.

Now you can open it.

Comment 3

[Daniel Holbert [:dholbert]]

Could you perhaps post a screencast (or a screenshot) demonstrating the issue?  I'm having a hard time getting anything clearly-unexpected to happen.

(Part of the trouble is that the rendering seems to depend on your precise window size, since the checkbox adds "bottom: 500px" to an abspos element that already has "top: 50px" -- basically, giving it its iframe-height minus 450px, which might be large or might be 0px.  I'm not clear how big of a window you're testing in.)

Comment 4

[Daniel Holbert [:dholbert]]

Attachment: testcase 1, original version (with a bit of extra cruft)

OK, I think I might see what you're talking about. Comparing against Chrome, with tall window heights, the .container element seems to clip the image horizontally in Firefox, when the image and container are tall (which makes the image wide but does not impact the container's width).

This seems like we might be treating the image's "height: 100%" as impossible-to-resolve when we compute intrinsic widths -- so we resolve the container's intrinsic width based on the image's intrinsic width.  But then we *do* resolve the height:100% during layout, so the image ends up taller and wider than we set aside space for.

Here's a testcase which demonstrates a version of the same problem.  Here, we've got:
 - a 600px-by-600px intrinsic-size canvas
 - ... whose height is shrunk to 90px, by virtue of it having "height: 100%" and it being inside of an abspos element with the "top" and "bottom" properties set.
 - ... and this shrinks its width as well.
 - ... but despite that, its parent ".container" has an absurdly-large intrinsic width (taken from the canvas's intrinsic width without any knowledge of the fact that its height is constrained).

Comment 5

[Daniel Holbert [:dholbert]]

Edge 15 matches our behavior, FWIW.  And Presto (pre-blink opera engine) does its own separate thing.

Behaviors on Testcase 1:
========================
* Firefox & Edge 15:         Purple canvas is small square. Red dotted container is wide rectangle.
* Opera 12.12 (presto):      Purple canvas and red dotted container are *both* wide rectangles.
* Chrome 61 and Safari 10.1: Purple canvas and red dotted container are *both* small squares.

(Here, "small" = 90px, and "wide" = 600px)

Comment 6

[Daniel Holbert [:dholbert]]

Attachment: testcase 1

(Simplified the testcase slightly -- removed an unnecessary wrapper div and some  "float" cruft left around from an earlier version that had more nesting.)

Comment 7

[Daniel Holbert [:dholbert]]

Attachment: testcase 2 (reveals brokenness around this in Edge and Blink)

Here's a slightly different reduction of the original testcase, preserving the ".box" wrapper as the abspos thing.

Firefox & Chrome each render this the same as they render "testcase 1" (and differently from each other), as described in comment 5.  Firefox renders the dotted area wide, and Chrome renders it as a square.

However: in Chrome, if you hover the purple square (making a tweak to the "bottom" property), then Chrome gets into a broken state where the dotted area is trailing the size of the purple thing that it's shrinkrapping by one layout. (So the dotted area is always too wide or too skinny.)

Edge has a different bug - it seems to decide on the width of the dotted area during the first layout (based in part on the value of "bottom" on the parent), BUT it doesn't update that width at all when I hover to dynamically update the parent's "bottom" value. It does update the size of the purple canvas and the *height* of the dotted area, though.

So, Edge and Chrome get the author's expected initial rendering (with the red dotted area shrinkwrapping the image/canvas inside of it), but their sizing is very fragile with respect to incremental changes...

Comment 8

[Daniel Holbert [:dholbert]]

Attachment: testcase 3 (same as testcase 2 but with initial state swapped with hover state)

Comment 9

[Daniel Holbert [:dholbert]]

(In reply to Daniel Holbert [:dholbert] from comment #7)
> So, Edge and Chrome get the author's expected initial rendering (with the
> red dotted area shrinkwrapping the image/canvas inside of it), but their
> sizing is very fragile with respect to incremental changes...

I filed an issue for Edge:
  https://developer.microsoft.com/en-us/microsoft-edge/platform/issues/12345999/

...and I commented on this existing Chrome issue that was linked in comment 0 (I think the problem I'm seeing might be related):
  https://bugs.chromium.org/p/chromium/issues/detail?id=719441

Comment 10

[Daniel Holbert [:dholbert]]

Sorry, mis-paste -- the Chrome issue I commented on was actually this one (the other from comment 0):
 https://bugs.chromium.org/p/chromium/issues/detail?id=719452

Comment 11

[yisibl]

@Daniel Holbert Thank you for your detailed testing. Do you have any plans to fix this problem?

Comment 12

[Daniel Holbert [:dholbert]]

I'd need to look at the spec interactions to convince myself that it's actually a bug & what the correct behavior is for variations of these testcases.  As noted above, we're not entirely on our own, on our current behavior -- we're consistent with Edge in a simplified version of the testcase ("testcase 1") -- and where we disagree, Edge isn't consistent with itself (testcase 2).

(Also: I'm mostly tasked with helping fix layout/css performance issues right now (as part of the "quantum flow" project) -- and since this isn't in that bucket, it's not at the top of my personal priority list at the moment.)

Setting needinfo=me to at least look at the specs & figure out if there's spec support for any particular behavior, though.

Comment 13

[Daniel Holbert [:dholbert]]

Also FWIW, this is not a regression -- our behavior on the attached testcases has been the same since before Firefox 4.

(Tested Nightly/Minefield 3.7a1pre 2009-12-01)

Comment 14

[Daniel Holbert [:dholbert]]

So I think the closest thing to authoritative spec-text about this is in CSS2 10.3.2 "Inline, replaced elements": 

> 10.3 Calculating widths and margins
> -----------------------------------
> The values of an element's 'width' [...] propert[y] as used for layout [...]
>
> 10.3.2 Inline, replaced elements
> --------------------------------
> ... if 'width' has a computed value of 'auto',
> 'height' has some other computed value, and the
> element does have an intrinsic ratio; then the
> used value of 'width' is:
>    (used height) * (intrinsic ratio)
https://drafts.csswg.org/css2/visudet.html#inline-replaced-width

The <canvas> in my attached testcases (and the img in the reporter's JSBin testcase) fits the qualifications here -- auto width, non-auto height. This spec text makes it clear that it *is* appropriate that its used 'width' should be determined based off of its aspect ratio + used height.

The key thing here, though, is: how should the abspos element determine its width, and should this calculation benefit from the information about the image's used width (given that this derives from the abspos element's implicitly-constrained 'height' from 'top'/'bottom').  I can't find any hard text about how this is supposed to work at the moment -- the closest is CSS2 10.3.7:
> If all three of 'left', 'width', and 'right' are 'auto':
> [...] set 'left' to the static position and apply rule
>  number three below
> [...]
> 3. 'width' and 'right' are 'auto' and 'left' is not 'auto',
> then the width is shrink-to-fit 
> [...]
> CSS 2.2 does not define the exact algorithm.
https://drafts.csswg.org/css2/visudet.html#abs-non-replaced-width

I thought the intrinsic sizing spec defined shrink-to-fit, but it doesn't really seem to:
> Note: This specification does not define how to determine these sizes.
> https://drafts.csswg.org/css-sizing-3/#intrinsic-sizes

So, bottom-line -- I think the spec kind of leaves this situation undefined. BUT, I think it makes sense that we should be consistent about honoring the chain of percentage heights here -- and since we honor it for determining the final size, we should honor it for determining the container's intrinsic width as well.  So, IMO, this is a valid bug.

Comment 15

[Daniel Holbert [:dholbert]]

(In reply to Daniel Holbert [:dholbert] from comment #7)
> Created attachment 8877784 [details]
> testcase 2 (reveals brokenness around this in Edge and Blink)
> [...] in Chrome, if you hover the purple square (making a tweak to the
> "bottom" property), then Chrome gets into a broken state where the dotted
> area is trailing the size of the purple thing

BTW, the Chrome folks recently landed a patch for https://bugs.chromium.org/p/chromium/issues/detail?id=719452 , and that patch fixed this ^^ issue. (Just verified on latest Chrome Canary.)  Now their dotted border stays the same size as the purple canvas that it's shrinkwrapping, even after the dynamic hover sizing-tweak.

Comment 16

[Boris Zbarsky [:bzbarsky]]

This really needs to get sorted in the spec.  Per the spec, I'd expect the "100%" to become a computed "auto", because the parent's computed height is "auto".  And then it should become a used "auto", which is not what any browser does.  So the spec is just wrong here and needs to get fixed.  Do you know whether there's already an issue tracking that?

Comment 17

[Daniel Holbert [:dholbert]]

(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #16)
> This really needs to get sorted in the spec.  Per the spec, I'd expect the
> "100%" to become a computed "auto", because the parent's computed height is
> "auto".

Really, the main question here is: should that parent (<div class="container">)'s computed "height:auto" be treated as definite (for percent resolution) or not?

In normal block-layout, "height:auto" would not be definite (as bz's reading of the spec indicates), but in this case, it's on an abspos element with an explicit "top" and "bottom", which means we can resolve an exact used-value for the "height:auto" without having to do any layout, by solving for "height" in the equation at https://drafts.csswg.org/css2/visudet.html#abs-non-replaced-width .  In this case, we can resolve it to 90px.

It seems that Chrome is consistently treating the .container's "height:auto" as definite, whereas Firefox & Edge are treating it as indefinite at first (when measuring its intrinsic width as 600px from its child) and then definite later (when doing reflow inside of it, when we gladly resolve its child's height:100% to 90px).

I don't know if there are spec issues tracking this.  However, setting that aside, one observation about the width-determination: I do see that the css-sizing-3 spec has evolved a bit since comment 14 -- it now kind-of defines how to come up with the "shrink-to-fit width" of .container. Specifically, it says:

  # [...these are definitions...:]
  # fit-content size
  # fit-content inline size
  # fit-content block size
  #   If the available space in a given axis is finite,
  #   equal to min(max-content size, max(min-content size,
                                         stretch-fit size)).
  #   [...]
  #   Note: This is called the “shrink-to-fit” width in CSS2.1§10.3.5 
https://drafts.csswg.org/css-sizing-3/#auto-box-sizes

...and then it defines the three quantities there (max-content, min-content, & stretch-fit) sizes kind of hand-wavily, but it does define the children's contributions a bit more clearly:
  # A box’s min-content contribution/max-content contribution
  # in each axis is the size of the content box of a hypothetical
  # auto-sized float that contains only that box, if that hypothetical
  # float’s containing block is zero-sized/infinitely-sized.
[Interjection from dholbert -- I think the implication here is that the hypothetical float's containing block is implied to be *the same as the actual box's containing block*, except that it's zero-sized/infinitely-sized *in the axis that we're measuring. I should probably clarify this with the spec editors to be sure I'm interpreting that correctly.]
[Continuing the spec-quote:]
  # However, in the case of a replaced box with a percentage-based
  # width/max-width/height/max-height, the percentage is resolved to
  # zero when calculating the min-content contribution in the
  # corresponding axis.
https://drafts.csswg.org/css-sizing-3/#intrinsic-contribution

(The "However" text doesn't actually apply in this case RE resolving the shrink-to-fit-width, since the percent value is in the opposite axis [the height]. So while that text says to resolve some percentages to 0, it doesn't say to resolve *this* percentage to 0 in this case.)

So -- if my "Interjection" is correct, then I think this all means that in testcase 1, <canvas> has a 90px min-content & max-content width contribution, since that's content-width that it gets (90px in both cases) if I give it "float:left" and set the width of its container to something small or large (or anything at all).  This all means that its container should probably be using that "min-content & max-content contribution" in its fit-content width (aka its shrink-to-fit width), and it should end up being 90px wide as well, since this is the only contribution.  All of which is to say: given my interpretation of the spec here, I think Chrome's behavior is correct -- though I think we could also stand to clarify the spec a bit. Leaving needinfo open to file some spec bugs.

(Sorry if this is a bit long/stream-of-consciousness.)

Comment 18

[Boris Zbarsky [:bzbarsky]]

Ah.  So the key is that https://drafts.csswg.org/css-sizing-3/#sizing-properties defines the computed value of "height" quite differently from CSS2.1.  The percentage is kept into the computed value phase, and all the interesting bits happen at used value time...

Comment 19

[Daniel Holbert [:dholbert]]

Ah, good point!

 - CSS21 said that percentage heights should "compute to auto", on non-abspos elements, if the CB's height "is not specified explicitly (i.e., it depends on content height)".  Since the CB height here is literally *not specified* (it has "height:auto"), then I think we would indeed fall into this special case.  (Though: the CB height also does not depend on content height, since the CB is abspos and has top/bottom set. So this goes against the "i.e." parenthetical - but I'll bet that's just because this is a case the spec authors didn't think about.)
  https://www.w3.org/TR/CSS21/visudet.html#the-height-property

 - CSS-sizing-3 says that the computed value is "as specified, with lengths made absolute" (i.e. percentages are kept, as you note, and do different things at used-value-time)
 https://drafts.csswg.org/css-sizing-3/#sizing-properties

All of which is to say: I think this is *basically* specced reasonably[1], and we should change to match Chrome.

[1] "reasonably" = the parts that are left unclear in various specs can be reasonably inferred from interoperable behavior & simpler testcases -- e.g. my "Interjection" hand-waving in comment 17.

Comment 20

[Daniel Holbert [:dholbert]]

(I filed https://github.com/w3c/csswg-drafts/issues/2128 on the piece of comment 17 that I think is most unclear/bogus in the spec. But I don't think this bug here depends much on the outcome of that spec-issue.)