1317168 - [css-grid] Assertion failure: inflation != 1.0f || ancestorAutoSize.ISize(aWM) == autoSize.ISize(aWM) (Incorrect size computed by ComputeAutoSize?), at layout/forms/nsTextControlFrame.cpp:504

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Mats Palmgren (inactive)]

data:text/html,<div style="display:grid; grid:100px/100px"><input></div>

Comment 1

[Mats Palmgren (inactive)]

Attachment: part 1 - nsTextControlFrame::CalcIntrinsicSize is infallible so make it return the size result rather than NS_OK. Also, make it const. (idempotent patch)

Comment 2

[Mats Palmgren (inactive)]

Attachment: part 2 - [css-grid] Use the nsContainerFrame::ComputeAutoSize result when it might have been clamped.

The problem was that nsContainerFrame::ComputeAutoSize is affected by min-size
clamping whereas CalcIntrinsicSize is not.  So we need to return the potentially
clamped ISize, and only assert they produce the same ISize otherwise.

Comment 3

[Mats Palmgren (inactive)]

Attachment: part 3 - [css-grid] Reftests for <input> grid items.

Comment 4

[Mats Palmgren (inactive)]

Attachment: part 1 - nsTextControlFrame::CalcIntrinsicSize is infallible so make it return the size result rather than NS_OK. Also, make it const.

FYI, the change to GetPrefISize actually does change the reflow logging
slightly.  IIRC, we GetPref/MinISize needs to follow the pattern
  DISPLAY_PREF_WIDTH(this, result);
  ...
  result = ...;
  return result;

because these macros logs the 'result' value on exit.
The current GetPrefISize code fails to do that, so I fixed it while I'm here.

Comment 5

[Daniel Holbert [:dholbert]]

Comment on attachment 8810269 [details] [diff] [review]
part 1 - nsTextControlFrame::CalcIntrinsicSize is infallible so make it return the size result rather than NS_OK. Also, make it const.

Review of attachment 8810269 [details] [diff] [review]:
-----------------------------------------------------------------

r=me

Comment 6

[Daniel Holbert [:dholbert]]

Comment on attachment 8810267 [details] [diff] [review]
part 2 - [css-grid] Use the nsContainerFrame::ComputeAutoSize result when it might have been clamped.

Review of attachment 8810267 [details] [diff] [review]:
-----------------------------------------------------------------

Commit message nit:
> Bug 1317168 part 2 - [css-grid] Use the nsContainerFrame::ComputeAutoSize result when it might have been clamped. r=dholbert

This is a bit too generic -- it should mention "text frames" or "nsTextControlFrame" somewhere.

::: layout/forms/nsTextControlFrame.cpp
@@ +477,5 @@
>    float inflation = nsLayoutUtils::FontSizeInflationFor(this);
>    LogicalSize autoSize = CalcIntrinsicSize(aRenderingContext, aWM, inflation);
> +
> +  // Note: nsContainerFrame::ComputeAutoSize only computes the inline-size, and
> +  // only for 'auto'.  The returned block-size is always NS_UNCONSTRAINEDSIZE.

Please reword this last sentence to make it clearer *whose* returned block size you're talking about. (this method here? nsContainerFrame::ComputeAutoSize? or both? Right now I can't tell)

(e.g. if you're just specifically talking about nsContainerFrame impl in this last sentence, then s/The returned/Its returned/.)

@@ +483,5 @@
> +  if (iSizeCoord.GetUnit() == eStyleUnit_Auto) {
> +    if (aFlags & ComputeSizeFlags::eIClampMarginBoxMinSize) {
> +      // Use the potentially clamped inline-size.
> +      autoSize.ISize(aWM) =
> +        nsContainerFrame::ComputeAutoSize(aRenderingContext, aWM,

I think we won't be getting any font-inflation information here (based on the assertion below which allows for nsContainerFrame::ComputeAutoSize to disagree when inflation is non-1.0).

This is maybe OK, since we don't ship font-inflation default-enabled anywhere at this point. You should probably add an XXX comment to note this issue, though?  Something like:

 // CalcIntrinsicSize isn't aware of grid-item margin-box clamping, so we have to fall
 // back to nsContainerFrame's ComputeAutoSize impl (which does handle margin-box clamping).
 // Unfortunately, nsContainerFrame doesn't handle font inflation, but maybe that's OK
 // because we don't ship it default-enabled...?

(I'm hand-waving a bit here about what handles what, but I'm guessing my hand-waving is accurate based on the context around this code)

Comment 7

[Mats Palmgren (inactive)]

> I think we won't be getting any font-inflation information here

nsFrame::ShrinkWidthToFit does handle it though, so I'm not sure what
the issue here might be...

> ... we don't ship font-inflation default-enabled anywhere at this point

Perhaps we should just rip out all of that code then?  For a few reasons:
1. IIRC, it's actually quite a bit of a performance drag even when it's
   preffed off
2. IIRC, dbaron said somewhere that the approach we took isn't working
   very well (perhaps this is the reason it's disabled?)
3. maintaining dead code/comments is a waste of time

OTOH, perhaps we should decide/investigate a better approach first and
see if there's is some code worth keeping...  if indeed we think it's
a feature worth having at all.

Comment 8

[Daniel Holbert [:dholbert]]

(In reply to Mats Palmgren (:mats) from comment #7)
> > I think we won't be getting any font-inflation information here
> 
> nsFrame::ShrinkWidthToFit does handle it though, so I'm not sure what
> the issue here might be...

Nor am I -- I'm just going by the assertion condition.

> > ... we don't ship font-inflation default-enabled anywhere at this point
> 
> Perhaps we should just rip out all of that code then?
> [...]
> OTOH, perhaps we should decide/investigate a better approach first and
> see if there's is some code worth keeping...  if indeed we think it's
> a feature worth having at all.

(Yeah -- if we end up landing a replacement, it might conceivably want to share some code [or at least insert itself at similar points in the code], so it might be useful to do the removal-and-replacement all at once. *shrug*.)

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/87e598f49765
part 1 - nsTextControlFrame::CalcIntrinsicSize is infallible so make it return the size result rather than NS_OK.  Also, make it const.  r=dholbert
https://hg.mozilla.org/integration/mozilla-inbound/rev/33bab2311a77
part 2 - [css-grid] Fall back to using nsContainerFrame::ComputeAutoSize when the nsTextControlFrame min-content size needs to have grid item clamping applied.  r=dholbert
https://hg.mozilla.org/integration/mozilla-inbound/rev/cb5c261faec0
part 3 - [css-grid] Reftests for <input> grid items.

Comment 10

[Mats Palmgren (inactive)]

Comment on attachment 8810269 [details] [diff] [review]
part 1 - nsTextControlFrame::CalcIntrinsicSize is infallible so make it return the size result rather than NS_OK. Also, make it const.

Approval Request Comment
[Feature/regressing bug #]: CSS Grid
[User impact if declined]: web-compat risk
[Describe test coverage new/current, TreeHerder]: have reftests, all tests pass
[Risks and why]: low risk
[String/UUID change made/needed]: none

This is part of the CSS Grid release, please uplift to Aurora 52, thanks.

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/87e598f49765
https://hg.mozilla.org/mozilla-central/rev/33bab2311a77
https://hg.mozilla.org/mozilla-central/rev/cb5c261faec0

Comment 12

[Julien Cristau [:jcristau]]

Comment on attachment 8810269 [details] [diff] [review]
part 1 - nsTextControlFrame::CalcIntrinsicSize is infallible so make it return the size result rather than NS_OK. Also, make it const.

css-grid fixes for aurora52

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/d74bba27e731
https://hg.mozilla.org/releases/mozilla-aurora/rev/23b83d8ff9ec
https://hg.mozilla.org/releases/mozilla-aurora/rev/ef7152cecd69