Skip to content

[css-grid-2] Resolving padding in indefinite intrinsic size calculation and using that to resolve indefinite size? #8963

Open

Description

@KurtCattiSchmidt

Spec

I'm looking at grid-auto-repeat-max-size-001.html in order to fix this Compat 2023 bug in Chromium

...however, it appears that both Blink and WebKit make more sense to me than what Gecko is doing, although Gecko's behavior is marked as passing.

Note that this is in relation to sub-cases 16 and 19, which Blink and WebKit are marked as failing and Gecko is marked as passing. The other cases are all in agreement.

I'll explain what Blink and WebKit are doing, and why I think that is the proper behavior.

No width or height are specified on the grid, so for sizing purposes, we fall back to the default intrinsic size of 300px by 150px. However, with this fallback, we are able to determine a final size of the grid by the end of the grid layout algorithm that differs from 300px by 150px.

Padding is 10%, on an indefinite inline axis, which falls back to 300px, so we land at 30px padding. This is agreed upon in all implementations.

Grid-template is set to repeat(auto-fill, 50px) / repeat(auto-fill, 100px); which we can resolve to two repetitions in both directions to fill the indefinite 300px width and 150px indefinite height (it can't be 3 repetitions in either axis due to the padding). This is also agreed upon in all implementations.

This is where things diverge in Gecko. For the inline size, Blink/Webkit add up the 30px padding on each side, along with 2 repetitions of 100px for each column, and land at 260px. Gecko sticks with the default 300px intrinsic size, even though it is possible to calculate an intrinsic size based on the fixed column sizes. There seems to be some extra padding after the grid items in Gecko, which is how it lands at 300px. I can't find any reference to this in the grid spec.

For block size, interestingly, Blink/WebKit/Gecko all agree on adding up padding-top and padding-bottom (30px + 30px) along with two rows of 50px, landing at 160px.

It's not clear to my why Gecko is inconsistent between the inline and block direction here. The grid spec implies that if fixed sizes are specified on track sizes (as is happening here), that those will be used for intrinsic size calculation.

If there is something I'm missing here, and that Gecko is indeed doing this correctly, I'd be happy to update Chromium's implementation. Otherwise, I'll update the expected sizes in the test case.

@bfgeek created a reduced case that demonstrates the extra padding Gecko is adding: https://www.software.hixie.ch/utilities/js/live-dom-viewer/?saved=11785

Activity

  1. bfgeek commented on Jun 13, 2023

    @bfgeek

    @dholbert - Do you know where this additional padding is coming from? (We are super confused).
    Testcase:

    <!DOCTYPE html>
    <div style="width: 300px; border: solid 3px;">
      <div style="
        background: gray;
        box-sizing: border-box;
        display: grid;
        padding: 10%;
        width: min-content;
        max-width: 300px;
        grid-template-columns: repeat(auto-fill, 100px);
        grid-template-rows: 100px;">
          <div style="
            background: lime;
            grid-column: -2;"></div>
      </div>
    </div>
    
    <div style="width: 300px; border: solid 3px;">
      <div style="
        background: gray;
        box-sizing: border-box;
        display: grid;
        padding: 10%;
        width: min-content;
        max-width: 300px;
        grid-template-columns: repeat(2, 100px);
        grid-template-rows: 100px;">
          <div style="
            background: lime;
            grid-column: -2;"></div>
      </div>
    </div>
    

    (only difference is auto-fill vs. 2 in grid-template-columns) which resolve to the same thing in both cases.

  2. Loirooriol commented on Jun 13, 2023

    @Loirooriol
    Contributor

    we fall back to the default intrinsic size of 300px by 150px

    This doesn't make sense to me, the grid is not a replaced element. https://drafts.csswg.org/css-sizing-3/#intrinsic-sizes says that 300x150 is for "replaced elements without natural sizes".

    AFAIK what should happen is basically this:

    • 1st we need to know the intrinsic contribution of the grid container. So we resolve repeat(auto-fill, 100px). Per https://www.w3.org/TR/css-grid-2/#auto-repeat it should be 3 repetitions, using the max-width. So we run the track sizing algorithm under a min/max-content constraint, we get 3 tracks of 100px each and no gap, so the grid is 300px.
    • Then we use the 300px as the size of the content area of the grid container. There is also a padding: 10%, but per https://drafts.csswg.org/css-sizing-3/#min-percentage-contribution it resolves to 0.
    • So the intrinsic contribution is 300px, and the parent element is also sized to 300px.
    • Then we lay out the grid container "for real".
    • We can now resolve padding: 10% against the parent, so padding becomes 30px.
    • This means that the border area would be 360px, but we have max-width: 300px; box-sizing: border-box, so the content area shrinks to 240px so that the border area doesn't exceed 300px.
    • Now when resolving repeat(auto-fill, 100px) we can only have 2 repetitions without overflowing the content area. So we get 2 tracks of 100px each.

    I think it should be analogous in the block axis, maybe Firefox does a different thing since it doesn't implement the 2nd pass (https://bugzilla.mozilla.org/show_bug.cgi?id=1481876).

  3. Loirooriol commented on Jun 13, 2023

    @Loirooriol
    Contributor

    In other words, I think it's Blink the one that adds some weird spacing between the grid and its parent in this case:

    <!DOCTYPE html>
    <div style="width: min-content; border: solid 3px;">
      <div style="
        background: gray;
        box-sizing: border-box;
        display: grid;
        padding: 10%;
        width: min-content;
        max-width: 200px;
        grid-template-columns: repeat(auto-fill, 100px) minmax(0, 1fr);
        grid-template-rows: 100px">
          <div style="background: lime"></div>
          <div style="background: yellow"></div>
      </div>
    </div>
  4. bfgeek commented on Jun 13, 2023

    @bfgeek

    Then we use the 300px as the size of the content area of the grid container. There is also a padding: 10%, but per https://drafts.csswg.org/css-sizing-3/#min-percentage-contribution it resolves to 0.

    This is likely the crux of the issue. I think this part of the spec is incorrect and likely should change this. Blink/Webkit will resolve percentages if they are resolvable (e.g. we'll resolve the %-padding for the top-most container). And if un-resolvable - they'll resolve against zero.

    E.g. this yeilds better results for cases like:
    https://www.software.hixie.ch/utilities/js/live-dom-viewer/?saved=11788

    (In firefox you'll end up with something not-square for the above case).
    %-padding on the container almost never matters, except for height resolution, and now in grid as its resolving min/max widths.

  5. KurtCattiSchmidt commented on Jun 13, 2023

    @KurtCattiSchmidt
    CollaboratorAuthor

    we fall back to the default intrinsic size of 300px by 150px

    This doesn't make sense to me, the grid is not a replaced element. https://drafts.csswg.org/css-sizing-3/#intrinsic-sizes says that 300x150 is for "replaced elements without natural sizes".

    AFAIK what should happen is basically this:

    • 1st we need to know the intrinsic contribution of the grid container. So we resolve repeat(auto-fill, 100px). Per https://www.w3.org/TR/css-grid-2/#auto-repeat it should be 3 repetitions, using the max-width. So we run the track sizing algorithm under a min/max-content constraint, we get 3 tracks of 100px each and no gap, so the grid is 300px.
    • Then we use the 300px as the size of the content area of the grid container. There is also a padding: 10%, but per https://drafts.csswg.org/css-sizing-3/#min-percentage-contribution it resolves to 0.
    • So the intrinsic contribution is 300px, and the parent element is also sized to 300px.
    • Then we lay out the grid container "for real".
    • We can now resolve padding: 10% against the parent, so padding becomes 30px.
    • This means that the border area would be 360px, but we have max-width: 300px; box-sizing: border-box, so the content area shrinks to 240px so that the border area doesn't exceed 300px.
    • Now when resolving repeat(auto-fill, 100px) we can only have 2 repetitions without overflowing the content area. So we get 2 tracks of 100px each.

    I think it should be analogous in the block axis, maybe Firefox does a different thing since it doesn't implement the 2nd pass (https://bugzilla.mozilla.org/show_bug.cgi?id=1481876).

    The discrepancy here is actually in the inline size, where Blink/WebKit resolve to 240px as you describe, and Firefox is the one at 300px. After reading your breakdown, I believe I was incorrect in how some of these values are computed and that you described it more accurately.

    In the block axis, Firefox matches what Blink/Webkit are doing in the inline axis. I don't think it's the second-pass logic as you described.

    Are we in agreement that this particular test case expectations are wrong, and that the inline sizes for cases 16 and 19 should be 240px instead of 300px? If so, I can get a PR ready to update it.

  6. Loirooriol commented on Jun 13, 2023

    @Loirooriol
    Contributor

    Blink/WebKit resolve to 240px as you describe

    Now I'm very confused, in the test I'm getting this:

    Firefox Chromium
    Wrapper Content area: 300x160
    Padding area: 300x160
    Content area: 300x160
    Padding area: 300x160
    Grid container Content area: 240x100
    Padding area: 300x160
    Content area: 200x100
    Padding area: 260x160

    Are we in agreement that this particular test case expectations are wrong

    No, I agree with data-expected-width="300" (it checks offsetWidth, including the padding).

  7. Loirooriol commented on Jun 13, 2023

    @Loirooriol
    Contributor

    E.g. this yeilds better results for cases like: https://www.software.hixie.ch/utilities/js/live-dom-viewer/?saved=11788

    Here the padding: 10% is not cyclic, the container has a definite size, so I think it's something different.

    %-padding on the container almost never matters, except for height resolution, and now in grid as its resolving min/max widths

    I don't think this is something new, I think these should behave the same (like they do on Firefox): https://www.software.hixie.ch/utilities/js/live-dom-viewer/saved/11789

    <!DOCTYPE html>
    <style>
    .wrapper { width: min-content; border: solid 3px }
    .target { background: gray; box-sizing: border-box; padding: 10%; width: min-content; max-width: 200px; }
    .target.grid { display: grid; grid-template-columns: repeat(auto-fill, 100px); grid-template-rows: 100px }
    .target::before { content: ""; display: block; background: lime }
    .target.block::before { width: 200px; max-width: max(-1%, 100px); height: 100px }
    </style>
    <div class="wrapper">
      <div class="target block"></div>
    </div>
    <div class="wrapper">
      <div class="target grid"></div>
    </div>

    In both cases we have some contents with a width of 200px during intrinsic sizing which shrinks to 100px when laying out for real. But the padding area of .target.block keeps being 200px, I son't see why it should be different for .target-grid.

  8. KurtCattiSchmidt commented on Jun 13, 2023

    @KurtCattiSchmidt
    CollaboratorAuthor

    Blink/WebKit resolve to 240px as you describe

    Now I'm very confused, in the test I'm getting this:

    Firefox Chromium
    Wrapper Content area: 300x160
    Padding area: 300x160 Content area: 300x160
    Padding area: 300x160
    Grid container Content area: 240x100
    Padding area: 300x160 Content area: 200x100
    Padding area: 260x160

    Are we in agreement that this particular test case expectations are wrong

    No, I agree with data-expected-width="300" (it checks offsetWidth, including the padding).

    Sorry, I meant 260px. This is the testharness.js output for Blink/WebKit:

    .grid 16

    assert_equals:

    width expected 300 but got 260

    .grid 19

    assert_equals:

    width expected 300 but got 260

  9. Loirooriol commented on Jun 13, 2023

    @Loirooriol
    Contributor

    I don't think it's the second-pass logic as you described.

    Yeah, I guess the difference is because when finding the intrinsic size in the block axis, Firefox already knows the width of the wrapper so it can resolve the percentage. That is, the asymmetry would be because only the horizontal paddings are cyclic.

  10. bfgeek commented on Jun 13, 2023

    @bfgeek

    I don't think this is something new, I think these should behave the same (like they do on Firefox): https://www.software.hixie.ch/utilities/js/live-dom-viewer/saved/11789

    These two tests are pretty different, and aren't really comparable.

    That is, the asymmetry would be because only the horizontal paddings are cyclic.

    I don't think the %-padding is cyclic once inside the layout pass of the inner grid, you have a definite width once you work out what the outer "min-content" resolves to, and can resolve the %-padding - which in turn will alter the min-content size of the inner grid. This happens quite often.

    This is similar to the 2nd pass logic for height in grid. This will change the min-content size of a box.

  11. bfgeek commented on Jun 13, 2023

    @bfgeek

    E.g. concretely: https://www.software.hixie.ch/utilities/js/live-dom-viewer/?saved=11791

    Even though the boxes are sized by width: max-content we still resolve the %-padding during the layout pass. Grid shouldn't be different here?

  12. Loirooriol commented on Jun 13, 2023

    @Loirooriol
    Contributor

    Yeah, resolving padding: 10% can increase the outer size and make the element overflow, but it shouldn't affect width: min-content.

    Grid in Blink can contribute 300px to the parent, but resolve width: min-content to an outer size of 260px, seems strange.

  13. bfgeek commented on Jun 13, 2023

    @bfgeek

    Grid in Blink can contribute 300px to the parent, but resolve width: min-content to an outer size of 260px, seems strange.

    But this is just the padding resolving and causing the repeat() to evaluate differently. :). It might be strange, but unclear how its incorrect?

  14. bfgeek commented on Jun 13, 2023

    @bfgeek

    Additionally if we modify your testcase to make the outer div definite, e.g. the padding is definitely not cyclic, Firefox doesn't respect it:
    https://www.software.hixie.ch/utilities/js/live-dom-viewer/?saved=11792

  15. 13 remaining items

  16. dholbert commented on Dec 1, 2023

    @dholbert
    Member

    @dholbert Do you agree Firefox seems wrong for this case? https://www.software.hixie.ch/utilities/js/live-dom-viewer/?saved=11792

    Sorry, forgot about this thread. Yes, I agree that Firefox seems wrong for that case, and I think your logic in #8963 (comment) makes sense.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions