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] Static position should use content-box, not padding-box #3020

Closed
fantasai opened this issue Aug 15, 2018 · 6 comments
Closed

[css-grid] Static position should use content-box, not padding-box #3020

fantasai opened this issue Aug 15, 2018 · 6 comments

Comments

@fantasai
Copy link
Collaborator

@fantasai fantasai commented Aug 15, 2018

Tab and I just noticed that while every other layout mode uses the content box of the static position containing block to set the static position, grid uses the padding box.

We suspect this was an error and we should fix this. Anyone have a different opinion?

Spec: https://drafts.csswg.org/css-grid-1/#static-position
Testcase: http://software.hixie.ch/utilities/js/live-dom-viewer/saved/6105

<!DOCTYPE html>
<style>
.abspos { position: absolute; border: 4px solid rgba(0,0,0,.4) }
.container { float: left; border: solid; padding: 2em; width: 10em; height: 6em; background: yellow content-box }
</style>
<div class=container style="display: grid;"><span class=abspos></span>grid</div>
<div class=container style="display: flex;"><span class=abspos></span>flex</div>
<div class=container style="display: block;"><span class=abspos></span>block</div>
@css-meeting-bot
Copy link
Member

@css-meeting-bot css-meeting-bot commented Aug 22, 2018

The Working Group just discussed Static position should use content-box, not padding-box, and agreed to the following:

  • RESOLVED: align grid layout static position as content box rather then padding.
The full IRC log of that discussion <dael> Topic: Static position should use content-box, not padding-box
<dael> github: https://github.com//issues/3020
<dael> Rossen: TabAtkins and fantasai noticed this while reviewing Grid and they asked if we should s tick to this or align to everything else. Opinions?
<dael> Rossen: TabAtkins is calling in
<dael> fantasai: I imagine he'sin favor
<TabAtkins> ^_^
<dbaron> makes sense to me
<dael> Rossen: We can resolve to align grid layout static position as content box rather then padding. Objections?
<dael> RESOLVED: align grid layout static position as content box rather then padding.
@MatsPalmgren
Copy link

@MatsPalmgren MatsPalmgren commented Sep 18, 2018

@mrego FYI, there's also a bunch of tests under css/vendor-imports/mozilla/mozilla-central-reftests/align3/ they are all updated now.

@mrego
Copy link
Member

@mrego mrego commented Sep 18, 2018

@MatsPalmgren thanks for the info, probably we should import those in Chromium/WebKit too.

aarongable pushed a commit to chromium/chromium that referenced this issue Sep 18, 2018
This is a recent change by the CSSWG:
w3c/csswg-drafts#3020

The spec text (https://drafts.csswg.org/css-grid/#static-position):
  "The static position of an absolutely-positioned child
   of a grid container is determined as if it were the sole grid item
   in a grid area whose edges coincide with the content edges
   of the grid container."

The patch is just a simple change in
LayoutGrid::PrepareChildForPositionedLayout() to use border and padding.

BUG=884956
TEST=external/wpt/css/css-grid/abspos/absolute-positioning-grid-container-parent-001.html

Change-Id: I01a77dfe41abcfac00c6d45f517c6d216aaa8ff7
Reviewed-on: https://chromium-review.googlesource.com/1229893
Commit-Queue: Manuel Rego <rego@igalia.com>
Reviewed-by: Javier Fernandez <jfernandez@igalia.com>
Cr-Commit-Position: refs/heads/master@{#592010}
kisg pushed a commit to paul99/webkit-mips that referenced this issue Sep 18, 2018
https://bugs.webkit.org/show_bug.cgi?id=189698

Reviewed by Javier Fernandez.

LayoutTests/imported/w3c:

* web-platform-tests/css/css-grid/abspos/absolute-positioning-grid-container-parent-001.html:
Update tests from WPT.

Source/WebCore:

This is a recent change by the CSSWG:
w3c/csswg-drafts#3020

The spec text (https://drafts.csswg.org/css-grid/#static-position):
  "The static position of an absolutely-positioned child
   of a grid container is determined as if it were the sole grid item
   in a grid area whose edges coincide with the content edges
   of the grid container."

Test: imported/w3c/web-platform-tests/css/css-grid/abspos/absolute-positioning-grid-container-parent-001.html

* rendering/RenderGrid.cpp:
(WebCore::RenderGrid::prepareChildForPositionedLayout):
Simple change to use border and padding.


git-svn-id: http://svn.webkit.org/repository/webkit/trunk@236126 268f45cc-cd09-0410-ab3c-d52691b4dbfc
@mrego
Copy link
Member

@mrego mrego commented Sep 19, 2018

@MatsPalmgren one question about the tests. I see tests for self and content alignment in the case of flexboxes, but only for self alignment in the case of grid layout. Why?

For example, I'm wondering if for grid layout the following two examples. Should they have the same output (the abspos element should be horizontally and vertically centered)?

  • Self alignment:
<div style="display: grid; width: 200px; height: 100px; place-items: center;
            border: solid thick;">
  <div style="position: absolute; background: magenta;">abspos</div>
</div>
  • Content alignment:
<div style="display: grid; width: 200px; height: 100px; place-content: center;
            border: solid thick;">
  <div style="position: absolute; background: magenta;">abspos</div>
</div>
@MatsPalmgren
Copy link

@MatsPalmgren MatsPalmgren commented Dec 16, 2018

I see tests for self and content alignment in the case of flexboxes, but only for self alignment in the case of grid layout. Why?

I don't know, but many of our Grid tests are in a separate folder. There should be some content-alignment tests there. (We want to convert these to WPT tests and upstream them eventually. Just lack of resources that it hasn't happened yet...)

For example, I'm wondering if for grid layout the following two examples. Should they have the same output (the abspos element should be horizontally and vertically centered)?

Note that the grid container is not an abs.pos. containing block in your examples, so §9.2 applies. In particular:

The static position [CSS21] of an absolutely-positioned child of a grid container is determined as if it were the sole grid item in a grid area whose edges coincide with the content edges of the grid container.

In your first example (self-alignment), css-align says "use the box’s static position rectangle", i.e. "By default, the static position rectangle of the child of a grid container corresponds to the content edges of the grid container.", so that's what leads to the first abs.pos. being centered in the content-box.

In the second case (content-alignment), it's the grid of the grid-container that is aligned, but since the abs.pos. box isn't grid-aligned at all it's not affected by that. If you add position:relative to the container the child is still not affected by content-alignment since it's not anchored to any line in the grid (given that all its lines are auto). If you additionally add grid-area:1/1/1/1 to the abs.pos. item to anchor it with the top-left of the grid it's still not centered because a grid with zero tracks is special: "If there are no grid tracks (the explicit grid is empty, and no tracks were created in the implicit grid), the sole grid line in each axis is aligned with the start edge of the grid container.". This makes sense because the alignment subjects are the tracks (not lines) and with no tracks content-distribution doesn't apply. If you add a track, e.g. grid:0/0, then line 1 should be centered by content-alignment and then you can use self-alignment to align the item relative to that grid-area.

So this example should display the same as your first example:

<div style="display: grid; width: 200px; height: 100px; place-content: center; place-items: center; border: solid thick; grid:0/0; position:relative">
  <div style="grid-area:1/1/-1/-1; position: absolute; background: magenta;">abspos</div>
</div>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
5 participants
You can’t perform that action at this time.